diff mbox series

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

Message ID 20250409132942.2550719-3-andriy.shevchenko@linux.intel.com
State New
Headers show
Series gpiolib: acpi: Fix missing info filling | expand

Commit Message

Andy Shevchenko April 9, 2025, 1:27 p.m. UTC 
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(-)

Comments

Kees Bakker April 11, 2025, 6:22 p.m. UTC | #1 
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.
diff mbox series

Patch

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) {