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.
+ */
+
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;
--
2.43.0On 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
© 2016 - 2024 Red Hat, Inc.