| Message ID | 20230905185309.131295-12-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gpio: convert users to gpio_device_find() and remove gpiochip_find() | expand |
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
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); } /*