gpiolib: add an ioctl() for monitoring line status changes

From: Bartosz Golaszewski <[email protected]>

Checkpatch complains about using 'unsigned' instead of 'unsigned int'.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpio/gpiolib.c +1 -2

index 21b02a0064f8..a31797fe78fa 100644
@@ -3042,7 +3042,7 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc);
  * rely on gpio_request() having been called beforehand.
  */
 
-static int gpio_set_config(struct gpio_chip *gc, unsigned offset,
+static int gpio_set_config(struct gpio_chip *gc, unsigned int offset,
                           enum pin_config_param mode)
 {
        unsigned long config;
--

2.23.0

From: Bartosz Golaszewski <[email protected]>

Instead of calling the gpiochip's set_config() callback directly and checking its existence every time - just add a new routine that performs this check internally. Call it in gpio_set_config() and gpiod_set_transitory(). Also call it in gpiod_set_debounce() and drop the check for chip->set() as it's irrelevant to this config option.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/gpio/gpiolib.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

drivers/gpio/gpiolib.c +12 -10

index a31797fe78fa..72211407469f 100644
@@ -3042,6 +3042,15 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc);
  * rely on gpio_request() having been called beforehand.
  */
 
+static int gpio_do_set_config(struct gpio_chip *gc, unsigned int offset,
+                             enum pin_config_param mode)
+{
+       if (!gc->set_config)
+               return -ENOTSUPP;
+
+       return gc->set_config(gc, offset, mode);
+}
+
 static int gpio_set_config(struct gpio_chip *gc, unsigned int offset,
                           enum pin_config_param mode)
 {

@@ -3060,7 +3069,7 @@ static int gpio_set_config(struct gpio_chip *gc, unsigned int offset,
        }
 
        config = PIN_CONF_PACKED(mode, arg);
-       return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;
+       return gpio_do_set_config(gc, offset, mode);
 }
 
 static int gpio_set_bias(struct gpio_chip *chip, struct gpio_desc *desc)

@@ -3294,15 +3303,9 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 
        VALIDATE_DESC(desc);
        chip = desc->gdev->chip;
-       if (!chip->set || !chip->set_config) {
-               gpiod_dbg(desc,
-                         "%s: missing set() or set_config() operations\n",
-                         __func__);
-               return -ENOTSUPP;
-       }
 
        config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
-       return chip->set_config(chip, gpio_chip_hwgpio(desc), config);
+       return gpio_do_set_config(chip, gpio_chip_hwgpio(desc), config);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_debounce);
 

@@ -3339,7 +3342,7 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
        packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE,
                                          !transitory);
        gpio = gpio_chip_hwgpio(desc);
-       rc = chip->set_config(chip, gpio, packed);
+       rc = gpio_do_set_config(chip, gpio, packed);
        if (rc == -ENOTSUPP) {
                dev_dbg(&desc->gdev->dev, "Persistence not supported for GPIO %d\n",
                                gpio);
--

2.23.0

From: Bartosz Golaszewski <[email protected]>

gpiochip_get_desc() takes a u16 hwnum, but it turns out most users don't respect that and usually pass an unsigned int. Since implicit casting to a smaller type is dangerous - let's change the type of hwnum to unsigned int in gpiochip_get_desc() and in gpiochip_request_own_desc() where the size of hwnum is not respected either and who's a user of the former.

This is safe as we then check the hwnum against the number of lines before proceeding in gpiochip_get_desc().

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/gpio/gpiolib.c      | 5 +++--
 drivers/gpio/gpiolib.h      | 3 ++-
 include/linux/gpio/driver.h | 3 ++-
 3 files changed, 7 insertions(+), 4 deletions(-)

drivers/gpio/gpiolib.c +3 -2

index 72211407469f..b3ffb079e323 100644
@@ -140,7 +140,7 @@ EXPORT_SYMBOL_GPL(gpio_to_desc);
  * in the given chip for the specified hardware number.
  */
 struct gpio_desc *gpiochip_get_desc(struct gpio_chip *chip,
-                                   u16 hwnum)
+                                   unsigned int hwnum)
 {
        struct gpio_device *gdev = chip->gpiodev;
 

@@ -2990,7 +2990,8 @@ EXPORT_SYMBOL_GPL(gpiochip_is_requested);
  * A pointer to the GPIO descriptor, or an ERR_PTR()-encoded negative error
  * code on failure.
  */
-struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *chip, u16 hwnum,
+struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *chip,
+                                           unsigned int hwnum,
                                            const char *label,
                                            enum gpio_lookup_flags lflags,
                                            enum gpiod_flags dflags)

drivers/gpio/gpiolib.h +2 -1

index ca9bc1e4803c..a1cbeabadc69 100644
@@ -78,7 +78,8 @@ struct gpio_array {
        unsigned long           invert_mask[];
 };
 
-struct gpio_desc *gpiochip_get_desc(struct gpio_chip *chip, u16 hwnum);
+struct gpio_desc *gpiochip_get_desc(struct gpio_chip *chip,
+                                   unsigned int hwnum);
 int gpiod_get_array_value_complex(bool raw, bool can_sleep,
                                  unsigned int array_size,
                                  struct gpio_desc **desc_array,

include/linux/gpio/driver.h +2 -2

index e2480ef94c55..4f032de10bae 100644
@@ -715,7 +715,8 @@ gpiochip_remove_pin_ranges(struct gpio_chip *chip)
 
 #endif /* CONFIG_PINCTRL */
 
-struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *chip, u16 hwnum,
+struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *chip,
+                                           unsigned int hwnum,
                                            const char *label,
                                            enum gpio_lookup_flags lflags,
                                            enum gpiod_flags dflags);
--

2.23.0

From: Bartosz Golaszewski <[email protected]>

Unduplicate the ngpio check by simply calling gpiochip_get_desc() and checking its return value.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/gpio/gpiolib.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

drivers/gpio/gpiolib.c +3 -5

index b3ffb079e323..6ef55cc1188b 100644
@@ -678,14 +678,13 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
        /* Request each GPIO */
        for (i = 0; i < handlereq.lines; i++) {
                u32 offset = handlereq.lineoffsets[i];
-               struct gpio_desc *desc;
+               struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);
 
-               if (offset >= gdev->ngpio) {
-                       ret = -EINVAL;
+               if (IS_ERR(desc)) {
+                       ret = PTR_ERR(desc);
                        goto out_free_descs;
                }
 
-               desc = &gdev->descs[offset];
                ret = gpiod_request(desc, lh->label);
                if (ret)
                        goto out_free_descs;
--

2.23.0

From: Bartosz Golaszewski <[email protected]>

Unduplicate the ngpio check by simply calling gpiochip_get_desc() and checking its return value.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/gpio/gpiolib.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/gpio/gpiolib.c +3 -4

index 6ef55cc1188b..17796437d7be 100644
@@ -1009,8 +1009,9 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
        lflags = eventreq.handleflags;
        eflags = eventreq.eventflags;
 
-       if (offset >= gdev->ngpio)
-               return -EINVAL;
+       desc = gpiochip_get_desc(gdev->chip, offset);
+       if (IS_ERR(desc))
+               return PTR_ERR(desc);
 
        /* Return an error if a unknown flag is set */
        if ((lflags & ~GPIOHANDLE_REQUEST_VALID_FLAGS) ||

@@ -1048,7 +1049,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
                }
        }
 
-       desc = &gdev->descs[offset];
        ret = gpiod_request(desc, le->label);
        if (ret)
                goto out_free_label;
--

2.23.0

From: Bartosz Golaszewski <[email protected]>

Unduplicate the offset check by simply calling gpiochip_get_desc() and checking its return value.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/gpio/gpiolib.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/gpio/gpiolib.c +4 -4

index 17796437d7be..b7043946c029 100644
@@ -1175,10 +1175,11 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
                if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
                        return -EFAULT;
-               if (lineinfo.line_offset >= gdev->ngpio)
-                       return -EINVAL;
 
-               desc = &gdev->descs[lineinfo.line_offset];
+               desc = gpiochip_get_desc(chip, lineinfo.line_offset);
+               if (IS_ERR(desc))
+                       return PTR_ERR(desc);
+
                if (desc->name) {
                        strncpy(lineinfo.name, desc->name,
                                sizeof(lineinfo.name));
--

2.23.0

From: Bartosz Golaszewski <[email protected]>

Unduplicate the offset check by simply calling gpiochip_get_desc() and checking its return value.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/gpio/gpiolib.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/gpio/gpiolib.c +4 -4

index 17796437d7be..b7043946c029 100644
@@ -1175,10 +1175,11 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
                if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
                        return -EFAULT;
-               if (lineinfo.line_offset >= gdev->ngpio)
-                       return -EINVAL;
 
-               desc = &gdev->descs[lineinfo.line_offset];
+               desc = gpiochip_get_desc(chip, lineinfo.line_offset);
+               if (IS_ERR(desc))
+                       return PTR_ERR(desc);
+
                if (desc->name) {
                        strncpy(lineinfo.name, desc->name,
                                sizeof(lineinfo.name));
--

2.23.0

From: Bartosz Golaszewski <[email protected]>

The read_lock mutex is supposed to prevent collisions between reading and writing to the line event kfifo but it's actually only taken when the events are being read from it.

Drop the mutex entirely and reuse the spinlock made available to us in the waitqueue struct. Take the lock whenever the fifo is modified or inspected. Drop the call to kfifo_to_user() and instead first extract the new element from kfifo when the lock is taken and only then pass it on to the user after the spinlock is released.

+ return sizeof(event);

Quoted 7 lines

From: Bartosz Golaszewski <[email protected]>

The read_lock mutex is supposed to prevent collisions between reading and writing to the line event kfifo but it's actually only taken when the events are being read from it.

Quoted 6 lines

Drop the mutex entirely and reuse the spinlock made available to us in the waitqueue struct. Take the lock whenever the fifo is modified or inspected. Drop the call to kfifo_to_user() and instead first extract the new element from kfifo when the lock is taken and only then pass it on to the user after the spinlock is released.

My comments below.

drivers/gpio/gpiolib.c static __poll_t lineevent_poll(struct file *filep, · line 821

Quoted 4 lines

drivers/gpio/gpiolib.c +2 -0

+       spin_lock(&le->wait.lock);
        if (!kfifo_is_empty(&le->events))
                events = EPOLLIN | EPOLLRDNORM;
+       spin_unlock(&le->wait.lock);

Sound like a candidate to have kfifo_is_empty_spinlocked().

Quoted 6 lines

        struct lineevent_state *le = filep->private_data;

drivers/gpio/gpiolib.c +1 -1

-       unsigned int copied;
+       struct gpioevent_data event;
        int ret;

Quoted 2 lines

+ if (count < sizeof(event))

                return -EINVAL;

This still has an issue with compatible syscalls. See patch I have sent recently.
I dunno how you see is the better way: a) apply mine and rebase your series, or b) otherwise.
I can do b) if you think it shouldn't be backported.

Btw, either way we have a benifits for the following one (I see you drop kfifo_to_user() and add event variable on stack).

Quoted 1 line

+ return sizeof(event);

Also see comments in my patch regarding the event handling.

--
With Best Regards,
Andy Shevchenko

śr., 4 gru 2019 o 23:25 Andy Shevchenko <[email protected]> napisał(a):

Quoted 7 lines · Bartosz Golaszewski

Quoted 5 lines

drivers/gpio/gpiolib.c +2 -0

+       spin_lock(&le->wait.lock);
        if (!kfifo_is_empty(&le->events))
                events = EPOLLIN | EPOLLRDNORM;
+       spin_unlock(&le->wait.lock);

Quoted 1 line

Sound like a candidate to have kfifo_is_empty_spinlocked().

Yeah, I noticed but I thought I'd just add it later separately - it's always easier to merge self-contained series.

Quoted 5 lines

Quoted 4 lines

        struct lineevent_state *le = filep->private_data;

drivers/gpio/gpiolib.c +1 -1

-       unsigned int copied;
+       struct gpioevent_data event;
        int ret;

Quoted 9 lines

Quoted 2 lines

+ if (count < sizeof(event))

                return -EINVAL;

This still has an issue with compatible syscalls. See patch I have sent recently.
I dunno how you see is the better way: a) apply mine and rebase your series, or b) otherwise.
I can do b) if you think it shouldn't be backported.

Looking at your patch it seems to me it's best to rebase yours on top of this one - where I simply do copy_to_user() we can add a special case for 32-bit user-space. I can try to do this myself for v3 if you agree.

From: Bartosz Golaszewski <[email protected]>

Currently if the line-event kfifo is full, we just silently drop any new events. Add a ratelimited debug message so that we at least have some trace in the kernel log of event overflow.

drivers/gpio/gpiolib.c +3 -0

@@ -975,6 +975,9 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
        ret = kfifo_in_spinlocked(&le->events, &ge, 1, &le->wait.lock);
        if (ret)
                wake_up_poll(&le->wait, EPOLLIN);
+       else
+               pr_debug_ratelimited(
+                       "%s: event FIFO is full - event dropped\n", __func__);
 
        return IRQ_HANDLED;

Quoted 7 lines

From: Bartosz Golaszewski <[email protected]>

Currently if the line-event kfifo is full, we just silently drop any new events. Add a ratelimited debug message so that we at least have some trace in the kernel log of event overflow.

Hmm... I don't like prints in IRQ context (even threaded). Can we rather switch to trace points at some point?

From: Bartosz Golaszewski <[email protected]>

We'll soon be filling out the gpioline_info structure in multiple places. Add a separate function that given a gpio_desc sets all relevant fields.

drivers/gpio/gpiolib.c +3 -0

+       if (desc->name) {
+               strncpy(info->name, desc->name, sizeof(info->name));
+               info->name[sizeof(info->name) - 1] = '\0';

Quoted 10 lines

From: Bartosz Golaszewski <[email protected]>

We'll soon be filling out the gpioline_info structure in multiple places. Add a separate function that given a gpio_desc sets all relevant fields.

drivers/gpio/gpiolib.c +3 -0

+       if (desc->name) {
+               strncpy(info->name, desc->name, sizeof(info->name));
+               info->name[sizeof(info->name) - 1] = '\0';

drivers/gpio/gpiolib.c static int lineevent_create(struct gpio_device *gdev, void __user *ip) · line 1168

Quoted 10 lines

drivers/gpio/gpiolib.c +10 -0

+       } else {
+               info->name[0] = '\0';
+       }
+
+       if (desc->label) {
+               strncpy(info->consumer, desc->label, sizeof(info->consumer));
+               info->consumer[sizeof(info->consumer) - 1] = '\0';
+       } else {
+               info->consumer[0] = '\0';
+       }

I think we have to fix GCC warnings first and then do whatever this patch does.

--
With Best Regards,
Andy Shevchenko

śr., 4 gru 2019 o 23:30 Andy Shevchenko <[email protected]> napisał(a):

Quoted 16 lines · Bartosz Golaszewski

Quoted 14 lines

drivers/gpio/gpiolib.c +13 -0

+       if (desc->name) {
+               strncpy(info->name, desc->name, sizeof(info->name));
+               info->name[sizeof(info->name) - 1] = '\0';
+       } else {
+               info->name[0] = '\0';
+       }
+
+       if (desc->label) {
+               strncpy(info->consumer, desc->label, sizeof(info->consumer));
+               info->consumer[sizeof(info->consumer) - 1] = '\0';
+       } else {
+               info->consumer[0] = '\0';
+       }

Quoted 2 lines

I think we have to fix GCC warnings first and then do whatever this patch does.

What GCC warnings are you referring to exactly?

Quoted 20 lines · Bartosz Golaszewski

śr., 4 gru 2019 o 23:30 Andy Shevchenko <[email protected]> napisał(a):

Quoted 18 lines · Bartosz Golaszewski

Quoted 4 lines

drivers/gpio/gpiolib.c +3 -0

+       if (desc->name) {
+               strncpy(info->name, desc->name, sizeof(info->name));
+               info->name[sizeof(info->name) - 1] = '\0';

drivers/gpio/gpiolib.c static int lineevent_create(struct gpio_device *gdev, void __user *ip) · line 1168

Quoted 10 lines

drivers/gpio/gpiolib.c +10 -0

+       } else {
+               info->name[0] = '\0';
+       }
+
+       if (desc->label) {
+               strncpy(info->consumer, desc->label, sizeof(info->consumer));
+               info->consumer[sizeof(info->consumer) - 1] = '\0';
+       } else {
+               info->consumer[0] = '\0';
+       }

I think we have to fix GCC warnings first and then do whatever this patch does.

Quoted 1 line

What GCC warnings are you referring to exactly?

stncpy() against partial string without NUL-terminator.

So, if desc->label is longer than info->consumer, it will be copied partially. I don't check if the modern GCC clever enough to see the next operation which does the termination.

czw., 5 gru 2019 o 11:21 Andy Shevchenko <[email protected]> napisał(a):

Quoted 26 lines

Quoted 17 lines · Bartosz Golaszewski

śr., 4 gru 2019 o 23:30 Andy Shevchenko <[email protected]> napisał(a):

Quoted 16 lines · Bartosz Golaszewski

Quoted 14 lines

drivers/gpio/gpiolib.c +13 -0

+       if (desc->name) {
+               strncpy(info->name, desc->name, sizeof(info->name));
+               info->name[sizeof(info->name) - 1] = '\0';
+       } else {
+               info->name[0] = '\0';
+       }
+
+       if (desc->label) {
+               strncpy(info->consumer, desc->label, sizeof(info->consumer));
+               info->consumer[sizeof(info->consumer) - 1] = '\0';
+       } else {
+               info->consumer[0] = '\0';
+       }

Quoted 4 lines

Quoted 2 lines

I think we have to fix GCC warnings first and then do whatever this patch does.

What GCC warnings are you referring to exactly?

stncpy() against partial string without NUL-terminator.

Quoted 4 lines

So, if desc->label is longer than info->consumer, it will be copied partially. I don't check if the modern GCC clever enough to see the next operation which does the termination.

I'm not sure I get it. What warnings does it produce and in what environment?

I don't see any.

If you want it simpler - we can do snprintf(info->consumer, sizeof(info->consumer), desc->label ?: "").