Re: [PATCH v2 4/6] gpio: cdev: put emitting the line state events on a workqueue

Kent Gibson
2024-10-14T02:09:21Z

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.