[PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down

Bartosz Golaszewski posted 6 patches 2 years, 4 months ago
drivers/gpio/gpiolib-cdev.c | 101 ++++++++++++++++++++++++++++++------
drivers/gpio/gpiolib.c      |   7 +--
drivers/gpio/gpiolib.h      |   9 ++--
3 files changed, 94 insertions(+), 23 deletions(-)
[PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down
Posted by Bartosz Golaszewski 2 years, 4 months ago
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Wake up all three wake queues (the one associated with the character
device file, the one for V1 line events and the V2 line request one)
when the underlying GPIO device is unregistered. This way we won't get
stuck in poll() after the chip is gone as user-space will be forced to
go back into a new system call and will see that gdev->chip is NULL.

v1 -> v2:
- not much is left from v1, this time we don't repurpose the existing
  gpio_device notifier but add a new one so that cdev structures don't
  get unwanted events

Bartosz Golaszewski (6):
  gpiolib: rename the gpio_device notifier
  gpio: cdev: open-code to_gpio_chardev_data()
  gpiolib: add a second blocking notifier to struct gpio_device
  gpio: cdev: wake up chardev poll() on device unbind
  gpio: cdev: wake up linereq poll() on device unbind
  gpio: cdev: wake up lineevent poll() on device unbind

 drivers/gpio/gpiolib-cdev.c | 101 ++++++++++++++++++++++++++++++------
 drivers/gpio/gpiolib.c      |   7 +--
 drivers/gpio/gpiolib.h      |   9 ++--
 3 files changed, 94 insertions(+), 23 deletions(-)

-- 
2.39.2
Re: [PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down
Posted by Linus Walleij 2 years, 4 months ago
On Thu, Aug 17, 2023 at 8:50 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Wake up all three wake queues (the one associated with the character
> device file, the one for V1 line events and the V2 line request one)
> when the underlying GPIO device is unregistered. This way we won't get
> stuck in poll() after the chip is gone as user-space will be forced to
> go back into a new system call and will see that gdev->chip is NULL.
>
> v1 -> v2:
> - not much is left from v1, this time we don't repurpose the existing
>   gpio_device notifier but add a new one so that cdev structures don't
>   get unwanted events

This is nice, thanks for quick respin!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Re: [PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down
Posted by Kent Gibson 2 years, 4 months ago
On Thu, Aug 17, 2023 at 08:49:52PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Wake up all three wake queues (the one associated with the character
> device file, the one for V1 line events and the V2 line request one)
> when the underlying GPIO device is unregistered. This way we won't get
> stuck in poll() after the chip is gone as user-space will be forced to
> go back into a new system call and will see that gdev->chip is NULL.
> 
> v1 -> v2:
> - not much is left from v1, this time we don't repurpose the existing
>   gpio_device notifier but add a new one so that cdev structures don't
>   get unwanted events
> 
> Bartosz Golaszewski (6):
>   gpiolib: rename the gpio_device notifier
>   gpio: cdev: open-code to_gpio_chardev_data()
>   gpiolib: add a second blocking notifier to struct gpio_device
>   gpio: cdev: wake up chardev poll() on device unbind
>   gpio: cdev: wake up linereq poll() on device unbind
>   gpio: cdev: wake up lineevent poll() on device unbind
> 
>  drivers/gpio/gpiolib-cdev.c | 101 ++++++++++++++++++++++++++++++------
>  drivers/gpio/gpiolib.c      |   7 +--
>  drivers/gpio/gpiolib.h      |   9 ++--
>  3 files changed, 94 insertions(+), 23 deletions(-)
> 

I'm happier with this version.

Reviewed-by: Kent Gibson <warthog618@gmail.com>

Cheers,
Kent.
Re: [PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down
Posted by Andy Shevchenko 2 years, 4 months ago
On Thu, Aug 17, 2023 at 08:49:52PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Wake up all three wake queues (the one associated with the character
> device file, the one for V1 line events and the V2 line request one)
> when the underlying GPIO device is unregistered. This way we won't get
> stuck in poll() after the chip is gone as user-space will be forced to
> go back into a new system call and will see that gdev->chip is NULL.

Why can't you use the global device unbind notifications and filter out
what you are interested in?

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down
Posted by Bartosz Golaszewski 2 years, 4 months ago
On Fri, Aug 18, 2023 at 12:34 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Aug 17, 2023 at 08:49:52PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Wake up all three wake queues (the one associated with the character
> > device file, the one for V1 line events and the V2 line request one)
> > when the underlying GPIO device is unregistered. This way we won't get
> > stuck in poll() after the chip is gone as user-space will be forced to
> > go back into a new system call and will see that gdev->chip is NULL.
>
> Why can't you use the global device unbind notifications and filter out
> what you are interested in?
>

There's no truly global device unbind notification - only per-bus.
GPIO devices can reside on any bus, there are no limitations and so
we'd have to subscribe to all of them.

Bart
Re: [PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down
Posted by Andy Shevchenko 2 years, 4 months ago
On Fri, Aug 18, 2023 at 02:06:21PM +0200, Bartosz Golaszewski wrote:
> On Fri, Aug 18, 2023 at 12:34 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Thu, Aug 17, 2023 at 08:49:52PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Wake up all three wake queues (the one associated with the character
> > > device file, the one for V1 line events and the V2 line request one)
> > > when the underlying GPIO device is unregistered. This way we won't get
> > > stuck in poll() after the chip is gone as user-space will be forced to
> > > go back into a new system call and will see that gdev->chip is NULL.
> >
> > Why can't you use the global device unbind notifications and filter out
> > what you are interested in?
> 
> There's no truly global device unbind notification - only per-bus.
> GPIO devices can reside on any bus, there are no limitations and so
> we'd have to subscribe to all of them.

We have, but it requires a bit of code patching.
Look at device_platform_notify()/device_platform_notify_remove().

I noticed, btw, that platform_notify() and Co is a dead code :-)
Maybe it can be converted to a list and a manager of that list,
so specific cases can utilize it.

-- 
With Best Regards,
Andy Shevchenko


Re: [PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down
Posted by Bartosz Golaszewski 2 years, 4 months ago
On Fri, Aug 18, 2023 at 3:29 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Aug 18, 2023 at 02:06:21PM +0200, Bartosz Golaszewski wrote:
> > On Fri, Aug 18, 2023 at 12:34 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Thu, Aug 17, 2023 at 08:49:52PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > Wake up all three wake queues (the one associated with the character
> > > > device file, the one for V1 line events and the V2 line request one)
> > > > when the underlying GPIO device is unregistered. This way we won't get
> > > > stuck in poll() after the chip is gone as user-space will be forced to
> > > > go back into a new system call and will see that gdev->chip is NULL.
> > >
> > > Why can't you use the global device unbind notifications and filter out
> > > what you are interested in?
> >
> > There's no truly global device unbind notification - only per-bus.
> > GPIO devices can reside on any bus, there are no limitations and so
> > we'd have to subscribe to all of them.
>
> We have, but it requires a bit of code patching.
> Look at device_platform_notify()/device_platform_notify_remove().
>
> I noticed, btw, that platform_notify() and Co is a dead code :-)
> Maybe it can be converted to a list and a manager of that list,
> so specific cases can utilize it.
>

That's not going to happen anytime soon and I doubt Greg would like
the idea without more users interested in receiving these events. :(

This GPIO notifier is an implementation detail - once we do have
global notifiers, we can switch to using them instead.

Bart
Re: [PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down
Posted by Bartosz Golaszewski 2 years, 4 months ago
On Fri, Aug 18, 2023 at 3:36 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Fri, Aug 18, 2023 at 3:29 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Fri, Aug 18, 2023 at 02:06:21PM +0200, Bartosz Golaszewski wrote:
> > > On Fri, Aug 18, 2023 at 12:34 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Thu, Aug 17, 2023 at 08:49:52PM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > >
> > > > > Wake up all three wake queues (the one associated with the character
> > > > > device file, the one for V1 line events and the V2 line request one)
> > > > > when the underlying GPIO device is unregistered. This way we won't get
> > > > > stuck in poll() after the chip is gone as user-space will be forced to
> > > > > go back into a new system call and will see that gdev->chip is NULL.
> > > >
> > > > Why can't you use the global device unbind notifications and filter out
> > > > what you are interested in?
> > >
> > > There's no truly global device unbind notification - only per-bus.
> > > GPIO devices can reside on any bus, there are no limitations and so
> > > we'd have to subscribe to all of them.
> >
> > We have, but it requires a bit of code patching.
> > Look at device_platform_notify()/device_platform_notify_remove().
> >
> > I noticed, btw, that platform_notify() and Co is a dead code :-)
> > Maybe it can be converted to a list and a manager of that list,
> > so specific cases can utilize it.
> >
>
> That's not going to happen anytime soon and I doubt Greg would like
> the idea without more users interested in receiving these events. :(
>
> This GPIO notifier is an implementation detail - once we do have
> global notifiers, we can switch to using them instead.
>
> Bart

Seems to me the whole platform_notify() thing was meant for a
different purpose. There's no trace for it ever being used for
notifying users about driver bind and unbind events.

It appears to me too that adding a global notifier would be as simple
as extending the functionality of bus_notify() with a second, global
notifier chain. But I won't be the one to propose this to Greg KH. :)

Bart