| Message ID | 20230905185309.131295-12-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Wed, Sep 6, 2023 at 3:01 PM Hans de Goede <hdegoede@redhat.com> wrote: > > 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: 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/
Hi Bart, On 9/6/23 16:27, Bartosz Golaszewski wrote: > On Wed, Sep 6, 2023 at 3:01 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> 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: > > This makes sense! Maybe we'd need a good-old board file setting up all > non-described devices using the driver model? Right, this is pretty much exactly what the x86-android-tablets code does. Except that it does it for a bunch of boards in a single .ko / driver. There is a lot of commonality between these boards, so this allows sharing most of the code. The driver uses DMI matching, with the match's driver_data pointing to a description of which devices to instantiate and then the shared code takes care of instantiating those. About 90% of the data / code is __init or __initdata so both the code to instantiate the devices as well as the per board data is free-ed after module_init() has run. <snip> >> 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. Ok, I have just finished implementing this using the ii. method. I'll post a patch-series for this for review right after this email. After that series x86-android-tablets should no longer be a problem wrt using any private gpiolib APIs. Regards, Hans
On Sat, Sep 09, 2023 at 04:17:53PM +0200, Hans de Goede wrote: > On 9/6/23 16:27, Bartosz Golaszewski wrote: > > On Wed, Sep 6, 2023 at 3:01 PM Hans de Goede <hdegoede@redhat.com> wrote: ... > > This makes sense! Maybe we'd need a good-old board file setting up all > > non-described devices using the driver model? > > Right, this is pretty much exactly what the x86-android-tablets > code does. Except that it does it for a bunch of boards in a single > .ko / driver. There is a lot of commonality between these boards, > so this allows sharing most of the code. > > The driver uses DMI matching, with the match's driver_data pointing > to a description of which devices to instantiate and then the shared > code takes care of instantiating those. > > About 90% of the data / code is __init or __initdata so both > the code to instantiate the devices as well as the per board > data is free-ed after module_init() has run. ...which is nicely looked and isolated hack (or quirk if you prefer) that I like! Thanks, Hans, for maintaining that!
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); @@ -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); } /*