Re: [PATCH 000/182] Rid struct gpio_chip from container_of() usage

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Wed, Dec 9, 2015 at 8:30 PM, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
> On Wed, Dec 09, 2015 at 02:08:35PM +0100, Linus Walleij wrote:
>> This removes the use of container_of() constructions from *all*
>> GPIO drivers in the kernel. It is done by instead adding an
>> optional void *data pointer to the struct gpio_chip and an
>> accessor function, gpiochip_get_data() to get it from a driver.
>>
>> WHY?
>>
>> Because we want to have a proper userspace ABI for GPIO chips,
>> which involves using a character device that the user opens
>> and closes. While the character device is open, the underlying
>> kernel objects must not go away.
>>
>> Currently the GPIO drivers keep their state in the struct
>> gpio_chip, and that is often allocated by the drivers, very
>> often as a part of a containing per-instance state container
>> struct for the driver:
>>
>> struct foo_state {
>>    struct gpio_chip chip;  <- OMG my state is there
>> };
>>
>> Drivers cannot allocate and manage this state: if a user has the
>> character device open, the objects allocated must stay around
>> even if the driver goes away. Instead drivers need to pass a
>> descriptor to the GPIO core, and then the core should allocate
>> and manage the lifecycle of things related to the device, such
>> as the chardev itself or the struct device related to the GPIO
>> device.
>
> Yes, but it does not mean that the object that is being maintained by
> the subsystem and that us attached to character device needs to be
> gpio_chip itself. You can have something like
>
> struct gpio_chip_chardev {
>         struct cdev chardev;
>         struct gpio_chip *chip;
>         bool dead;
> };

There needs to be a struct device too, amongst other things.

>
> struct gpio_chip {
>         ...
>         struct gpio_chip_chardev *chardev;
>         ...
> };
>
> You alloctae the new structure when you register/export gpio chip in
> gpio subsystem core and leave all the individual drivers alone.

The current idea I have is something in the middle. Drivers have to
change a bit. The important part is that gpiolib handles allocation of
anything containing states. I'm thinking along the lines of Russell's
proposal to use netdev_alloc()'s design pattern.

The problem is that currently gpio_chip contains a lot of
stateful variables (like the dynamically allocated array of GPIO descriptors
etc) and those are used by the gpiolib core, so they have to be moved away
from gpio_chip.

So what happens if I don't change the design pattern:

int ret = gpiochip_add(&my_chip);
...
gpiochip_remove(&my_chip);

At this point we have to cross-reference the pointer to my chip to
find the chip to remove. This goes for anything that takes the struct
gpio_chip *
as parameter, like gpiochip_add_pin_range(), gpiochip_request_own_desc()
etc etc. So something inside gpiolib must take a gpio_chip * pointer and
turn that into the actual state container, e.g, a struct gpio_device.
Since struct gpio_chip needs to be static and stateless, it cannot contain
a pointer back to its struct gpio_device.

That means basically comparing pointers across a list of gpio_device's
to find it. And that's ... very kludgy. But if people think it's better to avoid
changing all drivers I will consider it.

I think it is better if the GPIO drivers get a handle on the
real gpio_device * to be used when calling these gpiochip_* related
functions and also in the callbacks, which is a bigger refactoring
indeed.

Part of this is trying to be elegant and mimic other subsystems and not
have GPIO be too hopelessly wayward about things.

If I compare to how struct input_dev is done, you appear to also use the
pattern Russell suggested with input_dev_allocate() akin to
netdev_alloc(), and the allocated struct holds all the vtable and states etc,
and I think it is a good pattern, and that GPIO should conform.

This current patch series however, just give us the equivalent of
input_get_drvdata()/input_set_drvdata() and that seems valuable on its
own, as it reduces code size and make things more readable.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html