[PATCH v2] iio: adc: ti-ads7138: replace kmalloc() with stack allocation in i2c_write_block

[PATCH v2] iio: adc: ti-ads7138: replace kmalloc() with stack allocation in i2c_write_block

[Giorgi Tchankvetadze]

The ads7138_i2c_write_block() function currently utilizes kmalloc()
to allocate a buffer for I2C transfers. However, the length
parameter passed to this function is strictly 2 bytes across all
driver invocations, making the total payload buffer size exactly 4 bytes.
Invoking the heap allocator for a 4-byte buffer introduces
unnecessary SLUB overhead. 

Replace the kmalloc() call with a statically sized 4-byte stack array.
Add a boundary check returning -EINVAL to ensure future driver
modifications do not overflow the stack buffer if a length
greater than 2 is requested.
Furthermore, dropping the dynamic allocation entirely removes the need to
include <linux/slab.h> (which was the original motivation for this patch)
and streamlines the PIO transfer path.

Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Giorgi Tchankvetadze <giorgitchankvetadze1997@gmail.com>
---
 drivers/iio/adc/ti-ads7138.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/ti-ads7138.c b/drivers/iio/adc/ti-ads7138.c
index ee5c1b8e3a8e..172893cbc17d 100644
--- a/drivers/iio/adc/ti-ads7138.c
+++ b/drivers/iio/adc/ti-ads7138.c
@@ -102,21 +102,20 @@ static const int ads7138_oversampling_ratios[] = {
 static int ads7138_i2c_write_block(const struct i2c_client *client, u8 reg,
 				   u8 *values, u8 length)
 {
+	u8 buf[4];
 	int ret;
-	int len = length + 2; /* "+ 2" for OPCODE and reg */
 
-	u8 *buf __free(kfree) = kmalloc(len, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
+	if (length != 2)
+		return -EINVAL;
 
 	buf[0] = ADS7138_OPCODE_BLOCK_WRITE;
 	buf[1] = reg;
 	memcpy(&buf[2], values, length);
 
-	ret = i2c_master_send(client, buf, len);
+	ret = i2c_master_send(client, buf, ARRAY_SIZE(buf));
 	if (ret < 0)
 		return ret;
-	if (ret != len)
+	if (ret != ARRAY_SIZE(buf))
 		return -EIO;
 
 	return 0;
-- 
2.52.0

Re: [PATCH v2] iio: adc: ti-ads7138: replace kmalloc() with stack allocation in i2c_write_block

[David Laight]

On Mon, 27 Apr 2026 15:27:07 +0400
Giorgi Tchankvetadze <giorgitchankvetadze1997@gmail.com> wrote:

> The ads7138_i2c_write_block() function currently utilizes kmalloc()
> to allocate a buffer for I2C transfers. However, the length
> parameter passed to this function is strictly 2 bytes across all
> driver invocations, making the total payload buffer size exactly 4 bytes.
> Invoking the heap allocator for a 4-byte buffer introduces
> unnecessary SLUB overhead. 

Have you confirmed that the buffer is never used for DMA?

Provided the lock that blocks concurrent access from two threads
is actually outside this code, a buffer for short transfers could
be allocated within 'struct i2c_client'.

	David

> 
> Replace the kmalloc() call with a statically sized 4-byte stack array.
> Add a boundary check returning -EINVAL to ensure future driver
> modifications do not overflow the stack buffer if a length
> greater than 2 is requested.
> Furthermore, dropping the dynamic allocation entirely removes the need to
> include <linux/slab.h> (which was the original motivation for this patch)
> and streamlines the PIO transfer path.
> 
> Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Giorgi Tchankvetadze <giorgitchankvetadze1997@gmail.com>
> ---
>  drivers/iio/adc/ti-ads7138.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads7138.c b/drivers/iio/adc/ti-ads7138.c
> index ee5c1b8e3a8e..172893cbc17d 100644
> --- a/drivers/iio/adc/ti-ads7138.c
> +++ b/drivers/iio/adc/ti-ads7138.c
> @@ -102,21 +102,20 @@ static const int ads7138_oversampling_ratios[] = {
>  static int ads7138_i2c_write_block(const struct i2c_client *client, u8 reg,
>  				   u8 *values, u8 length)
>  {
> +	u8 buf[4];
>  	int ret;
> -	int len = length + 2; /* "+ 2" for OPCODE and reg */
>  
> -	u8 *buf __free(kfree) = kmalloc(len, GFP_KERNEL);
> -	if (!buf)
> -		return -ENOMEM;
> +	if (length != 2)
> +		return -EINVAL;
>  
>  	buf[0] = ADS7138_OPCODE_BLOCK_WRITE;
>  	buf[1] = reg;
>  	memcpy(&buf[2], values, length);
>  
> -	ret = i2c_master_send(client, buf, len);
> +	ret = i2c_master_send(client, buf, ARRAY_SIZE(buf));
>  	if (ret < 0)
>  		return ret;
> -	if (ret != len)
> +	if (ret != ARRAY_SIZE(buf))
>  		return -EIO;
>  
>  	return 0;

Re: [PATCH v2] iio: adc: ti-ads7138: replace kmalloc() with stack allocation in i2c_write_block

[David Lechner]

On 4/27/26 8:31 AM, David Laight wrote:
> On Mon, 27 Apr 2026 15:27:07 +0400
> Giorgi Tchankvetadze <giorgitchankvetadze1997@gmail.com> wrote:
> 
>> The ads7138_i2c_write_block() function currently utilizes kmalloc()
>> to allocate a buffer for I2C transfers. However, the length
>> parameter passed to this function is strictly 2 bytes across all
>> driver invocations, making the total payload buffer size exactly 4 bytes.
>> Invoking the heap allocator for a 4-byte buffer introduces
>> unnecessary SLUB overhead. 
> 
> Have you confirmed that the buffer is never used for DMA?
> 
> Provided the lock that blocks concurrent access from two threads
> is actually outside this code, a buffer for short transfers could
> be allocated within 'struct i2c_client'.
> 
> 	David
> 
Giorgi,

This is why it is important to include a changelog and a link to the
previous discussion [1] in the cover letter or after --- in a single patch
when you submit a new revision. I think that would have answered David's
question.

[1]: https://lore.kernel.org/linux-iio/20260424081809.61841-2-giorgitchankvetadze1997@gmail.com/

Re: [PATCH v2] iio: adc: ti-ads7138: replace kmalloc() with stack allocation in i2c_write_block

[Giorgi Tchankvetadze]

On Mon, Apr 27, 2026 at 7:21 PM David Lechner <dlechner@baylibre.com> wrote:
>
> Giorgi,
>
> This is why it is important to include a changelog and a link to the
> previous discussion [1] in the cover letter or after --- in a single patch
> when you submit a new revision. I think that would have answered David's
> question.
>
> [1]: https://lore.kernel.org/linux-iio/20260424081809.61841-2-giorgitchankvetadze1997@gmail.com/
>
>

Yes, I see it now. Thanks for pointing this out.

I’ll include the revision changelog and the link to the previous
discussion in v3.


Giorgi

Re: [PATCH v2] iio: adc: ti-ads7138: replace kmalloc() with stack allocation in i2c_write_block

[David Laight]

On Mon, 27 Apr 2026 10:21:37 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 4/27/26 8:31 AM, David Laight wrote:
> > On Mon, 27 Apr 2026 15:27:07 +0400
> > Giorgi Tchankvetadze <giorgitchankvetadze1997@gmail.com> wrote:
> >   
> >> The ads7138_i2c_write_block() function currently utilizes kmalloc()
> >> to allocate a buffer for I2C transfers. However, the length
> >> parameter passed to this function is strictly 2 bytes across all
> >> driver invocations, making the total payload buffer size exactly 4 bytes.
> >> Invoking the heap allocator for a 4-byte buffer introduces
> >> unnecessary SLUB overhead.   
> > 
> > Have you confirmed that the buffer is never used for DMA?
> > 
> > Provided the lock that blocks concurrent access from two threads
> > is actually outside this code, a buffer for short transfers could
> > be allocated within 'struct i2c_client'.
> > 
> > 	David
> >   
> Giorgi,
> 
> This is why it is important to include a changelog and a link to the
> previous discussion [1] in the cover letter or after --- in a single patch
> when you submit a new revision. I think that would have answered David's
> question.
> 
> [1]: https://lore.kernel.org/linux-iio/20260424081809.61841-2-giorgitchankvetadze1997@gmail.com/
> 
> 

The thing is I remember issues with on-stack buffers being used for
dma - which worked before kernel stacks were allocated using vmalloc().
One of the solutions to that is to use kmalloc() for buffers that
could be on stack.

I think I found exactly 1 call to this function.
Indeed, just inlining the logic would make everything clearer.

	David

Re: [PATCH v2] iio: adc: ti-ads7138: replace kmalloc() with stack allocation in i2c_write_block

[Jonathan Cameron]

On Tue, 28 Apr 2026 10:36:17 +0100
David Laight <david.laight.linux@gmail.com> wrote:

> On Mon, 27 Apr 2026 10:21:37 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
> > On 4/27/26 8:31 AM, David Laight wrote:  
> > > On Mon, 27 Apr 2026 15:27:07 +0400
> > > Giorgi Tchankvetadze <giorgitchankvetadze1997@gmail.com> wrote:
> > >     
> > >> The ads7138_i2c_write_block() function currently utilizes kmalloc()
> > >> to allocate a buffer for I2C transfers. However, the length
> > >> parameter passed to this function is strictly 2 bytes across all
> > >> driver invocations, making the total payload buffer size exactly 4 bytes.
> > >> Invoking the heap allocator for a 4-byte buffer introduces
> > >> unnecessary SLUB overhead.     
> > > 
> > > Have you confirmed that the buffer is never used for DMA?
> > > 
> > > Provided the lock that blocks concurrent access from two threads
> > > is actually outside this code, a buffer for short transfers could
> > > be allocated within 'struct i2c_client'.
> > > 
> > > 	David
> > >     
> > Giorgi,
> > 
> > This is why it is important to include a changelog and a link to the
> > previous discussion [1] in the cover letter or after --- in a single patch
> > when you submit a new revision. I think that would have answered David's
> > question.
> > 
> > [1]: https://lore.kernel.org/linux-iio/20260424081809.61841-2-giorgitchankvetadze1997@gmail.com/
> > 
> >   
> 
> The thing is I remember issues with on-stack buffers being used for
> dma - which worked before kernel stacks were allocated using vmalloc().

That is a problem for some buses that define their basic bus access functions
to require DMA (and regmap on top of those).  E.g. SPI where your comment
would have been absolutely correct.

I2C does the opposite. Everything is bounced through DMA safe buffers unless
you explicitly opt in to say you are providing a DMA safe buffer.
That is only worth doing if the buffers are large and the bounce therefore
expensive.

Whilst we are here - something that has me doubting myself so I'll just
lay it out.

There is a fun corner where Sashiko is giving a false positive (I think
anyway!) in that a pair of rx/tx buffers can sit in the same cacheline
as long as no one modifies either whilst a bus transaction is in flight
and so the DMA hasn't yet occurred in both directions. This has the fun
side effect of part of the cacheline in the non coherent DMA management
code being marked for each direction. However, the mechanism for corruption
doesn't apply so it should be fine.  A common situation is where rx and
tx are the same buffer which is an extreme case of this and absolutely fine.

If I have this wrong we have a lot of broken drivers - so hopefully not!

> One of the solutions to that is to use kmalloc() for buffers that
> could be on stack.

Correct, just not needed here.

Jonathan

> 
> I think I found exactly 1 call to this function.
> Indeed, just inlining the logic would make everything clearer.
> 
> 	David

Re: [PATCH v2] iio: adc: ti-ads7138: replace kmalloc() with stack allocation in i2c_write_block

[Andy Shevchenko]

On Mon, Apr 27, 2026 at 03:27:07PM +0400, Giorgi Tchankvetadze wrote:
> The ads7138_i2c_write_block() function currently utilizes kmalloc()
> to allocate a buffer for I2C transfers. However, the length
> parameter passed to this function is strictly 2 bytes across all
> driver invocations, making the total payload buffer size exactly 4 bytes.
> Invoking the heap allocator for a 4-byte buffer introduces

> unnecessary SLUB overhead. 

Trailing space.

> Replace the kmalloc() call with a statically sized 4-byte stack array.
> Add a boundary check returning -EINVAL to ensure future driver
> modifications do not overflow the stack buffer if a length
> greater than 2 is requested.
> Furthermore, dropping the dynamic allocation entirely removes the need to
> include <linux/slab.h> (which was the original motivation for this patch)
> and streamlines the PIO transfer path.

...

> -	ret = i2c_master_send(client, buf, len);
> +	ret = i2c_master_send(client, buf, ARRAY_SIZE(buf));

The 'buf' is of u8 type, hence sizeof() is enough.

...

> -	if (ret != len)
> +	if (ret != ARRAY_SIZE(buf))

Ditto.

-- 
With Best Regards,
Andy Shevchenko