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