| Message ID | 20241010-gpio-notify-in-kernel-events-v2-5-b560411f7c59@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: notify user-space about config changes in the kernel | expand |
On Thu, Oct 10, 2024 at 11:10:26AM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > With everything else ready, we can now switch to using the atomic > notifier for line state events which will allow us to notify user-space > about direction changes from atomic context. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/gpio/gpiolib-cdev.c | 22 ++++++++++++++++------ > drivers/gpio/gpiolib.c | 6 +++--- > drivers/gpio/gpiolib.h | 2 +- > 3 files changed, 20 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > index 2677134b52cd..7eae0b17a1d6 100644 > --- a/drivers/gpio/gpiolib-cdev.c > +++ b/drivers/gpio/gpiolib-cdev.c > @@ -2673,6 +2673,16 @@ static int lineinfo_changed_notify(struct notifier_block *nb, > if (!test_bit(gpio_chip_hwgpio(desc), cdev->watched_lines)) > return NOTIFY_DONE; > > + /* > + * This is called from atomic context (with a spinlock taken by the > + * atomic notifier chain). Any sleeping calls must be done outside of > + * this function in process context of the dedicated workqueue. > + * > + * Let's gather as much info as possible from the descriptor and > + * postpone just the call to pinctrl_gpio_can_use_line() until the work > + * is executed. > + */ > + Should be in patch 4? You aren't otherwise changing that function here. Cheers, Kent.
On Mon, Oct 14, 2024 at 4:11 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > + /* > > + * This is called from atomic context (with a spinlock taken by the > > + * atomic notifier chain). Any sleeping calls must be done outside of > > + * this function in process context of the dedicated workqueue. > > + * > > + * Let's gather as much info as possible from the descriptor and > > + * postpone just the call to pinctrl_gpio_can_use_line() until the work > > + * is executed. > > + */ > > + > > Should be in patch 4? You aren't otherwise changing that function here. > Until this patch, the comment isn't really true, so I figured it makes more sense here. Bart
On Mon, Oct 14, 2024 at 09:48:16AM +0200, Bartosz Golaszewski wrote: > On Mon, Oct 14, 2024 at 4:11 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > + /* > > > + * This is called from atomic context (with a spinlock taken by the > > > + * atomic notifier chain). Any sleeping calls must be done outside of > > > + * this function in process context of the dedicated workqueue. > > > + * > > > + * Let's gather as much info as possible from the descriptor and > > > + * postpone just the call to pinctrl_gpio_can_use_line() until the work > > > + * is executed. > > > + */ > > > + > > > > Should be in patch 4? You aren't otherwise changing that function here. > > > > Until this patch, the comment isn't really true, so I figured it makes > more sense here. > So the validity of the comment depends on how the function is being called? Then perhaps you should reword it as well. Cheers, Kent.
On Mon, Oct 14, 2024 at 11:24 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Mon, Oct 14, 2024 at 09:48:16AM +0200, Bartosz Golaszewski wrote: > > On Mon, Oct 14, 2024 at 4:11 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > + /* > > > > + * This is called from atomic context (with a spinlock taken by the > > > > + * atomic notifier chain). Any sleeping calls must be done outside of > > > > + * this function in process context of the dedicated workqueue. > > > > + * > > > > + * Let's gather as much info as possible from the descriptor and > > > > + * postpone just the call to pinctrl_gpio_can_use_line() until the work > > > > + * is executed. > > > > + */ > > > > + > > > > > > Should be in patch 4? You aren't otherwise changing that function here. > > > > > > > Until this patch, the comment isn't really true, so I figured it makes > > more sense here. > > > > So the validity of the comment depends on how the function is being called? > Then perhaps you should reword it as well. > The validity of the comment depends on the type of the notifier used. As long as it's a blocking notifier, it's called with a mutex taken - it's process context. When we switch to the atomic notifier, this function is now called with a spinlock taken, so it's considered atomic. Bart
On Mon, Oct 14, 2024 at 11:27:05AM +0200, Bartosz Golaszewski wrote: > On Mon, Oct 14, 2024 at 11:24 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Mon, Oct 14, 2024 at 09:48:16AM +0200, Bartosz Golaszewski wrote: > > > On Mon, Oct 14, 2024 at 4:11 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > + /* > > > > > + * This is called from atomic context (with a spinlock taken by the > > > > > + * atomic notifier chain). Any sleeping calls must be done outside of > > > > > + * this function in process context of the dedicated workqueue. > > > > > + * > > > > > + * Let's gather as much info as possible from the descriptor and > > > > > + * postpone just the call to pinctrl_gpio_can_use_line() until the work > > > > > + * is executed. > > > > > + */ > > > > > + > > > > > > > > Should be in patch 4? You aren't otherwise changing that function here. > > > > > > > > > > Until this patch, the comment isn't really true, so I figured it makes > > > more sense here. > > > > > > > So the validity of the comment depends on how the function is being called? > > Then perhaps you should reword it as well. > > > > The validity of the comment depends on the type of the notifier used. > As long as it's a blocking notifier, it's called with a mutex taken - > it's process context. When we switch to the atomic notifier, this > function is now called with a spinlock taken, so it's considered > atomic. > Indeed - so the comment is brittle. Cheers, Kent.
On Mon, Oct 14, 2024 at 11:30 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Mon, Oct 14, 2024 at 11:27:05AM +0200, Bartosz Golaszewski wrote: > > On Mon, Oct 14, 2024 at 11:24 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Mon, Oct 14, 2024 at 09:48:16AM +0200, Bartosz Golaszewski wrote: > > > > On Mon, Oct 14, 2024 at 4:11 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > > > + /* > > > > > > + * This is called from atomic context (with a spinlock taken by the > > > > > > + * atomic notifier chain). Any sleeping calls must be done outside of > > > > > > + * this function in process context of the dedicated workqueue. > > > > > > + * > > > > > > + * Let's gather as much info as possible from the descriptor and > > > > > > + * postpone just the call to pinctrl_gpio_can_use_line() until the work > > > > > > + * is executed. > > > > > > + */ > > > > > > + > > > > > > > > > > Should be in patch 4? You aren't otherwise changing that function here. > > > > > > > > > > > > > Until this patch, the comment isn't really true, so I figured it makes > > > > more sense here. > > > > > > > > > > So the validity of the comment depends on how the function is being called? > > > Then perhaps you should reword it as well. > > > > > > > The validity of the comment depends on the type of the notifier used. > > As long as it's a blocking notifier, it's called with a mutex taken - > > it's process context. When we switch to the atomic notifier, this > > function is now called with a spinlock taken, so it's considered > > atomic. > > > > Indeed - so the comment is brittle. > I'm not sure what you're saying. We know it's an atomic notifier, we assign this callback to the block and register by calling atomic_notifier_chain_register(). I fail to see why you consider it "brittle". Bart
On Mon, Oct 14, 2024 at 11:32:24AM +0200, Bartosz Golaszewski wrote: > On Mon, Oct 14, 2024 at 11:30 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Mon, Oct 14, 2024 at 11:27:05AM +0200, Bartosz Golaszewski wrote: > > > On Mon, Oct 14, 2024 at 11:24 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > On Mon, Oct 14, 2024 at 09:48:16AM +0200, Bartosz Golaszewski wrote: > > > > > On Mon, Oct 14, 2024 at 4:11 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > > > > > + /* > > > > > > > + * This is called from atomic context (with a spinlock taken by the > > > > > > > + * atomic notifier chain). Any sleeping calls must be done outside of > > > > > > > + * this function in process context of the dedicated workqueue. > > > > > > > + * > > > > > > > + * Let's gather as much info as possible from the descriptor and > > > > > > > + * postpone just the call to pinctrl_gpio_can_use_line() until the work > > > > > > > + * is executed. > > > > > > > + */ > > > > > > > + > > > > > > > > > > > > Should be in patch 4? You aren't otherwise changing that function here. > > > > > > > > > > > > > > > > Until this patch, the comment isn't really true, so I figured it makes > > > > > more sense here. > > > > > > > > > > > > > So the validity of the comment depends on how the function is being called? > > > > Then perhaps you should reword it as well. > > > > > > > > > > The validity of the comment depends on the type of the notifier used. > > > As long as it's a blocking notifier, it's called with a mutex taken - > > > it's process context. When we switch to the atomic notifier, this > > > function is now called with a spinlock taken, so it's considered > > > atomic. > > > > > > > Indeed - so the comment is brittle. > > > > I'm not sure what you're saying. We know it's an atomic notifier, we > assign this callback to the block and register by calling > atomic_notifier_chain_register(). I fail to see why you consider it > "brittle". > I realise that - I'm not sure how to rephrase. The comment is describing changes in behaviour that were added in a previous patch. The comment should describe the change in behaviour there and in a generic way that is independent of the notifier chain type. Tying it to the notifier chain type is what makes it brittle - if that is changed in the future then the comment becomes confusing or invalid. I'm not sure that adds anything to what I've already said. It isn't a deal breaker - just seems like poor form to me. Cheers, Kent.
On Mon, Oct 14, 2024 at 11:55 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Mon, Oct 14, 2024 at 11:32:24AM +0200, Bartosz Golaszewski wrote: > > On Mon, Oct 14, 2024 at 11:30 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Mon, Oct 14, 2024 at 11:27:05AM +0200, Bartosz Golaszewski wrote: > > > > On Mon, Oct 14, 2024 at 11:24 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > On Mon, Oct 14, 2024 at 09:48:16AM +0200, Bartosz Golaszewski wrote: > > > > > > On Mon, Oct 14, 2024 at 4:11 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > > > > > > > + /* > > > > > > > > + * This is called from atomic context (with a spinlock taken by the > > > > > > > > + * atomic notifier chain). Any sleeping calls must be done outside of > > > > > > > > + * this function in process context of the dedicated workqueue. > > > > > > > > + * > > > > > > > > + * Let's gather as much info as possible from the descriptor and > > > > > > > > + * postpone just the call to pinctrl_gpio_can_use_line() until the work > > > > > > > > + * is executed. > > > > > > > > + */ > > > > > > > > + > > > > > > > > > > > > > > Should be in patch 4? You aren't otherwise changing that function here. > > > > > > > > > > > > > > > > > > > Until this patch, the comment isn't really true, so I figured it makes > > > > > > more sense here. > > > > > > > > > > > > > > > > So the validity of the comment depends on how the function is being called? > > > > > Then perhaps you should reword it as well. > > > > > > > > > > > > > The validity of the comment depends on the type of the notifier used. > > > > As long as it's a blocking notifier, it's called with a mutex taken - > > > > it's process context. When we switch to the atomic notifier, this > > > > function is now called with a spinlock taken, so it's considered > > > > atomic. > > > > > > > > > > Indeed - so the comment is brittle. > > > > > > > I'm not sure what you're saying. We know it's an atomic notifier, we > > assign this callback to the block and register by calling > > atomic_notifier_chain_register(). I fail to see why you consider it > > "brittle". > > > > > I realise that - I'm not sure how to rephrase. > The comment is describing changes in behaviour that were added in a previous > patch. The comment should describe the change in behaviour there and in a > generic way that is independent of the notifier chain type. Tying it to the > notifier chain type is what makes it brittle - if that is changed in the > future then the comment becomes confusing or invalid. > > I'm not sure that adds anything to what I've already said. > It isn't a deal breaker - just seems like poor form to me. > Ok, let me see what I can do for v3. Bart
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index 2677134b52cd..7eae0b17a1d6 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -2673,6 +2673,16 @@ static int lineinfo_changed_notify(struct notifier_block *nb, if (!test_bit(gpio_chip_hwgpio(desc), cdev->watched_lines)) return NOTIFY_DONE; + /* + * This is called from atomic context (with a spinlock taken by the + * atomic notifier chain). Any sleeping calls must be done outside of + * this function in process context of the dedicated workqueue. + * + * Let's gather as much info as possible from the descriptor and + * postpone just the call to pinctrl_gpio_can_use_line() until the work + * is executed. + */ + ctx = kzalloc(sizeof(*ctx), GFP_ATOMIC); if (!ctx) { pr_err("Failed to allocate memory for line info notification\n"); @@ -2837,8 +2847,8 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file) cdev->gdev = gpio_device_get(gdev); cdev->lineinfo_changed_nb.notifier_call = lineinfo_changed_notify; - ret = blocking_notifier_chain_register(&gdev->line_state_notifier, - &cdev->lineinfo_changed_nb); + ret = atomic_notifier_chain_register(&gdev->line_state_notifier, + &cdev->lineinfo_changed_nb); if (ret) goto out_free_bitmap; @@ -2862,8 +2872,8 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file) blocking_notifier_chain_unregister(&gdev->device_notifier, &cdev->device_unregistered_nb); out_unregister_line_notifier: - blocking_notifier_chain_unregister(&gdev->line_state_notifier, - &cdev->lineinfo_changed_nb); + atomic_notifier_chain_unregister(&gdev->line_state_notifier, + &cdev->lineinfo_changed_nb); out_free_bitmap: gpio_device_put(gdev); bitmap_free(cdev->watched_lines); @@ -2887,8 +2897,8 @@ static int gpio_chrdev_release(struct inode *inode, struct file *file) blocking_notifier_chain_unregister(&gdev->device_notifier, &cdev->device_unregistered_nb); - blocking_notifier_chain_unregister(&gdev->line_state_notifier, - &cdev->lineinfo_changed_nb); + atomic_notifier_chain_unregister(&gdev->line_state_notifier, + &cdev->lineinfo_changed_nb); bitmap_free(cdev->watched_lines); gpio_device_put(gdev); kfree(cdev); diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 839036b116a2..9b10f47832d5 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1026,7 +1026,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, } } - BLOCKING_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier); + ATOMIC_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier); BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier); ret = init_srcu_struct(&gdev->srcu); @@ -4089,8 +4089,8 @@ EXPORT_SYMBOL_GPL(gpiod_set_array_value_cansleep); void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action) { - blocking_notifier_call_chain(&desc->gdev->line_state_notifier, - action, desc); + atomic_notifier_call_chain(&desc->gdev->line_state_notifier, + action, desc); } /** diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index d24cd9e8b17c..2799157a1f6b 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -72,7 +72,7 @@ struct gpio_device { const char *label; void *data; struct list_head list; - struct blocking_notifier_head line_state_notifier; + struct atomic_notifier_head line_state_notifier; struct workqueue_struct *line_state_wq; struct blocking_notifier_head device_notifier; struct srcu_struct srcu;