Re: [RFT PATCH 11/21] platform: x86: android-tablets: don't access GPIOLIB private members

[Hans de Goede <hdegoede@redhat.com>]

Hi Bartosz,

On 9/5/23 20:52, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We're slowly removing cases of abuse of the GPIOLIB public API. One of
> the biggest issues is looking up and accessing struct gpio_chip whose
> life-time is tied to the provider and which can disappear from under any
> user at any given moment. We have provided new interfaces that use the
> opaque struct gpio_device which is reference counted and will soon be
> thorougly protected with appropriate locking.
> 
> Stop using old interfaces in this driver and switch to safer
> alternatives.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

First of all sorry for the issues this hack-ish kernel module
is causing for cleaning up gpiolib APIs.

I don't know how close a look you took at the code, so first of
all let me try to briefly explain what this hackish kernel module
is for:

There are some x86_64/ACPI tablets which shipped with Android as
factory OS. On these tablets the device-specific (BSP style)
kernel has things like the touchscreen driver simply having
a hardcoded I2C bus-number + I2C client address. Combined
with also hardcoded GPIO numbers (using the old number base APIs)
for any GPIOs it needs.

So the original Android kernels do not need the devices
to be properly described in ACPI and the ACPI tables are
just one big copy and paste job from some BSP which do
not accurately describe the hardware at all.

x86-android-tablets.ko identifies affected models by their
DMI strings and then manually instantiates things like
i2c-clients for the touchscreen, accelerometer and also
other stuff. Yes this is ugly but it allows mainline kernels
to run pretty well on these devices since other then
the messed up ACPI tables these are pretty standard x86/ACPI
tablets.

I hope this explains the hacks, now on to the problems
these are causing:

> ---
>  .../platform/x86/x86-android-tablets/core.c   | 38 ++++++++++---------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/platform/x86/x86-android-tablets/core.c b/drivers/platform/x86/x86-android-tablets/core.c
> index 2fd6060a31bb..687f84cd193c 100644
> --- a/drivers/platform/x86/x86-android-tablets/core.c
> +++ b/drivers/platform/x86/x86-android-tablets/core.c
> @@ -12,6 +12,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/dmi.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/gpio/machine.h>
>  #include <linux/irq.h>
> @@ -21,27 +22,28 @@
>  #include <linux/string.h>
>  
>  #include "x86-android-tablets.h"
> -/* For gpiochip_get_desc() which is EXPORT_SYMBOL_GPL() */
> -#include "../../../gpio/gpiolib.h"
> -#include "../../../gpio/gpiolib-acpi.h"
> -
> -static int gpiochip_find_match_label(struct gpio_chip *gc, void *data)
> -{
> -	return gc->label && !strcmp(gc->label, data);
> -}
>  
>  int x86_android_tablet_get_gpiod(const char *label, int pin, struct gpio_desc **desc)
>  {
> +	struct gpio_device *gdev;
>  	struct gpio_desc *gpiod;
> -	struct gpio_chip *chip;
>  
> -	chip = gpiochip_find((void *)label, gpiochip_find_match_label);
> -	if (!chip) {
> -		pr_err("error cannot find GPIO chip %s\n", label);
> +	/*
> +	 * FIXME: handle GPIOs correctly! This driver should really use struct
> +	 * device and GPIO lookup tables.
> +	 *
> +	 * WONTDO: We do leak this reference, but the whole approach to getting
> +	 * GPIOs in this driver is such an abuse of the GPIOLIB API that it
> +	 * doesn't make it much worse and it's the only way to keep the
> +	 * interrupt requested later functional...
> +	 */
> +	gdev = gpio_device_find_by_label(label);
> +	if (!gdev) {
> +		pr_err("error cannot find GPIO device %s\n", label);
>  		return -ENODEV;
>  	}
>  
> -	gpiod = gpiochip_get_desc(chip, pin);
> +	gpiod = gpio_device_get_desc(gdev, pin);
>  	if (IS_ERR(gpiod)) {
>  		pr_err("error %ld getting GPIO %s %d\n", PTR_ERR(gpiod), label, pin);
>  		return PTR_ERR(gpiod);


So rather then the above I think what needs to happen here
(and I can hopefully make some time for that this weekend) is:

1. Have the x86-android-tablets code instantiate a
   "x86-android-tablets" platform-dev
2. Have the code generate a gpiod_lookup_table for all GPIOs
   for which it currently uses x86_android_tablet_get_gpiod()
   with the .dev_id set to "x86-android-tablets"
3. Use regular gpiod_get() on the "x86-android-tablets" pdev
   to get the desc.

I think this should solve all the issues with 
x86_android_tablet_get_gpiod() poking inside
gpiolib external since now it is only using
public gpiolib APIs, right ?

One question about 2. there are 2 ways to do this:

i. Have the module_init() function loop over all
x86_dev_info members which will result in calling
x86_android_tablet_get_gpiod() and have it generate
one big gpiod_lookup_table for all GPIOs needed
in one go. At which point x86_android_tablet_get_gpiod()
goes away and can be directly replaced with gpiod_get()
calls on the pdev.

ii. Keep x86_android_tablet_get_gpiod() and have it
generate a gpiod_lookup_table with just 1 entry for
the GPIO which its caller wants. Register the lookup
table, do the gpiod_get() and then immediately
unregister the lookup table again.

ii. Would be easier for me to implement, especially
since there is also some custom (board specific)
init code calling x86_android_tablet_get_gpiod()
which would require some special handling for i.

OTOH I guess some people will consider ii. somewhat
ugly, although AFAICT it is perfectly ok to use
the gpiolib lookup APIs this way.

Can you please let me known if you are ok with ii,
or if you would prefer me going with solution i. ?

That way when I can make some time to start working
on this I can pick the preferred solution right away.



> @@ -257,9 +259,9 @@ static void x86_android_tablet_cleanup(void)
>  
>  static __init int x86_android_tablet_init(void)
>  {
> +	struct gpio_device *gdev __free(gpio_device_put) = NULL;
>  	const struct x86_dev_info *dev_info;
>  	const struct dmi_system_id *id;
> -	struct gpio_chip *chip;
>  	int i, ret = 0;
>  
>  	id = dmi_first_match(x86_android_tablet_ids);
> @@ -273,13 +275,13 @@ static __init int x86_android_tablet_init(void)
>  	 * _AEI (ACPI Event Interrupt) handlers, disable these.
>  	 */
>  	if (dev_info->invalid_aei_gpiochip) {
> -		chip = gpiochip_find(dev_info->invalid_aei_gpiochip,
> -				     gpiochip_find_match_label);
> -		if (!chip) {
> +		gdev = gpio_device_find_by_label(
> +				dev_info->invalid_aei_gpiochip);
> +		if (!gdev) {
>  			pr_err("error cannot find GPIO chip %s\n", dev_info->invalid_aei_gpiochip);
>  			return -ENODEV;
>  		}
> -		acpi_gpiochip_free_interrupts(chip);
> +		acpi_gpio_device_free_interrupts(gdev);
>  	}
>  
>  	/*

After some recent improvements there is only 1 board left which sets
dev_info->invalid_aei_gpiochip and that can easily be replaced with
with adding 1 extra entry to gpiolib_acpi_quirks[] inside
drivers/gpio/gpiolib-acpi.c .

So I believe the right solution here is to just remove
dev_info->invalid_aei_gpiochip support for x86-android-tablets
all together and then at least x86-android-tablets will no
longer be making any hackish acpi_gpiochip_free_interrupts() calls.

I don't want to make any promises wrt the timing, but I should
be able to prepare a set of patches which simply removes all
the private gpiolib API use from x86-android-tablets, so that
you don't need to workaround that in this patch series.

With some luck I can have an immutable branch with 6.6-rc1 +
such a patch-series ready for you soon after 6.6-rc1 is
released.

Regards,

Hans