[PATCH 18/44] drivers/gpio: use min() instead of min_t()

david.laight.linux@gmail.com posted 44 patches 1 week, 5 days ago
There is a newer version of this series
[PATCH 18/44] drivers/gpio: use min() instead of min_t()
Posted by david.laight.linux@gmail.com 1 week, 5 days ago
From: David Laight <david.laight.linux@gmail.com>

min_t(u16, a, b) casts an 'unsigned long' to 'u16'.
Use min(a, b) instead as it promotes the both values to int
and so cannot discard significant bits.

In this case the values should be ok.

Detected by an extra check added to min_t().

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 drivers/gpio/gpiolib-acpi-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
index d441c1236d8c..83dd227dbbec 100644
--- a/drivers/gpio/gpiolib-acpi-core.c
+++ b/drivers/gpio/gpiolib-acpi-core.c
@@ -1099,7 +1099,7 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 		return AE_BAD_PARAMETER;
 	}
 
-	length = min_t(u16, agpio->pin_table_length, pin_index + bits);
+	length = min(agpio->pin_table_length, pin_index + bits);
 	for (i = pin_index; i < length; ++i) {
 		unsigned int pin = agpio->pin_table[i];
 		struct acpi_gpio_connection *conn;
-- 
2.39.5
Re: [PATCH 18/44] drivers/gpio: use min() instead of min_t()
Posted by Andy Shevchenko 1 week, 4 days ago
On Wed, Nov 19, 2025 at 10:41:14PM +0000, david.laight.linux@gmail.com wrote:
> 
> min_t(u16, a, b) casts an 'unsigned long' to 'u16'.
> Use min(a, b) instead as it promotes the both values to int
> and so cannot discard significant bits.
> 
> In this case the values should be ok.
> 
> Detected by an extra check added to min_t().

In most of the patches you need to follow the commonly used Subject prefix.
This can be done by doing

	git log --oneline --no-merges -- $FILE(S)_OF_INTEREST

For example,

$ git log --no-decorate --oneline --no-merges -- drivers/gpio/gpiolib-acpi*
b1055678a016 gpiolib: acpi: Use %pe when passing an error pointer to dev_err()
e4a77f9c85a5 gpiolib: acpi: Make set debounce errors non fatal
19c839a98c73 gpiolib: acpi: initialize acpi_gpio_info struct
3712ce9fa501 gpiolib: acpi: Ignore touchpad wakeup on GPD G1619-05
16c07342b542 gpiolib: acpi: Program debounce when finding GPIO
23800ad1265f gpiolib: acpi: Add quirk for ASUS ProArt PX13
...


> acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,

> -	length = min_t(u16, agpio->pin_table_length, pin_index + bits);
> +	length = min(agpio->pin_table_length, pin_index + bits);

Now, if you look closer at the code, the pin_index alone has the problem you
are targeting here. On top of that the iterator and 'length' are signed, while
the result of min_t(u16) is unsigned (however it has no difference in this case).

...

TL;DR: I apply this patch with subject changed, but I think more work needs to
be done if you want to fix it fully.

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH 18/44] drivers/gpio: use min() instead of min_t()
Posted by David Laight 1 week, 4 days ago
On Thu, 20 Nov 2025 10:01:29 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Nov 19, 2025 at 10:41:14PM +0000, david.laight.linux@gmail.com wrote:
> > 
> > min_t(u16, a, b) casts an 'unsigned long' to 'u16'.
> > Use min(a, b) instead as it promotes the both values to int
> > and so cannot discard significant bits.
> > 
> > In this case the values should be ok.
> > 
> > Detected by an extra check added to min_t().  
> 
> In most of the patches you need to follow the commonly used Subject prefix.
> This can be done by doing

I did look up quite a few files to see what had been used previously.
But it is a bit tedious with 44 patches.

> 	git log --oneline --no-merges -- $FILE(S)_OF_INTEREST

I wasn't aware of that spell :-)

...
> 
> > acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,  
> 
> > -	length = min_t(u16, agpio->pin_table_length, pin_index + bits);
> > +	length = min(agpio->pin_table_length, pin_index + bits);  
> 
> Now, if you look closer at the code, the pin_index alone has the problem you
> are targeting here.

The compiler warning happens because 'pin_index + bits' is 'int' and the compiler
doesn't know the value fits in 16 bits.
It should fit, but only if the caller passes in valid data.

> On top of that the iterator and 'length' are signed, while
> the result of min_t(u16) is unsigned (however it has no difference in this case).

Actually the result type of min_t(u16) is 'int' (:? promotes char/short to int).
So the u16 cast does '(pin_index + bits) & 0xffff', everything is then promoted
to 'int' for all the comparisons (etc).

	David

> 
> ...
> 
> TL;DR: I apply this patch with subject changed, but I think more work needs to
> be done if you want to fix it fully.
>
Re: [PATCH 18/44] drivers/gpio: use min() instead of min_t()
Posted by Andy Shevchenko 6 days, 16 hours ago
On Thu, Nov 20, 2025 at 09:37:43AM +0000, David Laight wrote:
> On Thu, 20 Nov 2025 10:01:29 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Nov 19, 2025 at 10:41:14PM +0000, david.laight.linux@gmail.com wrote:
> > > 
> > > min_t(u16, a, b) casts an 'unsigned long' to 'u16'.
> > > Use min(a, b) instead as it promotes the both values to int
> > > and so cannot discard significant bits.
> > > 
> > > In this case the values should be ok.
> > > 
> > > Detected by an extra check added to min_t().  

...

> > > acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,  
> > 
> > > -	length = min_t(u16, agpio->pin_table_length, pin_index + bits);
> > > +	length = min(agpio->pin_table_length, pin_index + bits);  
> > 
> > Now, if you look closer at the code, the pin_index alone has the problem you
> > are targeting here.
> 
> The compiler warning happens because 'pin_index + bits' is 'int' and the compiler
> doesn't know the value fits in 16 bits.
> It should fit, but only if the caller passes in valid data.

I meant that assignment to pin_index already cuts the higher bits
from the input.

> > On top of that the iterator and 'length' are signed, while
> > the result of min_t(u16) is unsigned (however it has no difference in this case).
> 
> Actually the result type of min_t(u16) is 'int' (:? promotes char/short to int).
> So the u16 cast does '(pin_index + bits) & 0xffff', everything is then promoted
> to 'int' for all the comparisons (etc).

Sure, but the value is positive even if int is signed. That's why I put
a remark in the parentheses that it has no difference in this case.

...

> > TL;DR: I apply this patch with subject changed, but I think more work needs to
> > be done if you want to fix it fully.

-- 
With Best Regards,
Andy Shevchenko