[PATCH v2 0/4] devres: Provide krealloc_array

[PATCH v2 0/4] devres: Provide krealloc_array

[James Clark]

Changes since v1:

 * Style fix

-----------------------

Hi,

I had a use for a devm realloc_array in a separate change, so I've
added one and updated all the obvious existing uses of it that I could
find. This is basically a copy paste of the one in slab.h

Applies to v6.3-rc1

Thanks
James


James Clark (4):
  devres: Provide krealloc_array
  hwmon: pmbus: Use devm_krealloc_array
  iio: adc: Use devm_krealloc_array
  serial: qcom_geni: Use devm_krealloc_array

 .../driver-api/driver-model/devres.rst          |  1 +
 drivers/hwmon/pmbus/pmbus_core.c                |  6 +++---
 drivers/iio/adc/xilinx-ams.c                    |  9 +++------
 drivers/iio/adc/xilinx-xadc-core.c              | 17 +++++++----------
 drivers/tty/serial/qcom_geni_serial.c           |  6 +++---
 include/linux/device.h                          | 10 ++++++++++
 6 files changed, 27 insertions(+), 22 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] devres: Provide krealloc_array

[James Clark]

There is no krealloc_array equivalent in devres. Users would have to
do their own multiplication overflow check so provide one.

Signed-off-by: James Clark <james.clark@arm.com>
---
 Documentation/driver-api/driver-model/devres.rst |  1 +
 include/linux/device.h                           | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index 4249eb4239e0..8be086b3f829 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -364,6 +364,7 @@ MEM
   devm_kmalloc_array()
   devm_kmemdup()
   devm_krealloc()
+  devm_krealloc_array()
   devm_kstrdup()
   devm_kstrdup_const()
   devm_kvasprintf()
diff --git a/include/linux/device.h b/include/linux/device.h
index 1508e637bb26..0dd5956c8516 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -223,6 +223,16 @@ static inline void *devm_kcalloc(struct device *dev,
 {
 	return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO);
 }
+static inline __realloc_size(3, 4) void * __must_check
+devm_krealloc_array(struct device *dev, void *p, size_t new_n, size_t new_size, gfp_t flags)
+{
+	size_t bytes;
+
+	if (unlikely(check_mul_overflow(new_n, new_size, &bytes)))
+		return NULL;
+	return devm_krealloc(dev, p, bytes, flags);
+}
+
 void devm_kfree(struct device *dev, const void *p);
 char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc;
 const char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp);
-- 
2.34.1

Re: [PATCH v2 1/4] devres: Provide krealloc_array

[Jonathan Cameron]

On Thu,  9 Mar 2023 15:03:30 +0000
James Clark <james.clark@arm.com> wrote:

> There is no krealloc_array equivalent in devres. Users would have to
> do their own multiplication overflow check so provide one.
> 
> Signed-off-by: James Clark <james.clark@arm.com>

Trivial comment inline, but otherwise seems reasonable to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  Documentation/driver-api/driver-model/devres.rst |  1 +
>  include/linux/device.h                           | 10 ++++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
> index 4249eb4239e0..8be086b3f829 100644
> --- a/Documentation/driver-api/driver-model/devres.rst
> +++ b/Documentation/driver-api/driver-model/devres.rst
> @@ -364,6 +364,7 @@ MEM
>    devm_kmalloc_array()
>    devm_kmemdup()
>    devm_krealloc()
> +  devm_krealloc_array()
>    devm_kstrdup()
>    devm_kstrdup_const()
>    devm_kvasprintf()
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 1508e637bb26..0dd5956c8516 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -223,6 +223,16 @@ static inline void *devm_kcalloc(struct device *dev,
>  {
>  	return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO);
>  }
> +static inline __realloc_size(3, 4) void * __must_check
> +devm_krealloc_array(struct device *dev, void *p, size_t new_n, size_t new_size, gfp_t flags)
> +{
> +	size_t bytes;
> +
> +	if (unlikely(check_mul_overflow(new_n, new_size, &bytes)))
> +		return NULL;

For consistency with krealloc_array and other stuff in device.h that is
of similar 'shape' add a blank line here.

> +	return devm_krealloc(dev, p, bytes, flags);
> +}
> +
>  void devm_kfree(struct device *dev, const void *p);
>  char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc;
>  const char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp);

[PATCH v2 2/4] hwmon: pmbus: Use devm_krealloc_array

[James Clark]

Now that it exists, use it instead of doing the multiplication manually.

Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: James Clark <james.clark@arm.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 95e95783972a..e7bee25a3706 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -1190,9 +1190,9 @@ static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
 {
 	if (data->num_attributes >= data->max_attributes - 1) {
 		int new_max_attrs = data->max_attributes + PMBUS_ATTR_ALLOC_SIZE;
-		void *new_attrs = devm_krealloc(data->dev, data->group.attrs,
-						new_max_attrs * sizeof(void *),
-						GFP_KERNEL);
+		void *new_attrs = devm_krealloc_array(data->dev, data->group.attrs,
+						      new_max_attrs, sizeof(void *),
+						      GFP_KERNEL);
 		if (!new_attrs)
 			return -ENOMEM;
 		data->group.attrs = new_attrs;
-- 
2.34.1

Re: [PATCH v2 2/4] hwmon: pmbus: Use devm_krealloc_array

[Jonathan Cameron]

On Thu,  9 Mar 2023 15:03:31 +0000
James Clark <james.clark@arm.com> wrote:

> Now that it exists, use it instead of doing the multiplication manually.
> 
> Acked-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: James Clark <james.clark@arm.com>
FWIW LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 95e95783972a..e7bee25a3706 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -1190,9 +1190,9 @@ static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
>  {
>  	if (data->num_attributes >= data->max_attributes - 1) {
>  		int new_max_attrs = data->max_attributes + PMBUS_ATTR_ALLOC_SIZE;
> -		void *new_attrs = devm_krealloc(data->dev, data->group.attrs,
> -						new_max_attrs * sizeof(void *),
> -						GFP_KERNEL);
> +		void *new_attrs = devm_krealloc_array(data->dev, data->group.attrs,
> +						      new_max_attrs, sizeof(void *),
> +						      GFP_KERNEL);
>  		if (!new_attrs)
>  			return -ENOMEM;
>  		data->group.attrs = new_attrs;

[PATCH v2 3/4] iio: adc: Use devm_krealloc_array

[James Clark]

Now that it exists, use it instead of doing the multiplication and
checking for overflow manually.

Signed-off-by: James Clark <james.clark@arm.com>
---
 drivers/iio/adc/xilinx-ams.c       |  9 +++------
 drivers/iio/adc/xilinx-xadc-core.c | 17 +++++++----------
 2 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
index 34cf336b3490..f0b71a1220e0 100644
--- a/drivers/iio/adc/xilinx-ams.c
+++ b/drivers/iio/adc/xilinx-ams.c
@@ -1263,7 +1263,7 @@ static int ams_parse_firmware(struct iio_dev *indio_dev)
 	struct device *dev = indio_dev->dev.parent;
 	struct fwnode_handle *child = NULL;
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
-	size_t ams_size, dev_size;
+	size_t ams_size;
 	int ret, ch_cnt = 0, i, rising_off, falling_off;
 	unsigned int num_channels = 0;
 
@@ -1320,11 +1320,8 @@ static int ams_parse_firmware(struct iio_dev *indio_dev)
 		}
 	}
 
-	dev_size = array_size(sizeof(*dev_channels), num_channels);
-	if (dev_size == SIZE_MAX)
-		return -ENOMEM;
-
-	dev_channels = devm_krealloc(dev, ams_channels, dev_size, GFP_KERNEL);
+	dev_channels = devm_krealloc_array(dev, ams_channels, num_channels,
+					   sizeof(*dev_channels), GFP_KERNEL);
 	if (!dev_channels)
 		return -ENOMEM;
 
diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 292f2892d223..dba73300f894 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -613,20 +613,17 @@ static int xadc_update_scan_mode(struct iio_dev *indio_dev,
 	const unsigned long *mask)
 {
 	struct xadc *xadc = iio_priv(indio_dev);
-	size_t new_size, n;
+	size_t n;
 	void *data;
 
 	n = bitmap_weight(mask, indio_dev->masklength);
 
-	if (check_mul_overflow(n, sizeof(*xadc->data), &new_size))
-		return -ENOMEM;
-
-	data = devm_krealloc(indio_dev->dev.parent, xadc->data,
-			     new_size, GFP_KERNEL);
+	data = devm_krealloc_array(indio_dev->dev.parent, xadc->data,
+				   n, sizeof(*xadc->data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	memset(data, 0, new_size);
+	memset(data, 0, n * sizeof(*xadc->data));
 	xadc->data = data;
 
 	return 0;
@@ -1281,9 +1278,9 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, unsigned int *conf, int irq)
 	}
 
 	indio_dev->num_channels = num_channels;
-	indio_dev->channels = devm_krealloc(dev, channels,
-					    sizeof(*channels) * num_channels,
-					    GFP_KERNEL);
+	indio_dev->channels = devm_krealloc_array(dev, channels,
+						  num_channels, sizeof(*channels),
+						  GFP_KERNEL);
 	/* If we can't resize the channels array, just use the original */
 	if (!indio_dev->channels)
 		indio_dev->channels = channels;
-- 
2.34.1

Re: [PATCH v2 3/4] iio: adc: Use devm_krealloc_array

[Jonathan Cameron]

On Thu,  9 Mar 2023 15:03:32 +0000
James Clark <james.clark@arm.com> wrote:

> Now that it exists, use it instead of doing the multiplication and
> checking for overflow manually.
> 
> Signed-off-by: James Clark <james.clark@arm.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/iio/adc/xilinx-ams.c       |  9 +++------
>  drivers/iio/adc/xilinx-xadc-core.c | 17 +++++++----------
>  2 files changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> index 34cf336b3490..f0b71a1220e0 100644
> --- a/drivers/iio/adc/xilinx-ams.c
> +++ b/drivers/iio/adc/xilinx-ams.c
> @@ -1263,7 +1263,7 @@ static int ams_parse_firmware(struct iio_dev *indio_dev)
>  	struct device *dev = indio_dev->dev.parent;
>  	struct fwnode_handle *child = NULL;
>  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> -	size_t ams_size, dev_size;
> +	size_t ams_size;
>  	int ret, ch_cnt = 0, i, rising_off, falling_off;
>  	unsigned int num_channels = 0;
>  
> @@ -1320,11 +1320,8 @@ static int ams_parse_firmware(struct iio_dev *indio_dev)
>  		}
>  	}
>  
> -	dev_size = array_size(sizeof(*dev_channels), num_channels);
> -	if (dev_size == SIZE_MAX)
> -		return -ENOMEM;
> -
> -	dev_channels = devm_krealloc(dev, ams_channels, dev_size, GFP_KERNEL);
> +	dev_channels = devm_krealloc_array(dev, ams_channels, num_channels,
> +					   sizeof(*dev_channels), GFP_KERNEL);
>  	if (!dev_channels)
>  		return -ENOMEM;
>  
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 292f2892d223..dba73300f894 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -613,20 +613,17 @@ static int xadc_update_scan_mode(struct iio_dev *indio_dev,
>  	const unsigned long *mask)
>  {
>  	struct xadc *xadc = iio_priv(indio_dev);
> -	size_t new_size, n;
> +	size_t n;
>  	void *data;
>  
>  	n = bitmap_weight(mask, indio_dev->masklength);
>  
> -	if (check_mul_overflow(n, sizeof(*xadc->data), &new_size))
> -		return -ENOMEM;
> -
> -	data = devm_krealloc(indio_dev->dev.parent, xadc->data,
> -			     new_size, GFP_KERNEL);
> +	data = devm_krealloc_array(indio_dev->dev.parent, xadc->data,
> +				   n, sizeof(*xadc->data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
> -	memset(data, 0, new_size);
> +	memset(data, 0, n * sizeof(*xadc->data));
>  	xadc->data = data;
>  
>  	return 0;
> @@ -1281,9 +1278,9 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, unsigned int *conf, int irq)
>  	}
>  
>  	indio_dev->num_channels = num_channels;
> -	indio_dev->channels = devm_krealloc(dev, channels,
> -					    sizeof(*channels) * num_channels,
> -					    GFP_KERNEL);
> +	indio_dev->channels = devm_krealloc_array(dev, channels,
> +						  num_channels, sizeof(*channels),
> +						  GFP_KERNEL);
>  	/* If we can't resize the channels array, just use the original */
>  	if (!indio_dev->channels)
>  		indio_dev->channels = channels;

[PATCH v2 4/4] serial: qcom_geni: Use devm_krealloc_array

[James Clark]

Now that it exists, use it instead of doing the multiplication manually.

Signed-off-by: James Clark <james.clark@arm.com>
---
 drivers/tty/serial/qcom_geni_serial.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index d69592e5e2ec..23fc33d182ac 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1056,9 +1056,9 @@ static int setup_fifos(struct qcom_geni_serial_port *port)
 		(port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE;
 
 	if (port->rx_buf && (old_rx_fifo_depth != port->rx_fifo_depth) && port->rx_fifo_depth) {
-		port->rx_buf = devm_krealloc(uport->dev, port->rx_buf,
-					     port->rx_fifo_depth * sizeof(u32),
-					     GFP_KERNEL);
+		port->rx_buf = devm_krealloc_array(uport->dev, port->rx_buf,
+						   port->rx_fifo_depth, sizeof(u32),
+						   GFP_KERNEL);
 		if (!port->rx_buf)
 			return -ENOMEM;
 	}
-- 
2.34.1

Re: [PATCH v2 4/4] serial: qcom_geni: Use devm_krealloc_array

[Jonathan Cameron]

On Thu,  9 Mar 2023 15:03:33 +0000
James Clark <james.clark@arm.com> wrote:

> Now that it exists, use it instead of doing the multiplication manually.
> 
> Signed-off-by: James Clark <james.clark@arm.com>

Hmm. I've stared at the users of this for a bit, and it's not actually obvious
that it's being used as an array of u32.  The only typed user of this is as
the 2nd parameter of  
tty_insert_flip_string() which is an unsigned char *

I wonder if that sizeof(u32) isn't a 'correct' description of where the 4 is coming
from even if it has the right value?  Perhaps the fifo depth is just a multiple of 4?

Jonathan



> ---
>  drivers/tty/serial/qcom_geni_serial.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index d69592e5e2ec..23fc33d182ac 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1056,9 +1056,9 @@ static int setup_fifos(struct qcom_geni_serial_port *port)
>  		(port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE;
>  
>  	if (port->rx_buf && (old_rx_fifo_depth != port->rx_fifo_depth) && port->rx_fifo_depth) {
> -		port->rx_buf = devm_krealloc(uport->dev, port->rx_buf,
> -					     port->rx_fifo_depth * sizeof(u32),
> -					     GFP_KERNEL);
> +		port->rx_buf = devm_krealloc_array(uport->dev, port->rx_buf,
> +						   port->rx_fifo_depth, sizeof(u32),
> +						   GFP_KERNEL);
>  		if (!port->rx_buf)
>  			return -ENOMEM;
>  	}

Re: [PATCH v2 4/4] serial: qcom_geni: Use devm_krealloc_array

[James Clark]

On 11/03/2023 19:18, Jonathan Cameron wrote:
> On Thu,  9 Mar 2023 15:03:33 +0000
> James Clark <james.clark@arm.com> wrote:
> 
>> Now that it exists, use it instead of doing the multiplication manually.
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
> 
> Hmm. I've stared at the users of this for a bit, and it's not actually obvious
> that it's being used as an array of u32.  The only typed user of this is as
> the 2nd parameter of  
> tty_insert_flip_string() which is an unsigned char *
> 
> I wonder if that sizeof(u32) isn't a 'correct' description of where the 4 is coming
> from even if it has the right value?  Perhaps the fifo depth is just a multiple of 4?
> 
> Jonathan
> 

The commit that added it (b8caf69a6946) seems to hint that something
reads from it in words. And I see this:

  /* We always configure 4 bytes per FIFO word */
  #define BYTES_PER_FIFO_WORD		4U

Perhaps sizeof(u32) isn't as accurate of a description as using
BYTES_PER_FIFO_WORD but I'd be reluctant to make a change because I
don't really understand the implications.

There is also this in handle_rx_console():

  unsigned char buf[sizeof(u32)];

James

> 
> 
>> ---
>>  drivers/tty/serial/qcom_geni_serial.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>> index d69592e5e2ec..23fc33d182ac 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -1056,9 +1056,9 @@ static int setup_fifos(struct qcom_geni_serial_port *port)
>>  		(port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE;
>>  
>>  	if (port->rx_buf && (old_rx_fifo_depth != port->rx_fifo_depth) && port->rx_fifo_depth) {
>> -		port->rx_buf = devm_krealloc(uport->dev, port->rx_buf,
>> -					     port->rx_fifo_depth * sizeof(u32),
>> -					     GFP_KERNEL);
>> +		port->rx_buf = devm_krealloc_array(uport->dev, port->rx_buf,
>> +						   port->rx_fifo_depth, sizeof(u32),
>> +						   GFP_KERNEL);
>>  		if (!port->rx_buf)
>>  			return -ENOMEM;
>>  	}
> 

Re: [PATCH v2 4/4] serial: qcom_geni: Use devm_krealloc_array

[Jonathan Cameron]

On Fri, 17 Mar 2023 11:34:49 +0000
James Clark <james.clark@arm.com> wrote:

> On 11/03/2023 19:18, Jonathan Cameron wrote:
> > On Thu,  9 Mar 2023 15:03:33 +0000
> > James Clark <james.clark@arm.com> wrote:
> >   
> >> Now that it exists, use it instead of doing the multiplication manually.
> >>
> >> Signed-off-by: James Clark <james.clark@arm.com>  
> > 
> > Hmm. I've stared at the users of this for a bit, and it's not actually obvious
> > that it's being used as an array of u32.  The only typed user of this is as
> > the 2nd parameter of  
> > tty_insert_flip_string() which is an unsigned char *
> > 
> > I wonder if that sizeof(u32) isn't a 'correct' description of where the 4 is coming
> > from even if it has the right value?  Perhaps the fifo depth is just a multiple of 4?
> > 
> > Jonathan
> >   
> 
> The commit that added it (b8caf69a6946) seems to hint that something
> reads from it in words. And I see this:
> 
>   /* We always configure 4 bytes per FIFO word */
>   #define BYTES_PER_FIFO_WORD		4U
> 
> Perhaps sizeof(u32) isn't as accurate of a description as using
> BYTES_PER_FIFO_WORD but I'd be reluctant to make a change because I
> don't really understand the implications.

Agreed with your analysis.  + fully understand why you don't want to change
it. 

I'd be tempted to take the view that whilst it's allocated in 4 byte chunks
because it's accessed elsewhere as a set of 1 byte entries, krealloc_array
isn't appropriate and so just leave it with devm_krealloc()

Risk is that a steady stream of patches will turn up 'fixing' this as
it will be easy for people to find with a script.  Maybe better to just add
a comment (either with or without your patch).
> 
> There is also this in handle_rx_console():
> 
>   unsigned char buf[sizeof(u32)];
> 
> James
> 
> > 
> >   
> >> ---
> >>  drivers/tty/serial/qcom_geni_serial.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> >> index d69592e5e2ec..23fc33d182ac 100644
> >> --- a/drivers/tty/serial/qcom_geni_serial.c
> >> +++ b/drivers/tty/serial/qcom_geni_serial.c
> >> @@ -1056,9 +1056,9 @@ static int setup_fifos(struct qcom_geni_serial_port *port)
> >>  		(port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE;
> >>  
> >>  	if (port->rx_buf && (old_rx_fifo_depth != port->rx_fifo_depth) && port->rx_fifo_depth) {
> >> -		port->rx_buf = devm_krealloc(uport->dev, port->rx_buf,
> >> -					     port->rx_fifo_depth * sizeof(u32),
> >> -					     GFP_KERNEL);
> >> +		port->rx_buf = devm_krealloc_array(uport->dev, port->rx_buf,
> >> +						   port->rx_fifo_depth, sizeof(u32),
> >> +						   GFP_KERNEL);
> >>  		if (!port->rx_buf)
> >>  			return -ENOMEM;
> >>  	}  
> >   

Re: [PATCH v2 4/4] serial: qcom_geni: Use devm_krealloc_array

[James Clark]

On 18/03/2023 17:34, Jonathan Cameron wrote:
> On Fri, 17 Mar 2023 11:34:49 +0000
> James Clark <james.clark@arm.com> wrote:
> 
>> On 11/03/2023 19:18, Jonathan Cameron wrote:
>>> On Thu,  9 Mar 2023 15:03:33 +0000
>>> James Clark <james.clark@arm.com> wrote:
>>>   
>>>> Now that it exists, use it instead of doing the multiplication manually.
>>>>
>>>> Signed-off-by: James Clark <james.clark@arm.com>  
>>>
>>> Hmm. I've stared at the users of this for a bit, and it's not actually obvious
>>> that it's being used as an array of u32.  The only typed user of this is as
>>> the 2nd parameter of  
>>> tty_insert_flip_string() which is an unsigned char *
>>>
>>> I wonder if that sizeof(u32) isn't a 'correct' description of where the 4 is coming
>>> from even if it has the right value?  Perhaps the fifo depth is just a multiple of 4?
>>>
>>> Jonathan
>>>   
>>
>> The commit that added it (b8caf69a6946) seems to hint that something
>> reads from it in words. And I see this:
>>
>>   /* We always configure 4 bytes per FIFO word */
>>   #define BYTES_PER_FIFO_WORD		4U
>>
>> Perhaps sizeof(u32) isn't as accurate of a description as using
>> BYTES_PER_FIFO_WORD but I'd be reluctant to make a change because I
>> don't really understand the implications.
> 
> Agreed with your analysis.  + fully understand why you don't want to change
> it. 
> 
> I'd be tempted to take the view that whilst it's allocated in 4 byte chunks
> because it's accessed elsewhere as a set of 1 byte entries, krealloc_array
> isn't appropriate and so just leave it with devm_krealloc()
> 
> Risk is that a steady stream of patches will turn up 'fixing' this as
> it will be easy for people to find with a script.  Maybe better to just add
> a comment (either with or without your patch).

Ok that makes sense to me. I can add a comment instead this patch to
change this one.

>>
>> There is also this in handle_rx_console():
>>
>>   unsigned char buf[sizeof(u32)];
>>
>> James
>>
>>>
>>>   
>>>> ---
>>>>  drivers/tty/serial/qcom_geni_serial.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>>>> index d69592e5e2ec..23fc33d182ac 100644
>>>> --- a/drivers/tty/serial/qcom_geni_serial.c
>>>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>>>> @@ -1056,9 +1056,9 @@ static int setup_fifos(struct qcom_geni_serial_port *port)
>>>>  		(port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE;
>>>>  
>>>>  	if (port->rx_buf && (old_rx_fifo_depth != port->rx_fifo_depth) && port->rx_fifo_depth) {
>>>> -		port->rx_buf = devm_krealloc(uport->dev, port->rx_buf,
>>>> -					     port->rx_fifo_depth * sizeof(u32),
>>>> -					     GFP_KERNEL);
>>>> +		port->rx_buf = devm_krealloc_array(uport->dev, port->rx_buf,
>>>> +						   port->rx_fifo_depth, sizeof(u32),
>>>> +						   GFP_KERNEL);
>>>>  		if (!port->rx_buf)
>>>>  			return -ENOMEM;
>>>>  	}  
>>>   
>