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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Wed, Dec 9, 2015 at 2:44 PM, Russell King - ARM Linux
<linux@xxxxxxxxxxxxxxxx> wrote:

Thanks Russell, I think you speed up the design and shorten the
development time by providing these ideas, so it is much, much
appreciated.

> On Wed, Dec 09, 2015 at 02:08:35PM +0100, Linus Walleij wrote:
>> 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.
>
> Okay, so you stop the gpio_chip struct from going away.

Actually I was going to create a new struct gpio_device, but yes.

> What
> about the code which is called via gpio_chip - say, if userspace
> keeps the chardev open, and someone rmmod's the driver providing
> the GPIO driver.

The idea was that when what is today gpiochip_remove() is called by the
gpiochip driver, the static data pointer to the vtable with all
callbacks is set to NULL (in some safe way), and the code avoids
calling these, so the device goes numb.

I think you give me the right solution to this "safe way" below,
thanks!

> I'm not sure that splitting up objects in this way really solves
> anything at all.  Yes, it divorses the driver's private data from
> the subsystem data, but is that really an advantage?

I like the design pattern you describe below, and
I have no strong opinion on it. If there is a precedent in the kernel
to do it with a separate alloc_foo() function I can do that.

(Would like Greg's and/or Johan's opinion on this though.)

> Network drivers have a similar issue, and the way this problem is
> solved there is that alloc_netdev() is always used to allocate the
> subsystem data structure and any driver private data structure as
> one allocation, and the lifetime of both objects remains under the
> control of the subsystem.
>
> The allocated memory is only freed when the last user goes away,
> and net has protection to prevent an unregistered driver from
> being called (via locks on every path into the layer.)

That's a viable way to solve this.

The refactoring would be bigger in size and different. The current
patch set would still be needed though, as the drivers will
still not be able to use the container_of() design pattern with this
design either, as the gpiolib core handles allocation of its
own struct and then some more.

With what I had in mind it would be something like:

struct foo_gpio {
      struct gpio_device *gd;
};

const struct gpio_ops foo_ops = {
     .set = ...
     .get = ...
};

foo = devm_kzalloc(dev, ...);
foo->gd = gpio_add_device(&foo_ops, ...);
...
gpio_remove_device(foo->gd);


But with the alloc_gpiodev() pattern we get this:

struct foo_gpio {
....
};

static void setup(struct gpio_device *gd)
{
   gd->set = ...;
   gd->get = ...;
}

foo = gpio_alloc_device(sizeof(foo), name, setup);
struct gpio_device *gd = gpio_add_device(foo);
...
gpio_remove_device(gd);

The setup call is clever to use also with GPIO because we
can use that to setup mmio-gpio drivers and easier refactor
those drivers that assign members to gpio_chip dynamically.

Also I think this case would involve getting rid of all static
vtables and only use that setup call to assign function
pointers akin to what we have for gpio_chip. Lots of work,
but I like it.

We can do allocation in early boot these days even if
the gpiochip is added with an early initcall right?

> Things get a little more complex with gpio, because there's the
> issue that some methods are spinlocked while others can take
> semaphores, but it should be possible to come up with a solution
> to that - maybe an atomic_t which is incremented whenever we're
> in some operation provided it's >= 0 (otherwise it fails), and
> decremented when the operation completes.  We can then control
> in the unregistration path further GPIO accesses, and also
> prevent new accesses occuring by setting the atomic_t to -1.
> This shouldn't require any additional locking in any path.  It
> does mean that the unregistration path needs careful thought to
> ensure that when we set it to -1, we wait for it to be dropped
> by the appropriate amount.

Yeah we will both in spinlocks/hotpath and
semaphores/mutexes/slowpath in these ops for sure :/

The atomic hack should work: I understand that you mean
we read it and set it to -1 and if it was 2 wait for it to
become -3, then conclude unregistration with callbacks
numbed.

Then there is a particular case that occurs with USB or similar
pluggable buses, where there is a glitch or powercycle on the
bus, and the same device goes away and comes back in
a few milliseconds, and that means it should reattach to the
character device that is already open.

Making that work is however non-trivial :(

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