| Message ID | 20210408011637.5361-3-zev@bewilderbeest.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | serial: 8250_aspeed_vuart: generalized DT properties | expand |
On Thu, 8 Apr 2021, at 10:46, Zev Weiss wrote: > This splits dedicated aspeed_vuart_set_{sirq,lpc_address}() functions > out of the sysfs store functions in preparation for adding DT > properties that will be poking the same registers. While we're at it, > these functions now provide some basic bounds-checking on their > arguments. > > Signed-off-by: Zev Weiss <zev@bewilderbeest.net> > --- > drivers/tty/serial/8250/8250_aspeed_vuart.c | 51 ++++++++++++++------- > 1 file changed, 35 insertions(+), 16 deletions(-) > > diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c > b/drivers/tty/serial/8250/8250_aspeed_vuart.c > index c33e02cbde93..8433f8dbb186 100644 > --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c > +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c > @@ -72,22 +72,31 @@ static ssize_t lpc_address_show(struct device *dev, > return snprintf(buf, PAGE_SIZE - 1, "0x%x\n", addr); > } > > +static int aspeed_vuart_set_lpc_address(struct aspeed_vuart *vuart, u32 addr) > +{ > + if (addr > U16_MAX) > + return -EINVAL; > + > + writeb(addr >> 8, vuart->regs + ASPEED_VUART_ADDRH); > + writeb(addr >> 0, vuart->regs + ASPEED_VUART_ADDRL); > + > + return 0; > +} > + > static ssize_t lpc_address_store(struct device *dev, > struct device_attribute *attr, > const char *buf, size_t count) > { > struct aspeed_vuart *vuart = dev_get_drvdata(dev); > - unsigned long val; > + u32 val; > int err; > > - err = kstrtoul(buf, 0, &val); > + err = kstrtou32(buf, 0, &val); > if (err) > return err; > > - writeb(val >> 8, vuart->regs + ASPEED_VUART_ADDRH); > - writeb(val >> 0, vuart->regs + ASPEED_VUART_ADDRL); > - > - return count; > + err = aspeed_vuart_set_lpc_address(vuart, val); > + return err ? : count; > } > > static DEVICE_ATTR_RW(lpc_address); > @@ -105,27 +114,37 @@ static ssize_t sirq_show(struct device *dev, > return snprintf(buf, PAGE_SIZE - 1, "%u\n", reg); > } > > +static int aspeed_vuart_set_sirq(struct aspeed_vuart *vuart, u32 sirq) > +{ > + u8 reg; > + > + if (sirq > (ASPEED_VUART_GCRB_HOST_SIRQ_MASK >> ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT)) > + return -EINVAL; > + > + sirq <<= ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT; > + sirq &= ASPEED_VUART_GCRB_HOST_SIRQ_MASK; This might be less verbose if we reordered things a little: ``` sirq <<= ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT; if (sirq & ASPEED_VUART_GCRB_HOST_SIRQ_MASK) return -EINVAL; sirq &= ASPEED_VUART_GCRB_HOST_SIRQ_MASK; ``` But otherwise it looks okay, so Reviewed-by: Andrew Jeffery <andrew@aj.id.au> > + > + reg = readb(vuart->regs + ASPEED_VUART_GCRB); > + reg &= ~ASPEED_VUART_GCRB_HOST_SIRQ_MASK; > + reg |= sirq; > + writeb(reg, vuart->regs + ASPEED_VUART_GCRB); > + > + return 0; > +} > + > static ssize_t sirq_store(struct device *dev, struct device_attribute > *attr, > const char *buf, size_t count) > { > struct aspeed_vuart *vuart = dev_get_drvdata(dev); > unsigned long val; > int err; > - u8 reg; > > err = kstrtoul(buf, 0, &val); > if (err) > return err; > > - val <<= ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT; > - val &= ASPEED_VUART_GCRB_HOST_SIRQ_MASK; > - > - reg = readb(vuart->regs + ASPEED_VUART_GCRB); > - reg &= ~ASPEED_VUART_GCRB_HOST_SIRQ_MASK; > - reg |= val; > - writeb(reg, vuart->regs + ASPEED_VUART_GCRB); > - > - return count; > + err = aspeed_vuart_set_sirq(vuart, val); > + return err ? : count; > } > > static DEVICE_ATTR_RW(sirq); > -- > 2.31.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Fri, Apr 09, 2021 at 12:06:16AM CDT, Andrew Jeffery wrote: > > >On Thu, 8 Apr 2021, at 10:46, Zev Weiss wrote: >> This splits dedicated aspeed_vuart_set_{sirq,lpc_address}() functions >> out of the sysfs store functions in preparation for adding DT >> properties that will be poking the same registers. While we're at it, >> these functions now provide some basic bounds-checking on their >> arguments. >> >> Signed-off-by: Zev Weiss <zev@bewilderbeest.net> >> --- >> drivers/tty/serial/8250/8250_aspeed_vuart.c | 51 ++++++++++++++------- >> 1 file changed, 35 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c >> b/drivers/tty/serial/8250/8250_aspeed_vuart.c >> index c33e02cbde93..8433f8dbb186 100644 >> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c >> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c >> @@ -72,22 +72,31 @@ static ssize_t lpc_address_show(struct device *dev, >> return snprintf(buf, PAGE_SIZE - 1, "0x%x\n", addr); >> } >> >> +static int aspeed_vuart_set_lpc_address(struct aspeed_vuart *vuart, u32 addr) >> +{ >> + if (addr > U16_MAX) >> + return -EINVAL; >> + >> + writeb(addr >> 8, vuart->regs + ASPEED_VUART_ADDRH); >> + writeb(addr >> 0, vuart->regs + ASPEED_VUART_ADDRL); >> + >> + return 0; >> +} >> + >> static ssize_t lpc_address_store(struct device *dev, >> struct device_attribute *attr, >> const char *buf, size_t count) >> { >> struct aspeed_vuart *vuart = dev_get_drvdata(dev); >> - unsigned long val; >> + u32 val; >> int err; >> >> - err = kstrtoul(buf, 0, &val); >> + err = kstrtou32(buf, 0, &val); >> if (err) >> return err; >> >> - writeb(val >> 8, vuart->regs + ASPEED_VUART_ADDRH); >> - writeb(val >> 0, vuart->regs + ASPEED_VUART_ADDRL); >> - >> - return count; >> + err = aspeed_vuart_set_lpc_address(vuart, val); >> + return err ? : count; >> } >> >> static DEVICE_ATTR_RW(lpc_address); >> @@ -105,27 +114,37 @@ static ssize_t sirq_show(struct device *dev, >> return snprintf(buf, PAGE_SIZE - 1, "%u\n", reg); >> } >> >> +static int aspeed_vuart_set_sirq(struct aspeed_vuart *vuart, u32 sirq) >> +{ >> + u8 reg; >> + >> + if (sirq > (ASPEED_VUART_GCRB_HOST_SIRQ_MASK >> ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT)) >> + return -EINVAL; >> + >> + sirq <<= ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT; >> + sirq &= ASPEED_VUART_GCRB_HOST_SIRQ_MASK; > >This might be less verbose if we reordered things a little: > >``` >sirq <<= ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT; >if (sirq & ASPEED_VUART_GCRB_HOST_SIRQ_MASK) > return -EINVAL; >sirq &= ASPEED_VUART_GCRB_HOST_SIRQ_MASK; >``` Hmm, that (or something similar, perhaps with a '~' on the mask in the if condition?) does seem like it'd be a nice improvement, though I suppose it'd also mean we'd fail to reject some way-out-of-range sirq values (e.g. if it had its MSB set) -- so I think I'll leave it as is, just in the name of thoroughness/paranoia? > >But otherwise it looks okay, so > >Reviewed-by: Andrew Jeffery <andrew@aj.id.au> > Thanks.
On Fri, 9 Apr 2021, at 16:31, Zev Weiss wrote: > On Fri, Apr 09, 2021 at 12:06:16AM CDT, Andrew Jeffery wrote: > > > > > >On Thu, 8 Apr 2021, at 10:46, Zev Weiss wrote: > >> This splits dedicated aspeed_vuart_set_{sirq,lpc_address}() functions > >> out of the sysfs store functions in preparation for adding DT > >> properties that will be poking the same registers. While we're at it, > >> these functions now provide some basic bounds-checking on their > >> arguments. > >> > >> Signed-off-by: Zev Weiss <zev@bewilderbeest.net> > >> --- > >> drivers/tty/serial/8250/8250_aspeed_vuart.c | 51 ++++++++++++++------- > >> 1 file changed, 35 insertions(+), 16 deletions(-) > >> > >> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c > >> b/drivers/tty/serial/8250/8250_aspeed_vuart.c > >> index c33e02cbde93..8433f8dbb186 100644 > >> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c > >> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c > >> @@ -72,22 +72,31 @@ static ssize_t lpc_address_show(struct device *dev, > >> return snprintf(buf, PAGE_SIZE - 1, "0x%x\n", addr); > >> } > >> > >> +static int aspeed_vuart_set_lpc_address(struct aspeed_vuart *vuart, u32 addr) > >> +{ > >> + if (addr > U16_MAX) > >> + return -EINVAL; > >> + > >> + writeb(addr >> 8, vuart->regs + ASPEED_VUART_ADDRH); > >> + writeb(addr >> 0, vuart->regs + ASPEED_VUART_ADDRL); > >> + > >> + return 0; > >> +} > >> + > >> static ssize_t lpc_address_store(struct device *dev, > >> struct device_attribute *attr, > >> const char *buf, size_t count) > >> { > >> struct aspeed_vuart *vuart = dev_get_drvdata(dev); > >> - unsigned long val; > >> + u32 val; > >> int err; > >> > >> - err = kstrtoul(buf, 0, &val); > >> + err = kstrtou32(buf, 0, &val); > >> if (err) > >> return err; > >> > >> - writeb(val >> 8, vuart->regs + ASPEED_VUART_ADDRH); > >> - writeb(val >> 0, vuart->regs + ASPEED_VUART_ADDRL); > >> - > >> - return count; > >> + err = aspeed_vuart_set_lpc_address(vuart, val); > >> + return err ? : count; > >> } > >> > >> static DEVICE_ATTR_RW(lpc_address); > >> @@ -105,27 +114,37 @@ static ssize_t sirq_show(struct device *dev, > >> return snprintf(buf, PAGE_SIZE - 1, "%u\n", reg); > >> } > >> > >> +static int aspeed_vuart_set_sirq(struct aspeed_vuart *vuart, u32 sirq) > >> +{ > >> + u8 reg; > >> + > >> + if (sirq > (ASPEED_VUART_GCRB_HOST_SIRQ_MASK >> ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT)) > >> + return -EINVAL; > >> + > >> + sirq <<= ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT; > >> + sirq &= ASPEED_VUART_GCRB_HOST_SIRQ_MASK; > > > >This might be less verbose if we reordered things a little: > > > >``` > >sirq <<= ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT; > >if (sirq & ASPEED_VUART_GCRB_HOST_SIRQ_MASK) > > return -EINVAL; > >sirq &= ASPEED_VUART_GCRB_HOST_SIRQ_MASK; > >``` > > Hmm, that (or something similar, perhaps with a '~' on the mask in the > if condition?) does seem like it'd be a nice improvement, though I > suppose it'd also mean we'd fail to reject some way-out-of-range sirq > values (e.g. if it had its MSB set) -- so I think I'll leave it as is, > just in the name of thoroughness/paranoia? Yeah, fair enough. I was considering smaller errors :) Andrew
On Fri, Apr 09, 2021 at 02:24:08AM CDT, Andy Shevchenko wrote: >On Thursday, April 8, 2021, Zev Weiss <zev@bewilderbeest.net> wrote: > >> This splits dedicated aspeed_vuart_set_{sirq,lpc_address}() functions >> out of the sysfs store functions in preparation for adding DT >> properties that will be poking the same registers. While we're at it, >> these functions now provide some basic bounds-checking on their >> arguments. >> >> > >Please, use prefix “serial: 8250_aspeed_vuart:” instead of what you have in >the subject line. I think I have told this already > > Ah, sorry -- I fixed the cover letter after your first comment (which had definitely been under-tagged); for the patches themselves I was following the example of the last patch in that particular area (8d310c9107a2), though I guess that wasn't the right model to follow. I'll use the requested format in the future.
On Fri, Apr 9, 2021 at 10:38 AM Zev Weiss <zev@bewilderbeest.net> wrote: > > On Fri, Apr 09, 2021 at 02:24:08AM CDT, Andy Shevchenko wrote: > >On Thursday, April 8, 2021, Zev Weiss <zev@bewilderbeest.net> wrote: > > > >> This splits dedicated aspeed_vuart_set_{sirq,lpc_address}() functions > >> out of the sysfs store functions in preparation for adding DT > >> properties that will be poking the same registers. While we're at it, > >> these functions now provide some basic bounds-checking on their > >> arguments. > >> > >> > > > >Please, use prefix “serial: 8250_aspeed_vuart:” instead of what you have in > >the subject line. I think I have told this already > > > > > > Ah, sorry -- I fixed the cover letter after your first comment (which > had definitely been under-tagged); for the patches themselves I was > following the example of the last patch in that particular area > (8d310c9107a2), though I guess that wasn't the right model to follow. > I'll use the requested format in the future. Just random amount of most recent patches against 8250 driver: e47eb5241a8f serial: 8250: Avoid new transfers when shutting down e49950d3e737 serial: 8250_dma: use linear buffer for transmit 34255381fabd serial: 8250_port: Try to run DMA Rx on timeout condition 7d7dec450a66 8250_tegra: clean up tegra_uart_handle_break c3ae3dc896fa serial: 8250_pci: Drop bogus __refdata annotation d96f04d347e4 serial: 8250_omap: Avoid FIFO corruption caused by MDR1 access 6e4e636e0e3e serial: 8250-mtk: Fix reference leak in mtk8250_probe a609c58086e3 tty: serial: 8250: 8250_port: Move prototypes to shared location 6f9918504129 serial: 8250: 8250_omap: Fix unused variable warning d4548b14dd7e serial: 8250: 8250_omap: Fix possible array out of bounds access 912ab37c7987 serial: 8250_mtk: Fix uart_get_baud_rate warning 439c7183e5b9 serial: 8250: 8250_omap: Disable RX interrupt after DMA enable 32ed248042d1 tty: serial: 8250: serial_cs: Remove unused/unchecked variable 'err' 85985a3dcd74 serial: 8250_dw: Fix clk-notifier/port suspend deadlock c8dff3aa8241 serial: 8250: Skip uninitialized TTY port baud rate update 7718453e3696 serial: 8250: Discard RTS/DTS setting from clock update method 409cc4541ade serial: 8250_fsl: Fix TX interrupt handling condition 3c5a87be170a serial: 8250_pci: Add Realtek 816a and 816b ea4de367e57d tty: serial: 8250_mtk: set regshift for mmio32 57cee0713118 serial: 8250_pci: Remove unused function get_pci_irq() 11361610b005 serial: 8250_fsl: Add ACPI support
diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c index c33e02cbde93..8433f8dbb186 100644 --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c @@ -72,22 +72,31 @@ static ssize_t lpc_address_show(struct device *dev, return snprintf(buf, PAGE_SIZE - 1, "0x%x\n", addr); } +static int aspeed_vuart_set_lpc_address(struct aspeed_vuart *vuart, u32 addr) +{ + if (addr > U16_MAX) + return -EINVAL; + + writeb(addr >> 8, vuart->regs + ASPEED_VUART_ADDRH); + writeb(addr >> 0, vuart->regs + ASPEED_VUART_ADDRL); + + return 0; +} + static ssize_t lpc_address_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { struct aspeed_vuart *vuart = dev_get_drvdata(dev); - unsigned long val; + u32 val; int err; - err = kstrtoul(buf, 0, &val); + err = kstrtou32(buf, 0, &val); if (err) return err; - writeb(val >> 8, vuart->regs + ASPEED_VUART_ADDRH); - writeb(val >> 0, vuart->regs + ASPEED_VUART_ADDRL); - - return count; + err = aspeed_vuart_set_lpc_address(vuart, val); + return err ? : count; } static DEVICE_ATTR_RW(lpc_address); @@ -105,27 +114,37 @@ static ssize_t sirq_show(struct device *dev, return snprintf(buf, PAGE_SIZE - 1, "%u\n", reg); } +static int aspeed_vuart_set_sirq(struct aspeed_vuart *vuart, u32 sirq) +{ + u8 reg; + + if (sirq > (ASPEED_VUART_GCRB_HOST_SIRQ_MASK >> ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT)) + return -EINVAL; + + sirq <<= ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT; + sirq &= ASPEED_VUART_GCRB_HOST_SIRQ_MASK; + + reg = readb(vuart->regs + ASPEED_VUART_GCRB); + reg &= ~ASPEED_VUART_GCRB_HOST_SIRQ_MASK; + reg |= sirq; + writeb(reg, vuart->regs + ASPEED_VUART_GCRB); + + return 0; +} + static ssize_t sirq_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { struct aspeed_vuart *vuart = dev_get_drvdata(dev); unsigned long val; int err; - u8 reg; err = kstrtoul(buf, 0, &val); if (err) return err; - val <<= ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT; - val &= ASPEED_VUART_GCRB_HOST_SIRQ_MASK; - - reg = readb(vuart->regs + ASPEED_VUART_GCRB); - reg &= ~ASPEED_VUART_GCRB_HOST_SIRQ_MASK; - reg |= val; - writeb(reg, vuart->regs + ASPEED_VUART_GCRB); - - return count; + err = aspeed_vuart_set_sirq(vuart, val); + return err ? : count; } static DEVICE_ATTR_RW(sirq);
This splits dedicated aspeed_vuart_set_{sirq,lpc_address}() functions out of the sysfs store functions in preparation for adding DT properties that will be poking the same registers. While we're at it, these functions now provide some basic bounds-checking on their arguments. Signed-off-by: Zev Weiss <zev@bewilderbeest.net> --- drivers/tty/serial/8250/8250_aspeed_vuart.c | 51 ++++++++++++++------- 1 file changed, 35 insertions(+), 16 deletions(-)