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

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

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