| Message ID | 20241010-gpio-notify-in-kernel-events-v2-0-b560411f7c59@linaro.org |
|---|---|
| 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
We currently only emit events on changed line config to user-space on changes made from user-space. Users have no way of getting notified about in-kernel changes. This series improves the situation and also contains a couple other related improvements. This is a reworked approach which gets and stores as much info (all of it actually, except for the pinctrl state) the moment the event occurrs but moves the actual queueing of the event on the kfifo to a dedicated high-priority, ordered workqueue. Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> --- Changes in v2: - put all line-info events emitting on a workqueue to allow queueing them from atomic context - switch the notifier type used for emitting info events to atomic - add a patch notifying user-space about drivers requesting their own descs - drop patches that were picked up - Link to v1: https://lore.kernel.org/r/20241004-gpio-notify-in-kernel-events-v1-0-8ac29e1df4fe@linaro.org --- Bartosz Golaszewski (6): gpiolib: notify user-space when a driver requests its own desc gpio: cdev: prepare gpio_desc_to_lineinfo() for being called from atomic gpiolib: add a per-gpio_device line state notification workqueue gpio: cdev: put emitting the line state events on a workqueue gpiolib: switch the line state notifier to atomic gpiolib: notify user-space about in-kernel line state changes drivers/gpio/gpiolib-cdev.c | 126 +++++++++++++++++++++++++++++++++----------- drivers/gpio/gpiolib.c | 79 +++++++++++++++++++++++---- drivers/gpio/gpiolib.h | 8 ++- 3 files changed, 171 insertions(+), 42 deletions(-) --- base-commit: b7adfb6076ff0c1ebbde56d1903daa3d07db92c5 change-id: 20240924-gpio-notify-in-kernel-events-07cd912153e8 Best regards,