Re: [PATCH v4 10/20] gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL

[Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>]

On Fri, Aug 14, 2020 at 5:04 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> Add support for GPIO_V2_LINE_SET_CONFIG_IOCTL, the uAPI v2
> line set config ioctl.
>
> Signed-off-by: Kent Gibson <warthog618@xxxxxxxxx>
> ---
>  drivers/gpio/gpiolib-cdev.c | 92 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 1d42a01f5414..04472c2b6678 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -16,6 +16,7 @@
>  #include <linux/kernel.h>
>  #include <linux/kfifo.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/poll.h>
>  #include <linux/spinlock.h>
> @@ -418,6 +419,8 @@ struct edge_detector {
>   * @seqno: the sequence number for edge events generated on all lines in
>   * this line request.  Note that this is not used when @num_descs is 1, as
>   * the line_seqno is then the same and is cheaper to calculate.
> + * @config_mutex: mutex for serializing ioctl() calls to ensure consistency
> + * of configuration, particularly multi-step accesses to desc flags.
>   * @edets: an array of edge detectors, of size @num_descs
>   * @descs: the GPIO descriptors held by this line request, with @num_descs
>   * elements.
> @@ -429,6 +432,7 @@ struct line {
>         wait_queue_head_t wait;
>         DECLARE_KFIFO_PTR(events, struct gpio_v2_line_event);
>         atomic_t seqno;
> +       struct mutex config_mutex;
>         struct edge_detector *edets;
>         struct gpio_desc *descs[];
>  };
> @@ -703,6 +707,30 @@ static int gpio_v2_line_config_validate(struct gpio_v2_line_config *lc,
>         return 0;
>  }
>
> +static int gpio_v2_line_config_change_validate(struct line *line,
> +                                              struct gpio_v2_line_config *lc)
> +{
> +       int i;
> +       u64 flags;
> +       struct gpio_desc *desc;
> +
> +       for (i = 0; i < line->num_descs; i++) {
> +               desc = line->descs[i];
> +               flags = gpio_v2_line_config_flags(lc, i);
> +               /* disallow edge detection changes */
> +               if (line->edets[i].flags != (flags & GPIO_V2_LINE_EDGE_FLAGS))
> +                       return -EINVAL;
> +
> +               if (line->edets[i].flags) {
> +                       /* disallow polarity changes */
> +                       if (test_bit(FLAG_ACTIVE_LOW, &desc->flags) !=
> +                           ((flags & GPIO_V2_LINE_FLAG_ACTIVE_LOW) != 0))
> +                               return -EINVAL;
> +               }
> +       }
> +       return 0;
> +}
> +
>  static void gpio_v2_line_config_flags_to_desc_flags(u64 flags,
>                                                     unsigned long *flagsp)
>  {
> @@ -783,6 +811,67 @@ static long line_get_values(struct line *line, void __user *ip)
>         return 0;
>  }
>
> +static long line_set_config_locked(struct line *line,
> +                                  struct gpio_v2_line_config *lc)

I think that the general consensus in the kernel is to suffix
functions that need to be called with some lock taken outside them
with _unlocked (as in: the function is unlocked on its own).
Alternatively such routines are prefixed with __ (__line_set_config())
but I know Linus prefers the former.

Bart

> +{
> +       struct gpio_desc *desc;
> +       int i, ret;
> +       u64 flags;
> +
> +       ret = gpio_v2_line_config_change_validate(line, lc);
> +       if (ret)
> +               return ret;
> +
> +       for (i = 0; i < line->num_descs; i++) {
> +               desc = line->descs[i];
> +               flags = gpio_v2_line_config_flags(lc, i);
> +
> +               gpio_v2_line_config_flags_to_desc_flags(flags, &desc->flags);
> +               /*
> +                * Lines have to be requested explicitly for input
> +                * or output, else the line will be treated "as is".
> +                */
> +               if (flags & GPIO_V2_LINE_FLAG_OUTPUT) {
> +                       int val = gpio_v2_line_config_output_value(lc, i);
> +
> +                       edge_detector_stop(&line->edets[i]);
> +                       ret = gpiod_direction_output(desc, val);
> +                       if (ret)
> +                               return ret;
> +               } else if (flags & GPIO_V2_LINE_FLAG_INPUT) {
> +                       ret = gpiod_direction_input(desc);
> +                       if (ret)
> +                               return ret;
> +               }
> +
> +               blocking_notifier_call_chain(&desc->gdev->notifier,
> +                                            GPIO_V2_LINE_CHANGED_CONFIG,
> +                                            desc);
> +       }
> +       return 0;
> +}
> +
> +static long line_set_config(struct line *line, void __user *ip)
> +{
> +       struct gpio_v2_line_config lc;
> +       int ret;
> +
> +       if (copy_from_user(&lc, ip, sizeof(lc)))
> +               return -EFAULT;
> +
> +       ret = gpio_v2_line_config_validate(&lc, line->num_descs);
> +       if (ret)
> +               return ret;
> +
> +       mutex_lock(&line->config_mutex);
> +
> +       ret = line_set_config_locked(line, &lc);
> +
> +       mutex_unlock(&line->config_mutex);
> +
> +       return ret;
> +}
> +
>  static long line_ioctl(struct file *file, unsigned int cmd,
>                        unsigned long arg)
>  {
> @@ -791,6 +880,8 @@ static long line_ioctl(struct file *file, unsigned int cmd,
>
>         if (cmd == GPIO_V2_LINE_GET_VALUES_IOCTL)
>                 return line_get_values(line, ip);
> +       else if (cmd == GPIO_V2_LINE_SET_CONFIG_IOCTL)
> +               return line_set_config(line, ip);
>
>         return -EINVAL;
>  }
> @@ -964,6 +1055,7 @@ static int line_create(struct gpio_device *gdev, void __user *ip)
>                 }
>         }
>
> +       mutex_init(&line->config_mutex);
>         init_waitqueue_head(&line->wait);
>         if (has_edge_detection) {
>                 size = lr.event_buffer_size;
> --
> 2.28.0
>