drivers/gpio/gpiolib-cdev.c | 127 +++++++++++++++++++++++++++++------- drivers/gpio/gpiolib.h | 3 +- 2 files changed, 105 insertions(+), 25 deletions(-)
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. Bartosz Golaszewski (5): gpio: cdev: ignore notifications other than line status changes gpio: cdev: rename the notifier block and notify callback 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 | 127 +++++++++++++++++++++++++++++------- drivers/gpio/gpiolib.h | 3 +- 2 files changed, 105 insertions(+), 25 deletions(-) -- 2.39.2
On Wed, Aug 16, 2023 at 2:20 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. > > Bartosz Golaszewski (5): > gpio: cdev: ignore notifications other than line status changes > gpio: cdev: rename the notifier block and notify callback > 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 I see why this is needed and while the whole notification chain is a bit clunky I really cannot think about anything better so: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Wed, Aug 16, 2023 at 11:41:06PM +0200, Linus Walleij wrote: > On Wed, Aug 16, 2023 at 2:20 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. > > > > Bartosz Golaszewski (5): > > gpio: cdev: ignore notifications other than line status changes > > gpio: cdev: rename the notifier block and notify callback > > 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 > > I see why this is needed and while the whole notification chain > is a bit clunky I really cannot think about anything better so: > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > The issue I have is with the repurposing/reuse of the existing notifier block that sends line changed events to the chardev. Correct me if I'm wrong, but now all line requests will receive those events as well. They have no business receiving those events, and it scales badly. My preference would be for a separate nb for the chip removal to keep those two classes of events distinct. Cheers, Kent.
On Thu, Aug 17, 2023 at 6:41 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Wed, Aug 16, 2023 at 11:41:06PM +0200, Linus Walleij wrote: > > On Wed, Aug 16, 2023 at 2:20 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. > > > > > > Bartosz Golaszewski (5): > > > gpio: cdev: ignore notifications other than line status changes > > > gpio: cdev: rename the notifier block and notify callback > > > 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 > > > > I see why this is needed and while the whole notification chain > > is a bit clunky I really cannot think about anything better so: > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > > > The issue I have is with the repurposing/reuse of the existing notifier > block that sends line changed events to the chardev. > Correct me if I'm wrong, but now all line requests will receive those > events as well. > They have no business receiving those events, and it scales badly. > > My preference would be for a separate nb for the chip removal to keep > those two classes of events distinct. > I would normally agree if there was a risk of abuse of those notifications by drivers but this is all private to gpiolib. And line requests that receive line state notifications simply ignore them. This isn't a bottleneck codepath IMO so where's the issue? We would be using a second notifier head of 40 bytes to struct gpio_device for no reason. It's your code for most part so if you insist, I can rework it but I'm more in favor of repurposing the existing notifier. Bartosz
On Thu, Aug 17, 2023 at 09:27:37AM +0200, Bartosz Golaszewski wrote: > On Thu, Aug 17, 2023 at 6:41 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Wed, Aug 16, 2023 at 11:41:06PM +0200, Linus Walleij wrote: > > > On Wed, Aug 16, 2023 at 2:20 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. > > > > > > > > Bartosz Golaszewski (5): > > > > gpio: cdev: ignore notifications other than line status changes > > > > gpio: cdev: rename the notifier block and notify callback > > > > 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 > > > > > > I see why this is needed and while the whole notification chain > > > is a bit clunky I really cannot think about anything better so: > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > > > > > > The issue I have is with the repurposing/reuse of the existing notifier > > block that sends line changed events to the chardev. > > Correct me if I'm wrong, but now all line requests will receive those > > events as well. > > They have no business receiving those events, and it scales badly. > > > > My preference would be for a separate nb for the chip removal to keep > > those two classes of events distinct. > > > > I would normally agree if there was a risk of abuse of those > notifications by drivers but this is all private to gpiolib. And line > requests that receive line state notifications simply ignore them. > This isn't a bottleneck codepath IMO so where's the issue? We would be > using a second notifier head of 40 bytes to struct gpio_device for no > reason. > Yeah, this is a space/time trade-off, and you've gone with space over time. I would select time over space. 40 bytes per device is negligable, and there is never a case where the line request wants to see a change event - it either relates to a different request, or it was triggered by the request itself. Is there an echo in here ;-)? Cheers, Kent.
On Thu, Aug 17, 2023 at 9:37 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Thu, Aug 17, 2023 at 09:27:37AM +0200, Bartosz Golaszewski wrote: > > On Thu, Aug 17, 2023 at 6:41 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Wed, Aug 16, 2023 at 11:41:06PM +0200, Linus Walleij wrote: > > > > On Wed, Aug 16, 2023 at 2:20 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. > > > > > > > > > > Bartosz Golaszewski (5): > > > > > gpio: cdev: ignore notifications other than line status changes > > > > > gpio: cdev: rename the notifier block and notify callback > > > > > 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 > > > > > > > > I see why this is needed and while the whole notification chain > > > > is a bit clunky I really cannot think about anything better so: > > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > > > > > > > > > The issue I have is with the repurposing/reuse of the existing notifier > > > block that sends line changed events to the chardev. > > > Correct me if I'm wrong, but now all line requests will receive those > > > events as well. > > > They have no business receiving those events, and it scales badly. > > > > > > My preference would be for a separate nb for the chip removal to keep > > > those two classes of events distinct. > > > > > > > I would normally agree if there was a risk of abuse of those > > notifications by drivers but this is all private to gpiolib. And line > > requests that receive line state notifications simply ignore them. > > This isn't a bottleneck codepath IMO so where's the issue? We would be > > using a second notifier head of 40 bytes to struct gpio_device for no > > reason. > > > > Yeah, this is a space/time trade-off, and you've gone with space over > time. I would select time over space. > 40 bytes per device is negligable, and there is never a case where the > line request wants to see a change event - it either relates to a > different request, or it was triggered by the request itself. > Is there an echo in here ;-)? > Ok, I'll rework it for v2. Bart
On Thu, Aug 17, 2023 at 6:41 AM Kent Gibson <warthog618@gmail.com> wrote: > My preference would be for a separate nb for the chip removal to keep > those two classes of events distinct. That's a good point. Bart do you think you can rework it as such? Yours, Linus Walleij
© 2016 - 2025 Red Hat, Inc.