[PATCH v2 00/11] gpiolib: add an ioctl() for monitoring line status changes

[PATCH v2 00/11] gpiolib: add an ioctl() for monitoring line status changes

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

When discussing the recent user-space changes with Kent and while working
on dbus API for libgpiod I noticed that we really don't have any way of
keeping the line info synchronized between the kernel and user-space
processes. We can of course periodically re-read the line information or
even do it every time we want to read a property but this isn't optimal.

This series adds a new ioctl() that allows user-space to set up a watch on
the GPIO chardev file-descriptor which can then be polled for events
emitted by the kernel when the line is requested, released or its status
changed. This of course doesn't require the line to be requested. Multiple
user-space processes can watch the same lines.

The first couple patches just fix some issues I noticed when implementing
the new interface. Patch 10/11 provides the actual ioctl() implementation
while patch 11/11 adds a simple user-space program to tools that can be
used to watch the line info changes.

v1: https://lkml.org/lkml/2019/11/27/327

v1 -> v2:
- rework the main patch of the series: re-use the existing file-descriptor
  associated with an open character device
- add a patch adding a debug message when the line event kfifo is full and
  we're discarding another event
- rework the locking mechanism for lineevent kfifo: reuse the spinlock
  from the waitqueue structure
- other minor changes

Bartosz Golaszewski (11):
  gpiolib: use 'unsigned int' instead of 'unsigned' in gpio_set_config()
  gpiolib: have a single place of calling set_config()
  gpiolib: convert the type of hwnum to unsigned int in
    gpiochip_get_desc()
  gpiolib: use gpiochip_get_desc() in linehandle_create()
  gpiolib: use gpiochip_get_desc() in lineevent_create()
  gpiolib: use gpiochip_get_desc() in gpio_ioctl()
  gpiolib: rework the locking mechanism for lineevent kfifo
  gpiolib: emit a debug message when adding events to a full kfifo
  gpiolib: provide a dedicated function for setting lineinfo
  gpiolib: add new ioctl() for monitoring changes in line info
  tools: gpio: implement gpio-watch

 drivers/gpio/gpiolib.c      | 378 ++++++++++++++++++++++++++----------
 drivers/gpio/gpiolib.h      |   5 +-
 include/linux/gpio/driver.h |   3 +-
 include/uapi/linux/gpio.h   |  24 +++
 tools/gpio/.gitignore       |   1 +
 tools/gpio/Build            |   1 +
 tools/gpio/Makefile         |  11 +-
 tools/gpio/gpio-watch.c     | 112 +++++++++++
 8 files changed, 434 insertions(+), 101 deletions(-)
 create mode 100644 tools/gpio/gpio-watch.c

-- 
2.23.0

[PATCH v2 01/11] gpiolib: use 'unsigned int' instead of 'unsigned' in gpio_set_config()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 21b02a0064f8..a31797fe78fa 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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

[PATCH v2 02/11] gpiolib: have a single place of calling set_config()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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 <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a31797fe78fa..72211407469f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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

[PATCH v2 03/11] gpiolib: convert the type of hwnum to unsigned int in gpiochip_get_desc()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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 <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c      | 5 +++--
 drivers/gpio/gpiolib.h      | 3 ++-
 include/linux/gpio/driver.h | 3 ++-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 72211407469f..b3ffb079e323 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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)
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index ca9bc1e4803c..a1cbeabadc69 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -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,
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index e2480ef94c55..4f032de10bae 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -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

[PATCH v2 04/11] gpiolib: use gpiochip_get_desc() in linehandle_create()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index b3ffb079e323..6ef55cc1188b 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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

[PATCH v2 05/11] gpiolib: use gpiochip_get_desc() in lineevent_create()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6ef55cc1188b..17796437d7be 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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

[PATCH v2 06/11] gpiolib: use gpiochip_get_desc() in gpio_ioctl()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 17796437d7be..b7043946c029 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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

[PATCH v2 07/11] gpiolib: rework the locking mechanism for lineevent kfifo

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c | 54 ++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index b7043946c029..43f90eca6d45 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -788,8 +788,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
  * @irq: the interrupt that trigger in response to events on this GPIO
  * @wait: wait queue that handles blocking reads of events
  * @events: KFIFO for the GPIO events
- * @read_lock: mutex lock to protect reads from colliding with adding
- * new events to the FIFO
  * @timestamp: cache for the timestamp storing it between hardirq
  * and IRQ thread, used to bring the timestamp close to the actual
  * event
@@ -802,7 +800,6 @@ struct lineevent_state {
 	int irq;
 	wait_queue_head_t wait;
 	DECLARE_KFIFO(events, struct gpioevent_data, 16);
-	struct mutex read_lock;
 	u64 timestamp;
 };
 
@@ -818,8 +815,10 @@ static __poll_t lineevent_poll(struct file *filep,
 
 	poll_wait(filep, &le->wait, wait);
 
+	spin_lock(&le->wait.lock);
 	if (!kfifo_is_empty(&le->events))
 		events = EPOLLIN | EPOLLRDNORM;
+	spin_unlock(&le->wait.lock);
 
 	return events;
 }
@@ -831,43 +830,47 @@ static ssize_t lineevent_read(struct file *filep,
 			      loff_t *f_ps)
 {
 	struct lineevent_state *le = filep->private_data;
-	unsigned int copied;
+	struct gpioevent_data event;
 	int ret;
 
-	if (count < sizeof(struct gpioevent_data))
+	if (count < sizeof(event))
 		return -EINVAL;
 
-	do {
+	for (;;) {
+		spin_lock(&le->wait.lock);
 		if (kfifo_is_empty(&le->events)) {
-			if (filep->f_flags & O_NONBLOCK)
+			if (filep->f_flags & O_NONBLOCK) {
+				spin_unlock(&le->wait.lock);
 				return -EAGAIN;
+			}
 
-			ret = wait_event_interruptible(le->wait,
+			ret = wait_event_interruptible_locked(le->wait,
 					!kfifo_is_empty(&le->events));
-			if (ret)
+			if (ret) {
+				spin_unlock(&le->wait.lock);
 				return ret;
-		}
+			}
 
-		if (mutex_lock_interruptible(&le->read_lock))
-			return -ERESTARTSYS;
-		ret = kfifo_to_user(&le->events, buf, count, &copied);
-		mutex_unlock(&le->read_lock);
+		}
 
-		if (ret)
-			return ret;
+		ret = kfifo_out(&le->events, &event, 1);
+		spin_unlock(&le->wait.lock);
+		if (ret == 1)
+			break;
 
 		/*
-		 * If we couldn't read anything from the fifo (a different
-		 * thread might have been faster) we either return -EAGAIN if
-		 * the file descriptor is non-blocking, otherwise we go back to
-		 * sleep and wait for more data to arrive.
+		 * We should never get here since we're holding the lock from
+		 * the moment we noticed a new event until calling kfifo_out()
+		 * but we must check the return value. On the off-chance - just
+		 * go back to waiting.
 		 */
-		if (copied == 0 && (filep->f_flags & O_NONBLOCK))
-			return -EAGAIN;
+	}
 
-	} while (copied == 0);
+	ret = copy_to_user(buf, &event, sizeof(event));
+	if (ret)
+		return -EFAULT;
 
-	return copied;
+	return sizeof(event);
 }
 
 static int lineevent_release(struct inode *inode, struct file *filep)
@@ -969,7 +972,7 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
 		return IRQ_NONE;
 	}
 
-	ret = kfifo_put(&le->events, ge);
+	ret = kfifo_in_spinlocked(&le->events, &ge, 1, &le->wait.lock);
 	if (ret)
 		wake_up_poll(&le->wait, EPOLLIN);
 
@@ -1084,7 +1087,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 
 	INIT_KFIFO(le->events);
 	init_waitqueue_head(&le->wait);
-	mutex_init(&le->read_lock);
 
 	/* Request a thread to read the events */
 	ret = request_threaded_irq(le->irq,
-- 
2.23.0

[PATCH v2 08/11] gpiolib: emit a debug message when adding events to a full kfifo

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 43f90eca6d45..c89d297da270 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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;
 }
-- 
2.23.0

[PATCH v2 09/11] gpiolib: provide a dedicated function for setting lineinfo

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c | 98 ++++++++++++++++++++++++------------------
 1 file changed, 55 insertions(+), 43 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c89d297da270..711963aa9239 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1145,6 +1145,60 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	return ret;
 }
 
+static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
+				  struct gpioline_info *info)
+{
+	struct gpio_chip *chip = desc->gdev->chip;
+	unsigned long flags;
+
+	spin_lock_irqsave(&gpio_lock, flags);
+
+	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';
+	}
+
+	/*
+	 * Userspace only need to know that the kernel is using this GPIO so
+	 * it can't use it.
+	 */
+	info->flags = 0;
+	if (test_bit(FLAG_REQUESTED, &desc->flags) ||
+	    test_bit(FLAG_IS_HOGGED, &desc->flags) ||
+	    test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
+	    test_bit(FLAG_EXPORT, &desc->flags) ||
+	    test_bit(FLAG_SYSFS, &desc->flags) ||
+	    !pinctrl_gpio_can_use_line(chip->base + info->line_offset))
+		info->flags |= GPIOLINE_FLAG_KERNEL;
+	if (test_bit(FLAG_IS_OUT, &desc->flags))
+		info->flags |= GPIOLINE_FLAG_IS_OUT;
+	if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
+		info->flags |= GPIOLINE_FLAG_ACTIVE_LOW;
+	if (test_bit(FLAG_OPEN_DRAIN, &desc->flags))
+		info->flags |= (GPIOLINE_FLAG_OPEN_DRAIN |
+				GPIOLINE_FLAG_IS_OUT);
+	if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
+		info->flags |= (GPIOLINE_FLAG_OPEN_SOURCE |
+				GPIOLINE_FLAG_IS_OUT);
+	if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
+		info->flags |= GPIOLINE_FLAG_BIAS_DISABLE;
+	if (test_bit(FLAG_PULL_DOWN, &desc->flags))
+		info->flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
+	if (test_bit(FLAG_PULL_UP, &desc->flags))
+		info->flags |= GPIOLINE_FLAG_BIAS_PULL_UP;
+
+	spin_unlock_irqrestore(&gpio_lock, flags);
+}
+
 /*
  * gpio_ioctl() - ioctl handler for the GPIO chardev
  */
@@ -1185,49 +1239,7 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (IS_ERR(desc))
 			return PTR_ERR(desc);
 
-		if (desc->name) {
-			strncpy(lineinfo.name, desc->name,
-				sizeof(lineinfo.name));
-			lineinfo.name[sizeof(lineinfo.name)-1] = '\0';
-		} else {
-			lineinfo.name[0] = '\0';
-		}
-		if (desc->label) {
-			strncpy(lineinfo.consumer, desc->label,
-				sizeof(lineinfo.consumer));
-			lineinfo.consumer[sizeof(lineinfo.consumer)-1] = '\0';
-		} else {
-			lineinfo.consumer[0] = '\0';
-		}
-
-		/*
-		 * Userspace only need to know that the kernel is using
-		 * this GPIO so it can't use it.
-		 */
-		lineinfo.flags = 0;
-		if (test_bit(FLAG_REQUESTED, &desc->flags) ||
-		    test_bit(FLAG_IS_HOGGED, &desc->flags) ||
-		    test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
-		    test_bit(FLAG_EXPORT, &desc->flags) ||
-		    test_bit(FLAG_SYSFS, &desc->flags) ||
-		    !pinctrl_gpio_can_use_line(chip->base + lineinfo.line_offset))
-			lineinfo.flags |= GPIOLINE_FLAG_KERNEL;
-		if (test_bit(FLAG_IS_OUT, &desc->flags))
-			lineinfo.flags |= GPIOLINE_FLAG_IS_OUT;
-		if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
-			lineinfo.flags |= GPIOLINE_FLAG_ACTIVE_LOW;
-		if (test_bit(FLAG_OPEN_DRAIN, &desc->flags))
-			lineinfo.flags |= (GPIOLINE_FLAG_OPEN_DRAIN |
-					   GPIOLINE_FLAG_IS_OUT);
-		if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
-			lineinfo.flags |= (GPIOLINE_FLAG_OPEN_SOURCE |
-					   GPIOLINE_FLAG_IS_OUT);
-		if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
-			lineinfo.flags |= GPIOLINE_FLAG_BIAS_DISABLE;
-		if (test_bit(FLAG_PULL_DOWN, &desc->flags))
-			lineinfo.flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
-		if (test_bit(FLAG_PULL_UP, &desc->flags))
-			lineinfo.flags |= GPIOLINE_FLAG_BIAS_PULL_UP;
+		gpio_desc_to_lineinfo(desc, &lineinfo);
 
 		if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
 			return -EFAULT;
-- 
2.23.0

Re: [PATCH v2 06/11] gpiolib: use gpiochip_get_desc() in gpio_ioctl()

[Rainer Sickinger]

GREAT WORK

Am Mi., 4. Dez. 2019 um 17:04 Uhr schrieb Bartosz Golaszewski <brgl@bgdev.pl>:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Unduplicate the offset check by simply calling gpiochip_get_desc() and
> checking its return value.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpiolib.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 17796437d7be..b7043946c029 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -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
>

Re: [PATCH v2 01/11] gpiolib: use 'unsigned int' instead of 'unsigned' in gpio_set_config()

[Andy Shevchenko]

On Wed, Dec 4, 2019 at 6:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Checkpatch complains about using 'unsigned' instead of 'unsigned int'.
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpiolib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 21b02a0064f8..a31797fe78fa 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -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
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 02/11] gpiolib: have a single place of calling set_config()

[Andy Shevchenko]

On Wed, Dec 4, 2019 at 6:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> 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.
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpiolib.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index a31797fe78fa..72211407469f 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -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
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 03/11] gpiolib: convert the type of hwnum to unsigned int in gpiochip_get_desc()

[Andy Shevchenko]

On Wed, Dec 4, 2019 at 6:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> 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().
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpiolib.c      | 5 +++--
>  drivers/gpio/gpiolib.h      | 3 ++-
>  include/linux/gpio/driver.h | 3 ++-
>  3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 72211407469f..b3ffb079e323 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -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)
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index ca9bc1e4803c..a1cbeabadc69 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -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,
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index e2480ef94c55..4f032de10bae 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -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
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 04/11] gpiolib: use gpiochip_get_desc() in linehandle_create()

[Andy Shevchenko]

On Wed, Dec 4, 2019 at 7:18 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Unduplicate the ngpio check by simply calling gpiochip_get_desc() and
> checking its return value.
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Fun fact, I have for longer than a month a similar series, though not
baked well, and never had time to send it up.
Thanks you did this!

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpiolib.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index b3ffb079e323..6ef55cc1188b 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -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
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 05/11] gpiolib: use gpiochip_get_desc() in lineevent_create()

[Andy Shevchenko]

On Wed, Dec 4, 2019 at 6:05 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Unduplicate the ngpio check by simply calling gpiochip_get_desc() and
> checking its return value.
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpiolib.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 6ef55cc1188b..17796437d7be 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -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
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 06/11] gpiolib: use gpiochip_get_desc() in gpio_ioctl()

[Andy Shevchenko]

On Wed, Dec 4, 2019 at 6:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Unduplicate the offset check by simply calling gpiochip_get_desc() and
> checking its return value.
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>


> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpiolib.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 17796437d7be..b7043946c029 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -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
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 07/11] gpiolib: rework the locking mechanism for lineevent kfifo

[Andy Shevchenko]

On Wed, Dec 4, 2019 at 6:01 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> 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.
>

My comments below.

> +       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().


>         struct lineevent_state *le = filep->private_data;
> -       unsigned int copied;
> +       struct gpioevent_data event;
>         int ret;

> +       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).

> +       return sizeof(event);

Also see comments in my patch regarding the event handling.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 08/11] gpiolib: emit a debug message when adding events to a full kfifo

[Andy Shevchenko]

On Wed, Dec 4, 2019 at 6:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> 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?

> @@ -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;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/11] gpiolib: provide a dedicated function for setting lineinfo

[Andy Shevchenko]

On Wed, Dec 4, 2019 at 6:02 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> 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.

> +       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';
> +       }

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/11] gpiolib: provide a dedicated function for setting lineinfo

[Bartosz Golaszewski]

śr., 4 gru 2019 o 23:30 Andy Shevchenko <andy.shevchenko@gmail.com> napisał(a):
>
> On Wed, Dec 4, 2019 at 6:02 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > 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.
>
> > +       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';
> > +       }
>
> I think we have to fix GCC warnings first and then do whatever this patch does.
>

What GCC warnings are you referring to exactly?

Bart

> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2 07/11] gpiolib: rework the locking mechanism for lineevent kfifo

[Bartosz Golaszewski]

śr., 4 gru 2019 o 23:25 Andy Shevchenko <andy.shevchenko@gmail.com> napisał(a):
>
> On Wed, Dec 4, 2019 at 6:01 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > 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.
> >
>
> My comments below.
>
> > +       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().

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

>
>
> >         struct lineevent_state *le = filep->private_data;
> > -       unsigned int copied;
> > +       struct gpioevent_data event;
> >         int ret;
>
> > +       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.

Bart

> Btw, either way we have a benifits for the following one (I see you
> drop kfifo_to_user() and add event variable on stack).
>
> > +       return sizeof(event);
>
> Also see comments in my patch regarding the event handling.
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2 09/11] gpiolib: provide a dedicated function for setting lineinfo

[Andy Shevchenko]

On Thu, Dec 5, 2019 at 11:28 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> śr., 4 gru 2019 o 23:30 Andy Shevchenko <andy.shevchenko@gmail.com> napisał(a):
> > On Wed, Dec 4, 2019 at 6:02 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > > +       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';
> > > +       }
> >
> > 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.

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 07/11] gpiolib: rework the locking mechanism for lineevent kfifo

[Andy Shevchenko]

On Thu, Dec 5, 2019 at 11:31 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> śr., 4 gru 2019 o 23:25 Andy Shevchenko <andy.shevchenko@gmail.com> napisał(a):
> > On Wed, Dec 4, 2019 at 6:01 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > > +       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().
>
> Yeah, I noticed but I thought I'd just add it later separately - it's
> always easier to merge self-contained series.

...and easier to forget about.
But it's up to you :-)

> > >         struct lineevent_state *le = filep->private_data;
> > > -       unsigned int copied;
> > > +       struct gpioevent_data event;
> > >         int ret;
> >
> > > +       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.

Yea, I'm fine with it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/11] gpiolib: provide a dedicated function for setting lineinfo

[Bartosz Golaszewski]

czw., 5 gru 2019 o 11:21 Andy Shevchenko <andy.shevchenko@gmail.com> napisał(a):
>
> On Thu, Dec 5, 2019 at 11:28 AM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> > śr., 4 gru 2019 o 23:30 Andy Shevchenko <andy.shevchenko@gmail.com> napisał(a):
> > > On Wed, Dec 4, 2019 at 6:02 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > > > +       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';
> > > > +       }
> > >
> > > 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.
>
> 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 ?: "")`.

Bart

Re: [PATCH v2 09/11] gpiolib: provide a dedicated function for setting lineinfo

[Andy Shevchenko]

On Thu, Dec 5, 2019 at 3:45 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> czw., 5 gru 2019 o 11:21 Andy Shevchenko <andy.shevchenko@gmail.com> napisał(a):
> > On Thu, Dec 5, 2019 at 11:28 AM Bartosz Golaszewski
> > <bgolaszewski@baylibre.com> wrote:
> > > śr., 4 gru 2019 o 23:30 Andy Shevchenko <andy.shevchenko@gmail.com> napisał(a):
> > > > On Wed, Dec 4, 2019 at 6:02 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > > > > +       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';
> > > > > +       }
> > > >
> > > > 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.
> >
> > 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?

Some kind of
warning: ‘strncpy’ specified bound 16 equals destination size
[-Wstringop-truncation]

> I don't see any.

Good, I just checked and see none as well. It means GCC understands
that strncpy() is followed by guaranteed NUL-termination.

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

It makes sense only in above context.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 08/11] gpiolib: emit a debug message when adding events to a full kfifo

[Bartosz Golaszewski]

śr., 4 gru 2019 o 23:28 Andy Shevchenko <andy.shevchenko@gmail.com> napisał(a):
>
> On Wed, Dec 4, 2019 at 6:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > 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?
>

This is something that will be very rare and unlikely - I don't see
how trace points will help here with all the boiler-plate.

Bart