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

[Kent Gibson <warthog618@gmail.com>]

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.