| Message ID | 20231219171903.3530985-10-hugo@hugovil.com |
|---|---|
| State | New |
| Headers | show |
| Series | serial: sc16is7xx: fixes, cleanups and improvements | expand |
On Tue, Dec 19, 2023 at 12:18:53PM -0500, Hugo Villeneuve wrote: > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > Add macro to hold the maximum number of UART ports per IC/device. ... > - if (count < 0 || count > ARRAY_SIZE(irda_port)) > + if (count < 0 || count > SC16IS7XX_MAX_PORTS) ARRAY_SIZE() is more robust than this. What if you change to support different devices where this won't be as defined? > return; ... > - if (count < 0 || count > ARRAY_SIZE(mctrl_port)) > + if (count < 0 || count > SC16IS7XX_MAX_PORTS) > return 0; Ditto. ... > + WARN_ON(devtype->nr_uart > SC16IS7XX_MAX_PORTS); Not sure about this, perhaps it's fine. Otherwise looks reasonable.
On Wed, 20 Dec 2023 17:50:34 +0200 Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > On Tue, Dec 19, 2023 at 12:18:53PM -0500, Hugo Villeneuve wrote: > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > Add macro to hold the maximum number of UART ports per IC/device. > > ... > > > - if (count < 0 || count > ARRAY_SIZE(irda_port)) > > + if (count < 0 || count > SC16IS7XX_MAX_PORTS) > > ARRAY_SIZE() is more robust than this. What if you change to support different > devices where this won't be as defined? Hi, not sure that I understand your point, because SC16IS7XX_MAX_PORTS is the maximum for all devices supported by this driver. The irda_port array always has a fixed number of elements set to SC16IS7XX_MAX_PORTS, even if the device that we are probing has only one port for example. But I can change it back to ARRAY_SIZE(irda_port) if you want. > > > return; > > ... > > > - if (count < 0 || count > ARRAY_SIZE(mctrl_port)) > > + if (count < 0 || count > SC16IS7XX_MAX_PORTS) > > return 0; > > Ditto. > > ... > > > + WARN_ON(devtype->nr_uart > SC16IS7XX_MAX_PORTS); > > Not sure about this, perhaps it's fine. This check is only there if we add support for a new device and we incorrectly set nr_uart to an incorrect value, which will cause other problems anyway, of course :) This could be removed. Hugo Villeneuve
On Thu, Dec 21, 2023 at 11:41:03AM -0500, Hugo Villeneuve wrote: > On Wed, 20 Dec 2023 17:50:34 +0200 > Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > On Tue, Dec 19, 2023 at 12:18:53PM -0500, Hugo Villeneuve wrote: ... > > > - if (count < 0 || count > ARRAY_SIZE(irda_port)) > > > + if (count < 0 || count > SC16IS7XX_MAX_PORTS) > > > > ARRAY_SIZE() is more robust than this. What if you change to support different > > devices where this won't be as defined? > > not sure that I understand your point, because SC16IS7XX_MAX_PORTS is > the maximum for all devices supported by this driver. The irda_port > array always has a fixed number of elements set to SC16IS7XX_MAX_PORTS, > even if the device that we are probing has only one port for example. For current models of the device, yes. Who knows the future? Also, ARRAY_SIZE() make it less points to update if ever needed. > But I can change it back to ARRAY_SIZE(irda_port) if you want. Please change it back. > > > return; ... > > > + WARN_ON(devtype->nr_uart > SC16IS7XX_MAX_PORTS); > > > > Not sure about this, perhaps it's fine. > > This check is only there if we add support for a new device and we > incorrectly set nr_uart to an incorrect value, which will cause other > problems anyway, of course :) > > This could be removed. Let's remove. We can add it back in case something like this (quite unlikely) happens.
On Thu, 21 Dec 2023 18:55:17 +0200 Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > On Thu, Dec 21, 2023 at 11:41:03AM -0500, Hugo Villeneuve wrote: > > On Wed, 20 Dec 2023 17:50:34 +0200 > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > > On Tue, Dec 19, 2023 at 12:18:53PM -0500, Hugo Villeneuve wrote: > > ... > > > > > - if (count < 0 || count > ARRAY_SIZE(irda_port)) > > > > + if (count < 0 || count > SC16IS7XX_MAX_PORTS) > > > > > > ARRAY_SIZE() is more robust than this. What if you change to support different > > > devices where this won't be as defined? > > > > not sure that I understand your point, because SC16IS7XX_MAX_PORTS is > > the maximum for all devices supported by this driver. The irda_port > > array always has a fixed number of elements set to SC16IS7XX_MAX_PORTS, > > even if the device that we are probing has only one port for example. > > For current models of the device, yes. Who knows the future? > Also, ARRAY_SIZE() make it less points to update if ever needed. > > > But I can change it back to ARRAY_SIZE(irda_port) if you want. > > Please change it back. > > > > > return; > > ... > > > > > + WARN_ON(devtype->nr_uart > SC16IS7XX_MAX_PORTS); > > > > > > Not sure about this, perhaps it's fine. > > > > This check is only there if we add support for a new device and we > > incorrectly set nr_uart to an incorrect value, which will cause other > > problems anyway, of course :) > > > > This could be removed. > > Let's remove. We can add it back in case something like this (quite unlikely) > happens. Ok, will do both for v2. Hugo Villeneuve
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c index 3fb99b6929f3..29844e2eefc5 100644 --- a/drivers/tty/serial/sc16is7xx.c +++ b/drivers/tty/serial/sc16is7xx.c @@ -28,6 +28,7 @@ #define SC16IS7XX_NAME "sc16is7xx" #define SC16IS7XX_MAX_DEVS 8 +#define SC16IS7XX_MAX_PORTS 2 /* Maximum number of UART ports per IC. */ /* SC16IS7XX register definitions */ #define SC16IS7XX_RHR_REG (0x00) /* RX FIFO */ @@ -1396,11 +1397,11 @@ static void sc16is7xx_setup_irda_ports(struct sc16is7xx_port *s) int i; int ret; int count; - u32 irda_port[2]; + u32 irda_port[SC16IS7XX_MAX_PORTS]; struct device *dev = s->p[0].port.dev; count = device_property_count_u32(dev, "irda-mode-ports"); - if (count < 0 || count > ARRAY_SIZE(irda_port)) + if (count < 0 || count > SC16IS7XX_MAX_PORTS) return; ret = device_property_read_u32_array(dev, "irda-mode-ports", @@ -1423,11 +1424,11 @@ static int sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s, int i; int ret; int count; - u32 mctrl_port[2]; + u32 mctrl_port[SC16IS7XX_MAX_PORTS]; struct device *dev = s->p[0].port.dev; count = device_property_count_u32(dev, "nxp,modem-control-line-ports"); - if (count < 0 || count > ARRAY_SIZE(mctrl_port)) + if (count < 0 || count > SC16IS7XX_MAX_PORTS) return 0; ret = device_property_read_u32_array(dev, "nxp,modem-control-line-ports", @@ -1471,6 +1472,8 @@ static int sc16is7xx_probe(struct device *dev, int i, ret; struct sc16is7xx_port *s; + WARN_ON(devtype->nr_uart > SC16IS7XX_MAX_PORTS); + for (i = 0; i < devtype->nr_uart; i++) if (IS_ERR(regmaps[i])) return PTR_ERR(regmaps[i]); @@ -1730,7 +1733,7 @@ static unsigned int sc16is7xx_regmap_port_mask(unsigned int port_id) static int sc16is7xx_spi_probe(struct spi_device *spi) { const struct sc16is7xx_devtype *devtype; - struct regmap *regmaps[2]; + struct regmap *regmaps[SC16IS7XX_MAX_PORTS]; unsigned int i; int ret; @@ -1800,7 +1803,7 @@ MODULE_ALIAS("spi:sc16is7xx"); static int sc16is7xx_i2c_probe(struct i2c_client *i2c) { const struct sc16is7xx_devtype *devtype; - struct regmap *regmaps[2]; + struct regmap *regmaps[SC16IS7XX_MAX_PORTS]; unsigned int i; devtype = (struct sc16is7xx_devtype *)i2c_get_match_data(i2c);