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@xxxxxxxxxxxx>
> Closes: https://lore.kernel.org/r/9715c8dd-38df-48fd-a9d1-7a78163dc989@xxxxxxxxxxxx
> Fixes: 8b4f52ef7a41 ("gpiolib: acpi: Deduplicate some code in __acpi_find_gpio()")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
>   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