Message-ID: <CAMRc=Mfmp3Nd5jwNWr=kc8RFO-arFDwEvLxj5Qu9_1OOXR2gHQ@mail.gmail.com>
Date:   Wed, 6 Sep 2023 16:27:14 +0200
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Aaro Koskinen <aaro.koskinen@....fi>,
        Janusz Krzysztofik <jmkrzyszt@...il.com>,
        Tony Lindgren <tony@...mide.com>,
        Russell King <linux@...linux.org.uk>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Dipen Patel <dipenp@...dia.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Mark Gross <markgross@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-omap@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
        linux-acpi@...r.kernel.org, timestamp@...ts.linux.dev,
        linux-tegra@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [RFT PATCH 11/21] platform: x86: android-tablets: don't access
 GPIOLIB private members

On Wed, Sep 6, 2023 at 3:01 PM Hans de Goede <hdegoede@...hat.com> wrote:
>
> Hi Bartosz,
>
> On 9/5/23 20:52, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@...aro.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@...aro.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:

This makes sense! Maybe we'd need a good-old board file setting up all
non-described devices using the driver model?

>
> > ---
> >  .../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. ?
>

I am fine with ii. I have recently sent a patch that does exactly that
in one of the SPI drivers. It's ugly but it's better than what we have
now.

> 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.
>

That would be awesome, thanks a lot!

> Regards,
>
> Hans
>
>
>

Bart

[1] https://lore.kernel.org/lkml/d57a99ce-77eb-409f-8371-95f2658fa0c0@sirena.org.uk/T/

Powered by blists - more mailing lists