[PATCH v1 0/2] gpiolib: acpi: Fix missing info filling

[PATCH v1 0/2] gpiolib: acpi: Fix missing info filling

[Andy Shevchenko]

Kees reported that code, while being refactored, missed the point of
filling the info structure which supplies GPIO flags to the upper layer.
Indeed, without that part the GPIO expander get no IRQ on Intel Edison,
for example. Fix this in this series.

Andy Shevchenko (2):
  gpiolib: acpi: Use temporary variable for struct acpi_gpio_info
  gpiolib: acpi: Make sure we fill struct acpi_gpio_info

 drivers/gpio/gpiolib-acpi.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

-- 
2.47.2

[PATCH v1 1/2] gpiolib: acpi: Use temporary variable for struct acpi_gpio_info

[Andy Shevchenko]

Use temporary variable to access the struct acpi_gpio_info members.
This will help further changes to be cleaner. No functional change
intended.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index bebb74528cfe..5b6344f0d065 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -752,6 +752,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 {
 	struct acpi_gpio_lookup *lookup = data;
 	struct acpi_gpio_params *params = &lookup->params;
+	struct acpi_gpio_info *info = &lookup->info;
 
 	if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
 		return 1;
@@ -762,7 +763,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 		struct gpio_desc *desc;
 		u16 pin_index;
 
-		if (lookup->info.quirks & ACPI_GPIO_QUIRK_ONLY_GPIOIO && gpioint)
+		if (info->quirks & ACPI_GPIO_QUIRK_ONLY_GPIOIO && gpioint)
 			params->crs_entry_index++;
 
 		if (lookup->n++ != params->crs_entry_index)
@@ -772,16 +773,16 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 		if (pin_index >= agpio->pin_table_length)
 			return 1;
 
-		if (lookup->info.quirks & ACPI_GPIO_QUIRK_ABSOLUTE_NUMBER)
+		if (info->quirks & ACPI_GPIO_QUIRK_ABSOLUTE_NUMBER)
 			desc = gpio_to_desc(agpio->pin_table[pin_index]);
 		else
 			desc = acpi_get_gpiod(agpio->resource_source.string_ptr,
 					      agpio->pin_table[pin_index]);
 		lookup->desc = desc;
-		lookup->info.pin_config = agpio->pin_config;
-		lookup->info.debounce = agpio->debounce_timeout;
-		lookup->info.gpioint = gpioint;
-		lookup->info.wake_capable = acpi_gpio_irq_is_wake(&lookup->info.adev->dev, agpio);
+		info->pin_config = agpio->pin_config;
+		info->debounce = agpio->debounce_timeout;
+		info->gpioint = gpioint;
+		info->wake_capable = acpi_gpio_irq_is_wake(&info->adev->dev, agpio);
 
 		/*
 		 * Polarity and triggering are only specified for GpioInt
@@ -790,14 +791,14 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 		 * - ACPI_ACTIVE_LOW == GPIO_ACTIVE_LOW
 		 * - ACPI_ACTIVE_HIGH == GPIO_ACTIVE_HIGH
 		 */
-		if (lookup->info.gpioint) {
-			lookup->info.polarity = agpio->polarity;
-			lookup->info.triggering = agpio->triggering;
+		if (info->gpioint) {
+			info->polarity = agpio->polarity;
+			info->triggering = agpio->triggering;
 		} else {
-			lookup->info.polarity = params->active_low;
+			info->polarity = params->active_low;
 		}
 
-		lookup->info.flags = acpi_gpio_to_gpiod_flags(agpio, lookup->info.polarity);
+		info->flags = acpi_gpio_to_gpiod_flags(agpio, info->polarity);
 	}
 
 	return 1;
@@ -805,7 +806,8 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 
 static int acpi_gpio_resource_lookup(struct acpi_gpio_lookup *lookup)
 {
-	struct acpi_device *adev = lookup->info.adev;
+	struct acpi_gpio_info *info = &lookup->info;
+	struct acpi_device *adev = info->adev;
 	struct list_head res_list;
 	int ret;
 
@@ -830,6 +832,7 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode, const char *p
 {
 	struct fwnode_reference_args args;
 	struct acpi_gpio_params *params = &lookup->params;
+	struct acpi_gpio_info *info = &lookup->info;
 	unsigned int index = params->crs_entry_index;
 	unsigned int quirks = 0;
 	int ret;
@@ -857,8 +860,8 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode, const char *p
 	params->line_index = args.args[1];
 	params->active_low = !!args.args[2];
 
-	lookup->info.adev = to_acpi_device_node(args.fwnode);
-	lookup->info.quirks = quirks;
+	info->adev = to_acpi_device_node(args.fwnode);
+	info->quirks = quirks;
 
 	return 0;
 }
-- 
2.47.2

[PATCH v1 2/2] gpiolib: acpi: Make sure we fill struct acpi_gpio_info

[Andy Shevchenko]

The previous refactoring missed the filling of the struct acpi_gpio_info
and that's how the lot of the code got eliminated. Restore those pieces
by passing the pointer all down in the call stack.

With this, the code grows by ~6%, but in conjunction with the previous
refactoring it still gives -387 bytes

add/remove: 2/0 grow/shrink: 5/1 up/down: 852/-35 (817)
Function                                     old     new   delta
acpi_dev_gpio_irq_wake_get_by                129     695    +566
acpi_find_gpio                               216     354    +138
acpi_find_gpio.__UNIQUE_ID_ddebug504           -      56     +56
acpi_dev_gpio_irq_wake_get_by.__UNIQUE_ID_ddebug506       -      56     +56
acpi_populate_gpio_lookup                    536     548     +12
acpi_gpio_property_lookup                    414     426     +12
acpi_get_gpiod_by_index                      307     319     +12
__acpi_find_gpio                             638     603     -35
Total: Before=14154, After=14971, chg +5.77%

As a positive side effect, it improves memory footprint for
struct acpi_gpio_lookup. `pahole` difference before and after:

-       /* size: 64, cachelines: 1, members: 4 */
-       /* member types with holes: 1, total: 1 */

+       /* size: 32, cachelines: 1, members: 4 */

Reported-by: Kees Bakker <kees@ijzerbout.nl>
Closes: https://lore.kernel.org/r/9715c8dd-38df-48fd-a9d1-7a78163dc989@ijzerbout.nl
Fixes: 8b4f52ef7a41 ("gpiolib: acpi: Deduplicate some code in __acpi_find_gpio()")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 5b6344f0d065..2ac9c7b31908 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -742,8 +742,8 @@ static int acpi_gpio_update_gpiod_lookup_flags(unsigned long *lookupflags,
 }
 
 struct acpi_gpio_lookup {
-	struct acpi_gpio_info info;
 	struct acpi_gpio_params params;
+	struct acpi_gpio_info *info;
 	struct gpio_desc *desc;
 	int n;
 };
@@ -752,7 +752,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 {
 	struct acpi_gpio_lookup *lookup = data;
 	struct acpi_gpio_params *params = &lookup->params;
-	struct acpi_gpio_info *info = &lookup->info;
+	struct acpi_gpio_info *info = lookup->info;
 
 	if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
 		return 1;
@@ -806,7 +806,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 
 static int acpi_gpio_resource_lookup(struct acpi_gpio_lookup *lookup)
 {
-	struct acpi_gpio_info *info = &lookup->info;
+	struct acpi_gpio_info *info = lookup->info;
 	struct acpi_device *adev = info->adev;
 	struct list_head res_list;
 	int ret;
@@ -832,7 +832,7 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode, const char *p
 {
 	struct fwnode_reference_args args;
 	struct acpi_gpio_params *params = &lookup->params;
-	struct acpi_gpio_info *info = &lookup->info;
+	struct acpi_gpio_info *info = lookup->info;
 	unsigned int index = params->crs_entry_index;
 	unsigned int quirks = 0;
 	int ret;
@@ -893,8 +893,8 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode, const char *p
 static int acpi_get_gpiod_by_index(struct acpi_device *adev, const char *propname,
 				   struct acpi_gpio_lookup *lookup)
 {
-	struct acpi_gpio_info *info = &lookup->info;
 	struct acpi_gpio_params *params = &lookup->params;
+	struct acpi_gpio_info *info = lookup->info;
 	int ret;
 
 	if (propname) {
@@ -975,6 +975,7 @@ __acpi_find_gpio(struct fwnode_handle *fwnode, const char *con_id, unsigned int
 
 	memset(&lookup, 0, sizeof(lookup));
 	lookup.params.crs_entry_index = idx;
+	lookup.info = info;
 
 	/* Try first from _DSD */
 	for_each_gpio_property_name(propname, con_id) {
-- 
2.47.2

Re: [PATCH v1 0/2] gpiolib: acpi: Fix missing info filling

[Mika Westerberg]

On Wed, Apr 09, 2025 at 04:27:52PM +0300, Andy Shevchenko wrote:
> Kees reported that code, while being refactored, missed the point of
> filling the info structure which supplies GPIO flags to the upper layer.
> Indeed, without that part the GPIO expander get no IRQ on Intel Edison,
> for example. Fix this in this series.
> 
> Andy Shevchenko (2):
>   gpiolib: acpi: Use temporary variable for struct acpi_gpio_info
>   gpiolib: acpi: Make sure we fill struct acpi_gpio_info

Both,

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v1 0/2] gpiolib: acpi: Fix missing info filling

[Andy Shevchenko]

On Wed, Apr 09, 2025 at 04:44:43PM +0300, Mika Westerberg wrote:
> On Wed, Apr 09, 2025 at 04:27:52PM +0300, Andy Shevchenko wrote:
> > Kees reported that code, while being refactored, missed the point of
> > filling the info structure which supplies GPIO flags to the upper layer.
> > Indeed, without that part the GPIO expander get no IRQ on Intel Edison,
> > for example. Fix this in this series.
> > 
> > Andy Shevchenko (2):
> >   gpiolib: acpi: Use temporary variable for struct acpi_gpio_info
> >   gpiolib: acpi: Make sure we fill struct acpi_gpio_info
> 
> Both,
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Pushed to my review and testing queue, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] gpiolib: acpi: Make sure we fill struct acpi_gpio_info

[Kees Bakker]

Op 09-04-2025 om 15:27 schreef Andy Shevchenko:
> The previous refactoring missed the filling of the struct acpi_gpio_info
> and that's how the lot of the code got eliminated. Restore those pieces
> by passing the pointer all down in the call stack.
>
> With this, the code grows by ~6%, but in conjunction with the previous
> refactoring it still gives -387 bytes
>
> add/remove: 2/0 grow/shrink: 5/1 up/down: 852/-35 (817)
> Function                                     old     new   delta
> acpi_dev_gpio_irq_wake_get_by                129     695    +566
> acpi_find_gpio                               216     354    +138
> acpi_find_gpio.__UNIQUE_ID_ddebug504           -      56     +56
> acpi_dev_gpio_irq_wake_get_by.__UNIQUE_ID_ddebug506       -      56     +56
> acpi_populate_gpio_lookup                    536     548     +12
> acpi_gpio_property_lookup                    414     426     +12
> acpi_get_gpiod_by_index                      307     319     +12
> __acpi_find_gpio                             638     603     -35
> Total: Before=14154, After=14971, chg +5.77%
>
> As a positive side effect, it improves memory footprint for
> struct acpi_gpio_lookup. `pahole` difference before and after:
>
> -       /* size: 64, cachelines: 1, members: 4 */
> -       /* member types with holes: 1, total: 1 */
>
> +       /* size: 32, cachelines: 1, members: 4 */
>
> Reported-by: Kees Bakker <kees@ijzerbout.nl>
> Closes: https://lore.kernel.org/r/9715c8dd-38df-48fd-a9d1-7a78163dc989@ijzerbout.nl
> Fixes: 8b4f52ef7a41 ("gpiolib: acpi: Deduplicate some code in __acpi_find_gpio()")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/gpio/gpiolib-acpi.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 5b6344f0d065..2ac9c7b31908 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -742,8 +742,8 @@ static int acpi_gpio_update_gpiod_lookup_flags(unsigned long *lookupflags,
>   }
>   
>   struct acpi_gpio_lookup {
> -	struct acpi_gpio_info info;
>   	struct acpi_gpio_params params;
> +	struct acpi_gpio_info *info;
>   	struct gpio_desc *desc;
>   	int n;
>   };
> @@ -752,7 +752,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
>   {
>   	struct acpi_gpio_lookup *lookup = data;
>   	struct acpi_gpio_params *params = &lookup->params;
> -	struct acpi_gpio_info *info = &lookup->info;
> +	struct acpi_gpio_info *info = lookup->info;
>   
>   	if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
>   		return 1;
> @@ -806,7 +806,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
>   
>   static int acpi_gpio_resource_lookup(struct acpi_gpio_lookup *lookup)
>   {
> -	struct acpi_gpio_info *info = &lookup->info;
> +	struct acpi_gpio_info *info = lookup->info;
>   	struct acpi_device *adev = info->adev;
>   	struct list_head res_list;
>   	int ret;
> @@ -832,7 +832,7 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode, const char *p
>   {
>   	struct fwnode_reference_args args;
>   	struct acpi_gpio_params *params = &lookup->params;
> -	struct acpi_gpio_info *info = &lookup->info;
> +	struct acpi_gpio_info *info = lookup->info;
>   	unsigned int index = params->crs_entry_index;
>   	unsigned int quirks = 0;
>   	int ret;
> @@ -893,8 +893,8 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode, const char *p
>   static int acpi_get_gpiod_by_index(struct acpi_device *adev, const char *propname,
>   				   struct acpi_gpio_lookup *lookup)
>   {
> -	struct acpi_gpio_info *info = &lookup->info;
>   	struct acpi_gpio_params *params = &lookup->params;
> +	struct acpi_gpio_info *info = lookup->info;
>   	int ret;
>   
>   	if (propname) {
> @@ -975,6 +975,7 @@ __acpi_find_gpio(struct fwnode_handle *fwnode, const char *con_id, unsigned int
>   
>   	memset(&lookup, 0, sizeof(lookup));
>   	lookup.params.crs_entry_index = idx;
> +	lookup.info = info;
>   
>   	/* Try first from _DSD */
>   	for_each_gpio_property_name(propname, con_id) {
Can you check and confirm that at least info.gpioint is filled in (or 
initialized)?
The callers of `__acpi_find_gpio` pass in an uninitialized `struct 
acpi_gpio_info`
and after the call they read `info.gpioint`.
-- 
Kees

Re: [PATCH v1 2/2] gpiolib: acpi: Make sure we fill struct acpi_gpio_info

[Andy Shevchenko]

Wed, Apr 09, 2025 at 08:16:59PM +0200, Kees Bakker kirjoitti:
> Op 09-04-2025 om 15:27 schreef Andy Shevchenko:

...

> Can you check and confirm that at least info.gpioint is filled in (or
> initialized)?

Yes, I can confirm this. And that's how I have tested it, on Intel
Edison/Arduino the first GPIO expander (PCAL9555, serviced by
drivers/gpio/gpio-pca953x.c) is able to deliver an interrupt to the SoC.

Before this series that doesn't show up, now it works as expected.

> The callers of `__acpi_find_gpio` pass in an uninitialized `struct
> acpi_gpio_info`

True.

> and after the call they read `info.gpioint`.

...when GPIO descriptor is valid.

...

Yes, I agree that NULLifying info maybe good to have, but I don't see currently
if we have bugs with it. Can you be more specific in case you have observed
anything wrong?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] gpiolib: acpi: Make sure we fill struct acpi_gpio_info

[Kees Bakker]

Op 10-04-2025 om 20:41 schreef Andy Shevchenko:
> Wed, Apr 09, 2025 at 08:16:59PM +0200, Kees Bakker kirjoitti:
>> Op 09-04-2025 om 15:27 schreef Andy Shevchenko:
> ...
>
>> Can you check and confirm that at least info.gpioint is filled in (or
>> initialized)?
> Yes, I can confirm this. And that's how I have tested it, on Intel
> Edison/Arduino the first GPIO expander (PCAL9555, serviced by
> drivers/gpio/gpio-pca953x.c) is able to deliver an interrupt to the SoC.
>
> Before this series that doesn't show up, now it works as expected.
>
>> The callers of `__acpi_find_gpio` pass in an uninitialized `struct
>> acpi_gpio_info`
> True.
>
>> and after the call they read `info.gpioint`.
> ...when GPIO descriptor is valid.
>
> ...
>
> Yes, I agree that NULLifying info maybe good to have, but I don't see currently
> if we have bugs with it. Can you be more specific in case you have observed
> anything wrong?
Thanks for confirming.
I have nothing specific, just manually reviewing the (new) patch.
So, I guess this code in `acpi_populate_gpio_lookup` takes care of it.
         lookup->desc = desc;
         info->pin_config = agpio->pin_config;
         info->debounce = agpio->debounce_timeout;
         info->gpioint = gpioint;
         info->wake_capable = acpi_gpio_irq_is_wake(&info->adev->dev, 
agpio);
That would sound plausible.
-- 
Kees