| Message ID | 20241010-gpio-notify-in-kernel-events-v2-4-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:25AM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > In order to allow line state notifications to be emitted from atomic > context (for instance: from gpiod_direction_input/output()), we must > stop calling any sleeping functions in lineinfo_changed_notify(). To > that end let's use the new workqueue. > > Let's atomically allocate small structures containing the required data > and fill it with information immediately upon being notified about the > change except for the pinctrl state which will be retrieved later from > process context. We can pretty reliably do this as pin functions are > typically set once per boot. > > Let's make sure to bump the reference count of GPIO device and the GPIO > character device file descriptor to keep both alive until the event was > queued. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/gpio/gpiolib-cdev.c | 72 +++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 60 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > index dec6c9a6005f..2677134b52cd 100644 > --- a/drivers/gpio/gpiolib-cdev.c > +++ b/drivers/gpio/gpiolib-cdev.c > @@ -2451,6 +2451,7 @@ struct gpio_chardev_data { > #ifdef CONFIG_GPIO_CDEV_V1 > atomic_t watch_abi_version; > #endif > + struct file *fp; > }; > > static int chipinfo_get(struct gpio_chardev_data *cdev, void __user *ip) > @@ -2621,29 +2622,75 @@ static long gpio_ioctl_compat(struct file *file, unsigned int cmd, > } > #endif > > +struct lineinfo_changed_ctx { > + struct work_struct work; > + struct gpio_v2_line_info_changed chg; > + struct gpio_device *gdev; > + struct gpio_chardev_data *cdev; > +}; > + > +static void lineinfo_changed_func(struct work_struct *work) > +{ > + struct lineinfo_changed_ctx *ctx = > + container_of(work, struct lineinfo_changed_ctx, work); > + struct gpio_chip *gc; > + int ret; > + > + scoped_guard(srcu, &ctx->gdev->srcu) { > + gc = srcu_dereference(ctx->gdev->chip, &ctx->gdev->srcu); > + if (!gc) > + return; > + > + /* > + * We're doing this late because it's a sleeping function. Pin > + * functions are in general much more static and while it's not > + * 100% bullet-proof, it's good enough for most cases. > + */ > + if (!pinctrl_gpio_can_use_line(gc, ctx->chg.info.offset)) > + ctx->chg.info.flags |= GPIO_V2_LINE_FLAG_USED; > + } > + This block should be conditional on GPIO_V2_LINE_FLAG_USED not already being set - most of the time it will be and then this is pointless work. Cheers, Kent.
On Mon, Oct 14, 2024 at 4:09 AM Kent Gibson <warthog618@gmail.com> wrote: > > > + > > + scoped_guard(srcu, &ctx->gdev->srcu) { > > + gc = srcu_dereference(ctx->gdev->chip, &ctx->gdev->srcu); > > + if (!gc) > > + return; > > + > > + /* > > + * We're doing this late because it's a sleeping function. Pin > > + * functions are in general much more static and while it's not > > + * 100% bullet-proof, it's good enough for most cases. > > + */ > > + if (!pinctrl_gpio_can_use_line(gc, ctx->chg.info.offset)) > > + ctx->chg.info.flags |= GPIO_V2_LINE_FLAG_USED; > > + } > > + > > This block should be conditional on GPIO_V2_LINE_FLAG_USED not already > being set - most of the time it will be and then this is pointless work. > Good point, thanks! Bart
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index dec6c9a6005f..2677134b52cd 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -2451,6 +2451,7 @@ struct gpio_chardev_data { #ifdef CONFIG_GPIO_CDEV_V1 atomic_t watch_abi_version; #endif + struct file *fp; }; static int chipinfo_get(struct gpio_chardev_data *cdev, void __user *ip) @@ -2621,29 +2622,75 @@ static long gpio_ioctl_compat(struct file *file, unsigned int cmd, } #endif +struct lineinfo_changed_ctx { + struct work_struct work; + struct gpio_v2_line_info_changed chg; + struct gpio_device *gdev; + struct gpio_chardev_data *cdev; +}; + +static void lineinfo_changed_func(struct work_struct *work) +{ + struct lineinfo_changed_ctx *ctx = + container_of(work, struct lineinfo_changed_ctx, work); + struct gpio_chip *gc; + int ret; + + scoped_guard(srcu, &ctx->gdev->srcu) { + gc = srcu_dereference(ctx->gdev->chip, &ctx->gdev->srcu); + if (!gc) + return; + + /* + * We're doing this late because it's a sleeping function. Pin + * functions are in general much more static and while it's not + * 100% bullet-proof, it's good enough for most cases. + */ + if (!pinctrl_gpio_can_use_line(gc, ctx->chg.info.offset)) + ctx->chg.info.flags |= GPIO_V2_LINE_FLAG_USED; + } + + ret = kfifo_in_spinlocked(&ctx->cdev->events, &ctx->chg, 1, + &ctx->cdev->wait.lock); + if (ret) + wake_up_poll(&ctx->cdev->wait, EPOLLIN); + else + pr_debug_ratelimited("lineinfo event FIFO is full - event dropped\n"); + + gpio_device_put(ctx->gdev); + fput(ctx->cdev->fp); + kfree(ctx); +} + static int lineinfo_changed_notify(struct notifier_block *nb, unsigned long action, void *data) { struct gpio_chardev_data *cdev = container_of(nb, struct gpio_chardev_data, lineinfo_changed_nb); - struct gpio_v2_line_info_changed chg; + struct lineinfo_changed_ctx *ctx; struct gpio_desc *desc = data; - int ret; if (!test_bit(gpio_chip_hwgpio(desc), cdev->watched_lines)) return NOTIFY_DONE; - memset(&chg, 0, sizeof(chg)); - chg.event_type = action; - chg.timestamp_ns = ktime_get_ns(); - gpio_desc_to_lineinfo(desc, &chg.info, false); - supinfo_to_lineinfo(desc, &chg.info); + ctx = kzalloc(sizeof(*ctx), GFP_ATOMIC); + if (!ctx) { + pr_err("Failed to allocate memory for line info notification\n"); + return NOTIFY_DONE; + } - ret = kfifo_in_spinlocked(&cdev->events, &chg, 1, &cdev->wait.lock); - if (ret) - wake_up_poll(&cdev->wait, EPOLLIN); - else - pr_debug_ratelimited("lineinfo event FIFO is full - event dropped\n"); + ctx->chg.event_type = action; + ctx->chg.timestamp_ns = ktime_get_ns(); + gpio_desc_to_lineinfo(desc, &ctx->chg.info, true); + supinfo_to_lineinfo(desc, &ctx->chg.info); + /* Keep the GPIO device alive until we emit the event. */ + ctx->gdev = gpio_device_get(desc->gdev); + ctx->cdev = cdev; + /* Keep the file descriptor alive too. */ + get_file(ctx->cdev->fp); + + INIT_WORK(&ctx->work, lineinfo_changed_func); + queue_work(ctx->gdev->line_state_wq, &ctx->work); return NOTIFY_OK; } @@ -2803,6 +2850,7 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file) goto out_unregister_line_notifier; file->private_data = cdev; + cdev->fp = file; ret = nonseekable_open(inode, file); if (ret)