This patchset defines and implements adds a new version of the
GPIO CDEV uAPI to address existing 32/64bit alignment issues, add
support for debounce and event sequence numbers, and provide some
future proofing by adding padding reserved for future use.
The series can be partitioned into three sets; the first twelve
are minor code tidy ups or fixes that I ran across while implementing V2,
the next seven contain the V2 uAPI implementation proper, and the final
three port the GPIO tools to the V2 uAPI.
The more complicated patches include their own commentary where appropriate.
Cheers,
Kent.
Kent Gibson (22):
gpiolib: move gpiolib-sysfs function declarations into their own
header
gpiolib: cdev: sort includes
gpiolib: cdev: minor indentation fixes
gpiolib: cdev: refactor gpiohandle_flags_to_desc_flags
gpiolib: cdev: rename 'filep' and 'filp' to 'file' to be consistent
with other use
gpiolib: cdev: rename numdescs to num_descs
gpiolib: cdev: remove pointless decrement of i
gpiolib: cdev: complete the irq/thread timestamp handshake
gpiolib: cdev: rename priv to gcdev
gpiolib: cdev: fix minor race in GET_LINEINFO_WATCH
gpiolib: cdev: remove recalculation of offset
gpio: uapi: define GPIO_MAX_NAME_SIZE for array sizes
gpio: uapi: define uAPI V2
gpiolib: make cdev a build option
gpiolib: add build option for CDEV V1 ABI
gpiolib: cdev: add V2 uAPI implementation to parity with V1
gpiolib: cdev: report edge detection in lineinfo
gpiolib: cdev: support setting debounce
gpio: uapi: document uAPI V1 as deprecated
tools: gpio: switch tools to V2 uAPI
tools: gpio: add debounce support to gpio-event-mon
tools: gpio: support monitoring multiple lines
drivers/gpio/Kconfig | 28 +-
drivers/gpio/Makefile | 2 +-
drivers/gpio/gpiolib-cdev.c | 1610 ++++++++++++++++++++++++++++------
drivers/gpio/gpiolib-cdev.h | 15 +
drivers/gpio/gpiolib-sysfs.c | 1 +
drivers/gpio/gpiolib-sysfs.h | 24 +
drivers/gpio/gpiolib.c | 3 +
drivers/gpio/gpiolib.h | 24 +-
include/uapi/linux/gpio.h | 280 +++++-
tools/gpio/gpio-event-mon.c | 133 +--
tools/gpio/gpio-hammer.c | 28 +-
tools/gpio/gpio-utils.c | 107 +--
tools/gpio/gpio-utils.h | 48 +-
tools/gpio/gpio-watch.c | 10 +-
tools/gpio/lsgpio.c | 112 ++-
15 files changed, 1933 insertions(+), 492 deletions(-)
create mode 100644 drivers/gpio/gpiolib-sysfs.h
base-commit: 84651e81ee3323c7d544edfa6ac6026425fe5a52
--
2.27.0
Sort the includes of gpiolib-cdev.c to make it easier to identify if a
module is included and to avoid duplication.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index b8b872724628..55a9b7b44304 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1,24 +1,24 @@
// SPDX-License-Identifier: GPL-2.0
+#include <linux/anon_inodes.h>
#include <linux/bitmap.h>
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/interrupt.h>
-#include <linux/irqreturn.h>
-#include <linux/spinlock.h>
+#include <linux/cdev.h>
+#include <linux/compat.h>
#include <linux/device.h>
#include <linux/err.h>
+#include <linux/file.h>
#include <linux/gpio.h>
#include <linux/gpio/driver.h>
-#include <linux/pinctrl/consumer.h>
-#include <linux/cdev.h>
-#include <linux/uaccess.h>
-#include <linux/compat.h>
-#include <linux/anon_inodes.h>
-#include <linux/file.h>
+#include <linux/interrupt.h>
+#include <linux/irqreturn.h>
+#include <linux/kernel.h>
#include <linux/kfifo.h>
+#include <linux/module.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/poll.h>
+#include <linux/spinlock.h>
#include <linux/timekeeping.h>
+#include <linux/uaccess.h>
#include <uapi/linux/gpio.h>
#include "gpiolib.h"
--
2.27.0
Make indentation consistent with other use to improve readability.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 55a9b7b44304..889ed2dc9e58 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -98,7 +98,7 @@ static int linehandle_validate_flags(u32 flags)
/* Only one bias flag can be set. */
if (((flags & GPIOHANDLE_REQUEST_BIAS_DISABLE) &&
(flags & (GPIOHANDLE_REQUEST_BIAS_PULL_DOWN |
- GPIOHANDLE_REQUEST_BIAS_PULL_UP))) ||
+ GPIOHANDLE_REQUEST_BIAS_PULL_UP))) ||
((flags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN) &&
(flags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)))
return -EINVAL;
@@ -212,11 +212,11 @@ static long linehandle_ioctl(struct file *filep, unsigned int cmd,
/* Reuse the array setting function */
return gpiod_set_array_value_complex(false,
- true,
- lh->numdescs,
- lh->descs,
- NULL,
- vals);
+ true,
+ lh->numdescs,
+ lh->descs,
+ NULL,
+ vals);
} else if (cmd == GPIOHANDLE_SET_CONFIG_IOCTL) {
return linehandle_set_config(lh, ip);
}
@@ -225,7 +225,7 @@ static long linehandle_ioctl(struct file *filep, unsigned int cmd,
#ifdef CONFIG_COMPAT
static long linehandle_ioctl_compat(struct file *filep, unsigned int cmd,
- unsigned long arg)
+ unsigned long arg)
{
return linehandle_ioctl(filep, cmd, (unsigned long)compat_ptr(arg));
}
@@ -428,7 +428,7 @@ struct lineevent_state {
GPIOEVENT_REQUEST_FALLING_EDGE)
static __poll_t lineevent_poll(struct file *filep,
- struct poll_table_struct *wait)
+ struct poll_table_struct *wait)
{
struct lineevent_state *le = filep->private_data;
__poll_t events = 0;
@@ -720,11 +720,11 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
/* Request a thread to read the events */
ret = request_threaded_irq(le->irq,
- lineevent_irq_handler,
- lineevent_irq_thread,
- irqflags,
- le->label,
- le);
+ lineevent_irq_handler,
+ lineevent_irq_thread,
+ irqflags,
+ le->label,
+ le);
if (ret)
goto out_free_desc;
@@ -1052,7 +1052,7 @@ static ssize_t lineinfo_watch_read(struct file *filep, char __user *buf,
static int gpio_chrdev_open(struct inode *inode, struct file *filp)
{
struct gpio_device *gdev = container_of(inode->i_cdev,
- struct gpio_device, chrdev);
+ struct gpio_device, chrdev);
struct gpio_chardev_data *priv;
int ret = -ENOMEM;
--
2.27.0
Refactor the mapping from handle flags to desc flags into a helper
function.
The assign_bit is overkill where it is replacing the set_bit cases, as is
rechecking bits known to be clear in some circumstances, but the DRY
simplification more than makes up for any performance degradation,
especially as this is not a hot path.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 60 ++++++++++++-------------------------
1 file changed, 19 insertions(+), 41 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 889ed2dc9e58..e64613b8d0ba 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -106,6 +106,22 @@ static int linehandle_validate_flags(u32 flags)
return 0;
}
+static void linehandle_flags_to_desc_flags(u32 lflags, unsigned long *flagsp)
+{
+ assign_bit(FLAG_ACTIVE_LOW, flagsp,
+ lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW);
+ assign_bit(FLAG_OPEN_DRAIN, flagsp,
+ lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN);
+ assign_bit(FLAG_OPEN_SOURCE, flagsp,
+ lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE);
+ assign_bit(FLAG_PULL_UP, flagsp,
+ lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP);
+ assign_bit(FLAG_PULL_DOWN, flagsp,
+ lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN);
+ assign_bit(FLAG_BIAS_DISABLE, flagsp,
+ lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE);
+}
+
static long linehandle_set_config(struct linehandle_state *lh,
void __user *ip)
{
@@ -113,7 +129,6 @@ static long linehandle_set_config(struct linehandle_state *lh,
struct gpio_desc *desc;
int i, ret;
u32 lflags;
- unsigned long *flagsp;
if (copy_from_user(&gcnf, ip, sizeof(gcnf)))
return -EFAULT;
@@ -125,25 +140,7 @@ static long linehandle_set_config(struct linehandle_state *lh,
for (i = 0; i < lh->numdescs; i++) {
desc = lh->descs[i];
- flagsp = &desc->flags;
-
- assign_bit(FLAG_ACTIVE_LOW, flagsp,
- lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW);
-
- assign_bit(FLAG_OPEN_DRAIN, flagsp,
- lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN);
-
- assign_bit(FLAG_OPEN_SOURCE, flagsp,
- lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE);
-
- assign_bit(FLAG_PULL_UP, flagsp,
- lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP);
-
- assign_bit(FLAG_PULL_DOWN, flagsp,
- lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN);
-
- assign_bit(FLAG_BIAS_DISABLE, flagsp,
- lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE);
+ linehandle_flags_to_desc_flags(gcnf.flags, &desc->flags);
/*
* Lines have to be requested explicitly for input
@@ -306,19 +303,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
goto out_free_descs;
lh->descs[i] = desc;
count = i + 1;
-
- if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
- set_bit(FLAG_ACTIVE_LOW, &desc->flags);
- if (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN)
- set_bit(FLAG_OPEN_DRAIN, &desc->flags);
- if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
- set_bit(FLAG_OPEN_SOURCE, &desc->flags);
- if (lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE)
- set_bit(FLAG_BIAS_DISABLE, &desc->flags);
- if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)
- set_bit(FLAG_PULL_DOWN, &desc->flags);
- if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)
- set_bit(FLAG_PULL_UP, &desc->flags);
+ linehandle_flags_to_desc_flags(handlereq.flags, &desc->flags);
ret = gpiod_set_transitory(desc, false);
if (ret < 0)
@@ -685,14 +670,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
le->desc = desc;
le->eflags = eflags;
- if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
- set_bit(FLAG_ACTIVE_LOW, &desc->flags);
- if (lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE)
- set_bit(FLAG_BIAS_DISABLE, &desc->flags);
- if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)
- set_bit(FLAG_PULL_DOWN, &desc->flags);
- if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)
- set_bit(FLAG_PULL_UP, &desc->flags);
+ linehandle_flags_to_desc_flags(lflags, &desc->flags);
ret = gpiod_direction_input(desc);
if (ret)
--
2.27.0
Rename 'filep' and 'filp' to 'file' to be consistent with other use
and improve readability.
Signed-off-by: Kent Gibson <[email protected]>
---
The code was using both "filep" and "filp" and I flip flopped between
which one to change to until looking at code elsewhere in the kernel
where "struct file *file" is the most common and so going with that.
drivers/gpio/gpiolib-cdev.c | 70 ++++++++++++++++++-------------------
1 file changed, 35 insertions(+), 35 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index e64613b8d0ba..0d3a799e09ae 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -164,10 +164,10 @@ static long linehandle_set_config(struct linehandle_state *lh,
return 0;
}
-static long linehandle_ioctl(struct file *filep, unsigned int cmd,
+static long linehandle_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
- struct linehandle_state *lh = filep->private_data;
+ struct linehandle_state *lh = file->private_data;
void __user *ip = (void __user *)arg;
struct gpiohandle_data ghd;
DECLARE_BITMAP(vals, GPIOHANDLES_MAX);
@@ -221,16 +221,16 @@ static long linehandle_ioctl(struct file *filep, unsigned int cmd,
}
#ifdef CONFIG_COMPAT
-static long linehandle_ioctl_compat(struct file *filep, unsigned int cmd,
+static long linehandle_ioctl_compat(struct file *file, unsigned int cmd,
unsigned long arg)
{
- return linehandle_ioctl(filep, cmd, (unsigned long)compat_ptr(arg));
+ return linehandle_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
}
#endif
-static int linehandle_release(struct inode *inode, struct file *filep)
+static int linehandle_release(struct inode *inode, struct file *file)
{
- struct linehandle_state *lh = filep->private_data;
+ struct linehandle_state *lh = file->private_data;
struct gpio_device *gdev = lh->gdev;
int i;
@@ -412,13 +412,13 @@ struct lineevent_state {
(GPIOEVENT_REQUEST_RISING_EDGE | \
GPIOEVENT_REQUEST_FALLING_EDGE)
-static __poll_t lineevent_poll(struct file *filep,
+static __poll_t lineevent_poll(struct file *file,
struct poll_table_struct *wait)
{
- struct lineevent_state *le = filep->private_data;
+ struct lineevent_state *le = file->private_data;
__poll_t events = 0;
- poll_wait(filep, &le->wait, wait);
+ poll_wait(file, &le->wait, wait);
if (!kfifo_is_empty_spinlocked_noirqsave(&le->events, &le->wait.lock))
events = EPOLLIN | EPOLLRDNORM;
@@ -427,12 +427,12 @@ static __poll_t lineevent_poll(struct file *filep,
}
-static ssize_t lineevent_read(struct file *filep,
+static ssize_t lineevent_read(struct file *file,
char __user *buf,
size_t count,
loff_t *f_ps)
{
- struct lineevent_state *le = filep->private_data;
+ struct lineevent_state *le = file->private_data;
struct gpioevent_data ge;
ssize_t bytes_read = 0;
int ret;
@@ -448,7 +448,7 @@ static ssize_t lineevent_read(struct file *filep,
return bytes_read;
}
- if (filep->f_flags & O_NONBLOCK) {
+ if (file->f_flags & O_NONBLOCK) {
spin_unlock(&le->wait.lock);
return -EAGAIN;
}
@@ -481,9 +481,9 @@ static ssize_t lineevent_read(struct file *filep,
return bytes_read;
}
-static int lineevent_release(struct inode *inode, struct file *filep)
+static int lineevent_release(struct inode *inode, struct file *file)
{
- struct lineevent_state *le = filep->private_data;
+ struct lineevent_state *le = file->private_data;
struct gpio_device *gdev = le->gdev;
free_irq(le->irq, le);
@@ -494,10 +494,10 @@ static int lineevent_release(struct inode *inode, struct file *filep)
return 0;
}
-static long lineevent_ioctl(struct file *filep, unsigned int cmd,
+static long lineevent_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
- struct lineevent_state *le = filep->private_data;
+ struct lineevent_state *le = file->private_data;
void __user *ip = (void __user *)arg;
struct gpiohandle_data ghd;
@@ -524,10 +524,10 @@ static long lineevent_ioctl(struct file *filep, unsigned int cmd,
}
#ifdef CONFIG_COMPAT
-static long lineevent_ioctl_compat(struct file *filep, unsigned int cmd,
+static long lineevent_ioctl_compat(struct file *file, unsigned int cmd,
unsigned long arg)
{
- return lineevent_ioctl(filep, cmd, (unsigned long)compat_ptr(arg));
+ return lineevent_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
}
#endif
@@ -826,9 +826,9 @@ struct gpio_chardev_data {
/*
* gpio_ioctl() - ioctl handler for the GPIO chardev
*/
-static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
- struct gpio_chardev_data *priv = filp->private_data;
+ struct gpio_chardev_data *priv = file->private_data;
struct gpio_device *gdev = priv->gdev;
struct gpio_chip *gc = gdev->chip;
void __user *ip = (void __user *)arg;
@@ -919,10 +919,10 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
}
#ifdef CONFIG_COMPAT
-static long gpio_ioctl_compat(struct file *filp, unsigned int cmd,
+static long gpio_ioctl_compat(struct file *file, unsigned int cmd,
unsigned long arg)
{
- return gpio_ioctl(filp, cmd, (unsigned long)compat_ptr(arg));
+ return gpio_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
}
#endif
@@ -958,13 +958,13 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
return NOTIFY_OK;
}
-static __poll_t lineinfo_watch_poll(struct file *filep,
+static __poll_t lineinfo_watch_poll(struct file *file,
struct poll_table_struct *pollt)
{
- struct gpio_chardev_data *priv = filep->private_data;
+ struct gpio_chardev_data *priv = file->private_data;
__poll_t events = 0;
- poll_wait(filep, &priv->wait, pollt);
+ poll_wait(file, &priv->wait, pollt);
if (!kfifo_is_empty_spinlocked_noirqsave(&priv->events,
&priv->wait.lock))
@@ -973,10 +973,10 @@ static __poll_t lineinfo_watch_poll(struct file *filep,
return events;
}
-static ssize_t lineinfo_watch_read(struct file *filep, char __user *buf,
+static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
size_t count, loff_t *off)
{
- struct gpio_chardev_data *priv = filep->private_data;
+ struct gpio_chardev_data *priv = file->private_data;
struct gpioline_info_changed event;
ssize_t bytes_read = 0;
int ret;
@@ -992,7 +992,7 @@ static ssize_t lineinfo_watch_read(struct file *filep, char __user *buf,
return bytes_read;
}
- if (filep->f_flags & O_NONBLOCK) {
+ if (file->f_flags & O_NONBLOCK) {
spin_unlock(&priv->wait.lock);
return -EAGAIN;
}
@@ -1024,10 +1024,10 @@ static ssize_t lineinfo_watch_read(struct file *filep, char __user *buf,
/**
* gpio_chrdev_open() - open the chardev for ioctl operations
* @inode: inode for this chardev
- * @filp: file struct for storing private data
+ * @file: file struct for storing private data
* Returns 0 on success
*/
-static int gpio_chrdev_open(struct inode *inode, struct file *filp)
+static int gpio_chrdev_open(struct inode *inode, struct file *file)
{
struct gpio_device *gdev = container_of(inode->i_cdev,
struct gpio_device, chrdev);
@@ -1057,9 +1057,9 @@ static int gpio_chrdev_open(struct inode *inode, struct file *filp)
goto out_free_bitmap;
get_device(&gdev->dev);
- filp->private_data = priv;
+ file->private_data = priv;
- ret = nonseekable_open(inode, filp);
+ ret = nonseekable_open(inode, file);
if (ret)
goto out_unregister_notifier;
@@ -1078,12 +1078,12 @@ static int gpio_chrdev_open(struct inode *inode, struct file *filp)
/**
* gpio_chrdev_release() - close chardev after ioctl operations
* @inode: inode for this chardev
- * @filp: file struct for storing private data
+ * @file: file struct for storing private data
* Returns 0 on success
*/
-static int gpio_chrdev_release(struct inode *inode, struct file *filp)
+static int gpio_chrdev_release(struct inode *inode, struct file *file)
{
- struct gpio_chardev_data *priv = filp->private_data;
+ struct gpio_chardev_data *priv = file->private_data;
struct gpio_device *gdev = priv->gdev;
bitmap_free(priv->watched_lines);
--
2.27.0
Rename numdescs to num_descs to be more consistent with the naming of
other counters and improve readability.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 0d3a799e09ae..b39e7ef8c0d4 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -39,13 +39,13 @@
* @gdev: the GPIO device the handle pertains to
* @label: consumer label used to tag descriptors
* @descs: the GPIO descriptors held by this handle
- * @numdescs: the number of descriptors held in the descs array
+ * @num_descs: the number of descriptors held in the descs array
*/
struct linehandle_state {
struct gpio_device *gdev;
const char *label;
struct gpio_desc *descs[GPIOHANDLES_MAX];
- u32 numdescs;
+ u32 num_descs;
};
#define GPIOHANDLE_REQUEST_VALID_FLAGS \
@@ -138,7 +138,7 @@ static long linehandle_set_config(struct linehandle_state *lh,
if (ret)
return ret;
- for (i = 0; i < lh->numdescs; i++) {
+ for (i = 0; i < lh->num_descs; i++) {
desc = lh->descs[i];
linehandle_flags_to_desc_flags(gcnf.flags, &desc->flags);
@@ -177,7 +177,7 @@ static long linehandle_ioctl(struct file *file, unsigned int cmd,
/* NOTE: It's ok to read values of output lines. */
int ret = gpiod_get_array_value_complex(false,
true,
- lh->numdescs,
+ lh->num_descs,
lh->descs,
NULL,
vals);
@@ -185,7 +185,7 @@ static long linehandle_ioctl(struct file *file, unsigned int cmd,
return ret;
memset(&ghd, 0, sizeof(ghd));
- for (i = 0; i < lh->numdescs; i++)
+ for (i = 0; i < lh->num_descs; i++)
ghd.values[i] = test_bit(i, vals);
if (copy_to_user(ip, &ghd, sizeof(ghd)))
@@ -204,13 +204,13 @@ static long linehandle_ioctl(struct file *file, unsigned int cmd,
return -EFAULT;
/* Clamp all values to [0,1] */
- for (i = 0; i < lh->numdescs; i++)
+ for (i = 0; i < lh->num_descs; i++)
__assign_bit(i, vals, ghd.values[i]);
/* Reuse the array setting function */
return gpiod_set_array_value_complex(false,
true,
- lh->numdescs,
+ lh->num_descs,
lh->descs,
NULL,
vals);
@@ -234,7 +234,7 @@ static int linehandle_release(struct inode *inode, struct file *file)
struct gpio_device *gdev = lh->gdev;
int i;
- for (i = 0; i < lh->numdescs; i++)
+ for (i = 0; i < lh->num_descs; i++)
gpiod_free(lh->descs[i]);
kfree(lh->label);
kfree(lh);
@@ -333,7 +333,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
}
/* Let i point at the last handle */
i--;
- lh->numdescs = handlereq.lines;
+ lh->num_descs = handlereq.lines;
fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
if (fd < 0) {
@@ -364,7 +364,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
fd_install(fd, file);
dev_dbg(&gdev->dev, "registered chardev handle for %d lines\n",
- lh->numdescs);
+ lh->num_descs);
return 0;
--
2.27.0
Remove pointless decrement of variable, and associated comment.
While i is used subsequently, it is re-initialized so this decrement
serves no purpose.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index b39e7ef8c0d4..d50339ef6f05 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -331,8 +331,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
offset);
}
- /* Let i point at the last handle */
- i--;
lh->num_descs = handlereq.lines;
fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
--
2.27.0
Reset the timestamp field to 0 after using it in lineevent_irq_thread.
The timestamp is set by lineevent_irq_handler and is tested by
lineevent_irq_thread to determine if it is called from a nested theaded
interrupt.
lineevent_irq_thread is assuming that the nested, or otherwise, status
of the IRQ is static, i.e. it is either always nested or never nested.
This change removes that assumption, resetting the timestamp so it can
be re-used to determine the nested state of subsequent interrupts.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index d50339ef6f05..1e8e0a0a9b51 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -559,6 +559,8 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
else
ge.timestamp = le->timestamp;
+ le->timestamp = 0;
+
if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE
&& le->eflags & GPIOEVENT_REQUEST_FALLING_EDGE) {
int level = gpiod_get_value_cansleep(le->desc);
--
2.27.0
Rename priv to gcdev to improve readability.
The name "priv" indicates that the object is pointed to by
file->private_data, not what the object is actually is.
It is always used to point to a struct gpio_chardev_data so renaming
it to gcdev seemed as good as anything, and certainly clearer than "priv".
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 90 ++++++++++++++++++-------------------
1 file changed, 45 insertions(+), 45 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 1e8e0a0a9b51..5f5b715ed7f7 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -828,8 +828,8 @@ struct gpio_chardev_data {
*/
static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
- struct gpio_chardev_data *priv = file->private_data;
- struct gpio_device *gdev = priv->gdev;
+ struct gpio_chardev_data *gcdev = file->private_data;
+ struct gpio_device *gdev = gcdev->gdev;
struct gpio_chip *gc = gdev->chip;
void __user *ip = (void __user *)arg;
struct gpio_desc *desc;
@@ -889,7 +889,7 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
hwgpio = gpio_chip_hwgpio(desc);
- if (test_bit(hwgpio, priv->watched_lines))
+ if (test_bit(hwgpio, gcdev->watched_lines))
return -EBUSY;
gpio_desc_to_lineinfo(desc, &lineinfo);
@@ -897,7 +897,7 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
return -EFAULT;
- set_bit(hwgpio, priv->watched_lines);
+ set_bit(hwgpio, gcdev->watched_lines);
return 0;
} else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) {
if (copy_from_user(&offset, ip, sizeof(offset)))
@@ -909,10 +909,10 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
hwgpio = gpio_chip_hwgpio(desc);
- if (!test_bit(hwgpio, priv->watched_lines))
+ if (!test_bit(hwgpio, gcdev->watched_lines))
return -EBUSY;
- clear_bit(hwgpio, priv->watched_lines);
+ clear_bit(hwgpio, gcdev->watched_lines);
return 0;
}
return -EINVAL;
@@ -935,12 +935,12 @@ to_gpio_chardev_data(struct notifier_block *nb)
static int lineinfo_changed_notify(struct notifier_block *nb,
unsigned long action, void *data)
{
- struct gpio_chardev_data *priv = to_gpio_chardev_data(nb);
+ struct gpio_chardev_data *gcdev = to_gpio_chardev_data(nb);
struct gpioline_info_changed chg;
struct gpio_desc *desc = data;
int ret;
- if (!test_bit(gpio_chip_hwgpio(desc), priv->watched_lines))
+ if (!test_bit(gpio_chip_hwgpio(desc), gcdev->watched_lines))
return NOTIFY_DONE;
memset(&chg, 0, sizeof(chg));
@@ -949,9 +949,9 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
chg.timestamp = ktime_get_ns();
gpio_desc_to_lineinfo(desc, &chg.info);
- ret = kfifo_in_spinlocked(&priv->events, &chg, 1, &priv->wait.lock);
+ ret = kfifo_in_spinlocked(&gcdev->events, &chg, 1, &gcdev->wait.lock);
if (ret)
- wake_up_poll(&priv->wait, EPOLLIN);
+ wake_up_poll(&gcdev->wait, EPOLLIN);
else
pr_debug_ratelimited("lineinfo event FIFO is full - event dropped\n");
@@ -961,13 +961,13 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
static __poll_t lineinfo_watch_poll(struct file *file,
struct poll_table_struct *pollt)
{
- struct gpio_chardev_data *priv = file->private_data;
+ struct gpio_chardev_data *gcdev = file->private_data;
__poll_t events = 0;
- poll_wait(file, &priv->wait, pollt);
+ poll_wait(file, &gcdev->wait, pollt);
- if (!kfifo_is_empty_spinlocked_noirqsave(&priv->events,
- &priv->wait.lock))
+ if (!kfifo_is_empty_spinlocked_noirqsave(&gcdev->events,
+ &gcdev->wait.lock))
events = EPOLLIN | EPOLLRDNORM;
return events;
@@ -976,7 +976,7 @@ static __poll_t lineinfo_watch_poll(struct file *file,
static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
size_t count, loff_t *off)
{
- struct gpio_chardev_data *priv = file->private_data;
+ struct gpio_chardev_data *gcdev = file->private_data;
struct gpioline_info_changed event;
ssize_t bytes_read = 0;
int ret;
@@ -985,28 +985,28 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
return -EINVAL;
do {
- spin_lock(&priv->wait.lock);
- if (kfifo_is_empty(&priv->events)) {
+ spin_lock(&gcdev->wait.lock);
+ if (kfifo_is_empty(&gcdev->events)) {
if (bytes_read) {
- spin_unlock(&priv->wait.lock);
+ spin_unlock(&gcdev->wait.lock);
return bytes_read;
}
if (file->f_flags & O_NONBLOCK) {
- spin_unlock(&priv->wait.lock);
+ spin_unlock(&gcdev->wait.lock);
return -EAGAIN;
}
- ret = wait_event_interruptible_locked(priv->wait,
- !kfifo_is_empty(&priv->events));
+ ret = wait_event_interruptible_locked(gcdev->wait,
+ !kfifo_is_empty(&gcdev->events));
if (ret) {
- spin_unlock(&priv->wait.lock);
+ spin_unlock(&gcdev->wait.lock);
return ret;
}
}
- ret = kfifo_out(&priv->events, &event, 1);
- spin_unlock(&priv->wait.lock);
+ ret = kfifo_out(&gcdev->events, &event, 1);
+ spin_unlock(&gcdev->wait.lock);
if (ret != 1) {
ret = -EIO;
break;
@@ -1031,33 +1031,33 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
{
struct gpio_device *gdev = container_of(inode->i_cdev,
struct gpio_device, chrdev);
- struct gpio_chardev_data *priv;
+ struct gpio_chardev_data *gcdev;
int ret = -ENOMEM;
/* Fail on open if the backing gpiochip is gone */
if (!gdev->chip)
return -ENODEV;
- priv = kzalloc(sizeof(*priv), GFP_KERNEL);
- if (!priv)
+ gcdev = kzalloc(sizeof(*gcdev), GFP_KERNEL);
+ if (!gcdev)
return -ENOMEM;
- priv->watched_lines = bitmap_zalloc(gdev->chip->ngpio, GFP_KERNEL);
- if (!priv->watched_lines)
- goto out_free_priv;
+ gcdev->watched_lines = bitmap_zalloc(gdev->chip->ngpio, GFP_KERNEL);
+ if (!gcdev->watched_lines)
+ goto out_free_gcdev;
- init_waitqueue_head(&priv->wait);
- INIT_KFIFO(priv->events);
- priv->gdev = gdev;
+ init_waitqueue_head(&gcdev->wait);
+ INIT_KFIFO(gcdev->events);
+ gcdev->gdev = gdev;
- priv->lineinfo_changed_nb.notifier_call = lineinfo_changed_notify;
+ gcdev->lineinfo_changed_nb.notifier_call = lineinfo_changed_notify;
ret = atomic_notifier_chain_register(&gdev->notifier,
- &priv->lineinfo_changed_nb);
+ &gcdev->lineinfo_changed_nb);
if (ret)
goto out_free_bitmap;
get_device(&gdev->dev);
- file->private_data = priv;
+ file->private_data = gcdev;
ret = nonseekable_open(inode, file);
if (ret)
@@ -1067,11 +1067,11 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
out_unregister_notifier:
atomic_notifier_chain_unregister(&gdev->notifier,
- &priv->lineinfo_changed_nb);
+ &gcdev->lineinfo_changed_nb);
out_free_bitmap:
- bitmap_free(priv->watched_lines);
-out_free_priv:
- kfree(priv);
+ bitmap_free(gcdev->watched_lines);
+out_free_gcdev:
+ kfree(gcdev);
return ret;
}
@@ -1083,14 +1083,14 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
*/
static int gpio_chrdev_release(struct inode *inode, struct file *file)
{
- struct gpio_chardev_data *priv = file->private_data;
- struct gpio_device *gdev = priv->gdev;
+ struct gpio_chardev_data *gcdev = file->private_data;
+ struct gpio_device *gdev = gcdev->gdev;
- bitmap_free(priv->watched_lines);
+ bitmap_free(gcdev->watched_lines);
atomic_notifier_chain_unregister(&gdev->notifier,
- &priv->lineinfo_changed_nb);
+ &gcdev->lineinfo_changed_nb);
put_device(&gdev->dev);
- kfree(priv);
+ kfree(gcdev);
return 0;
}
--
2.27.0
Merge separate usage of test_bit/set_bit into test_and_set_bit to remove
the possibility of a race between the test and set.
Similarly test_bit and clear_bit.
In the existing code it is possible for two threads to race past the
test_bit and then set or clear the watch bit, and neither return EBUSY.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 5f5b715ed7f7..a727709b24a9 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -889,7 +889,7 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
hwgpio = gpio_chip_hwgpio(desc);
- if (test_bit(hwgpio, gcdev->watched_lines))
+ if (test_and_set_bit(hwgpio, gcdev->watched_lines))
return -EBUSY;
gpio_desc_to_lineinfo(desc, &lineinfo);
@@ -897,7 +897,6 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
return -EFAULT;
- set_bit(hwgpio, gcdev->watched_lines);
return 0;
} else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) {
if (copy_from_user(&offset, ip, sizeof(offset)))
@@ -909,10 +908,9 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
hwgpio = gpio_chip_hwgpio(desc);
- if (!test_bit(hwgpio, gcdev->watched_lines))
+ if (!test_and_clear_bit(hwgpio, gcdev->watched_lines))
return -EBUSY;
- clear_bit(hwgpio, gcdev->watched_lines);
return 0;
}
return -EINVAL;
--
2.27.0
Remove recalculation of offset from desc, where desc itself was calculated
from offset.
There is no benefit from for the desc -> hwgpio conversion in this
context. The only implicit benefit of the offset -> desc -> hwgpio is
the range check in the offset -> desc, but where desc is required you
still get that, and where desc isn't required it is simpler to perform
the range check directly.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index a727709b24a9..b6878fc87dfc 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -834,7 +834,6 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
void __user *ip = (void __user *)arg;
struct gpio_desc *desc;
__u32 offset;
- int hwgpio;
/* We fail any subsequent ioctl():s when the chip is gone */
if (!gc)
@@ -862,12 +861,11 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
return -EFAULT;
+ /* this doubles as a range check on line_offset */
desc = gpiochip_get_desc(gc, lineinfo.line_offset);
if (IS_ERR(desc))
return PTR_ERR(desc);
- hwgpio = gpio_chip_hwgpio(desc);
-
gpio_desc_to_lineinfo(desc, &lineinfo);
if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
@@ -883,13 +881,12 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
return -EFAULT;
+ /* this doubles as a range check on line_offset */
desc = gpiochip_get_desc(gc, lineinfo.line_offset);
if (IS_ERR(desc))
return PTR_ERR(desc);
- hwgpio = gpio_chip_hwgpio(desc);
-
- if (test_and_set_bit(hwgpio, gcdev->watched_lines))
+ if (test_and_set_bit(lineinfo.line_offset, gcdev->watched_lines))
return -EBUSY;
gpio_desc_to_lineinfo(desc, &lineinfo);
@@ -902,13 +899,10 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
if (copy_from_user(&offset, ip, sizeof(offset)))
return -EFAULT;
- desc = gpiochip_get_desc(gc, offset);
- if (IS_ERR(desc))
- return PTR_ERR(desc);
-
- hwgpio = gpio_chip_hwgpio(desc);
+ if (offset >= gcdev->gdev->ngpio)
+ return -EINVAL;
- if (!test_and_clear_bit(hwgpio, gcdev->watched_lines))
+ if (!test_and_clear_bit(offset, gcdev->watched_lines))
return -EBUSY;
return 0;
--
2.27.0
Replace constant array sizes with a macro constant to clarify the source
of array sizes, provide a place to document any constraints on the size,
and to simplify array sizing in userspace if constructing structs
from their composite fields.
Signed-off-by: Kent Gibson <[email protected]>
---
This change is not terribly important for V1, but in adding V2 more
documentation for the usage of this value is appropriate.
As it is also used with V1 it warrants a separate patch.
include/uapi/linux/gpio.h | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 0206383c0383..4e1139ab25bc 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -14,6 +14,11 @@
#include <linux/ioctl.h>
#include <linux/types.h>
+/*
+ * The maximum size of name and label arrays.
+ */
+#define GPIO_MAX_NAME_SIZE 32
+
/**
* struct gpiochip_info - Information about a certain GPIO chip
* @name: the Linux kernel name of this GPIO chip
@@ -22,8 +27,8 @@
* @lines: number of GPIO lines on this chip
*/
struct gpiochip_info {
- char name[32];
- char label[32];
+ char name[GPIO_MAX_NAME_SIZE];
+ char label[GPIO_MAX_NAME_SIZE];
__u32 lines;
};
@@ -52,8 +57,8 @@ struct gpiochip_info {
struct gpioline_info {
__u32 line_offset;
__u32 flags;
- char name[32];
- char consumer[32];
+ char name[GPIO_MAX_NAME_SIZE];
+ char consumer[GPIO_MAX_NAME_SIZE];
};
/* Maximum number of requested handles */
@@ -123,7 +128,7 @@ struct gpiohandle_request {
__u32 lineoffsets[GPIOHANDLES_MAX];
__u32 flags;
__u8 default_values[GPIOHANDLES_MAX];
- char consumer_label[32];
+ char consumer_label[GPIO_MAX_NAME_SIZE];
__u32 lines;
int fd;
};
@@ -182,7 +187,7 @@ struct gpioevent_request {
__u32 lineoffset;
__u32 handleflags;
__u32 eventflags;
- char consumer_label[32];
+ char consumer_label[GPIO_MAX_NAME_SIZE];
int fd;
};
--
2.27.0
Make the gpiolib-cdev module a build option. This allows the CDEV
interface to be removed from the kernel to reduce kernel size in
applications where is it not required, and provides the parent for
other other CDEV interface specific build options to follow.
Suggested-by: Bartosz Golaszewski <[email protected]>
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/Kconfig | 16 ++++++++++++++--
drivers/gpio/Makefile | 2 +-
drivers/gpio/gpiolib-cdev.h | 15 +++++++++++++++
3 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index c6b5c65c8405..affc1524bc2c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -66,8 +66,20 @@ config GPIO_SYSFS
This ABI is deprecated. If you want to use GPIO from userspace,
use the character device /dev/gpiochipN with the appropriate
- ioctl() operations instead. The character device is always
- available.
+ ioctl() operations instead.
+
+config GPIO_CDEV
+ bool "/dev/gpiochipN (character device interface)"
+ default y
+ help
+ Say Y here to add the character device /dev/gpiochipN interface
+ for GPIOs. The character device allows userspace to control GPIOs
+ using ioctl() operations.
+
+ Only say N is you are sure that the GPIO character device is not
+ required.
+
+ If unsure, say Y.
config GPIO_GENERIC
depends on HAS_IOMEM # Only for IOMEM drivers
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index ef666cfef9d0..45eb09808d12 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -7,8 +7,8 @@ obj-$(CONFIG_GPIOLIB) += gpiolib.o
obj-$(CONFIG_GPIOLIB) += gpiolib-devres.o
obj-$(CONFIG_GPIOLIB) += gpiolib-legacy.o
obj-$(CONFIG_GPIOLIB) += gpiolib-devprop.o
-obj-$(CONFIG_GPIOLIB) += gpiolib-cdev.o
obj-$(CONFIG_OF_GPIO) += gpiolib-of.o
+obj-$(CONFIG_GPIO_CDEV) += gpiolib-cdev.o
obj-$(CONFIG_GPIO_SYSFS) += gpiolib-sysfs.o
obj-$(CONFIG_GPIO_ACPI) += gpiolib-acpi.o
diff --git a/drivers/gpio/gpiolib-cdev.h b/drivers/gpio/gpiolib-cdev.h
index 973578e7ad10..19a4e3d57120 100644
--- a/drivers/gpio/gpiolib-cdev.h
+++ b/drivers/gpio/gpiolib-cdev.h
@@ -5,7 +5,22 @@
#include <linux/device.h>
+#ifdef CONFIG_GPIO_CDEV
+
int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt);
void gpiolib_cdev_unregister(struct gpio_device *gdev);
+#else
+
+static inline int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
+{
+ return 0;
+}
+
+static inline void gpiolib_cdev_unregister(struct gpio_device *gdev)
+{
+}
+
+#endif /* CONFIG_GPIO_CDEV */
+
#endif /* GPIOLIB_CDEV_H */
--
2.27.0
Add a new version of the uAPI to address existing 32/64bit alignment
issues, add support for debounce and event sequence numbers, and provide
some future proofing by adding padding reserved for future use.
The alignment issue relates to the gpioevent_data, which packs to different
sizes on 32bit and 64bit platforms. That creates problems for 32bit apps
running on 64bit kernels. The patch addresses that particular issue, and
the problem more generally, by adding pad fields that explicitly pad
structs out to 64bit boundaries, so they will pack to the same size now,
and even if some of the reserved padding is used for __u64 fields in the
future.
The lack of future proofing in V1 makes it impossible to, for example,
add the debounce feature that is included in v2.
The future proofing is addressed by providing reserved padding in all
structs for future features. Specifically, the line request,
config, info, info_changed and event structs receive updated versions,
and the first three new ioctls.
Signed-off-by: Kent Gibson <[email protected]>
---
I haven't added any padding to gpiochip_info, as I haven't seen any calls
for new features for the corresponding ioctl, but I'm open to updating that
as well.
As the majority of the structs and ioctls were being replaced, it seemed
opportune to rework some of the other aspects of the uAPI.
Firstly, I've reworked the flags field throughout. V1 has three different
flags fields, each with their own separate bit definitions. In V2 that is
collapsed to one. Further, the bits of the V2 flags field are used
as feature enable flags, with any other necessary configuration fields encoded
separately. This is simpler and clearer, while also providing a foundation
for adding features in the future.
I've also merged the handle and event requests into a single request, the
line request, as the two requests where mostly the same, other than the
edge detection provided by event requests. As a byproduct, the V2 uAPI
allows for multiple lines producing edge events on the same line handle.
This is a new capability as V1 only supports a single line in an event
request.
This means there are now only two types of file handle to be concerned with,
the chip and the line, and it is clearer which ioctls apply to which type
of handle.
There is also some minor renaming of fields for consistency compared to their
V1 counterparts, e.g. offset rather than lineoffset or line_offset, and
consumer rather than consumer_label.
Additionally, V1 GPIOHANDLES_MAX becomes GPIOLINES_MAX in V2 for clarity,
and the gpiohandle_data __u8 array becomes a bitmap gpioline_values.
The V2 uAPI is mostly just a reorganisation of V1, so userspace code,
particularly libgpiod, should easily port to it.
The padding sizes have been carried over from the RFC version, although the
seqnos added to the gpioline_event alone would've used the all of the padding
for that struct, had they not been added here. So I'm moderatly concerned
that those values are too small due to a lack of imagination on may part and
should be increased to decrease the probability of running out of space in
the padding and requiring creative solutions or even a V3.
Changes since the RFC:
- document the constraints on array sizes to maintain 32/64 alignment
- add sequence numbers to gpioline_event
- use bitmap for values instead of array of __u8
- gpioline_info_v2 contains gpioline_config instead of its composite fields
- provide constants for all array sizes, especially padding
- renamed "GPIOLINE_FLAG_V2_KERNEL" to "GPIOLINE_FLAG_V2_USED"
- renamed "default_values" to "values"
- made gpioline_direction zero based
- document clock used in gpioline_event timestamp
- add event_buffer_size to gpioline_request
include/uapi/linux/gpio.h | 237 ++++++++++++++++++++++++++++++++++++--
1 file changed, 230 insertions(+), 7 deletions(-)
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 4e1139ab25bc..e4ed6f79e332 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -11,11 +11,14 @@
#ifndef _UAPI_GPIO_H_
#define _UAPI_GPIO_H_
+#include <linux/kernel.h>
#include <linux/ioctl.h>
#include <linux/types.h>
/*
* The maximum size of name and label arrays.
+ *
+ * Must be a multiple of 8 to ensure 32/64bit alignment of structs.
*/
#define GPIO_MAX_NAME_SIZE 32
@@ -32,6 +35,211 @@ struct gpiochip_info {
__u32 lines;
};
+/*
+ * Maximum number of requested lines.
+ *
+ * Must be a multiple of 8 to ensure 32/64bit alignment of structs.
+ */
+#define GPIOLINES_MAX 64
+
+/* the number of __u64 required for a bitmap for GPIOLINES_MAX lines */
+#define GPIOLINES_BITMAP_SIZE __KERNEL_DIV_ROUND_UP(GPIOLINES_MAX, 64)
+
+enum gpioline_direction {
+ GPIOLINE_DIRECTION_INPUT = 0,
+ GPIOLINE_DIRECTION_OUTPUT = 1,
+};
+
+enum gpioline_drive {
+ GPIOLINE_DRIVE_PUSH_PULL = 0,
+ GPIOLINE_DRIVE_OPEN_DRAIN = 1,
+ GPIOLINE_DRIVE_OPEN_SOURCE = 2,
+};
+
+enum gpioline_bias {
+ GPIOLINE_BIAS_DISABLED = 0,
+ GPIOLINE_BIAS_PULL_UP = 1,
+ GPIOLINE_BIAS_PULL_DOWN = 2,
+};
+
+enum gpioline_edge {
+ GPIOLINE_EDGE_NONE = 0,
+ GPIOLINE_EDGE_RISING = 1,
+ GPIOLINE_EDGE_FALLING = 2,
+ GPIOLINE_EDGE_BOTH = 3,
+};
+
+/* Line flags - V2 */
+#define GPIOLINE_FLAG_V2_USED (1UL << 0) /* line is not available for request */
+#define GPIOLINE_FLAG_V2_ACTIVE_LOW (1UL << 1)
+#define GPIOLINE_FLAG_V2_DIRECTION (1UL << 2)
+#define GPIOLINE_FLAG_V2_DRIVE (1UL << 3)
+#define GPIOLINE_FLAG_V2_BIAS (1UL << 4)
+#define GPIOLINE_FLAG_V2_EDGE_DETECTION (1UL << 5)
+#define GPIOLINE_FLAG_V2_DEBOUNCE (1UL << 6)
+
+/*
+ * Struct padding sizes.
+ *
+ * These are sized to pad structs to 64bit boundaries.
+ * To maintain 32/64bit alignment, any arbitrary change must be even, as
+ * the pad elements are __u32.
+ */
+#define GPIOLINE_CONFIG_PAD_SIZE 7
+#define GPIOLINE_REQUEST_PAD_SIZE 5
+#define GPIOLINE_INFO_V2_PAD_SIZE 5
+#define GPIOLINE_INFO_CHANGED_V2_PAD_SIZE 5
+#define GPIOLINE_EVENT_PAD_SIZE 2
+
+/**
+ * struct gpioline_values - Values of GPIO lines
+ * @bitmap: a bitmap containing the value of the lines, set to 1 for active
+ * and 0 for inactive. Note that this is the logical value, which will be
+ * the opposite of the physical value if GPIOLINE_FLAG_V2_ACTIVE_LOW is
+ * set in flags.
+ */
+struct gpioline_values {
+ __u64 bitmap[GPIOLINES_BITMAP_SIZE];
+};
+
+/**
+ * struct gpioline_config - Configuration for GPIO lines
+ * @values: if the direction is GPIOLINE_DIRECTION_OUTPUT, the values that
+ * the lines will be set to. This field is write-only and is zeroed when
+ * returned within struct gpioline_info.
+ * @flags: flags for the GPIO lines, such as GPIOLINE_FLAG_V2_ACTIVE_LOW,
+ * GPIOLINE_FLAG_V2_DIRECTION etc, OR:ed together
+ * @direction: if GPIOLINE_FLAG_V2_DIRECTION is set in flags, the desired
+ * direction for the requested lines, with a value from enum
+ * gpioline_direction
+ * @drive: if GPIOLINE_FLAG_V2_DRIVE is set in flags, the desired drive for
+ * the requested lines, with a value from enum gpioline_drive
+ * @bias: if GPIOLINE_FLAG_V2_BIAS is set in flags, the desired bias for
+ * the requested lines, with a value from enum gpioline_bias
+ * @edge_detection: if GPIOLINE_FLAG_V2_EDGE_DETECTION is set in flags, the
+ * desired edge_detection for the requested lines, with a value from enum
+ * gpioline_edge
+ * @debounce_period: if GPIOLINE_FLAG_V2_DEBOUNCE is set in flags, the
+ * desired debounce period for the requested lines, in microseconds
+ * @padding: reserved for future use and must be zero filled
+ */
+struct gpioline_config {
+ struct gpioline_values values;
+ __u32 flags;
+ /* Note that the following four fields are equivalent to a single u32. */
+ __u8 direction;
+ __u8 drive;
+ __u8 bias;
+ __u8 edge_detection;
+ __u32 debounce_period;
+ __u32 padding[GPIOLINE_CONFIG_PAD_SIZE]; /* for future use */
+};
+
+/**
+ * struct gpioline_request - Information about a request for GPIO lines
+ * @offsets: an array of desired lines, specified by offset index for the
+ * associated GPIO device
+ * @consumer: a desired consumer label for the selected GPIO lines such as
+ * "my-bitbanged-relay"
+ * @config: requested configuration for the requested lines. Note that even
+ * if multiple lines are requested, the same configuration must be
+ * applicable to all of them. If you want lines with individual
+ * configuration, request them one by one. It is possible to select a batch
+ * of input or output lines, but they must all have the same configuration,
+ * i.e. all inputs or all outputs, all active low etc
+ * @num_lines: number of lines requested in this request, i.e. the number
+ * of valid fields in the GPIOLINES_MAX sized arrays, set to 1 to request a
+ * single line
+ * @event_buffer_size: a suggested minimum number of line events that the
+ * kernel should buffer. This is only relevant if edge_detection is
+ * enabled. Note that this is only a suggested value and the kernel may
+ * allocate a larger buffer or cap the size of the buffer. If this field is
+ * zero then the buffer size defaults to a minimum of num_lines*16.
+ * @padding: reserved for future use and must be zero filled
+ * @fd: if successful this field will contain a valid anonymous file handle
+ * after a GPIO_GET_LINE_IOCTL operation, zero or negative value means
+ * error
+ */
+struct gpioline_request {
+ __u32 offsets[GPIOLINES_MAX];
+ char consumer[GPIO_MAX_NAME_SIZE];
+ struct gpioline_config config;
+ __u32 num_lines;
+ __u32 event_buffer_size;
+ __u32 padding[GPIOLINE_REQUEST_PAD_SIZE]; /* for future use */
+ __s32 fd;
+};
+
+/**
+ * struct gpioline_info_v2 - Information about a certain GPIO line
+ * @name: the name of this GPIO line, such as the output pin of the line on
+ * the chip, a rail or a pin header name on a board, as specified by the
+ * gpio chip, may be empty
+ * @consumer: a functional name for the consumer of this GPIO line as set
+ * by whatever is using it, will be empty if there is no current user but
+ * may also be empty if the consumer doesn't set this up
+ * @config: the configuration of the line. Note that the values field is
+ * always zeroed - the line must be requested to read the values.
+ * @offset: the local offset on this GPIO device, fill this in when
+ * requesting the line information from the kernel
+ * @padding: reserved for future use
+ */
+struct gpioline_info_v2 {
+ char name[GPIO_MAX_NAME_SIZE];
+ char consumer[GPIO_MAX_NAME_SIZE];
+ struct gpioline_config config;
+ __u32 offset;
+ __u32 padding[GPIOLINE_INFO_V2_PAD_SIZE]; /* for future use */
+};
+
+/**
+ * struct gpioline_info_changed_v2 - Information about a change in status
+ * of a GPIO line
+ * @info: updated line information
+ * @timestamp: estimate of time of status change occurrence, in nanoseconds
+ * and GPIOLINE_CHANGED_CONFIG
+ * @event_type: one of GPIOLINE_CHANGED_REQUESTED, GPIOLINE_CHANGED_RELEASED
+ * @padding: reserved for future use
+ */
+struct gpioline_info_changed_v2 {
+ struct gpioline_info_v2 info;
+ __u64 timestamp;
+ __u32 event_type;
+ __u32 padding[GPIOLINE_INFO_CHANGED_V2_PAD_SIZE]; /* for future use */
+};
+
+enum gpioline_event_id {
+ GPIOLINE_EVENT_RISING_EDGE = 1,
+ GPIOLINE_EVENT_FALLING_EDGE = 2,
+};
+
+/**
+ * struct gpioline_event - The actual event being pushed to userspace
+ * @timestamp: best estimate of time of event occurrence, in nanoseconds.
+ * The timestamp is read from CLOCK_MONOTONIC and is intended to allow the
+ * accurate measurement of the time between events. It does not provide
+ * the wall-clock time.
+ * @id: event identifier with value from enum gpioline_event_id
+ * @offset: the offset of the line that triggered the event
+ * @seqno: the sequence number for this event in the sequence of events for
+ * all the lines in this line request
+ * @line_seqno: the sequence number for this event in the sequence of
+ * events on this particular line
+ * @padding: reserved for future use
+ */
+struct gpioline_event {
+ __u64 timestamp;
+ __u32 id;
+ __u32 offset;
+ __u32 seqno;
+ __u32 line_seqno;
+ __u32 padding[GPIOLINE_EVENT_PAD_SIZE]; /* for future use */
+};
+
+/*
+ * ABI V1
+ */
+
/* Informational flags */
#define GPIOLINE_FLAG_KERNEL (1UL << 0) /* Line used by the kernel */
#define GPIOLINE_FLAG_IS_OUT (1UL << 1)
@@ -149,8 +357,6 @@ struct gpiohandle_config {
__u32 padding[4]; /* padding for future use */
};
-#define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0a, struct gpiohandle_config)
-
/**
* struct gpiohandle_data - Information of values on a GPIO handle
* @values: when getting the state of lines this contains the current
@@ -161,9 +367,6 @@ struct gpiohandle_data {
__u8 values[GPIOHANDLES_MAX];
};
-#define GPIOHANDLE_GET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x08, struct gpiohandle_data)
-#define GPIOHANDLE_SET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x09, struct gpiohandle_data)
-
/* Eventrequest flags */
#define GPIOEVENT_REQUEST_RISING_EDGE (1UL << 0)
#define GPIOEVENT_REQUEST_FALLING_EDGE (1UL << 1)
@@ -207,11 +410,31 @@ struct gpioevent_data {
__u32 id;
};
+/*
+ * V1 and V2 ioctl()s
+ */
#define GPIO_GET_CHIPINFO_IOCTL _IOR(0xB4, 0x01, struct gpiochip_info)
+#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0C, __u32)
+
+/*
+ * V2 ioctl()s
+ */
+#define GPIO_GET_LINEINFO_V2_IOCTL _IOWR(0xB4, 0x05, struct gpioline_info_v2)
+#define GPIO_GET_LINEINFO_WATCH_V2_IOCTL _IOWR(0xB4, 0x06, struct gpioline_info_v2)
+#define GPIO_GET_LINE_IOCTL _IOWR(0xB4, 0x07, struct gpioline_request)
+#define GPIOLINE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0D, struct gpioline_config)
+#define GPIOLINE_GET_VALUES_IOCTL _IOWR(0xB4, 0x0E, struct gpioline_values)
+#define GPIOLINE_SET_VALUES_IOCTL _IOWR(0xB4, 0x0F, struct gpioline_values)
+
+/*
+ * V1 ioctl()s
+ */
#define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
-#define GPIO_GET_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0b, struct gpioline_info)
-#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0c, __u32)
#define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
#define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request)
+#define GPIOHANDLE_GET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x08, struct gpiohandle_data)
+#define GPIOHANDLE_SET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x09, struct gpiohandle_data)
+#define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0A, struct gpiohandle_config)
+#define GPIO_GET_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0B, struct gpioline_info)
#endif /* _UAPI_GPIO_H_ */
--
2.27.0
Update uAPI documentation to deprecate V1 structs and ioctls.
Signed-off-by: Kent Gibson <[email protected]>
---
include/uapi/linux/gpio.h | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index e4ed6f79e332..752d63f56b0d 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -238,6 +238,9 @@ struct gpioline_event {
/*
* ABI V1
+ *
+ * This version of the ABI is deprecated and will be removed in the future.
+ * Use the latest version if the ABI, defined above, instead.
*/
/* Informational flags */
@@ -261,6 +264,9 @@ struct gpioline_event {
* @consumer: a functional name for the consumer of this GPIO line as set by
* whatever is using it, will be empty if there is no current user but may
* also be empty if the consumer doesn't set this up
+ *
+ * This struct part of ABI V1 and is deprecated.
+ * Use struct gpioline_info_v2 instead.
*/
struct gpioline_info {
__u32 line_offset;
@@ -292,6 +298,9 @@ enum {
* guarantee there are no implicit holes between it and subsequent members.
* The 20-byte padding at the end makes sure we don't add any implicit padding
* at the end of the structure on 64-bit architectures.
+ *
+ * This struct part of ABI V1 and is deprecated.
+ * Use struct gpioline_info_changed_v2 instead.
*/
struct gpioline_info_changed {
struct gpioline_info info;
@@ -331,6 +340,9 @@ struct gpioline_info_changed {
* @fd: if successful this field will contain a valid anonymous file handle
* after a GPIO_GET_LINEHANDLE_IOCTL operation, zero or negative value
* means error
+ *
+ * This struct part of ABI V1 and is deprecated.
+ * Use struct gpioline_request instead.
*/
struct gpiohandle_request {
__u32 lineoffsets[GPIOHANDLES_MAX];
@@ -350,6 +362,9 @@ struct gpiohandle_request {
* this specifies the default output value, should be 0 (low) or
* 1 (high), anything else than 0 or 1 will be interpreted as 1 (high)
* @padding: reserved for future use and should be zero filled
+ *
+ * This struct part of ABI V1 and is deprecated.
+ * Use struct gpioline_config instead.
*/
struct gpiohandle_config {
__u32 flags;
@@ -362,6 +377,9 @@ struct gpiohandle_config {
* @values: when getting the state of lines this contains the current
* state of a line, when setting the state of lines these should contain
* the desired target state
+ *
+ * This struct part of ABI V1 and is deprecated.
+ * Use struct gpioline_values instead.
*/
struct gpiohandle_data {
__u8 values[GPIOHANDLES_MAX];
@@ -385,6 +403,9 @@ struct gpiohandle_data {
* @fd: if successful this field will contain a valid anonymous file handle
* after a GPIO_GET_LINEEVENT_IOCTL operation, zero or negative value
* means error
+ *
+ * This struct part of ABI V1 and is deprecated.
+ * Use struct gpioline_request instead.
*/
struct gpioevent_request {
__u32 lineoffset;
@@ -404,6 +425,9 @@ struct gpioevent_request {
* struct gpioevent_data - The actual event being pushed to userspace
* @timestamp: best estimate of time of event occurrence, in nanoseconds
* @id: event identifier
+ *
+ * This struct part of ABI V1 and is deprecated.
+ * Use struct gpioline_event instead.
*/
struct gpioevent_data {
__u64 timestamp;
@@ -428,6 +452,8 @@ struct gpioevent_data {
/*
* V1 ioctl()s
+ *
+ * These ioctl()s are deprecated. Use the V2 equivalent instead.
*/
#define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
#define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
--
2.27.0
Update all the GPIO tools to use uAPI V2 instead of uAPI V1.
The tools command lines and behaviour remain unchanged, although lsgpio
now reports additional information not available through V1, specifically
the edge detection and debounce configuration.
Signed-off-by: Kent Gibson <[email protected]>
---
tools/gpio/gpio-event-mon.c | 91 ++++++++++++++---------------
tools/gpio/gpio-hammer.c | 28 +++++----
tools/gpio/gpio-utils.c | 107 ++++++++++++++++++----------------
tools/gpio/gpio-utils.h | 48 +++++++++++++---
tools/gpio/gpio-watch.c | 10 ++--
tools/gpio/lsgpio.c | 112 +++++++++++++++++-------------------
6 files changed, 217 insertions(+), 179 deletions(-)
diff --git a/tools/gpio/gpio-event-mon.c b/tools/gpio/gpio-event-mon.c
index 30ed0e06f52a..d8d692f67b9e 100644
--- a/tools/gpio/gpio-event-mon.c
+++ b/tools/gpio/gpio-event-mon.c
@@ -23,17 +23,16 @@
#include <sys/ioctl.h>
#include <sys/types.h>
#include <linux/gpio.h>
+#include "gpio-utils.h"
int monitor_device(const char *device_name,
unsigned int line,
- uint32_t handleflags,
- uint32_t eventflags,
+ struct gpioline_config *config,
unsigned int loops)
{
- struct gpioevent_request req;
- struct gpiohandle_data data;
+ struct gpioline_values values;
char *chrdev_name;
- int fd;
+ int cfd, lfd;
int ret;
int i = 0;
@@ -41,44 +40,37 @@ int monitor_device(const char *device_name,
if (ret < 0)
return -ENOMEM;
- fd = open(chrdev_name, 0);
- if (fd == -1) {
+ cfd = open(chrdev_name, 0);
+ if (cfd == -1) {
ret = -errno;
fprintf(stderr, "Failed to open %s\n", chrdev_name);
- goto exit_close_error;
+ goto exit_free_name;
}
- req.lineoffset = line;
- req.handleflags = handleflags;
- req.eventflags = eventflags;
- strcpy(req.consumer_label, "gpio-event-mon");
-
- ret = ioctl(fd, GPIO_GET_LINEEVENT_IOCTL, &req);
- if (ret == -1) {
- ret = -errno;
- fprintf(stderr, "Failed to issue GET EVENT "
- "IOCTL (%d)\n",
- ret);
- goto exit_close_error;
- }
+ ret = gpiotools_request_line(device_name, &line, 1, config,
+ "gpio-event-mon");
+ if (ret < 0)
+ goto exit_device_close;
+ else
+ lfd = ret;
/* Read initial states */
- ret = ioctl(req.fd, GPIOHANDLE_GET_LINE_VALUES_IOCTL, &data);
- if (ret == -1) {
- ret = -errno;
- fprintf(stderr, "Failed to issue GPIOHANDLE GET LINE "
- "VALUES IOCTL (%d)\n",
+ ret = gpiotools_get_values(lfd, &values);
+ if (ret < 0) {
+ fprintf(stderr,
+ "Failed to issue GPIO LINE GET VALUES IOCTL (%d)\n",
ret);
- goto exit_close_error;
+ goto exit_line_close;
}
fprintf(stdout, "Monitoring line %d on %s\n", line, device_name);
- fprintf(stdout, "Initial line value: %d\n", data.values[0]);
+ fprintf(stdout, "Initial line value: %d\n",
+ gpiotools_test_bit(values.bitmap, 0));
while (1) {
- struct gpioevent_data event;
+ struct gpioline_event event;
- ret = read(req.fd, &event, sizeof(event));
+ ret = read(lfd, &event, sizeof(event));
if (ret == -1) {
if (errno == -EAGAIN) {
fprintf(stderr, "nothing available\n");
@@ -96,12 +88,14 @@ int monitor_device(const char *device_name,
ret = -EIO;
break;
}
- fprintf(stdout, "GPIO EVENT %llu: ", event.timestamp);
+ fprintf(stdout, "GPIO EVENT at %llu on line %d (%d|%d) ",
+ event.timestamp, event.offset, event.line_seqno,
+ event.seqno);
switch (event.id) {
- case GPIOEVENT_EVENT_RISING_EDGE:
+ case GPIOLINE_EVENT_RISING_EDGE:
fprintf(stdout, "rising edge");
break;
- case GPIOEVENT_EVENT_FALLING_EDGE:
+ case GPIOLINE_EVENT_FALLING_EDGE:
fprintf(stdout, "falling edge");
break;
default:
@@ -114,9 +108,13 @@ int monitor_device(const char *device_name,
break;
}
-exit_close_error:
- if (close(fd) == -1)
+exit_line_close:
+ if (close(lfd) == -1)
+ perror("Failed to close line file");
+exit_device_close:
+ if (close(cfd) == -1)
perror("Failed to close GPIO character device file");
+exit_free_name:
free(chrdev_name);
return ret;
}
@@ -144,10 +142,12 @@ int main(int argc, char **argv)
const char *device_name = NULL;
unsigned int line = -1;
unsigned int loops = 0;
- uint32_t handleflags = GPIOHANDLE_REQUEST_INPUT;
- uint32_t eventflags = 0;
+ struct gpioline_config config;
int c;
+ memset(&config, 0, sizeof(config));
+ config.flags = GPIOLINE_FLAG_V2_DIRECTION | GPIOLINE_FLAG_V2_EDGE_DETECTION;
+ config.direction = GPIOLINE_DIRECTION_INPUT;
while ((c = getopt(argc, argv, "c:n:o:dsrf?")) != -1) {
switch (c) {
case 'c':
@@ -160,16 +160,18 @@ int main(int argc, char **argv)
line = strtoul(optarg, NULL, 10);
break;
case 'd':
- handleflags |= GPIOHANDLE_REQUEST_OPEN_DRAIN;
+ config.flags |= GPIOLINE_FLAG_V2_DRIVE;
+ config.drive = GPIOLINE_DRIVE_OPEN_DRAIN;
break;
case 's':
- handleflags |= GPIOHANDLE_REQUEST_OPEN_SOURCE;
+ config.flags |= GPIOLINE_FLAG_V2_DRIVE;
+ config.drive = GPIOLINE_DRIVE_OPEN_SOURCE;
break;
case 'r':
- eventflags |= GPIOEVENT_REQUEST_RISING_EDGE;
+ config.edge_detection |= GPIOLINE_EDGE_RISING;
break;
case 'f':
- eventflags |= GPIOEVENT_REQUEST_FALLING_EDGE;
+ config.edge_detection |= GPIOLINE_EDGE_FALLING;
break;
case '?':
print_usage();
@@ -181,11 +183,10 @@ int main(int argc, char **argv)
print_usage();
return -1;
}
- if (!eventflags) {
+ if (!config.edge_detection) {
printf("No flags specified, listening on both rising and "
"falling edges\n");
- eventflags = GPIOEVENT_REQUEST_BOTH_EDGES;
+ config.edge_detection = GPIOLINE_EDGE_BOTH;
}
- return monitor_device(device_name, line, handleflags,
- eventflags, loops);
+ return monitor_device(device_name, line, &config, loops);
}
diff --git a/tools/gpio/gpio-hammer.c b/tools/gpio/gpio-hammer.c
index 9fd926e8cb52..d517ff7d5646 100644
--- a/tools/gpio/gpio-hammer.c
+++ b/tools/gpio/gpio-hammer.c
@@ -25,23 +25,26 @@
int hammer_device(const char *device_name, unsigned int *lines, int nlines,
unsigned int loops)
{
- struct gpiohandle_data data;
+ struct gpioline_values values;
+ struct gpioline_config config;
char swirr[] = "-\\|/";
int fd;
int ret;
int i, j;
unsigned int iteration = 0;
- memset(&data.values, 0, sizeof(data.values));
- ret = gpiotools_request_linehandle(device_name, lines, nlines,
- GPIOHANDLE_REQUEST_OUTPUT, &data,
- "gpio-hammer");
+ memset(&config, 0, sizeof(config));
+ config.flags = GPIOLINE_FLAG_V2_DIRECTION;
+ config.direction = GPIOLINE_DIRECTION_OUTPUT;
+
+ ret = gpiotools_request_line(device_name, lines, nlines,
+ &config, "gpio-hammer");
if (ret < 0)
goto exit_error;
else
fd = ret;
- ret = gpiotools_get_values(fd, &data);
+ ret = gpiotools_get_values(fd, &values);
if (ret < 0)
goto exit_close_error;
@@ -53,7 +56,7 @@ int hammer_device(const char *device_name, unsigned int *lines, int nlines,
}
fprintf(stdout, "] on %s, initial states: [", device_name);
for (i = 0; i < nlines; i++) {
- fprintf(stdout, "%d", data.values[i]);
+ fprintf(stdout, "%d", gpiotools_test_bit(values.bitmap, i));
if (i != (nlines - 1))
fprintf(stdout, ", ");
}
@@ -64,14 +67,14 @@ int hammer_device(const char *device_name, unsigned int *lines, int nlines,
while (1) {
/* Invert all lines so we blink */
for (i = 0; i < nlines; i++)
- data.values[i] = !data.values[i];
+ gpiotools_change_bit(values.bitmap, i);
- ret = gpiotools_set_values(fd, &data);
+ ret = gpiotools_set_values(fd, &values);
if (ret < 0)
goto exit_close_error;
/* Re-read values to get status */
- ret = gpiotools_get_values(fd, &data);
+ ret = gpiotools_get_values(fd, &values);
if (ret < 0)
goto exit_close_error;
@@ -82,7 +85,8 @@ int hammer_device(const char *device_name, unsigned int *lines, int nlines,
fprintf(stdout, "[");
for (i = 0; i < nlines; i++) {
- fprintf(stdout, "%d: %d", lines[i], data.values[i]);
+ fprintf(stdout, "%d: %d", lines[i],
+ gpiotools_test_bit(values.bitmap, i));
if (i != (nlines - 1))
fprintf(stdout, ", ");
}
@@ -97,7 +101,7 @@ int hammer_device(const char *device_name, unsigned int *lines, int nlines,
ret = 0;
exit_close_error:
- gpiotools_release_linehandle(fd);
+ gpiotools_release_line(fd);
exit_error:
return ret;
}
diff --git a/tools/gpio/gpio-utils.c b/tools/gpio/gpio-utils.c
index 06003789e7c7..5f2ff0f1e4e6 100644
--- a/tools/gpio/gpio-utils.c
+++ b/tools/gpio/gpio-utils.c
@@ -33,34 +33,33 @@
* release these lines.
*/
/**
- * gpiotools_request_linehandle() - request gpio lines in a gpiochip
+ * gpiotools_request_line() - request gpio lines in a gpiochip
* @device_name: The name of gpiochip without prefix "/dev/",
* such as "gpiochip0"
* @lines: An array desired lines, specified by offset
* index for the associated GPIO device.
* @nline: The number of lines to request.
- * @flag: The new flag for requsted gpio. Reference
- * "linux/gpio.h" for the meaning of flag.
+ * @config: The new config for requested gpio. Reference
+ * "linux/gpio.h" for config details.
* @data: Default value will be set to gpio when flag is
* GPIOHANDLE_REQUEST_OUTPUT.
- * @consumer_label: The name of consumer, such as "sysfs",
+ * @consumer: The name of consumer, such as "sysfs",
* "powerkey". This is useful for other users to
* know who is using.
*
* Request gpio lines through the ioctl provided by chardev. User
* could call gpiotools_set_values() and gpiotools_get_values() to
* read and write respectively through the returned fd. Call
- * gpiotools_release_linehandle() to release these lines after that.
+ * gpiotools_release_line() to release these lines after that.
*
* Return: On success return the fd;
* On failure return the errno.
*/
-int gpiotools_request_linehandle(const char *device_name, unsigned int *lines,
- unsigned int nlines, unsigned int flag,
- struct gpiohandle_data *data,
- const char *consumer_label)
+int gpiotools_request_line(const char *device_name, unsigned int *lines,
+ unsigned int nlines, struct gpioline_config *config,
+ const char *consumer)
{
- struct gpiohandle_request req;
+ struct gpioline_request req;
char *chrdev_name;
int fd;
int i;
@@ -75,45 +74,44 @@ int gpiotools_request_linehandle(const char *device_name, unsigned int *lines,
ret = -errno;
fprintf(stderr, "Failed to open %s, %s\n",
chrdev_name, strerror(errno));
- goto exit_close_error;
+ goto exit_free_name;
}
+ memset(&req, 0, sizeof(req));
for (i = 0; i < nlines; i++)
- req.lineoffsets[i] = lines[i];
+ req.offsets[i] = lines[i];
- req.flags = flag;
- strcpy(req.consumer_label, consumer_label);
- req.lines = nlines;
- if (flag & GPIOHANDLE_REQUEST_OUTPUT)
- memcpy(req.default_values, data, sizeof(req.default_values));
+ req.config = *config;
+ strcpy(req.consumer, consumer);
+ req.num_lines = nlines;
- ret = ioctl(fd, GPIO_GET_LINEHANDLE_IOCTL, &req);
+ ret = ioctl(fd, GPIO_GET_LINE_IOCTL, &req);
if (ret == -1) {
ret = -errno;
fprintf(stderr, "Failed to issue %s (%d), %s\n",
- "GPIO_GET_LINEHANDLE_IOCTL", ret, strerror(errno));
+ "GPIO_GET_LINE_IOCTL", ret, strerror(errno));
}
-exit_close_error:
if (close(fd) == -1)
perror("Failed to close GPIO character device file");
+exit_free_name:
free(chrdev_name);
return ret < 0 ? ret : req.fd;
}
/**
* gpiotools_set_values(): Set the value of gpio(s)
* @fd: The fd returned by
- * gpiotools_request_linehandle().
- * @data: The array of values want to set.
+ * gpiotools_request_line().
+ * @values: The array of values want to set.
*
* Return: On success return 0;
* On failure return the errno.
*/
-int gpiotools_set_values(const int fd, struct gpiohandle_data *data)
+int gpiotools_set_values(const int fd, struct gpioline_values *values)
{
int ret;
- ret = ioctl(fd, GPIOHANDLE_SET_LINE_VALUES_IOCTL, data);
+ ret = ioctl(fd, GPIOLINE_SET_VALUES_IOCTL, values);
if (ret == -1) {
ret = -errno;
fprintf(stderr, "Failed to issue %s (%d), %s\n",
@@ -127,17 +125,17 @@ int gpiotools_set_values(const int fd, struct gpiohandle_data *data)
/**
* gpiotools_get_values(): Get the value of gpio(s)
* @fd: The fd returned by
- * gpiotools_request_linehandle().
- * @data: The array of values get from hardware.
+ * gpiotools_request_line().
+ * @values: The array of values get from hardware.
*
* Return: On success return 0;
* On failure return the errno.
*/
-int gpiotools_get_values(const int fd, struct gpiohandle_data *data)
+int gpiotools_get_values(const int fd, struct gpioline_values *values)
{
int ret;
- ret = ioctl(fd, GPIOHANDLE_GET_LINE_VALUES_IOCTL, data);
+ ret = ioctl(fd, GPIOLINE_GET_VALUES_IOCTL, values);
if (ret == -1) {
ret = -errno;
fprintf(stderr, "Failed to issue %s (%d), %s\n",
@@ -149,14 +147,14 @@ int gpiotools_get_values(const int fd, struct gpiohandle_data *data)
}
/**
- * gpiotools_release_linehandle(): Release the line(s) of gpiochip
+ * gpiotools_release_line(): Release the line(s) of gpiochip
* @fd: The fd returned by
- * gpiotools_request_linehandle().
+ * gpiotools_request_line().
*
* Return: On success return 0;
* On failure return the errno.
*/
-int gpiotools_release_linehandle(const int fd)
+int gpiotools_release_line(const int fd)
{
int ret;
@@ -180,11 +178,11 @@ int gpiotools_release_linehandle(const int fd)
*/
int gpiotools_get(const char *device_name, unsigned int line)
{
- struct gpiohandle_data data;
+ struct gpioline_values values;
unsigned int lines[] = {line};
- gpiotools_gets(device_name, lines, 1, &data);
- return data.values[0];
+ gpiotools_gets(device_name, lines, 1, &values);
+ return values.bitmap[0] & 1;
}
@@ -195,27 +193,30 @@ int gpiotools_get(const char *device_name, unsigned int line)
* @lines: An array desired lines, specified by offset
* index for the associated GPIO device.
* @nline: The number of lines to request.
- * @data: The array of values get from gpiochip.
+ * @values: The array of values get from gpiochip.
*
* Return: On success return 0;
* On failure return the errno.
*/
int gpiotools_gets(const char *device_name, unsigned int *lines,
- unsigned int nlines, struct gpiohandle_data *data)
+ unsigned int nlines, struct gpioline_values *values)
{
int fd;
int ret;
int ret_close;
+ struct gpioline_config config;
- ret = gpiotools_request_linehandle(device_name, lines, nlines,
- GPIOHANDLE_REQUEST_INPUT, data,
- CONSUMER);
+ memset(&config, 0, sizeof(config));
+ config.flags = GPIOLINE_FLAG_V2_DIRECTION;
+ config.direction = GPIOLINE_DIRECTION_INPUT;
+ ret = gpiotools_request_line(device_name, lines, nlines,
+ &config, CONSUMER);
if (ret < 0)
return ret;
fd = ret;
- ret = gpiotools_get_values(fd, data);
- ret_close = gpiotools_release_linehandle(fd);
+ ret = gpiotools_get_values(fd, values);
+ ret_close = gpiotools_release_line(fd);
return ret < 0 ? ret : ret_close;
}
@@ -232,11 +233,13 @@ int gpiotools_gets(const char *device_name, unsigned int *lines,
int gpiotools_set(const char *device_name, unsigned int line,
unsigned int value)
{
- struct gpiohandle_data data;
+ struct gpioline_values values;
unsigned int lines[] = {line};
- data.values[0] = value;
- return gpiotools_sets(device_name, lines, 1, &data);
+ memset(&values, 0, sizeof(values));
+ if (value)
+ values.bitmap[0] |= 1;
+ return gpiotools_sets(device_name, lines, 1, &values);
}
/**
@@ -246,22 +249,26 @@ int gpiotools_set(const char *device_name, unsigned int line,
* @lines: An array desired lines, specified by offset
* index for the associated GPIO device.
* @nline: The number of lines to request.
- * @data: The array of values set to gpiochip, must be
+ * @value: The array of values set to gpiochip, must be
* 0(low) or 1(high).
*
* Return: On success return 0;
* On failure return the errno.
*/
int gpiotools_sets(const char *device_name, unsigned int *lines,
- unsigned int nlines, struct gpiohandle_data *data)
+ unsigned int nlines, struct gpioline_values *values)
{
int ret;
+ struct gpioline_config config;
- ret = gpiotools_request_linehandle(device_name, lines, nlines,
- GPIOHANDLE_REQUEST_OUTPUT, data,
- CONSUMER);
+ memset(&config, 0, sizeof(config));
+ config.flags = GPIOLINE_FLAG_V2_DIRECTION;
+ config.direction = GPIOLINE_DIRECTION_OUTPUT;
+ config.values = *values;
+ ret = gpiotools_request_line(device_name, lines, nlines,
+ &config, CONSUMER);
if (ret < 0)
return ret;
- return gpiotools_release_linehandle(ret);
+ return gpiotools_release_line(ret);
}
diff --git a/tools/gpio/gpio-utils.h b/tools/gpio/gpio-utils.h
index cf37f13f3dcb..9357765b6b79 100644
--- a/tools/gpio/gpio-utils.h
+++ b/tools/gpio/gpio-utils.h
@@ -12,7 +12,9 @@
#ifndef _GPIO_UTILS_H_
#define _GPIO_UTILS_H_
+#include <stdbool.h>
#include <string.h>
+#include <linux/types.h>
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
@@ -22,20 +24,48 @@ static inline int check_prefix(const char *str, const char *prefix)
strncmp(str, prefix, strlen(prefix)) == 0;
}
-int gpiotools_request_linehandle(const char *device_name, unsigned int *lines,
- unsigned int nlines, unsigned int flag,
- struct gpiohandle_data *data,
- const char *consumer_label);
-int gpiotools_set_values(const int fd, struct gpiohandle_data *data);
-int gpiotools_get_values(const int fd, struct gpiohandle_data *data);
-int gpiotools_release_linehandle(const int fd);
+int gpiotools_request_line(const char *device_name, unsigned int *lines,
+ unsigned int nlines, struct gpioline_config *config,
+ const char *consumer_label);
+int gpiotools_set_values(const int fd, struct gpioline_values *values);
+int gpiotools_get_values(const int fd, struct gpioline_values *values);
+int gpiotools_release_line(const int fd);
int gpiotools_get(const char *device_name, unsigned int line);
int gpiotools_gets(const char *device_name, unsigned int *lines,
- unsigned int nlines, struct gpiohandle_data *data);
+ unsigned int nlines, struct gpioline_values *values);
int gpiotools_set(const char *device_name, unsigned int line,
unsigned int value);
int gpiotools_sets(const char *device_name, unsigned int *lines,
- unsigned int nlines, struct gpiohandle_data *data);
+ unsigned int nlines, struct gpioline_values *values);
+
+/* helper functions for gpioline_values bitmap bits */
+static inline void gpiotools_set_bit(__u64 b[], int n)
+{
+ b[n>>6] |= 1UL << (n%64);
+}
+
+static inline void gpiotools_change_bit(__u64 b[], int n)
+{
+ b[n>>6] ^= 1UL << (n%64);
+}
+
+static inline void gpiotools_clear_bit(__u64 b[], int n)
+{
+ b[n>>6] &= ~(1UL << (n%64));
+}
+
+static inline int gpiotools_test_bit(__u64 b[], int n)
+{
+ return !!(b[n>>6] & 1 << (n%64));
+}
+
+static inline void gpiotools_assign_bit(__u64 b[], int n, bool value)
+{
+ if (value)
+ gpiotools_set_bit(b, n);
+ else
+ gpiotools_clear_bit(b, n);
+}
#endif /* _GPIO_UTILS_H_ */
diff --git a/tools/gpio/gpio-watch.c b/tools/gpio/gpio-watch.c
index 5cea24fddfa7..0dd5a04ab250 100644
--- a/tools/gpio/gpio-watch.c
+++ b/tools/gpio/gpio-watch.c
@@ -21,8 +21,8 @@
int main(int argc, char **argv)
{
- struct gpioline_info_changed chg;
- struct gpioline_info req;
+ struct gpioline_info_changed_v2 chg;
+ struct gpioline_info_v2 req;
struct pollfd pfd;
int fd, i, j, ret;
char *event, *end;
@@ -40,11 +40,11 @@ int main(int argc, char **argv)
for (i = 0, j = 2; i < argc - 2; i++, j++) {
memset(&req, 0, sizeof(req));
- req.line_offset = strtoul(argv[j], &end, 0);
+ req.offset = strtoul(argv[j], &end, 0);
if (*end != '\0')
goto err_usage;
- ret = ioctl(fd, GPIO_GET_LINEINFO_WATCH_IOCTL, &req);
+ ret = ioctl(fd, GPIO_GET_LINEINFO_WATCH_V2_IOCTL, &req);
if (ret) {
perror("unable to set up line watch");
return EXIT_FAILURE;
@@ -87,7 +87,7 @@ int main(int argc, char **argv)
}
printf("line %u: %s at %llu\n",
- chg.info.line_offset, event, chg.timestamp);
+ chg.info.offset, event, chg.timestamp);
}
}
diff --git a/tools/gpio/lsgpio.c b/tools/gpio/lsgpio.c
index 8a71ad36f83b..0fd6adbb7de3 100644
--- a/tools/gpio/lsgpio.c
+++ b/tools/gpio/lsgpio.c
@@ -23,58 +23,53 @@
#include "gpio-utils.h"
-struct gpio_flag {
- char *name;
- unsigned long mask;
-};
-
-struct gpio_flag flagnames[] = {
- {
- .name = "kernel",
- .mask = GPIOLINE_FLAG_KERNEL,
- },
- {
- .name = "output",
- .mask = GPIOLINE_FLAG_IS_OUT,
- },
- {
- .name = "active-low",
- .mask = GPIOLINE_FLAG_ACTIVE_LOW,
- },
- {
- .name = "open-drain",
- .mask = GPIOLINE_FLAG_OPEN_DRAIN,
- },
- {
- .name = "open-source",
- .mask = GPIOLINE_FLAG_OPEN_SOURCE,
- },
- {
- .name = "pull-up",
- .mask = GPIOLINE_FLAG_BIAS_PULL_UP,
- },
- {
- .name = "pull-down",
- .mask = GPIOLINE_FLAG_BIAS_PULL_DOWN,
- },
- {
- .name = "bias-disabled",
- .mask = GPIOLINE_FLAG_BIAS_DISABLE,
- },
-};
-
-void print_flags(unsigned long flags)
+static void print_config(struct gpioline_config *config)
{
- int i;
- int printed = 0;
-
- for (i = 0; i < ARRAY_SIZE(flagnames); i++) {
- if (flags & flagnames[i].mask) {
- if (printed)
- fprintf(stdout, " ");
- fprintf(stdout, "%s", flagnames[i].name);
- printed++;
- }
+ const char *field_format = "%s";
+
+ if (config->flags & GPIOLINE_FLAG_V2_USED) {
+ fprintf(stdout, field_format, "used");
+ field_format = ", %s";
+ }
+
+ if (config->direction == GPIOLINE_DIRECTION_OUTPUT)
+ fprintf(stdout, field_format, "output");
+ else
+ fprintf(stdout, field_format, "input");
+
+ field_format = ", %s";
+
+ if (config->flags & GPIOLINE_FLAG_V2_ACTIVE_LOW)
+ fprintf(stdout, field_format, "active-low");
+
+ if (config->flags & GPIOLINE_FLAG_V2_DRIVE) {
+ if (config->drive == GPIOLINE_DRIVE_OPEN_DRAIN)
+ fprintf(stdout, field_format, "open-drain");
+ else if (config->drive == GPIOLINE_DRIVE_OPEN_SOURCE)
+ fprintf(stdout, field_format, "open-source");
+ }
+
+ if (config->flags & GPIOLINE_FLAG_V2_BIAS) {
+ if (config->bias == GPIOLINE_BIAS_DISABLED)
+ fprintf(stdout, field_format, "bias-disabled");
+ else if (config->bias == GPIOLINE_BIAS_PULL_UP)
+ fprintf(stdout, field_format, "pull-up");
+ else if (config->bias == GPIOLINE_BIAS_PULL_DOWN)
+ fprintf(stdout, field_format, "pull-down");
+ }
+
+ if (config->flags & GPIOLINE_FLAG_V2_EDGE_DETECTION) {
+ if (config->edge_detection == GPIOLINE_EDGE_BOTH)
+ fprintf(stdout, field_format, "both-edges");
+ else if (config->edge_detection == GPIOLINE_EDGE_RISING)
+ fprintf(stdout, field_format, "rising-edge");
+ else if (config->edge_detection == GPIOLINE_EDGE_FALLING)
+ fprintf(stdout, field_format, "falling-edge");
+ }
+
+ if (config->debounce_period) {
+ fprintf(stdout, ", debounce_period=%dusec",
+ config->debounce_period);
}
}
@@ -94,7 +89,7 @@ int list_device(const char *device_name)
if (fd == -1) {
ret = -errno;
fprintf(stderr, "Failed to open %s\n", chrdev_name);
- goto exit_close_error;
+ goto exit_free_name;
}
/* Inspect this GPIO chip */
@@ -109,18 +104,18 @@ int list_device(const char *device_name)
/* Loop over the lines and print info */
for (i = 0; i < cinfo.lines; i++) {
- struct gpioline_info linfo;
+ struct gpioline_info_v2 linfo;
memset(&linfo, 0, sizeof(linfo));
- linfo.line_offset = i;
+ linfo.offset = i;
- ret = ioctl(fd, GPIO_GET_LINEINFO_IOCTL, &linfo);
+ ret = ioctl(fd, GPIO_GET_LINEINFO_V2_IOCTL, &linfo);
if (ret == -1) {
ret = -errno;
perror("Failed to issue LINEINFO IOCTL\n");
goto exit_close_error;
}
- fprintf(stdout, "\tline %2d:", linfo.line_offset);
+ fprintf(stdout, "\tline %2d:", linfo.offset);
if (linfo.name[0])
fprintf(stdout, " \"%s\"", linfo.name);
else
@@ -129,9 +124,9 @@ int list_device(const char *device_name)
fprintf(stdout, " \"%s\"", linfo.consumer);
else
fprintf(stdout, " unused");
- if (linfo.flags) {
+ if (linfo.config.flags) {
fprintf(stdout, " [");
- print_flags(linfo.flags);
+ print_config(&linfo.config);
fprintf(stdout, "]");
}
fprintf(stdout, "\n");
@@ -141,6 +136,7 @@ int list_device(const char *device_name)
exit_close_error:
if (close(fd) == -1)
perror("Failed to close GPIO character device file");
+exit_free_name:
free(chrdev_name);
return ret;
}
--
2.27.0
Add support for setting debounce on a line via the GPIO uAPI.
Where debounce is not supported by hardware, a software debounce is
provided.
Signed-off-by: Kent Gibson <[email protected]>
---
The implementation of the software debouncer waits for the line to be
stable for the debounce period before determining if a level change,
and a corresponding edge event, has occurred. This provides maximum
protection against glitches, but also introduces a debounce_period
latency to edge events.
The software debouncer is integrated with the edge detection as it
utilises the line interrupt, and integration is simpler than getting
the two to interwork. Where software debounce AND edge detection is
required, the debouncer provides both.
Due to the tight integration between the debouncer and edge detection,
and to avoid particular corner cases, it is not allowed to alter the
debounce value if edge detection is enabled. Shanging the debounce with
edge detection enabled is a very unlikely use case, so it is preferable
to disallow it rather than complicate the code to allow it.
Should the user wish to alter the debounce value in such cases they will
need to release and re-aquire the line.
drivers/gpio/gpiolib-cdev.c | 212 +++++++++++++++++++++++++++++++++++-
drivers/gpio/gpiolib.h | 4 +
2 files changed, 214 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 7ba0929b2741..81c2fc4f0e49 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -394,6 +394,9 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
* for the corresponding line request. Ths is drawn from the @line.
* @line_seqno: the seqno for the current edge event in the sequence of
* events for this line.
+ * @sw_debounced: flag indicating if the software debouncer is active
+ * @work: the worker that implements software debouncing
+ * @level: the current debounced physical level of the line
*/
struct edge_detector {
struct line *line;
@@ -404,7 +407,15 @@ struct edge_detector {
*/
u64 timestamp;
u32 seqno;
+ /*
+ * line_seqno is used by either edge_irq_thread or debounce_work_func
+ * which are themselves mutually exclusive.
+ */
u32 line_seqno;
+ /* debouncer specific fields */
+ atomic_t sw_debounced;
+ struct delayed_work work;
+ atomic_t level;
};
/**
@@ -524,6 +535,10 @@ static int edge_detector_start(struct edge_detector *edet)
int ret, irq, irqflags = 0;
struct gpio_desc *desc;
+ if (atomic_read(&edet->sw_debounced))
+ /* debouncer is setup and will provide edge detection */
+ return 0;
+
desc = edge_detector_desc(edet);
irq = gpiod_to_irq(desc);
@@ -555,23 +570,192 @@ static int edge_detector_start(struct edge_detector *edet)
return 0;
}
+/*
+ * returns the current debounced value, or -1 if the debouncer is inactive.
+ */
+static int debounced_value(struct edge_detector *edet)
+{
+ int value;
+
+ if (!atomic_read(&edet->sw_debounced))
+ return -1;
+
+ /*
+ * minor race - debouncer may be stopped here, so edge_detector_stop
+ * must leave the value unchanged so the following will read the level
+ * from when the debouncer was last running.
+ */
+ value = atomic_read(&edet->level);
+
+ if (test_bit(FLAG_ACTIVE_LOW, &edge_detector_desc(edet)->flags))
+ value = !value;
+
+ return value;
+}
+
+static irqreturn_t debounce_irq_handler(int irq, void *p)
+{
+ struct edge_detector *edet = p;
+ struct gpio_desc *desc = edge_detector_desc(edet);
+
+ mod_delayed_work(system_wq,
+ &edet->work,
+ usecs_to_jiffies(atomic_read(&desc->debounce_period)));
+
+ return IRQ_HANDLED;
+}
+
+static void debounce_work_func(struct work_struct *work)
+{
+ struct gpioline_event le;
+ int ret, level, oldlevel;
+ struct edge_detector *edet =
+ container_of(work, struct edge_detector, work.work);
+ struct gpio_desc *desc = edge_detector_desc(edet);
+ struct line *line;
+
+ level = gpiod_get_raw_value_cansleep(desc);
+ if (level < 0) {
+ pr_debug_ratelimited("debouncer failed to read line value\n");
+ return;
+ }
+
+ oldlevel = atomic_xchg(&edet->level, level);
+ if (oldlevel == level)
+ return;
+
+ /* -- edge detection -- */
+ line = edet->line;
+ if (line->edge_detection == GPIOLINE_EDGE_NONE)
+ return;
+
+ /* switch from physical level to logical - if they differ */
+ if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
+ level = !level;
+
+ /* ignore edges that are not being monitored */
+ if (((line->edge_detection == GPIOLINE_EDGE_RISING) && (level == 0)) ||
+ ((line->edge_detection == GPIOLINE_EDGE_FALLING) && (level == 1)))
+ return;
+
+ /* Do not leak kernel stack to userspace */
+ memset(&le, 0, sizeof(le));
+
+ le.timestamp = ktime_get_ns();
+ le.offset = gpio_chip_hwgpio(desc);
+ edet->line_seqno++;
+ le.line_seqno = edet->line_seqno;
+ le.seqno = (line->num_descs == 1) ?
+ le.line_seqno : atomic_inc_return(&line->seqno);
+
+ if (level)
+ /* Emit low-to-high event */
+ le.id = GPIOLINE_EVENT_RISING_EDGE;
+ else
+ /* Emit high-to-low event */
+ le.id = GPIOLINE_EVENT_FALLING_EDGE;
+
+ ret = kfifo_in_spinlocked_noirqsave(&line->events, &le,
+ 1, &line->wait.lock);
+ if (ret)
+ wake_up_poll(&line->wait, EPOLLIN);
+ else
+ pr_debug_ratelimited("event FIFO is full - event dropped\n");
+}
+
+static int debounce_setup(struct edge_detector *edet,
+ unsigned int debounce_period)
+{
+ int ret, level, irq, irqflags;
+ struct gpio_desc *desc = edge_detector_desc(edet);
+
+ /* try hardware */
+ ret = gpiod_set_debounce(desc, debounce_period);
+ if (!ret) {
+ atomic_set(&desc->debounce_period, debounce_period);
+ return ret;
+ }
+ if (ret != -ENOTSUPP)
+ return ret;
+
+ if (debounce_period) {
+ /* setup software debounce */
+ level = gpiod_get_raw_value_cansleep(desc);
+ if (level < 0)
+ return level;
+
+ irq = gpiod_to_irq(desc);
+ if (irq <= 0)
+ return -ENODEV;
+
+ atomic_set(&edet->level, level);
+ edet->line_seqno = 0;
+ irqflags = IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING;
+ ret = request_irq(irq,
+ debounce_irq_handler,
+ irqflags,
+ edet->line->label,
+ edet);
+ if (ret)
+ return ret;
+
+ atomic_set(&edet->sw_debounced, 1);
+ edet->irq = irq;
+ }
+ atomic_set(&desc->debounce_period, debounce_period);
+ return 0;
+}
+
static void edge_detector_stop(struct edge_detector *edet)
{
+ struct gpio_desc *desc = edge_detector_desc(edet);
+
if (edet->irq) {
free_irq(edet->irq, edet);
edet->irq = 0;
}
+
+ cancel_delayed_work_sync(&edet->work);
+ atomic_set(&edet->sw_debounced, 0);
+ /* do not change edet->level - see comment in debounced_value */
+
+ if (desc)
+ atomic_set(&desc->debounce_period, 0);
+}
+
+static int debounce_update(struct edge_detector *edet,
+ unsigned int debounce_period)
+{
+ struct gpio_desc *desc = edge_detector_desc(edet);
+
+ if (atomic_read(&desc->debounce_period) == debounce_period)
+ return 0;
+
+ if (!atomic_read(&edet->sw_debounced))
+ return debounce_setup(edet, debounce_period);
+
+ if (!debounce_period)
+ edge_detector_stop(edet);
+ else
+ atomic_set(&desc->debounce_period, debounce_period);
+ return 0;
}
static int edge_detector_setup(struct edge_detector *edet,
struct gpioline_config *lc)
{
struct gpio_desc *desc = edge_detector_desc(edet);
+ int ret;
if (lc->edge_detection & GPIOLINE_EDGE_RISING)
set_bit(FLAG_EDGE_RISING, &desc->flags);
if (lc->edge_detection & GPIOLINE_EDGE_FALLING)
set_bit(FLAG_EDGE_FALLING, &desc->flags);
+ if (lc->flags & GPIOLINE_FLAG_V2_DEBOUNCE) {
+ ret = debounce_setup(edet, lc->debounce_period);
+ if (ret)
+ return ret;
+ }
if (lc->edge_detection)
return edge_detector_start(edet);
return 0;
@@ -703,6 +887,10 @@ static long line_set_config_locked(struct line *line,
if (test_bit(FLAG_ACTIVE_LOW, &desc->flags) !=
((lc->flags & GPIOLINE_FLAG_V2_ACTIVE_LOW) != 0))
return -EINVAL;
+
+ /* disallow debounce changes */
+ if (atomic_read(&desc->debounce_period) != lc->debounce_period)
+ return -EINVAL;
}
vals = (unsigned long *)lc->values.bitmap;
@@ -718,6 +906,7 @@ static long line_set_config_locked(struct line *line,
if (lc->direction == GPIOLINE_DIRECTION_OUTPUT) {
int val = test_bit(i, vals);
+ edge_detector_stop(&line->edets[i]);
ret = gpiod_direction_output(desc, val);
if (ret)
return ret;
@@ -725,6 +914,12 @@ static long line_set_config_locked(struct line *line,
ret = gpiod_direction_input(desc);
if (ret)
return ret;
+ if (lc->flags & GPIOLINE_FLAG_V2_DEBOUNCE) {
+ ret = debounce_update(&line->edets[i],
+ lc->debounce_period);
+ if (ret)
+ return ret;
+ }
}
}
@@ -762,7 +957,7 @@ static long line_ioctl(struct file *file, unsigned int cmd,
void __user *ip = (void __user *)arg;
struct gpioline_values glv;
unsigned long *vals = (unsigned long *)glv.bitmap;
- int ret;
+ int ret, i, value;
if (cmd == GPIOLINE_GET_VALUES_IOCTL) {
/* NOTE: It's ok to read values of output lines. */
@@ -776,6 +971,12 @@ static long line_ioctl(struct file *file, unsigned int cmd,
if (ret)
return ret;
+ for (i = 0; i < line->num_descs; i++) {
+ value = debounced_value(&line->edets[i]);
+ if (value >= 0)
+ assign_bit(i, vals, value);
+ }
+
if (copy_to_user(ip, &glv, sizeof(glv)))
return -EFAULT;
@@ -954,8 +1155,11 @@ static int line_create(struct gpio_device *gdev, void __user *ip)
if (!line->edets)
return -ENOMEM;
- for (i = 0; i < linereq.num_lines; i++)
+ for (i = 0; i < linereq.num_lines; i++) {
line->edets[i].line = line;
+ atomic_set(&line->edets[i].sw_debounced, 0);
+ INIT_DELAYED_WORK(&line->edets[i].work, debounce_work_func);
+ }
line->gdev = gdev;
get_device(&gdev->dev);
@@ -1589,6 +1793,10 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
lc->edge_detection |= GPIOLINE_EDGE_FALLING;
}
+ lc->debounce_period = atomic_read(&desc->debounce_period);
+ if (lc->debounce_period)
+ lc->flags |= GPIOLINE_FLAG_V2_DEBOUNCE;
+
spin_unlock_irqrestore(&gpio_lock, flags);
}
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 1dc6d2b191af..02a46af0c69f 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -124,6 +124,10 @@ struct gpio_desc {
#ifdef CONFIG_OF_DYNAMIC
struct device_node *hog;
#endif
+#ifdef CONFIG_GPIO_CDEV
+ /* debounce period in microseconds */
+ atomic_t debounce_period;
+#endif
};
int gpiod_request(struct gpio_desc *desc, const char *label);
--
2.27.0
Extend gpio-event-mon to support monitoring multiple lines.
This would require multiple lineevent requests to implement using uAPI V1,
but can be performed with a single line request using uAPI V2.
Signed-off-by: Kent Gibson <[email protected]>
---
tools/gpio/gpio-event-mon.c | 41 ++++++++++++++++++++++++++++---------
1 file changed, 31 insertions(+), 10 deletions(-)
diff --git a/tools/gpio/gpio-event-mon.c b/tools/gpio/gpio-event-mon.c
index ec90e44389dc..ed1adb8247c6 100644
--- a/tools/gpio/gpio-event-mon.c
+++ b/tools/gpio/gpio-event-mon.c
@@ -26,7 +26,8 @@
#include "gpio-utils.h"
int monitor_device(const char *device_name,
- unsigned int line,
+ unsigned int *lines,
+ unsigned int num_lines,
struct gpioline_config *config,
unsigned int loops)
{
@@ -47,7 +48,7 @@ int monitor_device(const char *device_name,
goto exit_free_name;
}
- ret = gpiotools_request_line(device_name, &line, 1, config,
+ ret = gpiotools_request_line(device_name, lines, num_lines, config,
"gpio-event-mon");
if (ret < 0)
goto exit_device_close;
@@ -63,9 +64,23 @@ int monitor_device(const char *device_name,
goto exit_line_close;
}
- fprintf(stdout, "Monitoring line %d on %s\n", line, device_name);
- fprintf(stdout, "Initial line value: %d\n",
- gpiotools_test_bit(values.bitmap, 0));
+ if (num_lines == 1) {
+ fprintf(stdout, "Monitoring line %d on %s\n", lines[0], device_name);
+ fprintf(stdout, "Initial line value: %d\n",
+ gpiotools_test_bit(values.bitmap, 0));
+ } else {
+ fprintf(stdout, "Monitoring lines %d", lines[0]);
+ for (i = 1; i < num_lines - 1; i++)
+ fprintf(stdout, ", %d", lines[i]);
+ fprintf(stdout, " and %d on %s\n", lines[i], device_name);
+ fprintf(stdout, "Initial line values: %d",
+ gpiotools_test_bit(values.bitmap, 0));
+ for (i = 1; i < num_lines - 1; i++)
+ fprintf(stdout, ", %d",
+ gpiotools_test_bit(values.bitmap, i));
+ fprintf(stdout, " and %d\n",
+ gpiotools_test_bit(values.bitmap, i));
+ }
while (1) {
struct gpioline_event event;
@@ -124,7 +139,7 @@ void print_usage(void)
fprintf(stderr, "Usage: gpio-event-mon [options]...\n"
"Listen to events on GPIO lines, 0->1 1->0\n"
" -n <name> Listen on GPIOs on a named device (must be stated)\n"
- " -o <n> Offset to monitor\n"
+ " -o <n> Offset of line to monitor (may be repeated)\n"
" -d Set line as open drain\n"
" -s Set line as open source\n"
" -r Listen for rising edges\n"
@@ -141,7 +156,8 @@ void print_usage(void)
int main(int argc, char **argv)
{
const char *device_name = NULL;
- unsigned int line = -1;
+ unsigned int lines[GPIOLINES_MAX];
+ unsigned int num_lines = 0;
unsigned int loops = 0;
struct gpioline_config config;
int c;
@@ -158,7 +174,12 @@ int main(int argc, char **argv)
device_name = optarg;
break;
case 'o':
- line = strtoul(optarg, NULL, 10);
+ if (num_lines >= GPIOLINES_MAX) {
+ print_usage();
+ return -1;
+ }
+ lines[num_lines] = strtoul(optarg, NULL, 10);
+ num_lines++;
break;
case 'b':
config.flags |= GPIOLINE_FLAG_V2_DEBOUNCE;
@@ -184,7 +205,7 @@ int main(int argc, char **argv)
}
}
- if (!device_name || line == -1) {
+ if (!device_name || num_lines == 0) {
print_usage();
return -1;
}
@@ -193,5 +214,5 @@ int main(int argc, char **argv)
"falling edges\n");
config.edge_detection = GPIOLINE_EDGE_BOTH;
}
- return monitor_device(device_name, line, &config, loops);
+ return monitor_device(device_name, lines, num_lines, &config, loops);
}
--
2.27.0
Add a build option to allow the removal of the CDEV v1 ABI.
Suggested-by: Bartosz Golaszewski <[email protected]>
Signed-off-by: Kent Gibson <[email protected]>
---
This patch is before the V2 implementation, and is non-functional until
that patch, as some parts of that patch would be written slightly
differently if removing V1 was not considered.
Adding this patch after that would necessitate revisiting the V2 changes,
so this ordering results in two simpler patches.
drivers/gpio/Kconfig | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index affc1524bc2c..b966a7dc1c9a 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -81,6 +81,18 @@ config GPIO_CDEV
If unsure, say Y.
+config GPIO_CDEV_V1
+ bool "Support GPIO ABI Version 1"
+ default y
+ depends on GPIO_CDEV
+ help
+ Say Y here to support version 1 of the GPIO CDEV ABI.
+
+ This ABI version is deprecated and will be removed in the future.
+ Please use the latest ABI for new developments.
+
+ If unsure, say Y.
+
config GPIO_GENERIC
depends on HAS_IOMEM # Only for IOMEM drivers
tristate
--
2.27.0
Add support for debouncing monitored lines to gpio-event-mon.
Signed-off-by: Kent Gibson <[email protected]>
---
tools/gpio/gpio-event-mon.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/tools/gpio/gpio-event-mon.c b/tools/gpio/gpio-event-mon.c
index d8d692f67b9e..ec90e44389dc 100644
--- a/tools/gpio/gpio-event-mon.c
+++ b/tools/gpio/gpio-event-mon.c
@@ -129,11 +129,12 @@ void print_usage(void)
" -s Set line as open source\n"
" -r Listen for rising edges\n"
" -f Listen for falling edges\n"
+ " -b <n> Debounce the line with period n microseconds\n"
" [-c <n>] Do <n> loops (optional, infinite loop if not stated)\n"
" -? This helptext\n"
"\n"
"Example:\n"
- "gpio-event-mon -n gpiochip0 -o 4 -r -f\n"
+ "gpio-event-mon -n gpiochip0 -o 4 -r -f -b 10000\n"
);
}
@@ -148,7 +149,7 @@ int main(int argc, char **argv)
memset(&config, 0, sizeof(config));
config.flags = GPIOLINE_FLAG_V2_DIRECTION | GPIOLINE_FLAG_V2_EDGE_DETECTION;
config.direction = GPIOLINE_DIRECTION_INPUT;
- while ((c = getopt(argc, argv, "c:n:o:dsrf?")) != -1) {
+ while ((c = getopt(argc, argv, "c:n:o:b:dsrf?")) != -1) {
switch (c) {
case 'c':
loops = strtoul(optarg, NULL, 10);
@@ -159,6 +160,10 @@ int main(int argc, char **argv)
case 'o':
line = strtoul(optarg, NULL, 10);
break;
+ case 'b':
+ config.flags |= GPIOLINE_FLAG_V2_DEBOUNCE;
+ config.debounce_period = strtoul(optarg, NULL, 10);
+ break;
case 'd':
config.flags |= GPIOLINE_FLAG_V2_DRIVE;
config.drive = GPIOLINE_DRIVE_OPEN_DRAIN;
--
2.27.0
Report the state of edge detection for a line in the gpioline_info_v2
returned by GPIO_GET_LINEINFO_V2_IOCTL, and indirectly for lines watched
by GPIO_GET_LINEINFO_WATCH_V2_IOCTL.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 14 ++++++++++++++
drivers/gpio/gpiolib.c | 2 ++
drivers/gpio/gpiolib.h | 2 ++
3 files changed, 18 insertions(+)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index d4a22d78953f..7ba0929b2741 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -566,6 +566,12 @@ static void edge_detector_stop(struct edge_detector *edet)
static int edge_detector_setup(struct edge_detector *edet,
struct gpioline_config *lc)
{
+ struct gpio_desc *desc = edge_detector_desc(edet);
+
+ if (lc->edge_detection & GPIOLINE_EDGE_RISING)
+ set_bit(FLAG_EDGE_RISING, &desc->flags);
+ if (lc->edge_detection & GPIOLINE_EDGE_FALLING)
+ set_bit(FLAG_EDGE_FALLING, &desc->flags);
if (lc->edge_detection)
return edge_detector_start(edet);
return 0;
@@ -1574,6 +1580,14 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
}
lc->edge_detection = 0;
+ if (test_bit(FLAG_EDGE_RISING, &desc->flags)) {
+ lc->flags |= GPIOLINE_FLAG_V2_EDGE_DETECTION;
+ lc->edge_detection |= GPIOLINE_EDGE_RISING;
+ }
+ if (test_bit(FLAG_EDGE_FALLING, &desc->flags)) {
+ lc->flags |= GPIOLINE_FLAG_V2_EDGE_DETECTION;
+ lc->edge_detection |= GPIOLINE_EDGE_FALLING;
+ }
spin_unlock_irqrestore(&gpio_lock, flags);
}
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 517c99ddf6c8..a5f2795e17b7 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2041,6 +2041,8 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
clear_bit(FLAG_PULL_UP, &desc->flags);
clear_bit(FLAG_PULL_DOWN, &desc->flags);
clear_bit(FLAG_BIAS_DISABLE, &desc->flags);
+ clear_bit(FLAG_EDGE_RISING, &desc->flags);
+ clear_bit(FLAG_EDGE_FALLING, &desc->flags);
clear_bit(FLAG_IS_HOGGED, &desc->flags);
#ifdef CONFIG_OF_DYNAMIC
desc->hog = NULL;
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 2dee4e1e12dc..1dc6d2b191af 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -114,6 +114,8 @@ struct gpio_desc {
#define FLAG_PULL_UP 13 /* GPIO has pull up enabled */
#define FLAG_PULL_DOWN 14 /* GPIO has pull down enabled */
#define FLAG_BIAS_DISABLE 15 /* GPIO has pull disabled */
+#define FLAG_EDGE_RISING 16 /* GPIO CDEV detects rising edge events */
+#define FLAG_EDGE_FALLING 17 /* GPIO CDEV detects falling edge events */
/* Connection label */
const char *label;
--
2.27.0
Add implementation of the V2 uAPI up to parity with V1.
Signed-off-by: Kent Gibson <[email protected]>
---
This patch only covers the V2 functionality that is a direct analogue of
the V1. New V2 functionality is added in subsequent patches.
The core of the implementation is the struct line, which is drawn from the
existing struct linehandle_state implementation, but modified to deal with
the V2 uAPI structs.
The struct edge_detector provides the the edge detection functionality,
and is drawn from the existing lineevent_state implementation.
The two uAPI versions are mostly independent - other than where they both
provide line info changes via reads on the chip fd. As the info change
structs differ between V1 and V2, the infowatch implementation tracks which
version of the infowatch ioctl, either GPIO_GET_LINEINFO_WATCH_IOCTL or
GPIO_GET_LINEINFO_WATCH_V2_IOCTL, initiates the initial watch and returns
the corresponding info change struct to the read. The version supported
on that fd locks to that version on the first watch request, so subsequent
watches from that process must use the same uAPI version.
drivers/gpio/gpiolib-cdev.c | 934 ++++++++++++++++++++++++++++++++++--
1 file changed, 905 insertions(+), 29 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index b6878fc87dfc..d4a22d78953f 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1,7 +1,9 @@
// SPDX-License-Identifier: GPL-2.0
#include <linux/anon_inodes.h>
+#include <linux/atomic.h>
#include <linux/bitmap.h>
+#include <linux/build_bug.h>
#include <linux/cdev.h>
#include <linux/compat.h>
#include <linux/device.h>
@@ -14,11 +16,13 @@
#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>
#include <linux/timekeeping.h>
#include <linux/uaccess.h>
+#include <linux/workqueue.h>
#include <uapi/linux/gpio.h>
#include "gpiolib.h"
@@ -34,6 +38,7 @@
* GPIO line handle management
*/
+#ifdef CONFIG_GPIO_CDEV_V1
/**
* struct linehandle_state - contains the state of a userspace handle
* @gdev: the GPIO device the handle pertains to
@@ -377,6 +382,699 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
put_device(&gdev->dev);
return ret;
}
+#endif /* CONFIG_GPIO_CDEV_V1 */
+
+/**
+ * struct edge_detector - contains the state of a line edge detector
+ * @line: the corresponding line request
+ * @irq: the interrupt triggered in response to events on this GPIO
+ * @timestamp: cache for the timestamp storing it between hardirq and IRQ
+ * thread, used to bring the timestamp close to the actual event
+ * @seqno: the seqno for the current edge event in the sequence of events
+ * for the corresponding line request. Ths is drawn from the @line.
+ * @line_seqno: the seqno for the current edge event in the sequence of
+ * events for this line.
+ */
+struct edge_detector {
+ struct line *line;
+ unsigned int irq;
+ /*
+ * timestamp and seqno are shared by edge_irq_handler and
+ * edge_irq_thread which are themselves mutually exclusive.
+ */
+ u64 timestamp;
+ u32 seqno;
+ u32 line_seqno;
+};
+
+/**
+ * struct line - contains the state of a userspace line request
+ * @gdev: the GPIO device the line request pertains to
+ * @label: consumer label used to tag descriptors
+ * @num_descs: the number of descriptors held in the descs array
+ * @edge_detection: the type of edge detection applied
+ * @wait: wait queue that handles blocking reads of events
+ * @events: KFIFO for the GPIO events
+ * @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 serializing GPIOLINE_SET_CONFIG_IOCTL ioctl() calls to
+ * ensure consistency of configuration changes.
+ * @edets: an array of edge detectors, of size @num_descs
+ * @descs: the GPIO descriptors held by this line request, with @num_descs
+ * elements.
+ */
+struct line {
+ struct gpio_device *gdev;
+ const char *label;
+ u32 num_descs;
+ enum gpioline_edge edge_detection;
+ wait_queue_head_t wait;
+ DECLARE_KFIFO_PTR(events, struct gpioline_event);
+ atomic_t seqno;
+ struct mutex config_mutex; /* serializes line_set_config calls */
+ struct edge_detector *edets;
+ /* descs must be last so it can be dynamically sized */
+ struct gpio_desc *descs[];
+};
+
+static inline struct gpio_desc *edge_detector_desc(
+ const struct edge_detector *edet)
+{
+ return edet->line->descs[edet - &edet->line->edets[0]];
+}
+
+static irqreturn_t edge_irq_thread(int irq, void *p)
+{
+ struct edge_detector *edet = p;
+ struct line *line = edet->line;
+ struct gpio_desc *desc = edge_detector_desc(edet);
+ struct gpioline_event le;
+ int ret;
+
+ /* Do not leak kernel stack to userspace */
+ memset(&le, 0, sizeof(le));
+
+ /*
+ * We may be running from a nested threaded interrupt in which case
+ * we didn't get the timestamp from edge_irq_handler().
+ */
+ if (!edet->timestamp) {
+ le.timestamp = ktime_get_ns();
+ if (line->num_descs != 1)
+ edet->seqno = atomic_inc_return(&line->seqno);
+ } else {
+ le.timestamp = edet->timestamp;
+ }
+
+ edet->timestamp = 0;
+
+ if (line->edge_detection == GPIOLINE_EDGE_BOTH) {
+ int level = gpiod_get_value_cansleep(desc);
+
+ if (level)
+ /* Emit low-to-high event */
+ le.id = GPIOLINE_EVENT_RISING_EDGE;
+ else
+ /* Emit high-to-low event */
+ le.id = GPIOLINE_EVENT_FALLING_EDGE;
+ } else if (line->edge_detection == GPIOLINE_EDGE_RISING) {
+ /* Emit low-to-high event */
+ le.id = GPIOLINE_EVENT_RISING_EDGE;
+ } else if (line->edge_detection == GPIOLINE_EDGE_FALLING) {
+ /* Emit high-to-low event */
+ le.id = GPIOLINE_EVENT_FALLING_EDGE;
+ } else {
+ return IRQ_NONE;
+ }
+ edet->line_seqno++;
+ le.line_seqno = edet->line_seqno;
+ le.seqno = (line->num_descs == 1) ? le.line_seqno : edet->seqno;
+ le.offset = gpio_chip_hwgpio(desc);
+
+ ret = kfifo_in_spinlocked_noirqsave(&line->events, &le,
+ 1, &line->wait.lock);
+ if (ret)
+ wake_up_poll(&line->wait, EPOLLIN);
+ else
+ pr_debug_ratelimited("event FIFO is full - event dropped\n");
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t edge_irq_handler(int irq, void *p)
+{
+ struct edge_detector *edet = p;
+ struct line *line = edet->line;
+
+ /*
+ * Just store the timestamp in hardirq context so we get it as
+ * close in time as possible to the actual event.
+ */
+ edet->timestamp = ktime_get_ns();
+
+ if (line->num_descs != 1)
+ edet->seqno = atomic_inc_return(&line->seqno);
+
+ return IRQ_WAKE_THREAD;
+}
+
+static int edge_detector_start(struct edge_detector *edet)
+{
+ int ret, irq, irqflags = 0;
+ struct gpio_desc *desc;
+
+ desc = edge_detector_desc(edet);
+ irq = gpiod_to_irq(desc);
+
+ if (irq <= 0)
+ return -ENODEV;
+
+ edet->seqno = 0;
+ edet->line_seqno = 0;
+
+ if (edet->line->edge_detection & GPIOLINE_EDGE_RISING)
+ irqflags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
+ IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
+ if (edet->line->edge_detection & GPIOLINE_EDGE_FALLING)
+ irqflags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
+ IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
+ irqflags |= IRQF_ONESHOT;
+
+ /* Request a thread to read the events */
+ ret = request_threaded_irq(irq,
+ edge_irq_handler,
+ edge_irq_thread,
+ irqflags,
+ edet->line->label,
+ edet);
+ if (ret)
+ return ret;
+
+ edet->irq = irq;
+ return 0;
+}
+
+static void edge_detector_stop(struct edge_detector *edet)
+{
+ if (edet->irq) {
+ free_irq(edet->irq, edet);
+ edet->irq = 0;
+ }
+}
+
+static int edge_detector_setup(struct edge_detector *edet,
+ struct gpioline_config *lc)
+{
+ if (lc->edge_detection)
+ return edge_detector_start(edet);
+ return 0;
+}
+
+static bool padding_not_zeroed(__u32 *padding, int pad_size)
+{
+ int i, sum = 0;
+
+ for (i = 0; i < pad_size; i++)
+ sum |= padding[i];
+
+ return sum;
+}
+
+#define GPIOLINE_CONFIG_VALID_FLAGS \
+ (GPIOLINE_FLAG_V2_ACTIVE_LOW | \
+ GPIOLINE_FLAG_V2_DIRECTION | \
+ GPIOLINE_FLAG_V2_DRIVE | \
+ GPIOLINE_FLAG_V2_BIAS | \
+ GPIOLINE_FLAG_V2_EDGE_DETECTION | \
+ GPIOLINE_FLAG_V2_DEBOUNCE)
+
+static int gpioline_config_validate(struct gpioline_config *config)
+{
+ u32 lflags = config->flags;
+
+ /* Return an error if an unknown flag is set */
+ if (lflags & ~GPIOLINE_CONFIG_VALID_FLAGS)
+ return -EINVAL;
+
+ /* Check ranges */
+ if (lflags & GPIOLINE_FLAG_V2_DIRECTION) {
+ if (config->direction > GPIOLINE_DIRECTION_OUTPUT)
+ return -EINVAL;
+ } else if (config->direction != 0) {
+ return -EINVAL;
+ }
+
+ if (lflags & GPIOLINE_FLAG_V2_DRIVE) {
+ if (config->drive > GPIOLINE_DRIVE_OPEN_SOURCE)
+ return -EINVAL;
+ } else if (config->drive != 0) {
+ return -EINVAL;
+ }
+
+ if (lflags & GPIOLINE_FLAG_V2_BIAS) {
+ if (config->bias > GPIOLINE_BIAS_PULL_DOWN)
+ return -EINVAL;
+ } else if (config->bias != 0) {
+ return -EINVAL;
+ }
+
+ if (lflags & GPIOLINE_FLAG_V2_EDGE_DETECTION) {
+ if (config->edge_detection > GPIOLINE_EDGE_BOTH)
+ return -EINVAL;
+ } else if (config->edge_detection != 0) {
+ return -EINVAL;
+ }
+
+ if (config->debounce_period && !(lflags & GPIOLINE_FLAG_V2_DEBOUNCE))
+ return -EINVAL;
+
+ /* Drive requires explicit output direction. */
+ if ((lflags & GPIOLINE_FLAG_V2_DRIVE) &&
+ (!(lflags & GPIOLINE_FLAG_V2_DIRECTION) ||
+ (config->direction != GPIOLINE_DIRECTION_OUTPUT)))
+ return -EINVAL;
+
+ /* Bias requires explicit direction. */
+ if ((lflags & GPIOLINE_FLAG_V2_BIAS) &&
+ !(lflags & GPIOLINE_FLAG_V2_DIRECTION))
+ return -EINVAL;
+
+ /* Edge detection requires explicit input direction. */
+ if ((lflags & GPIOLINE_FLAG_V2_EDGE_DETECTION) &&
+ (!(lflags & GPIOLINE_FLAG_V2_DIRECTION) ||
+ (config->direction != GPIOLINE_DIRECTION_INPUT)))
+ return -EINVAL;
+
+ /* Debounce requires explicit input direction. */
+ if ((lflags & GPIOLINE_FLAG_V2_DEBOUNCE) &&
+ (!(lflags & GPIOLINE_FLAG_V2_DIRECTION) ||
+ (config->direction != GPIOLINE_DIRECTION_INPUT)))
+ return -EINVAL;
+
+ if (padding_not_zeroed(config->padding, GPIOLINE_CONFIG_PAD_SIZE))
+ return -EINVAL;
+
+ return 0;
+}
+
+static void gpioline_config_to_desc_flags(struct gpioline_config *lc,
+ unsigned long *flagsp)
+{
+ assign_bit(FLAG_ACTIVE_LOW, flagsp,
+ lc->flags & GPIOLINE_FLAG_V2_ACTIVE_LOW);
+ if (lc->flags & GPIOLINE_FLAG_V2_DRIVE) {
+ assign_bit(FLAG_OPEN_DRAIN, flagsp,
+ lc->drive == GPIOLINE_DRIVE_OPEN_DRAIN);
+ assign_bit(FLAG_OPEN_SOURCE, flagsp,
+ lc->drive == GPIOLINE_DRIVE_OPEN_SOURCE);
+ }
+ if (lc->flags & GPIOLINE_FLAG_V2_BIAS) {
+ assign_bit(FLAG_PULL_UP, flagsp,
+ lc->bias == GPIOLINE_BIAS_PULL_UP);
+ assign_bit(FLAG_PULL_DOWN, flagsp,
+ lc->bias == GPIOLINE_BIAS_PULL_DOWN);
+ assign_bit(FLAG_BIAS_DISABLE, flagsp,
+ lc->bias == GPIOLINE_BIAS_DISABLED);
+ }
+}
+
+static long line_set_config_locked(struct line *line,
+ struct gpioline_config *lc)
+{
+ struct gpio_desc *desc;
+ unsigned long *vals;
+ int i, ret;
+
+ /* disallow edge detection changes */
+ if (line->edge_detection != lc->edge_detection)
+ return -EINVAL;
+
+ desc = line->descs[0];
+
+ if (line->edge_detection != GPIOLINE_EDGE_NONE) {
+ /* disallow polarity changes */
+ if (test_bit(FLAG_ACTIVE_LOW, &desc->flags) !=
+ ((lc->flags & GPIOLINE_FLAG_V2_ACTIVE_LOW) != 0))
+ return -EINVAL;
+ }
+
+ vals = (unsigned long *)lc->values.bitmap;
+
+ for (i = 0; i < line->num_descs; i++) {
+ desc = line->descs[i];
+ gpioline_config_to_desc_flags(lc, &desc->flags);
+ /*
+ * Lines have to be requested explicitly for input
+ * or output, else the line will be treated "as is".
+ */
+ if (lc->flags & GPIOLINE_FLAG_V2_DIRECTION) {
+ if (lc->direction == GPIOLINE_DIRECTION_OUTPUT) {
+ int val = test_bit(i, vals);
+
+ ret = gpiod_direction_output(desc, val);
+ if (ret)
+ return ret;
+ } else {
+ ret = gpiod_direction_input(desc);
+ if (ret)
+ return ret;
+ }
+ }
+
+ atomic_notifier_call_chain(&desc->gdev->notifier,
+ GPIOLINE_CHANGED_CONFIG, desc);
+ }
+ return 0;
+}
+
+static long line_set_config(struct line *line, void __user *ip)
+{
+ struct gpioline_config lc;
+ int ret;
+
+ if (copy_from_user(&lc, ip, sizeof(lc)))
+ return -EFAULT;
+
+ ret = gpioline_config_validate(&lc);
+ 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)
+{
+ struct line *line = file->private_data;
+ void __user *ip = (void __user *)arg;
+ struct gpioline_values glv;
+ unsigned long *vals = (unsigned long *)glv.bitmap;
+ int ret;
+
+ if (cmd == GPIOLINE_GET_VALUES_IOCTL) {
+ /* NOTE: It's ok to read values of output lines. */
+ memset(&glv, 0, sizeof(glv));
+ ret = gpiod_get_array_value_complex(false,
+ true,
+ line->num_descs,
+ line->descs,
+ NULL,
+ vals);
+ if (ret)
+ return ret;
+
+ if (copy_to_user(ip, &glv, sizeof(glv)))
+ return -EFAULT;
+
+ return 0;
+ } else if (cmd == GPIOLINE_SET_VALUES_IOCTL) {
+ /*
+ * All line descriptors were created at once with the same
+ * flags so just check if the first one is really output.
+ */
+ if (!test_bit(FLAG_IS_OUT, &line->descs[0]->flags))
+ return -EPERM;
+
+ if (copy_from_user(&glv, ip, sizeof(glv)))
+ return -EFAULT;
+
+ /* Reuse the array setting function */
+ return gpiod_set_array_value_complex(false,
+ true,
+ line->num_descs,
+ line->descs,
+ NULL,
+ vals);
+ } else if (cmd == GPIOLINE_SET_CONFIG_IOCTL) {
+ return line_set_config(line, ip);
+ }
+ return -EINVAL;
+}
+
+#ifdef CONFIG_COMPAT
+static long line_ioctl_compat(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ return line_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
+}
+#endif
+
+static __poll_t line_poll(struct file *file,
+ struct poll_table_struct *wait)
+{
+ struct line *line = file->private_data;
+ __poll_t events = 0;
+
+ poll_wait(file, &line->wait, wait);
+
+ if (!kfifo_is_empty_spinlocked_noirqsave(&line->events, &line->wait.lock))
+ events = EPOLLIN | EPOLLRDNORM;
+
+ return events;
+}
+
+static ssize_t line_read(struct file *file,
+ char __user *buf,
+ size_t count,
+ loff_t *f_ps)
+{
+ struct line *line = file->private_data;
+ struct gpioline_event le;
+ ssize_t bytes_read = 0;
+ int ret;
+
+ if (count < sizeof(le))
+ return -EINVAL;
+
+ do {
+ spin_lock(&line->wait.lock);
+ if (kfifo_is_empty(&line->events)) {
+ if (bytes_read) {
+ spin_unlock(&line->wait.lock);
+ return bytes_read;
+ }
+
+ if (file->f_flags & O_NONBLOCK) {
+ spin_unlock(&line->wait.lock);
+ return -EAGAIN;
+ }
+
+ ret = wait_event_interruptible_locked(line->wait,
+ !kfifo_is_empty(&line->events));
+ if (ret) {
+ spin_unlock(&line->wait.lock);
+ return ret;
+ }
+ }
+
+ ret = kfifo_out(&line->events, &le, 1);
+ spin_unlock(&line->wait.lock);
+ if (ret != 1) {
+ /*
+ * This should never happen - we were holding the lock
+ * from the moment we learned the fifo is no longer
+ * empty until now.
+ */
+ ret = -EIO;
+ break;
+ }
+
+ if (copy_to_user(buf + bytes_read, &le, sizeof(le)))
+ return -EFAULT;
+ bytes_read += sizeof(le);
+ } while (count >= bytes_read + sizeof(le));
+
+ return bytes_read;
+}
+
+static void line_free(struct line *line)
+{
+ int i;
+
+ for (i = 0; i < line->num_descs; i++) {
+ if (line->edets)
+ edge_detector_stop(&line->edets[i]);
+ if (line->descs[i])
+ gpiod_free(line->descs[i]);
+ }
+ kfifo_free(&line->events);
+ kfree(line->label);
+ kfree(line->edets);
+ kfree(line);
+ put_device(&line->gdev->dev);
+}
+
+static int line_release(struct inode *inode, struct file *file)
+{
+ struct line *line = file->private_data;
+
+ line_free(line);
+ return 0;
+}
+
+static const struct file_operations line_fileops = {
+ .release = line_release,
+ .read = line_read,
+ .poll = line_poll,
+ .owner = THIS_MODULE,
+ .llseek = noop_llseek,
+ .unlocked_ioctl = line_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = line_ioctl_compat,
+#endif
+};
+
+static int line_create(struct gpio_device *gdev, void __user *ip)
+{
+ struct gpioline_request linereq;
+ struct line *line;
+ struct file *file;
+ int fd, i, ret, size;
+ struct gpioline_config *lc;
+ unsigned long *vals;
+
+ if (copy_from_user(&linereq, ip, sizeof(linereq)))
+ return -EFAULT;
+ if ((linereq.num_lines == 0) || (linereq.num_lines > GPIOLINES_MAX))
+ return -EINVAL;
+
+ if (padding_not_zeroed(linereq.padding, GPIOLINE_REQUEST_PAD_SIZE))
+ return -EINVAL;
+
+ lc = &linereq.config;
+ ret = gpioline_config_validate(lc);
+ if (ret)
+ return ret;
+
+ /* event_buffer_size only valid with edge_detection */
+ if ((linereq.event_buffer_size) &&
+ !(linereq.config.flags & GPIOLINE_FLAG_V2_EDGE_DETECTION))
+ return -EINVAL;
+
+ line = kzalloc(struct_size(line, descs, linereq.num_lines),
+ GFP_KERNEL);
+ if (!line)
+ return -ENOMEM;
+
+ line->edets = kcalloc(linereq.num_lines, sizeof(*line->edets),
+ GFP_KERNEL);
+ if (!line->edets)
+ return -ENOMEM;
+
+ for (i = 0; i < linereq.num_lines; i++)
+ line->edets[i].line = line;
+
+ line->gdev = gdev;
+ get_device(&gdev->dev);
+
+ /* Make sure this is terminated */
+ linereq.consumer[sizeof(linereq.consumer)-1] = '\0';
+ if (strlen(linereq.consumer)) {
+ line->label = kstrdup(linereq.consumer, GFP_KERNEL);
+ if (!line->label) {
+ ret = -ENOMEM;
+ goto out_free_line;
+ }
+ }
+
+ mutex_init(&line->config_mutex);
+ init_waitqueue_head(&line->wait);
+ if (lc->edge_detection) {
+ size = linereq.event_buffer_size;
+
+ if (size > GPIOLINES_MAX*16)
+ size = GPIOLINES_MAX*16;
+ else if (size == 0)
+ size = linereq.num_lines*16;
+
+ ret = kfifo_alloc(&line->events, size, GFP_KERNEL);
+ if (ret)
+ goto out_free_line;
+
+ line->edge_detection = lc->edge_detection;
+ }
+
+ atomic_set(&line->seqno, 0);
+ line->num_descs = linereq.num_lines;
+ vals = (unsigned long *)lc->values.bitmap;
+
+ /* Request each GPIO */
+ for (i = 0; i < linereq.num_lines; i++) {
+ u32 offset = linereq.offsets[i];
+ struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);
+
+ if (IS_ERR(desc)) {
+ ret = PTR_ERR(desc);
+ goto out_free_line;
+ }
+
+ ret = gpiod_request(desc, line->label);
+ if (ret)
+ goto out_free_line;
+
+ line->descs[i] = desc;
+ gpioline_config_to_desc_flags(lc, &desc->flags);
+
+ ret = gpiod_set_transitory(desc, false);
+ if (ret < 0)
+ goto out_free_line;
+
+ /*
+ * Lines have to be requested explicitly for input
+ * or output, else the line will be treated "as is".
+ */
+ if (lc->flags & GPIOLINE_FLAG_V2_DIRECTION) {
+ if (lc->direction == GPIOLINE_DIRECTION_OUTPUT) {
+ int val = test_bit(i, vals);
+
+ ret = gpiod_direction_output(desc, val);
+ if (ret)
+ goto out_free_line;
+ } else {
+ ret = gpiod_direction_input(desc);
+ if (ret)
+ goto out_free_line;
+ ret = edge_detector_setup(&line->edets[i], lc);
+ if (ret)
+ goto out_free_line;
+ }
+ }
+
+ atomic_notifier_call_chain(&desc->gdev->notifier,
+ GPIOLINE_CHANGED_REQUESTED, desc);
+
+ dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
+ offset);
+ }
+
+ fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
+ if (fd < 0) {
+ ret = fd;
+ goto out_free_line;
+ }
+
+ file = anon_inode_getfile("gpio-line",
+ &line_fileops,
+ line,
+ O_RDONLY | O_CLOEXEC);
+ if (IS_ERR(file)) {
+ ret = PTR_ERR(file);
+ goto out_put_unused_fd;
+ }
+
+ linereq.fd = fd;
+ if (copy_to_user(ip, &linereq, sizeof(linereq))) {
+ /*
+ * fput() will trigger the release() callback, so do not go onto
+ * the regular error cleanup path here.
+ */
+ fput(file);
+ put_unused_fd(fd);
+ return -EFAULT;
+ }
+
+ fd_install(fd, file);
+
+ dev_dbg(&gdev->dev, "registered chardev handle for %d lines\n",
+ line->num_descs);
+
+ return 0;
+
+out_put_unused_fd:
+ put_unused_fd(fd);
+out_free_line:
+ line_free(line);
+ return ret;
+}
+
+#ifdef CONFIG_GPIO_CDEV_V1
/*
* GPIO line event management
@@ -750,10 +1448,63 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
return ret;
}
+static void gpioline_info_v2_to_v1(struct gpioline_info_v2 *info_v2,
+ struct gpioline_info *info_v1)
+{
+ int flagsv2 = info_v2->config.flags;
+
+ strncpy(info_v1->name, info_v2->name, sizeof(info_v1->name));
+ strncpy(info_v1->consumer, info_v2->consumer,
+ sizeof(info_v1->consumer));
+ info_v1->line_offset = info_v2->offset;
+ info_v1->flags = 0;
+
+ if (flagsv2 & GPIOLINE_FLAG_V2_USED)
+ info_v1->flags |= GPIOLINE_FLAG_KERNEL;
+
+ if (info_v2->config.direction == GPIOLINE_DIRECTION_OUTPUT)
+ info_v1->flags |= GPIOLINE_FLAG_IS_OUT;
+
+ if (flagsv2 & GPIOLINE_FLAG_V2_ACTIVE_LOW)
+ info_v1->flags |= GPIOLINE_FLAG_ACTIVE_LOW;
+
+ if (flagsv2 & GPIOLINE_FLAG_V2_DRIVE) {
+ enum gpioline_drive drive = info_v2->config.drive;
+
+ if (drive == GPIOLINE_DRIVE_OPEN_DRAIN)
+ info_v1->flags |= GPIOLINE_FLAG_OPEN_DRAIN;
+ else if (drive == GPIOLINE_DRIVE_OPEN_SOURCE)
+ info_v1->flags |= GPIOLINE_FLAG_OPEN_SOURCE;
+ }
+
+ if (flagsv2 & GPIOLINE_FLAG_V2_BIAS) {
+ enum gpioline_bias bias = info_v2->config.bias;
+
+ if (bias == GPIOLINE_BIAS_PULL_UP)
+ info_v1->flags |= GPIOLINE_FLAG_BIAS_PULL_UP;
+ else if (bias == GPIOLINE_BIAS_PULL_DOWN)
+ info_v1->flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
+ else if (bias == GPIOLINE_BIAS_DISABLED)
+ info_v1->flags |= GPIOLINE_FLAG_BIAS_DISABLE;
+ }
+}
+
+static void gpioline_info_changed_v2_to_v1(
+ struct gpioline_info_changed_v2 *lic_v2,
+ struct gpioline_info_changed *lic_v1)
+{
+ gpioline_info_v2_to_v1(&lic_v2->info, &lic_v1->info);
+ lic_v1->timestamp = lic_v2->timestamp;
+ lic_v1->event_type = lic_v2->event_type;
+}
+
+#endif /* CONFIG_GPIO_CDEV_V1 */
+
static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
- struct gpioline_info *info)
+ struct gpioline_info_v2 *info)
{
struct gpio_chip *gc = desc->gdev->chip;
+ struct gpioline_config *lc = &info->config;
bool ok_for_pinctrl;
unsigned long flags;
@@ -765,7 +1516,7 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
* lock common to both frameworks?
*/
ok_for_pinctrl =
- pinctrl_gpio_can_use_line(gc->base + info->line_offset);
+ pinctrl_gpio_can_use_line(gc->base + info->offset);
spin_lock_irqsave(&gpio_lock, flags);
@@ -783,34 +1534,46 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
info->consumer[0] = '\0';
}
+ lc->flags = GPIOLINE_FLAG_V2_DIRECTION;
/*
* 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) ||
!ok_for_pinctrl)
- info->flags |= GPIOLINE_FLAG_KERNEL;
- if (test_bit(FLAG_IS_OUT, &desc->flags))
- info->flags |= GPIOLINE_FLAG_IS_OUT;
+ lc->flags |= GPIOLINE_FLAG_V2_USED;
+ if (test_bit(FLAG_IS_OUT, &desc->flags)) {
+ lc->direction = GPIOLINE_DIRECTION_OUTPUT;
+ lc->flags |= GPIOLINE_FLAG_V2_DRIVE;
+ if (test_bit(FLAG_OPEN_DRAIN, &desc->flags))
+ lc->drive = GPIOLINE_DRIVE_OPEN_DRAIN;
+ else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
+ lc->drive = GPIOLINE_DRIVE_OPEN_SOURCE;
+ else
+ lc->drive = GPIOLINE_DRIVE_PUSH_PULL;
+ } else {
+ lc->direction = GPIOLINE_DIRECTION_INPUT;
+ }
+
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;
+ lc->flags |= GPIOLINE_FLAG_V2_ACTIVE_LOW;
+
+ if (test_bit(FLAG_BIAS_DISABLE, &desc->flags)) {
+ lc->flags |= GPIOLINE_FLAG_V2_BIAS;
+ lc->bias = GPIOLINE_BIAS_DISABLED;
+ } else if (test_bit(FLAG_PULL_DOWN, &desc->flags)) {
+ lc->flags |= GPIOLINE_FLAG_V2_BIAS;
+ lc->bias = GPIOLINE_BIAS_PULL_DOWN;
+ } else if (test_bit(FLAG_PULL_UP, &desc->flags)) {
+ lc->flags |= GPIOLINE_FLAG_V2_BIAS;
+ lc->bias = GPIOLINE_BIAS_PULL_UP;
+ }
+
+ lc->edge_detection = 0;
spin_unlock_irqrestore(&gpio_lock, flags);
}
@@ -818,11 +1581,62 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
struct gpio_chardev_data {
struct gpio_device *gdev;
wait_queue_head_t wait;
- DECLARE_KFIFO(events, struct gpioline_info_changed, 32);
+ DECLARE_KFIFO(events, struct gpioline_info_changed_v2, 32);
struct notifier_block lineinfo_changed_nb;
unsigned long *watched_lines;
+#ifdef CONFIG_GPIO_CDEV_V1
+ atomic_t watch_abi_version;
+#endif
};
+#ifdef CONFIG_GPIO_CDEV_V1
+static int lineinfo_ensure_abi_version(struct gpio_chardev_data *cdata,
+ unsigned int version)
+{
+ int abiv = atomic_read(&cdata->watch_abi_version);
+
+ if (abiv == 0) {
+ atomic_cmpxchg(&cdata->watch_abi_version, 0, version);
+ abiv = atomic_read(&cdata->watch_abi_version);
+ }
+ if (abiv != version)
+ return -EPERM;
+ return 0;
+}
+#endif
+
+static int lineinfo_get(struct gpio_chardev_data *gcdev, void __user *ip,
+ unsigned int cmd)
+{
+ struct gpio_desc *desc;
+ struct gpioline_info_v2 lineinfo;
+
+ if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
+ return -EFAULT;
+
+ if (padding_not_zeroed(lineinfo.padding, GPIOLINE_INFO_V2_PAD_SIZE))
+ return -EINVAL;
+
+ desc = gpiochip_get_desc(gcdev->gdev->chip, lineinfo.offset);
+ if (IS_ERR(desc))
+ return PTR_ERR(desc);
+
+ if (cmd == GPIO_GET_LINEINFO_WATCH_V2_IOCTL) {
+#ifdef CONFIG_GPIO_CDEV_V1
+ if (lineinfo_ensure_abi_version(gcdev, 2))
+ return -EPERM;
+#endif
+ if (test_and_set_bit(lineinfo.offset, gcdev->watched_lines))
+ return -EBUSY;
+ }
+ gpio_desc_to_lineinfo(desc, &lineinfo);
+
+ if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
+ return -EFAULT;
+
+ return 0;
+}
+
/*
* gpio_ioctl() - ioctl handler for the GPIO chardev
*/
@@ -855,8 +1669,10 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
if (copy_to_user(ip, &chipinfo, sizeof(chipinfo)))
return -EFAULT;
return 0;
+#ifdef CONFIG_GPIO_CDEV_V1
} else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
struct gpioline_info lineinfo;
+ struct gpioline_info_v2 lineinfo_v2;
if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
return -EFAULT;
@@ -866,7 +1682,9 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
if (IS_ERR(desc))
return PTR_ERR(desc);
- gpio_desc_to_lineinfo(desc, &lineinfo);
+ lineinfo_v2.offset = lineinfo.line_offset;
+ gpio_desc_to_lineinfo(desc, &lineinfo_v2);
+ gpioline_info_v2_to_v1(&lineinfo_v2, &lineinfo);
if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
return -EFAULT;
@@ -877,6 +1695,7 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
return lineevent_create(gdev, ip);
} else if (cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) {
struct gpioline_info lineinfo;
+ struct gpioline_info_v2 lineinfo_v2;
if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
return -EFAULT;
@@ -886,15 +1705,26 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
if (IS_ERR(desc))
return PTR_ERR(desc);
+ if (lineinfo_ensure_abi_version(gcdev, 1))
+ return -EPERM;
+
if (test_and_set_bit(lineinfo.line_offset, gcdev->watched_lines))
return -EBUSY;
- gpio_desc_to_lineinfo(desc, &lineinfo);
+ lineinfo_v2.offset = lineinfo.line_offset;
+ gpio_desc_to_lineinfo(desc, &lineinfo_v2);
+ gpioline_info_v2_to_v1(&lineinfo_v2, &lineinfo);
if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
return -EFAULT;
return 0;
+#endif /* CONFIG_GPIO_CDEV_V1 */
+ } else if (cmd == GPIO_GET_LINEINFO_V2_IOCTL ||
+ cmd == GPIO_GET_LINEINFO_WATCH_V2_IOCTL) {
+ return lineinfo_get(gcdev, ip, cmd);
+ } else if (cmd == GPIO_GET_LINE_IOCTL) {
+ return line_create(gdev, ip);
} else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) {
if (copy_from_user(&offset, ip, sizeof(offset)))
return -EFAULT;
@@ -928,7 +1758,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
unsigned long action, void *data)
{
struct gpio_chardev_data *gcdev = to_gpio_chardev_data(nb);
- struct gpioline_info_changed chg;
+ struct gpioline_info_changed_v2 chg;
struct gpio_desc *desc = data;
int ret;
@@ -936,7 +1766,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
return NOTIFY_DONE;
memset(&chg, 0, sizeof(chg));
- chg.info.line_offset = gpio_chip_hwgpio(desc);
+ chg.info.offset = gpio_chip_hwgpio(desc);
chg.event_type = action;
chg.timestamp = ktime_get_ns();
gpio_desc_to_lineinfo(desc, &chg.info);
@@ -969,12 +1799,16 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
size_t count, loff_t *off)
{
struct gpio_chardev_data *gcdev = file->private_data;
- struct gpioline_info_changed event;
+ struct gpioline_info_changed_v2 event;
ssize_t bytes_read = 0;
int ret;
+ size_t event_size;
- if (count < sizeof(event))
+#ifndef CONFIG_GPIO_CDEV_V1
+ event_size = sizeof(struct gpioline_info_changed_v2);
+ if (count < event_size)
return -EINVAL;
+#endif
do {
spin_lock(&gcdev->wait.lock);
@@ -996,7 +1830,17 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
return ret;
}
}
-
+#ifdef CONFIG_GPIO_CDEV_V1
+ /* must be after kfifo check so watch_abi_version is set */
+ if (atomic_read(&gcdev->watch_abi_version) == 2)
+ event_size = sizeof(struct gpioline_info_changed_v2);
+ else
+ event_size = sizeof(struct gpioline_info_changed);
+ if (count < event_size) {
+ spin_unlock(&gcdev->wait.lock);
+ return -EINVAL;
+ }
+#endif
ret = kfifo_out(&gcdev->events, &event, 1);
spin_unlock(&gcdev->wait.lock);
if (ret != 1) {
@@ -1005,9 +1849,23 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
/* We should never get here. See lineevent_read(). */
}
- if (copy_to_user(buf + bytes_read, &event, sizeof(event)))
+#ifdef CONFIG_GPIO_CDEV_V1
+ if (atomic_read(&gcdev->watch_abi_version) == 2) {
+ if (copy_to_user(buf + bytes_read, &event, event_size))
+ return -EFAULT;
+ } else {
+ struct gpioline_info_changed event_v1;
+
+ gpioline_info_changed_v2_to_v1(&event, &event_v1);
+ if (copy_to_user(buf + bytes_read, &event_v1,
+ event_size))
+ return -EFAULT;
+ }
+#else
+ if (copy_to_user(buf + bytes_read, &event, event_size))
return -EFAULT;
- bytes_read += sizeof(event);
+#endif
+ bytes_read += event_size;
} while (count >= bytes_read + sizeof(event));
return bytes_read;
@@ -1121,4 +1979,22 @@ int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
void gpiolib_cdev_unregister(struct gpio_device *gdev)
{
cdev_device_del(&gdev->chrdev, &gdev->dev);
+
+ /*
+ * array sizes must be a multiple of 8 to ensure 64bit alignment and
+ * to not create holes in the struct packing.
+ */
+ BUILD_BUG_ON(GPIOLINES_MAX % 8);
+ BUILD_BUG_ON(GPIO_MAX_NAME_SIZE % 8);
+
+ /*
+ * check that uAPI structs are 64bit aligned for 32/64bit
+ * compatibility
+ */
+ BUILD_BUG_ON(sizeof(struct gpioline_config) % 8);
+ BUILD_BUG_ON(sizeof(struct gpioline_request) % 8);
+ BUILD_BUG_ON(sizeof(struct gpioline_info_v2) % 8);
+ BUILD_BUG_ON(sizeof(struct gpioline_info_changed_v2) % 8);
+ BUILD_BUG_ON(sizeof(struct gpioline_event) % 8);
+ BUILD_BUG_ON(sizeof(struct gpioline_values) % 8);
}
--
2.27.0
Move gpiolib-sysfs function declarations into their own header.
These functions are in gpiolib-sysfs.c, and are only required by gpiolib.c,
and so should be in a module header, not giolib.h.
This brings gpiolib-sysfs into line with gpiolib-cdev, and is another step
towards removing the sysfs inferface.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-sysfs.c | 1 +
drivers/gpio/gpiolib-sysfs.h | 24 ++++++++++++++++++++++++
drivers/gpio/gpiolib.c | 1 +
drivers/gpio/gpiolib.h | 18 ------------------
4 files changed, 26 insertions(+), 18 deletions(-)
create mode 100644 drivers/gpio/gpiolib-sysfs.h
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 23e3d335cd54..18f94be1b458 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -11,6 +11,7 @@
#include <linux/ctype.h>
#include "gpiolib.h"
+#include "gpiolib-sysfs.h"
#define GPIO_IRQF_TRIGGER_FALLING BIT(0)
#define GPIO_IRQF_TRIGGER_RISING BIT(1)
diff --git a/drivers/gpio/gpiolib-sysfs.h b/drivers/gpio/gpiolib-sysfs.h
new file mode 100644
index 000000000000..ddd0e503f8eb
--- /dev/null
+++ b/drivers/gpio/gpiolib-sysfs.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef GPIOLIB_SYSFS_H
+#define GPIOLIB_SYSFS_H
+
+#ifdef CONFIG_GPIO_SYSFS
+
+int gpiochip_sysfs_register(struct gpio_device *gdev);
+void gpiochip_sysfs_unregister(struct gpio_device *gdev);
+
+#else
+
+static inline int gpiochip_sysfs_register(struct gpio_device *gdev)
+{
+ return 0;
+}
+
+static inline void gpiochip_sysfs_unregister(struct gpio_device *gdev)
+{
+}
+
+#endif /* CONFIG_GPIO_SYSFS */
+
+#endif /* GPIOLIB_SYSFS_H */
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4abd751314a5..517c99ddf6c8 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -26,6 +26,7 @@
#include "gpiolib-of.h"
#include "gpiolib-acpi.h"
#include "gpiolib-cdev.h"
+#include "gpiolib-sysfs.h"
#define CREATE_TRACE_POINTS
#include <trace/events/gpio.h>
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 9ed242316414..2dee4e1e12dc 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -175,22 +175,4 @@ static inline int gpio_chip_hwgpio(const struct gpio_desc *desc)
#define chip_dbg(gc, fmt, ...) \
dev_dbg(&gc->gpiodev->dev, "(%s): " fmt, gc->label, ##__VA_ARGS__)
-#ifdef CONFIG_GPIO_SYSFS
-
-int gpiochip_sysfs_register(struct gpio_device *gdev);
-void gpiochip_sysfs_unregister(struct gpio_device *gdev);
-
-#else
-
-static inline int gpiochip_sysfs_register(struct gpio_device *gdev)
-{
- return 0;
-}
-
-static inline void gpiochip_sysfs_unregister(struct gpio_device *gdev)
-{
-}
-
-#endif /* CONFIG_GPIO_SYSFS */
-
#endif /* GPIOLIB_H */
--
2.27.0
[ The copy_to_user() overflow code is weird. Why do we need to do a
atomic_read()? That suggests that there is a potential time of check
time of use bug where we do:
if (atomic_read(&gcdev->watch_abi_version) == 2) // <<-- time of check
event_size = sizeof(struct gpioline_info_changed_v2);
...
if (atomic_read(&gcdev->watch_abi_version) == 2) { // <<-- time of use
copy_to_user(blah, blah, event_size);
If the value for "gcdev->watch_abi_version" changes between the time
of check and the time of use then it can read beyond the end of the
buffer.
-- dan ]
Hi Kent,
Thank you for the patch! Perhaps something to improve:
url: https://github.com/0day-ci/linux/commits/Kent-Gibson/gpio-cdev-add-uAPI-V2/20200623-120634
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: openrisc-randconfig-m031-20200623 (attached as .config)
compiler: or1k-linux-gcc (GCC) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
smatch warnings:
drivers/gpio/gpiolib-cdev.c:891 line_free() error: dereferencing freed memory 'line'
drivers/gpio/gpiolib-cdev.c:949 line_create() warn: possible memory leak of 'line'
drivers/gpio/gpiolib-cdev.c:1860 lineinfo_watch_read() error: copy_to_user() '&event_v1' too small (104 vs 168)
# https://github.com/0day-ci/linux/commit/f3b3ae8752adc5ac33dcf83d49b0b02f2d7ef43b
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout f3b3ae8752adc5ac33dcf83d49b0b02f2d7ef43b
vim +/line +891 drivers/gpio/gpiolib-cdev.c
f3b3ae8752adc5 Kent Gibson 2020-06-23 877 static void line_free(struct line *line)
f3b3ae8752adc5 Kent Gibson 2020-06-23 878 {
f3b3ae8752adc5 Kent Gibson 2020-06-23 879 int i;
f3b3ae8752adc5 Kent Gibson 2020-06-23 880
f3b3ae8752adc5 Kent Gibson 2020-06-23 881 for (i = 0; i < line->num_descs; i++) {
f3b3ae8752adc5 Kent Gibson 2020-06-23 882 if (line->edets)
f3b3ae8752adc5 Kent Gibson 2020-06-23 883 edge_detector_stop(&line->edets[i]);
f3b3ae8752adc5 Kent Gibson 2020-06-23 884 if (line->descs[i])
f3b3ae8752adc5 Kent Gibson 2020-06-23 885 gpiod_free(line->descs[i]);
f3b3ae8752adc5 Kent Gibson 2020-06-23 886 }
f3b3ae8752adc5 Kent Gibson 2020-06-23 887 kfifo_free(&line->events);
f3b3ae8752adc5 Kent Gibson 2020-06-23 888 kfree(line->label);
f3b3ae8752adc5 Kent Gibson 2020-06-23 889 kfree(line->edets);
f3b3ae8752adc5 Kent Gibson 2020-06-23 890 kfree(line);
f3b3ae8752adc5 Kent Gibson 2020-06-23 @891 put_device(&line->gdev->dev);
f3b3ae8752adc5 Kent Gibson 2020-06-23 892 }
f3b3ae8752adc5 Kent Gibson 2020-06-23 893
f3b3ae8752adc5 Kent Gibson 2020-06-23 894 static int line_release(struct inode *inode, struct file *file)
f3b3ae8752adc5 Kent Gibson 2020-06-23 895 {
f3b3ae8752adc5 Kent Gibson 2020-06-23 896 struct line *line = file->private_data;
f3b3ae8752adc5 Kent Gibson 2020-06-23 897
f3b3ae8752adc5 Kent Gibson 2020-06-23 898 line_free(line);
f3b3ae8752adc5 Kent Gibson 2020-06-23 899 return 0;
f3b3ae8752adc5 Kent Gibson 2020-06-23 900 }
f3b3ae8752adc5 Kent Gibson 2020-06-23 901
f3b3ae8752adc5 Kent Gibson 2020-06-23 902 static const struct file_operations line_fileops = {
f3b3ae8752adc5 Kent Gibson 2020-06-23 903 .release = line_release,
f3b3ae8752adc5 Kent Gibson 2020-06-23 904 .read = line_read,
f3b3ae8752adc5 Kent Gibson 2020-06-23 905 .poll = line_poll,
f3b3ae8752adc5 Kent Gibson 2020-06-23 906 .owner = THIS_MODULE,
f3b3ae8752adc5 Kent Gibson 2020-06-23 907 .llseek = noop_llseek,
f3b3ae8752adc5 Kent Gibson 2020-06-23 908 .unlocked_ioctl = line_ioctl,
f3b3ae8752adc5 Kent Gibson 2020-06-23 909 #ifdef CONFIG_COMPAT
f3b3ae8752adc5 Kent Gibson 2020-06-23 910 .compat_ioctl = line_ioctl_compat,
f3b3ae8752adc5 Kent Gibson 2020-06-23 911 #endif
f3b3ae8752adc5 Kent Gibson 2020-06-23 912 };
f3b3ae8752adc5 Kent Gibson 2020-06-23 913
f3b3ae8752adc5 Kent Gibson 2020-06-23 914 static int line_create(struct gpio_device *gdev, void __user *ip)
f3b3ae8752adc5 Kent Gibson 2020-06-23 915 {
f3b3ae8752adc5 Kent Gibson 2020-06-23 916 struct gpioline_request linereq;
f3b3ae8752adc5 Kent Gibson 2020-06-23 917 struct line *line;
f3b3ae8752adc5 Kent Gibson 2020-06-23 918 struct file *file;
f3b3ae8752adc5 Kent Gibson 2020-06-23 919 int fd, i, ret, size;
f3b3ae8752adc5 Kent Gibson 2020-06-23 920 struct gpioline_config *lc;
f3b3ae8752adc5 Kent Gibson 2020-06-23 921 unsigned long *vals;
f3b3ae8752adc5 Kent Gibson 2020-06-23 922
f3b3ae8752adc5 Kent Gibson 2020-06-23 923 if (copy_from_user(&linereq, ip, sizeof(linereq)))
f3b3ae8752adc5 Kent Gibson 2020-06-23 924 return -EFAULT;
f3b3ae8752adc5 Kent Gibson 2020-06-23 925 if ((linereq.num_lines == 0) || (linereq.num_lines > GPIOLINES_MAX))
f3b3ae8752adc5 Kent Gibson 2020-06-23 926 return -EINVAL;
f3b3ae8752adc5 Kent Gibson 2020-06-23 927
f3b3ae8752adc5 Kent Gibson 2020-06-23 928 if (padding_not_zeroed(linereq.padding, GPIOLINE_REQUEST_PAD_SIZE))
f3b3ae8752adc5 Kent Gibson 2020-06-23 929 return -EINVAL;
f3b3ae8752adc5 Kent Gibson 2020-06-23 930
f3b3ae8752adc5 Kent Gibson 2020-06-23 931 lc = &linereq.config;
f3b3ae8752adc5 Kent Gibson 2020-06-23 932 ret = gpioline_config_validate(lc);
f3b3ae8752adc5 Kent Gibson 2020-06-23 933 if (ret)
f3b3ae8752adc5 Kent Gibson 2020-06-23 934 return ret;
f3b3ae8752adc5 Kent Gibson 2020-06-23 935
f3b3ae8752adc5 Kent Gibson 2020-06-23 936 /* event_buffer_size only valid with edge_detection */
f3b3ae8752adc5 Kent Gibson 2020-06-23 937 if ((linereq.event_buffer_size) &&
f3b3ae8752adc5 Kent Gibson 2020-06-23 938 !(linereq.config.flags & GPIOLINE_FLAG_V2_EDGE_DETECTION))
f3b3ae8752adc5 Kent Gibson 2020-06-23 939 return -EINVAL;
f3b3ae8752adc5 Kent Gibson 2020-06-23 940
f3b3ae8752adc5 Kent Gibson 2020-06-23 941 line = kzalloc(struct_size(line, descs, linereq.num_lines),
f3b3ae8752adc5 Kent Gibson 2020-06-23 942 GFP_KERNEL);
f3b3ae8752adc5 Kent Gibson 2020-06-23 943 if (!line)
f3b3ae8752adc5 Kent Gibson 2020-06-23 944 return -ENOMEM;
f3b3ae8752adc5 Kent Gibson 2020-06-23 945
f3b3ae8752adc5 Kent Gibson 2020-06-23 946 line->edets = kcalloc(linereq.num_lines, sizeof(*line->edets),
f3b3ae8752adc5 Kent Gibson 2020-06-23 947 GFP_KERNEL);
f3b3ae8752adc5 Kent Gibson 2020-06-23 948 if (!line->edets)
f3b3ae8752adc5 Kent Gibson 2020-06-23 @949 return -ENOMEM;
^^^^^^^^^^^^^^^
kfree(line) before returning.
f3b3ae8752adc5 Kent Gibson 2020-06-23 950
f3b3ae8752adc5 Kent Gibson 2020-06-23 951 for (i = 0; i < linereq.num_lines; i++)
f3b3ae8752adc5 Kent Gibson 2020-06-23 952 line->edets[i].line = line;
f3b3ae8752adc5 Kent Gibson 2020-06-23 953
f3b3ae8752adc5 Kent Gibson 2020-06-23 954 line->gdev = gdev;
f3b3ae8752adc5 Kent Gibson 2020-06-23 955 get_device(&gdev->dev);
f3b3ae8752adc5 Kent Gibson 2020-06-23 956
f3b3ae8752adc5 Kent Gibson 2020-06-23 957 /* Make sure this is terminated */
f3b3ae8752adc5 Kent Gibson 2020-06-23 958 linereq.consumer[sizeof(linereq.consumer)-1] = '\0';
f3b3ae8752adc5 Kent Gibson 2020-06-23 959 if (strlen(linereq.consumer)) {
f3b3ae8752adc5 Kent Gibson 2020-06-23 960 line->label = kstrdup(linereq.consumer, GFP_KERNEL);
f3b3ae8752adc5 Kent Gibson 2020-06-23 961 if (!line->label) {
f3b3ae8752adc5 Kent Gibson 2020-06-23 962 ret = -ENOMEM;
f3b3ae8752adc5 Kent Gibson 2020-06-23 963 goto out_free_line;
f3b3ae8752adc5 Kent Gibson 2020-06-23 964 }
f3b3ae8752adc5 Kent Gibson 2020-06-23 965 }
f3b3ae8752adc5 Kent Gibson 2020-06-23 966
f3b3ae8752adc5 Kent Gibson 2020-06-23 967 mutex_init(&line->config_mutex);
f3b3ae8752adc5 Kent Gibson 2020-06-23 968 init_waitqueue_head(&line->wait);
f3b3ae8752adc5 Kent Gibson 2020-06-23 969 if (lc->edge_detection) {
f3b3ae8752adc5 Kent Gibson 2020-06-23 970 size = linereq.event_buffer_size;
f3b3ae8752adc5 Kent Gibson 2020-06-23 971
f3b3ae8752adc5 Kent Gibson 2020-06-23 972 if (size > GPIOLINES_MAX*16)
f3b3ae8752adc5 Kent Gibson 2020-06-23 973 size = GPIOLINES_MAX*16;
f3b3ae8752adc5 Kent Gibson 2020-06-23 974 else if (size == 0)
f3b3ae8752adc5 Kent Gibson 2020-06-23 975 size = linereq.num_lines*16;
f3b3ae8752adc5 Kent Gibson 2020-06-23 976
f3b3ae8752adc5 Kent Gibson 2020-06-23 977 ret = kfifo_alloc(&line->events, size, GFP_KERNEL);
f3b3ae8752adc5 Kent Gibson 2020-06-23 978 if (ret)
f3b3ae8752adc5 Kent Gibson 2020-06-23 979 goto out_free_line;
f3b3ae8752adc5 Kent Gibson 2020-06-23 980
f3b3ae8752adc5 Kent Gibson 2020-06-23 981 line->edge_detection = lc->edge_detection;
f3b3ae8752adc5 Kent Gibson 2020-06-23 982 }
f3b3ae8752adc5 Kent Gibson 2020-06-23 983
f3b3ae8752adc5 Kent Gibson 2020-06-23 984 atomic_set(&line->seqno, 0);
f3b3ae8752adc5 Kent Gibson 2020-06-23 985 line->num_descs = linereq.num_lines;
f3b3ae8752adc5 Kent Gibson 2020-06-23 986 vals = (unsigned long *)lc->values.bitmap;
f3b3ae8752adc5 Kent Gibson 2020-06-23 987
f3b3ae8752adc5 Kent Gibson 2020-06-23 988 /* Request each GPIO */
f3b3ae8752adc5 Kent Gibson 2020-06-23 989 for (i = 0; i < linereq.num_lines; i++) {
f3b3ae8752adc5 Kent Gibson 2020-06-23 990 u32 offset = linereq.offsets[i];
f3b3ae8752adc5 Kent Gibson 2020-06-23 991 struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);
f3b3ae8752adc5 Kent Gibson 2020-06-23 992
f3b3ae8752adc5 Kent Gibson 2020-06-23 993 if (IS_ERR(desc)) {
f3b3ae8752adc5 Kent Gibson 2020-06-23 994 ret = PTR_ERR(desc);
f3b3ae8752adc5 Kent Gibson 2020-06-23 995 goto out_free_line;
f3b3ae8752adc5 Kent Gibson 2020-06-23 996 }
f3b3ae8752adc5 Kent Gibson 2020-06-23 997
f3b3ae8752adc5 Kent Gibson 2020-06-23 998 ret = gpiod_request(desc, line->label);
f3b3ae8752adc5 Kent Gibson 2020-06-23 999 if (ret)
f3b3ae8752adc5 Kent Gibson 2020-06-23 1000 goto out_free_line;
f3b3ae8752adc5 Kent Gibson 2020-06-23 1001
f3b3ae8752adc5 Kent Gibson 2020-06-23 1002 line->descs[i] = desc;
f3b3ae8752adc5 Kent Gibson 2020-06-23 1003 gpioline_config_to_desc_flags(lc, &desc->flags);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1004
f3b3ae8752adc5 Kent Gibson 2020-06-23 1005 ret = gpiod_set_transitory(desc, false);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1006 if (ret < 0)
f3b3ae8752adc5 Kent Gibson 2020-06-23 1007 goto out_free_line;
f3b3ae8752adc5 Kent Gibson 2020-06-23 1008
f3b3ae8752adc5 Kent Gibson 2020-06-23 1009 /*
f3b3ae8752adc5 Kent Gibson 2020-06-23 1010 * Lines have to be requested explicitly for input
f3b3ae8752adc5 Kent Gibson 2020-06-23 1011 * or output, else the line will be treated "as is".
f3b3ae8752adc5 Kent Gibson 2020-06-23 1012 */
f3b3ae8752adc5 Kent Gibson 2020-06-23 1013 if (lc->flags & GPIOLINE_FLAG_V2_DIRECTION) {
f3b3ae8752adc5 Kent Gibson 2020-06-23 1014 if (lc->direction == GPIOLINE_DIRECTION_OUTPUT) {
f3b3ae8752adc5 Kent Gibson 2020-06-23 1015 int val = test_bit(i, vals);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1016
f3b3ae8752adc5 Kent Gibson 2020-06-23 1017 ret = gpiod_direction_output(desc, val);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1018 if (ret)
f3b3ae8752adc5 Kent Gibson 2020-06-23 1019 goto out_free_line;
f3b3ae8752adc5 Kent Gibson 2020-06-23 1020 } else {
f3b3ae8752adc5 Kent Gibson 2020-06-23 1021 ret = gpiod_direction_input(desc);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1022 if (ret)
f3b3ae8752adc5 Kent Gibson 2020-06-23 1023 goto out_free_line;
f3b3ae8752adc5 Kent Gibson 2020-06-23 1024 ret = edge_detector_setup(&line->edets[i], lc);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1025 if (ret)
f3b3ae8752adc5 Kent Gibson 2020-06-23 1026 goto out_free_line;
f3b3ae8752adc5 Kent Gibson 2020-06-23 1027 }
f3b3ae8752adc5 Kent Gibson 2020-06-23 1028 }
f3b3ae8752adc5 Kent Gibson 2020-06-23 1029
f3b3ae8752adc5 Kent Gibson 2020-06-23 1030 atomic_notifier_call_chain(&desc->gdev->notifier,
f3b3ae8752adc5 Kent Gibson 2020-06-23 1031 GPIOLINE_CHANGED_REQUESTED, desc);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1032
f3b3ae8752adc5 Kent Gibson 2020-06-23 1033 dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
f3b3ae8752adc5 Kent Gibson 2020-06-23 1034 offset);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1035 }
f3b3ae8752adc5 Kent Gibson 2020-06-23 1036
f3b3ae8752adc5 Kent Gibson 2020-06-23 1037 fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1038 if (fd < 0) {
f3b3ae8752adc5 Kent Gibson 2020-06-23 1039 ret = fd;
f3b3ae8752adc5 Kent Gibson 2020-06-23 1040 goto out_free_line;
f3b3ae8752adc5 Kent Gibson 2020-06-23 1041 }
f3b3ae8752adc5 Kent Gibson 2020-06-23 1042
f3b3ae8752adc5 Kent Gibson 2020-06-23 1043 file = anon_inode_getfile("gpio-line",
f3b3ae8752adc5 Kent Gibson 2020-06-23 1044 &line_fileops,
f3b3ae8752adc5 Kent Gibson 2020-06-23 1045 line,
f3b3ae8752adc5 Kent Gibson 2020-06-23 1046 O_RDONLY | O_CLOEXEC);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1047 if (IS_ERR(file)) {
f3b3ae8752adc5 Kent Gibson 2020-06-23 1048 ret = PTR_ERR(file);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1049 goto out_put_unused_fd;
f3b3ae8752adc5 Kent Gibson 2020-06-23 1050 }
f3b3ae8752adc5 Kent Gibson 2020-06-23 1051
f3b3ae8752adc5 Kent Gibson 2020-06-23 1052 linereq.fd = fd;
f3b3ae8752adc5 Kent Gibson 2020-06-23 1053 if (copy_to_user(ip, &linereq, sizeof(linereq))) {
f3b3ae8752adc5 Kent Gibson 2020-06-23 1054 /*
f3b3ae8752adc5 Kent Gibson 2020-06-23 1055 * fput() will trigger the release() callback, so do not go onto
f3b3ae8752adc5 Kent Gibson 2020-06-23 1056 * the regular error cleanup path here.
f3b3ae8752adc5 Kent Gibson 2020-06-23 1057 */
f3b3ae8752adc5 Kent Gibson 2020-06-23 1058 fput(file);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1059 put_unused_fd(fd);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1060 return -EFAULT;
f3b3ae8752adc5 Kent Gibson 2020-06-23 1061 }
f3b3ae8752adc5 Kent Gibson 2020-06-23 1062
f3b3ae8752adc5 Kent Gibson 2020-06-23 1063 fd_install(fd, file);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1064
f3b3ae8752adc5 Kent Gibson 2020-06-23 1065 dev_dbg(&gdev->dev, "registered chardev handle for %d lines\n",
f3b3ae8752adc5 Kent Gibson 2020-06-23 1066 line->num_descs);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1067
f3b3ae8752adc5 Kent Gibson 2020-06-23 1068 return 0;
f3b3ae8752adc5 Kent Gibson 2020-06-23 1069
f3b3ae8752adc5 Kent Gibson 2020-06-23 1070 out_put_unused_fd:
f3b3ae8752adc5 Kent Gibson 2020-06-23 1071 put_unused_fd(fd);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1072 out_free_line:
f3b3ae8752adc5 Kent Gibson 2020-06-23 1073 line_free(line);
f3b3ae8752adc5 Kent Gibson 2020-06-23 1074 return ret;
f3b3ae8752adc5 Kent Gibson 2020-06-23 1075 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Tue, Jun 23, 2020 at 08:44:38PM +0300, Dan Carpenter wrote:
> [ The copy_to_user() overflow code is weird. Why do we need to do a
> atomic_read()? That suggests that there is a potential time of check
> time of use bug where we do:
>
It is weird, but you conveniently left out the guard comment:
/* must be after kfifo check so watch_abi_version is set */
> if (atomic_read(&gcdev->watch_abi_version) == 2) // <<-- time of check
> event_size = sizeof(struct gpioline_info_changed_v2);
>
For something to be in the fifo lineinfo_ensure_abi_version must've been
called. And the watch_abi_version can only be set once by
lineinfo_ensure_abi_version, so it cannot change between.
But point taken, I'll change the "time of use" condition to
if (event_size == sizeof(struct gpioline_info_changed_v2)) {
if (copy_to_user(buf + bytes_read, &event, event_size))
> ...
>
> if (atomic_read(&gcdev->watch_abi_version) == 2) { // <<-- time of use
> copy_to_user(blah, blah, event_size);
>
> If the value for "gcdev->watch_abi_version" changes between the time
> of check and the time of use then it can read beyond the end of the
> buffer.
>
> -- dan ]
>
> Hi Kent,
>
> Thank you for the patch! Perhaps something to improve:
>
> url: https://github.com/0day-ci/linux/commits/Kent-Gibson/gpio-cdev-add-uAPI-V2/20200623-120634
> base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
> config: openrisc-randconfig-m031-20200623 (attached as .config)
> compiler: or1k-linux-gcc (GCC) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
>
> smatch warnings:
> drivers/gpio/gpiolib-cdev.c:891 line_free() error: dereferencing freed memory 'line'
> drivers/gpio/gpiolib-cdev.c:949 line_create() warn: possible memory leak of 'line'
> drivers/gpio/gpiolib-cdev.c:1860 lineinfo_watch_read() error: copy_to_user() '&event_v1' too small (104 vs 168)
>
> # https://github.com/0day-ci/linux/commit/f3b3ae8752adc5ac33dcf83d49b0b02f2d7ef43b
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout f3b3ae8752adc5ac33dcf83d49b0b02f2d7ef43b
> vim +/line +891 drivers/gpio/gpiolib-cdev.c
>
> f3b3ae8752adc5 Kent Gibson 2020-06-23 877 static void line_free(struct line *line)
> f3b3ae8752adc5 Kent Gibson 2020-06-23 878 {
> f3b3ae8752adc5 Kent Gibson 2020-06-23 879 int i;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 880
> f3b3ae8752adc5 Kent Gibson 2020-06-23 881 for (i = 0; i < line->num_descs; i++) {
> f3b3ae8752adc5 Kent Gibson 2020-06-23 882 if (line->edets)
> f3b3ae8752adc5 Kent Gibson 2020-06-23 883 edge_detector_stop(&line->edets[i]);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 884 if (line->descs[i])
> f3b3ae8752adc5 Kent Gibson 2020-06-23 885 gpiod_free(line->descs[i]);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 886 }
> f3b3ae8752adc5 Kent Gibson 2020-06-23 887 kfifo_free(&line->events);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 888 kfree(line->label);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 889 kfree(line->edets);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 890 kfree(line);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 @891 put_device(&line->gdev->dev);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 892 }
> f3b3ae8752adc5 Kent Gibson 2020-06-23 893
> f3b3ae8752adc5 Kent Gibson 2020-06-23 894 static int line_release(struct inode *inode, struct file *file)
> f3b3ae8752adc5 Kent Gibson 2020-06-23 895 {
> f3b3ae8752adc5 Kent Gibson 2020-06-23 896 struct line *line = file->private_data;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 897
> f3b3ae8752adc5 Kent Gibson 2020-06-23 898 line_free(line);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 899 return 0;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 900 }
> f3b3ae8752adc5 Kent Gibson 2020-06-23 901
> f3b3ae8752adc5 Kent Gibson 2020-06-23 902 static const struct file_operations line_fileops = {
> f3b3ae8752adc5 Kent Gibson 2020-06-23 903 .release = line_release,
> f3b3ae8752adc5 Kent Gibson 2020-06-23 904 .read = line_read,
> f3b3ae8752adc5 Kent Gibson 2020-06-23 905 .poll = line_poll,
> f3b3ae8752adc5 Kent Gibson 2020-06-23 906 .owner = THIS_MODULE,
> f3b3ae8752adc5 Kent Gibson 2020-06-23 907 .llseek = noop_llseek,
> f3b3ae8752adc5 Kent Gibson 2020-06-23 908 .unlocked_ioctl = line_ioctl,
> f3b3ae8752adc5 Kent Gibson 2020-06-23 909 #ifdef CONFIG_COMPAT
> f3b3ae8752adc5 Kent Gibson 2020-06-23 910 .compat_ioctl = line_ioctl_compat,
> f3b3ae8752adc5 Kent Gibson 2020-06-23 911 #endif
> f3b3ae8752adc5 Kent Gibson 2020-06-23 912 };
> f3b3ae8752adc5 Kent Gibson 2020-06-23 913
> f3b3ae8752adc5 Kent Gibson 2020-06-23 914 static int line_create(struct gpio_device *gdev, void __user *ip)
> f3b3ae8752adc5 Kent Gibson 2020-06-23 915 {
> f3b3ae8752adc5 Kent Gibson 2020-06-23 916 struct gpioline_request linereq;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 917 struct line *line;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 918 struct file *file;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 919 int fd, i, ret, size;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 920 struct gpioline_config *lc;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 921 unsigned long *vals;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 922
> f3b3ae8752adc5 Kent Gibson 2020-06-23 923 if (copy_from_user(&linereq, ip, sizeof(linereq)))
> f3b3ae8752adc5 Kent Gibson 2020-06-23 924 return -EFAULT;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 925 if ((linereq.num_lines == 0) || (linereq.num_lines > GPIOLINES_MAX))
> f3b3ae8752adc5 Kent Gibson 2020-06-23 926 return -EINVAL;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 927
> f3b3ae8752adc5 Kent Gibson 2020-06-23 928 if (padding_not_zeroed(linereq.padding, GPIOLINE_REQUEST_PAD_SIZE))
> f3b3ae8752adc5 Kent Gibson 2020-06-23 929 return -EINVAL;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 930
> f3b3ae8752adc5 Kent Gibson 2020-06-23 931 lc = &linereq.config;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 932 ret = gpioline_config_validate(lc);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 933 if (ret)
> f3b3ae8752adc5 Kent Gibson 2020-06-23 934 return ret;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 935
> f3b3ae8752adc5 Kent Gibson 2020-06-23 936 /* event_buffer_size only valid with edge_detection */
> f3b3ae8752adc5 Kent Gibson 2020-06-23 937 if ((linereq.event_buffer_size) &&
> f3b3ae8752adc5 Kent Gibson 2020-06-23 938 !(linereq.config.flags & GPIOLINE_FLAG_V2_EDGE_DETECTION))
> f3b3ae8752adc5 Kent Gibson 2020-06-23 939 return -EINVAL;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 940
> f3b3ae8752adc5 Kent Gibson 2020-06-23 941 line = kzalloc(struct_size(line, descs, linereq.num_lines),
> f3b3ae8752adc5 Kent Gibson 2020-06-23 942 GFP_KERNEL);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 943 if (!line)
> f3b3ae8752adc5 Kent Gibson 2020-06-23 944 return -ENOMEM;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 945
> f3b3ae8752adc5 Kent Gibson 2020-06-23 946 line->edets = kcalloc(linereq.num_lines, sizeof(*line->edets),
> f3b3ae8752adc5 Kent Gibson 2020-06-23 947 GFP_KERNEL);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 948 if (!line->edets)
> f3b3ae8752adc5 Kent Gibson 2020-06-23 @949 return -ENOMEM;
> ^^^^^^^^^^^^^^^
> kfree(line) before returning.
>
Yeah, that is bad. Good pickup - it should be a goto out_free_line like
the one below for line->label.
Cheers,
Kent.
> f3b3ae8752adc5 Kent Gibson 2020-06-23 950
> f3b3ae8752adc5 Kent Gibson 2020-06-23 951 for (i = 0; i < linereq.num_lines; i++)
> f3b3ae8752adc5 Kent Gibson 2020-06-23 952 line->edets[i].line = line;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 953
> f3b3ae8752adc5 Kent Gibson 2020-06-23 954 line->gdev = gdev;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 955 get_device(&gdev->dev);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 956
> f3b3ae8752adc5 Kent Gibson 2020-06-23 957 /* Make sure this is terminated */
> f3b3ae8752adc5 Kent Gibson 2020-06-23 958 linereq.consumer[sizeof(linereq.consumer)-1] = '\0';
> f3b3ae8752adc5 Kent Gibson 2020-06-23 959 if (strlen(linereq.consumer)) {
> f3b3ae8752adc5 Kent Gibson 2020-06-23 960 line->label = kstrdup(linereq.consumer, GFP_KERNEL);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 961 if (!line->label) {
> f3b3ae8752adc5 Kent Gibson 2020-06-23 962 ret = -ENOMEM;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 963 goto out_free_line;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 964 }
> f3b3ae8752adc5 Kent Gibson 2020-06-23 965 }
> f3b3ae8752adc5 Kent Gibson 2020-06-23 966
> f3b3ae8752adc5 Kent Gibson 2020-06-23 967 mutex_init(&line->config_mutex);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 968 init_waitqueue_head(&line->wait);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 969 if (lc->edge_detection) {
> f3b3ae8752adc5 Kent Gibson 2020-06-23 970 size = linereq.event_buffer_size;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 971
> f3b3ae8752adc5 Kent Gibson 2020-06-23 972 if (size > GPIOLINES_MAX*16)
> f3b3ae8752adc5 Kent Gibson 2020-06-23 973 size = GPIOLINES_MAX*16;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 974 else if (size == 0)
> f3b3ae8752adc5 Kent Gibson 2020-06-23 975 size = linereq.num_lines*16;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 976
> f3b3ae8752adc5 Kent Gibson 2020-06-23 977 ret = kfifo_alloc(&line->events, size, GFP_KERNEL);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 978 if (ret)
> f3b3ae8752adc5 Kent Gibson 2020-06-23 979 goto out_free_line;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 980
> f3b3ae8752adc5 Kent Gibson 2020-06-23 981 line->edge_detection = lc->edge_detection;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 982 }
> f3b3ae8752adc5 Kent Gibson 2020-06-23 983
> f3b3ae8752adc5 Kent Gibson 2020-06-23 984 atomic_set(&line->seqno, 0);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 985 line->num_descs = linereq.num_lines;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 986 vals = (unsigned long *)lc->values.bitmap;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 987
> f3b3ae8752adc5 Kent Gibson 2020-06-23 988 /* Request each GPIO */
> f3b3ae8752adc5 Kent Gibson 2020-06-23 989 for (i = 0; i < linereq.num_lines; i++) {
> f3b3ae8752adc5 Kent Gibson 2020-06-23 990 u32 offset = linereq.offsets[i];
> f3b3ae8752adc5 Kent Gibson 2020-06-23 991 struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 992
> f3b3ae8752adc5 Kent Gibson 2020-06-23 993 if (IS_ERR(desc)) {
> f3b3ae8752adc5 Kent Gibson 2020-06-23 994 ret = PTR_ERR(desc);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 995 goto out_free_line;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 996 }
> f3b3ae8752adc5 Kent Gibson 2020-06-23 997
> f3b3ae8752adc5 Kent Gibson 2020-06-23 998 ret = gpiod_request(desc, line->label);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 999 if (ret)
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1000 goto out_free_line;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1001
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1002 line->descs[i] = desc;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1003 gpioline_config_to_desc_flags(lc, &desc->flags);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1004
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1005 ret = gpiod_set_transitory(desc, false);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1006 if (ret < 0)
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1007 goto out_free_line;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1008
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1009 /*
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1010 * Lines have to be requested explicitly for input
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1011 * or output, else the line will be treated "as is".
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1012 */
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1013 if (lc->flags & GPIOLINE_FLAG_V2_DIRECTION) {
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1014 if (lc->direction == GPIOLINE_DIRECTION_OUTPUT) {
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1015 int val = test_bit(i, vals);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1016
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1017 ret = gpiod_direction_output(desc, val);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1018 if (ret)
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1019 goto out_free_line;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1020 } else {
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1021 ret = gpiod_direction_input(desc);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1022 if (ret)
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1023 goto out_free_line;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1024 ret = edge_detector_setup(&line->edets[i], lc);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1025 if (ret)
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1026 goto out_free_line;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1027 }
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1028 }
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1029
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1030 atomic_notifier_call_chain(&desc->gdev->notifier,
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1031 GPIOLINE_CHANGED_REQUESTED, desc);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1032
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1033 dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1034 offset);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1035 }
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1036
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1037 fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1038 if (fd < 0) {
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1039 ret = fd;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1040 goto out_free_line;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1041 }
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1042
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1043 file = anon_inode_getfile("gpio-line",
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1044 &line_fileops,
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1045 line,
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1046 O_RDONLY | O_CLOEXEC);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1047 if (IS_ERR(file)) {
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1048 ret = PTR_ERR(file);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1049 goto out_put_unused_fd;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1050 }
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1051
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1052 linereq.fd = fd;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1053 if (copy_to_user(ip, &linereq, sizeof(linereq))) {
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1054 /*
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1055 * fput() will trigger the release() callback, so do not go onto
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1056 * the regular error cleanup path here.
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1057 */
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1058 fput(file);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1059 put_unused_fd(fd);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1060 return -EFAULT;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1061 }
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1062
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1063 fd_install(fd, file);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1064
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1065 dev_dbg(&gdev->dev, "registered chardev handle for %d lines\n",
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1066 line->num_descs);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1067
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1068 return 0;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1069
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1070 out_put_unused_fd:
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1071 put_unused_fd(fd);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1072 out_free_line:
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1073 line_free(line);
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1074 return ret;
> f3b3ae8752adc5 Kent Gibson 2020-06-23 1075 }
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/[email protected]
wt., 23 cze 2020 o 06:01 Kent Gibson <[email protected]> napisał(a):
>
> Move gpiolib-sysfs function declarations into their own header.
>
> These functions are in gpiolib-sysfs.c, and are only required by gpiolib.c,
> and so should be in a module header, not giolib.h.
>
> This brings gpiolib-sysfs into line with gpiolib-cdev, and is another step
> towards removing the sysfs inferface.
>
> Signed-off-by: Kent Gibson <[email protected]>
>
Reviewed-by: Bartosz Golaszewski <[email protected]>
wt., 23 cze 2020 o 06:01 Kent Gibson <[email protected]> napisał(a):
>
> Sort the includes of gpiolib-cdev.c to make it easier to identify if a
> module is included and to avoid duplication.
>
> Signed-off-by: Kent Gibson <[email protected]>
>
Reviewed-by: Bartosz Golaszewski <[email protected]>
wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
>
> Make indentation consistent with other use to improve readability.
>
> Signed-off-by: Kent Gibson <[email protected]>
>
Reviewed-by: Bartosz Golaszewski <[email protected]>
On Wed, Jun 24, 2020 at 03:34:11PM +0200, Bartosz Golaszewski wrote:
> wt., 23 cze 2020 o 06:01 Kent Gibson <[email protected]> napisał(a):
> >
> > Move gpiolib-sysfs function declarations into their own header.
> >
> > These functions are in gpiolib-sysfs.c, and are only required by gpiolib.c,
> > and so should be in a module header, not giolib.h.
> >
Of course now I see the typo - it should be gpiolib.h
> > This brings gpiolib-sysfs into line with gpiolib-cdev, and is another step
> > towards removing the sysfs inferface.
> >
> > Signed-off-by: Kent Gibson <[email protected]>
> >
>
> Reviewed-by: Bartosz Golaszewski <[email protected]>
wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
>
> Refactor the mapping from handle flags to desc flags into a helper
> function.
>
> The assign_bit is overkill where it is replacing the set_bit cases, as is
> rechecking bits known to be clear in some circumstances, but the DRY
> simplification more than makes up for any performance degradation,
> especially as this is not a hot path.
>
> Signed-off-by: Kent Gibson <[email protected]>
>
Agreed and I like the code shrink.
Reviewed-by: Bartosz Golaszewski <[email protected]>
wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
>
> Rename 'filep' and 'filp' to 'file' to be consistent with other use
> and improve readability.
>
> Signed-off-by: Kent Gibson <[email protected]>
>
Reviewed-by: Bartosz Golaszewski <[email protected]>
wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
>
> Rename numdescs to num_descs to be more consistent with the naming of
> other counters and improve readability.
>
> Signed-off-by: Kent Gibson <[email protected]>
>
Reviewed-by: Bartosz Golaszewski <[email protected]>
wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
>
> Remove pointless decrement of variable, and associated comment.
>
> While i is used subsequently, it is re-initialized so this decrement
> serves no purpose.
>
> Signed-off-by: Kent Gibson <[email protected]>
>
Reviewed-by: Bartosz Golaszewski <[email protected]>
wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
>
> Reset the timestamp field to 0 after using it in lineevent_irq_thread.
>
> The timestamp is set by lineevent_irq_handler and is tested by
> lineevent_irq_thread to determine if it is called from a nested theaded
> interrupt.
> lineevent_irq_thread is assuming that the nested, or otherwise, status
> of the IRQ is static, i.e. it is either always nested or never nested.
> This change removes that assumption, resetting the timestamp so it can
> be re-used to determine the nested state of subsequent interrupts.
>
> Signed-off-by: Kent Gibson <[email protected]>
>
This change makes sense to me but I'm having a hard time processing
the explanation. If we're requesting the interrupt and allocating the
lineevent state in the same function - how can we run into a situation
here the status of the irq would change like what you describe?
Bart
wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
>
> Rename priv to gcdev to improve readability.
>
> The name "priv" indicates that the object is pointed to by
> file->private_data, not what the object is actually is.
> It is always used to point to a struct gpio_chardev_data so renaming
> it to gcdev seemed as good as anything, and certainly clearer than "priv".
>
> Signed-off-by: Kent Gibson <[email protected]>
>
Ugh now it's gcdev and gdev everywhere and it doesn't really make it
more readable. Maybe chardev_data or cdev_data?
Bart
wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
>
> Merge separate usage of test_bit/set_bit into test_and_set_bit to remove
> the possibility of a race between the test and set.
>
> Similarly test_bit and clear_bit.
>
> In the existing code it is possible for two threads to race past the
> test_bit and then set or clear the watch bit, and neither return EBUSY.
>
> Signed-off-by: Kent Gibson <[email protected]>
>
Reviewed-by: Bartosz Golaszewski <[email protected]>
On Wed, Jun 24, 2020 at 04:00:42PM +0200, Bartosz Golaszewski wrote:
> wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
> >
> > Reset the timestamp field to 0 after using it in lineevent_irq_thread.
> >
> > The timestamp is set by lineevent_irq_handler and is tested by
> > lineevent_irq_thread to determine if it is called from a nested theaded
> > interrupt.
> > lineevent_irq_thread is assuming that the nested, or otherwise, status
> > of the IRQ is static, i.e. it is either always nested or never nested.
> > This change removes that assumption, resetting the timestamp so it can
> > be re-used to determine the nested state of subsequent interrupts.
> >
> > Signed-off-by: Kent Gibson <[email protected]>
> >
>
> This change makes sense to me but I'm having a hard time processing
> the explanation. If we're requesting the interrupt and allocating the
> lineevent state in the same function - how can we run into a situation
> here the status of the irq would change like what you describe?
>
I'm not totally sure myself, as my understanding of how interrupts are
shared in the kernel is pretty sketchy, but my concern is that if we
are sharing the irq then whoever we are sharing with may release the irq
and we go from nested to unnested. Or vice versa. Not sure if that is
valid, but that was my concern, and it seemed like a minor change to
cover it just in case.
Cheers,
Kent.
wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
>
> Replace constant array sizes with a macro constant to clarify the source
> of array sizes, provide a place to document any constraints on the size,
> and to simplify array sizing in userspace if constructing structs
> from their composite fields.
>
> Signed-off-by: Kent Gibson <[email protected]>
>
Reviewed-by: Bartosz Golaszewski <[email protected]>
On Wed, Jun 24, 2020 at 04:04:09PM +0200, Bartosz Golaszewski wrote:
> wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
> >
> > Rename priv to gcdev to improve readability.
> >
> > The name "priv" indicates that the object is pointed to by
> > file->private_data, not what the object is actually is.
> > It is always used to point to a struct gpio_chardev_data so renaming
> > it to gcdev seemed as good as anything, and certainly clearer than "priv".
> >
> > Signed-off-by: Kent Gibson <[email protected]>
> >
>
> Ugh now it's gcdev and gdev everywhere and it doesn't really make it
> more readable. Maybe chardev_data or cdev_data?
>
Agreed, it isn't ideal visually, but is at least more unique than priv.
Linus was going for short names recently (e.g. gc for gpiochip), so I was
going for something short.
And I try avoid names ending in _data or _state or similar where they
don't really add anything.
Would chardev or gchardev work for you?
Cheers,
Kent.
śr., 24 cze 2020 o 16:19 Kent Gibson <[email protected]> napisał(a):
>
> On Wed, Jun 24, 2020 at 04:04:09PM +0200, Bartosz Golaszewski wrote:
> > wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
> > >
> > > Rename priv to gcdev to improve readability.
> > >
> > > The name "priv" indicates that the object is pointed to by
> > > file->private_data, not what the object is actually is.
> > > It is always used to point to a struct gpio_chardev_data so renaming
> > > it to gcdev seemed as good as anything, and certainly clearer than "priv".
> > >
> > > Signed-off-by: Kent Gibson <[email protected]>
> > >
> >
> > Ugh now it's gcdev and gdev everywhere and it doesn't really make it
> > more readable. Maybe chardev_data or cdev_data?
> >
>
> Agreed, it isn't ideal visually, but is at least more unique than priv.
> Linus was going for short names recently (e.g. gc for gpiochip), so I was
> going for something short.
>
> And I try avoid names ending in _data or _state or similar where they
> don't really add anything.
>
> Would chardev or gchardev work for you?
>
Yes, chardev is fine. Even cdev is fine for me: gdev vs gcdev is
confusing but gdev vs cdev looks better IMO.
Bart
On Tue, Jun 23, 2020 at 7:04 AM Kent Gibson <[email protected]> wrote:
>
> Add a new version of the uAPI to address existing 32/64bit alignment
I think using - would be nice, like 32/64-bit (or at least space like
32/64 bit) as a common practice.
> issues, add support for debounce and event sequence numbers, and provide
> some future proofing by adding padding reserved for future use.
>
> The alignment issue relates to the gpioevent_data, which packs to different
> sizes on 32bit and 64bit platforms. That creates problems for 32bit apps
> running on 64bit kernels.
Dashes, please.
$ git grep -n -w '32 bit' | wc
1904
$ git grep -n -w '64 bit' | wc
1155
~3k
$ git grep -n -w '32bit' | wc
1405
$ git grep -n -w '64bit' | wc
870
~2.2k
$ git grep -n -w '64-bit' | wc
3196
$ git grep -n -w '32-bit' | wc
4369
~7.5k
> The patch addresses that particular issue, and
> the problem more generally, by adding pad fields that explicitly pad
> structs out to 64bit boundaries, so they will pack to the same size now,
> and even if some of the reserved padding is used for __u64 fields in the
> future.
>
> The lack of future proofing in V1 makes it impossible to, for example,
> add the debounce feature that is included in v2.
> The future proofing is addressed by providing reserved padding in all
> structs for future features. Specifically, the line request,
> config, info, info_changed and event structs receive updated versions,
> and the first three new ioctls.
...
> Firstly, I've reworked the flags field throughout. V1 has three different
> flags fields, each with their own separate bit definitions. In V2 that is
> collapsed to one. Further, the bits of the V2 flags field are used
> as feature enable flags, with any other necessary configuration fields encoded
> separately. This is simpler and clearer, while also providing a foundation
> for adding features in the future.
>
> I've also merged the handle and event requests into a single request, the
> line request, as the two requests where mostly the same, other than the
where-> were ? (yes I noticed it's not a part of commit message)
> edge detection provided by event requests. As a byproduct, the V2 uAPI
> allows for multiple lines producing edge events on the same line handle.
> This is a new capability as V1 only supports a single line in an event
> request.
>
> This means there are now only two types of file handle to be concerned with,
> the chip and the line, and it is clearer which ioctls apply to which type
> of handle.
>
> There is also some minor renaming of fields for consistency compared to their
> V1 counterparts, e.g. offset rather than lineoffset or line_offset, and
> consumer rather than consumer_label.
>
> Additionally, V1 GPIOHANDLES_MAX becomes GPIOLINES_MAX in V2 for clarity,
> and the gpiohandle_data __u8 array becomes a bitmap gpioline_values.
>
> The V2 uAPI is mostly just a reorganisation of V1, so userspace code,
> particularly libgpiod, should easily port to it.
>
> The padding sizes have been carried over from the RFC version, although the
> seqnos added to the gpioline_event alone would've used the all of the padding
> for that struct, had they not been added here. So I'm moderatly concerned
> that those values are too small due to a lack of imagination on may part and
> should be increased to decrease the probability of running out of space in
> the padding and requiring creative solutions or even a V3.
...
> +#include <linux/kernel.h>
Perhaps keep it in order?
...
> + * Must be a multiple of 8 to ensure 32/64bit alignment of structs.
Dash. And same for all cases like this.
...
> +/* the number of __u64 required for a bitmap for GPIOLINES_MAX lines */
the -> The ?
> +#define GPIOLINES_BITMAP_SIZE __KERNEL_DIV_ROUND_UP(GPIOLINES_MAX, 64)
Not sure we need this definition. The problem is that definitions in
the uAPI header are also part of uAPI. Here is just a calculus which
can be done manually since if anybody changes MAX, they will anyway
have to check if everything else is consistent. And yes, I'm not in
favour of including kernel.h here and there.
...
> +/*
> + * Struct padding sizes.
> + *
> + * These are sized to pad structs to 64bit boundaries.
> + * To maintain 32/64bit alignment, any arbitrary change must be even, as
> + * the pad elements are __u32.
> + */
> +#define GPIOLINE_CONFIG_PAD_SIZE 7
> +#define GPIOLINE_REQUEST_PAD_SIZE 5
> +#define GPIOLINE_INFO_V2_PAD_SIZE 5
> +#define GPIOLINE_INFO_CHANGED_V2_PAD_SIZE 5
> +#define GPIOLINE_EVENT_PAD_SIZE 2
I'm not sure they (definitions) should be part of UAPI. Can't you
simple hard code numbers per case?
...
> +/**
> + * struct gpioline_config - Configuration for GPIO lines
> + */
> +struct gpioline_config {
> + struct gpioline_values values;
> + __u32 flags;
> + /* Note that the following four fields are equivalent to a single u32. */
> + __u8 direction;
> + __u8 drive;
> + __u8 bias;
> + __u8 edge_detection;
> + __u32 debounce_period;
> + __u32 padding[GPIOLINE_CONFIG_PAD_SIZE]; /* for future use */
I would just put minimum here. If you need to extend you have to use
sizeof(struct) as a version of ABI.
> +};
I'm wondering how many lines (in average) the user usually changes at
once? One? Two?
Perhaps we need to be better with this, something like single line /
multiple lines?
So, having a struct for single line change being embedded multiple
times would allow to configure atomically several lines with different
requirements.
For example you can turn directions of the two lines for some kind of
half-duplex bit banging protocol.
I'm not sure about the rest, but to me it seems reasonable to have
single vs. multiple separation in some of the structures.
...
> +/*
> + * ABI V1
V1 -> v1
And below V2 -> v2.
> + */
--
With Best Regards,
Andy Shevchenko
wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
>
> Add a new version of the uAPI to address existing 32/64bit alignment
> issues, add support for debounce and event sequence numbers, and provide
> some future proofing by adding padding reserved for future use.
>
> The alignment issue relates to the gpioevent_data, which packs to different
> sizes on 32bit and 64bit platforms. That creates problems for 32bit apps
> running on 64bit kernels. The patch addresses that particular issue, and
> the problem more generally, by adding pad fields that explicitly pad
> structs out to 64bit boundaries, so they will pack to the same size now,
> and even if some of the reserved padding is used for __u64 fields in the
> future.
>
> The lack of future proofing in V1 makes it impossible to, for example,
> add the debounce feature that is included in v2.
> The future proofing is addressed by providing reserved padding in all
> structs for future features. Specifically, the line request,
> config, info, info_changed and event structs receive updated versions,
> and the first three new ioctls.
>
> Signed-off-by: Kent Gibson <[email protected]>
>
> ---
>
> I haven't added any padding to gpiochip_info, as I haven't seen any calls
> for new features for the corresponding ioctl, but I'm open to updating that
> as well.
>
> As the majority of the structs and ioctls were being replaced, it seemed
> opportune to rework some of the other aspects of the uAPI.
>
> Firstly, I've reworked the flags field throughout. V1 has three different
> flags fields, each with their own separate bit definitions. In V2 that is
> collapsed to one. Further, the bits of the V2 flags field are used
> as feature enable flags, with any other necessary configuration fields encoded
> separately. This is simpler and clearer, while also providing a foundation
> for adding features in the future.
>
> I've also merged the handle and event requests into a single request, the
> line request, as the two requests where mostly the same, other than the
> edge detection provided by event requests. As a byproduct, the V2 uAPI
> allows for multiple lines producing edge events on the same line handle.
> This is a new capability as V1 only supports a single line in an event
> request.
>
> This means there are now only two types of file handle to be concerned with,
> the chip and the line, and it is clearer which ioctls apply to which type
> of handle.
>
> There is also some minor renaming of fields for consistency compared to their
> V1 counterparts, e.g. offset rather than lineoffset or line_offset, and
> consumer rather than consumer_label.
>
> Additionally, V1 GPIOHANDLES_MAX becomes GPIOLINES_MAX in V2 for clarity,
> and the gpiohandle_data __u8 array becomes a bitmap gpioline_values.
>
> The V2 uAPI is mostly just a reorganisation of V1, so userspace code,
> particularly libgpiod, should easily port to it.
>
> The padding sizes have been carried over from the RFC version, although the
> seqnos added to the gpioline_event alone would've used the all of the padding
> for that struct, had they not been added here. So I'm moderatly concerned
> that those values are too small due to a lack of imagination on may part and
> should be increased to decrease the probability of running out of space in
> the padding and requiring creative solutions or even a V3.
>
> Changes since the RFC:
> - document the constraints on array sizes to maintain 32/64 alignment
> - add sequence numbers to gpioline_event
> - use bitmap for values instead of array of __u8
> - gpioline_info_v2 contains gpioline_config instead of its composite fields
> - provide constants for all array sizes, especially padding
> - renamed "GPIOLINE_FLAG_V2_KERNEL" to "GPIOLINE_FLAG_V2_USED"
> - renamed "default_values" to "values"
> - made gpioline_direction zero based
> - document clock used in gpioline_event timestamp
> - add event_buffer_size to gpioline_request
>
>
> include/uapi/linux/gpio.h | 237 ++++++++++++++++++++++++++++++++++++--
> 1 file changed, 230 insertions(+), 7 deletions(-)
>
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 4e1139ab25bc..e4ed6f79e332 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -11,11 +11,14 @@
> #ifndef _UAPI_GPIO_H_
> #define _UAPI_GPIO_H_
>
> +#include <linux/kernel.h>
> #include <linux/ioctl.h>
> #include <linux/types.h>
>
> /*
> * The maximum size of name and label arrays.
> + *
> + * Must be a multiple of 8 to ensure 32/64bit alignment of structs.
> */
> #define GPIO_MAX_NAME_SIZE 32
>
> @@ -32,6 +35,211 @@ struct gpiochip_info {
> __u32 lines;
> };
>
> +/*
> + * Maximum number of requested lines.
> + *
> + * Must be a multiple of 8 to ensure 32/64bit alignment of structs.
> + */
> +#define GPIOLINES_MAX 64
> +
> +/* the number of __u64 required for a bitmap for GPIOLINES_MAX lines */
> +#define GPIOLINES_BITMAP_SIZE __KERNEL_DIV_ROUND_UP(GPIOLINES_MAX, 64)
> +
> +enum gpioline_direction {
> + GPIOLINE_DIRECTION_INPUT = 0,
> + GPIOLINE_DIRECTION_OUTPUT = 1,
> +};
> +
> +enum gpioline_drive {
> + GPIOLINE_DRIVE_PUSH_PULL = 0,
> + GPIOLINE_DRIVE_OPEN_DRAIN = 1,
> + GPIOLINE_DRIVE_OPEN_SOURCE = 2,
> +};
> +
> +enum gpioline_bias {
> + GPIOLINE_BIAS_DISABLED = 0,
> + GPIOLINE_BIAS_PULL_UP = 1,
> + GPIOLINE_BIAS_PULL_DOWN = 2,
> +};
> +
> +enum gpioline_edge {
> + GPIOLINE_EDGE_NONE = 0,
> + GPIOLINE_EDGE_RISING = 1,
> + GPIOLINE_EDGE_FALLING = 2,
> + GPIOLINE_EDGE_BOTH = 3,
> +};
Nit: I'd just add a comment before these enums saying that values of 0
represent sensible default settings.
> +
> +/* Line flags - V2 */
> +#define GPIOLINE_FLAG_V2_USED (1UL << 0) /* line is not available for request */
> +#define GPIOLINE_FLAG_V2_ACTIVE_LOW (1UL << 1)
> +#define GPIOLINE_FLAG_V2_DIRECTION (1UL << 2)
> +#define GPIOLINE_FLAG_V2_DRIVE (1UL << 3)
> +#define GPIOLINE_FLAG_V2_BIAS (1UL << 4)
> +#define GPIOLINE_FLAG_V2_EDGE_DETECTION (1UL << 5)
> +#define GPIOLINE_FLAG_V2_DEBOUNCE (1UL << 6)
> +
> +/*
> + * Struct padding sizes.
> + *
> + * These are sized to pad structs to 64bit boundaries.
> + * To maintain 32/64bit alignment, any arbitrary change must be even, as
> + * the pad elements are __u32.
> + */
> +#define GPIOLINE_CONFIG_PAD_SIZE 7
> +#define GPIOLINE_REQUEST_PAD_SIZE 5
> +#define GPIOLINE_INFO_V2_PAD_SIZE 5
> +#define GPIOLINE_INFO_CHANGED_V2_PAD_SIZE 5
> +#define GPIOLINE_EVENT_PAD_SIZE 2
> +
Did someone request such definitions? IMO it's one of those rare
instances where I'd prefer to see magic numbers used in the structs.
I'm not sure if we need this indirection.
> +/**
> + * struct gpioline_values - Values of GPIO lines
> + * @bitmap: a bitmap containing the value of the lines, set to 1 for active
> + * and 0 for inactive. Note that this is the logical value, which will be
> + * the opposite of the physical value if GPIOLINE_FLAG_V2_ACTIVE_LOW is
> + * set in flags.
> + */
> +struct gpioline_values {
> + __u64 bitmap[GPIOLINES_BITMAP_SIZE];
> +};
Ok so now when embedding this structure we get something like
"config.values.bitmap". This looks fine with the exception that "bits"
would be even shorter and the information about this field being a
bitmap is not necessary. I hope this isn't too much bikeshedding. :)
> +
> +/**
> + * struct gpioline_config - Configuration for GPIO lines
> + * @values: if the direction is GPIOLINE_DIRECTION_OUTPUT, the values that
> + * the lines will be set to. This field is write-only and is zeroed when
> + * returned within struct gpioline_info.
> + * @flags: flags for the GPIO lines, such as GPIOLINE_FLAG_V2_ACTIVE_LOW,
> + * GPIOLINE_FLAG_V2_DIRECTION etc, OR:ed together
> + * @direction: if GPIOLINE_FLAG_V2_DIRECTION is set in flags, the desired
> + * direction for the requested lines, with a value from enum
> + * gpioline_direction
> + * @drive: if GPIOLINE_FLAG_V2_DRIVE is set in flags, the desired drive for
> + * the requested lines, with a value from enum gpioline_drive
> + * @bias: if GPIOLINE_FLAG_V2_BIAS is set in flags, the desired bias for
> + * the requested lines, with a value from enum gpioline_bias
> + * @edge_detection: if GPIOLINE_FLAG_V2_EDGE_DETECTION is set in flags, the
> + * desired edge_detection for the requested lines, with a value from enum
> + * gpioline_edge
> + * @debounce_period: if GPIOLINE_FLAG_V2_DEBOUNCE is set in flags, the
> + * desired debounce period for the requested lines, in microseconds
> + * @padding: reserved for future use and must be zero filled
> + */
> +struct gpioline_config {
> + struct gpioline_values values;
> + __u32 flags;
> + /* Note that the following four fields are equivalent to a single u32. */
> + __u8 direction;
> + __u8 drive;
> + __u8 bias;
> + __u8 edge_detection;
Limiting the size of these enum fields doesn't improve performance in
any measurable way. We already have 4 possible values for events. I'm
afraid that at some point in the future we'll add support for level
events or something else etc. and we'll run into a problem because we
only have 8 bits available. If there are no objections, I'd make it 32
bits wide.
For the same reason I was thinking that making flags 64 bits wouldn't
hurt either.
> + __u32 debounce_period;
> + __u32 padding[GPIOLINE_CONFIG_PAD_SIZE]; /* for future use */
> +};
> +
> +/**
> + * struct gpioline_request - Information about a request for GPIO lines
> + * @offsets: an array of desired lines, specified by offset index for the
> + * associated GPIO device
> + * @consumer: a desired consumer label for the selected GPIO lines such as
> + * "my-bitbanged-relay"
> + * @config: requested configuration for the requested lines. Note that even
> + * if multiple lines are requested, the same configuration must be
> + * applicable to all of them. If you want lines with individual
> + * configuration, request them one by one. It is possible to select a batch
> + * of input or output lines, but they must all have the same configuration,
> + * i.e. all inputs or all outputs, all active low etc
> + * @num_lines: number of lines requested in this request, i.e. the number
> + * of valid fields in the GPIOLINES_MAX sized arrays, set to 1 to request a
> + * single line
> + * @event_buffer_size: a suggested minimum number of line events that the
> + * kernel should buffer. This is only relevant if edge_detection is
> + * enabled. Note that this is only a suggested value and the kernel may
> + * allocate a larger buffer or cap the size of the buffer. If this field is
> + * zero then the buffer size defaults to a minimum of num_lines*16.
> + * @padding: reserved for future use and must be zero filled
> + * @fd: if successful this field will contain a valid anonymous file handle
> + * after a GPIO_GET_LINE_IOCTL operation, zero or negative value means
> + * error
> + */
> +struct gpioline_request {
> + __u32 offsets[GPIOLINES_MAX];
> + char consumer[GPIO_MAX_NAME_SIZE];
> + struct gpioline_config config;
> + __u32 num_lines;
> + __u32 event_buffer_size;
> + __u32 padding[GPIOLINE_REQUEST_PAD_SIZE]; /* for future use */
> + __s32 fd;
> +};
> +
> +/**
> + * struct gpioline_info_v2 - Information about a certain GPIO line
> + * @name: the name of this GPIO line, such as the output pin of the line on
> + * the chip, a rail or a pin header name on a board, as specified by the
> + * gpio chip, may be empty
> + * @consumer: a functional name for the consumer of this GPIO line as set
> + * by whatever is using it, will be empty if there is no current user but
> + * may also be empty if the consumer doesn't set this up
> + * @config: the configuration of the line. Note that the values field is
> + * always zeroed - the line must be requested to read the values.
> + * @offset: the local offset on this GPIO device, fill this in when
> + * requesting the line information from the kernel
> + * @padding: reserved for future use
> + */
> +struct gpioline_info_v2 {
> + char name[GPIO_MAX_NAME_SIZE];
> + char consumer[GPIO_MAX_NAME_SIZE];
> + struct gpioline_config config;
> + __u32 offset;
> + __u32 padding[GPIOLINE_INFO_V2_PAD_SIZE]; /* for future use */
> +};
> +
> +/**
> + * struct gpioline_info_changed_v2 - Information about a change in status
> + * of a GPIO line
> + * @info: updated line information
> + * @timestamp: estimate of time of status change occurrence, in nanoseconds
> + * and GPIOLINE_CHANGED_CONFIG
> + * @event_type: one of GPIOLINE_CHANGED_REQUESTED, GPIOLINE_CHANGED_RELEASED
> + * @padding: reserved for future use
> + */
> +struct gpioline_info_changed_v2 {
> + struct gpioline_info_v2 info;
> + __u64 timestamp;
> + __u32 event_type;
> + __u32 padding[GPIOLINE_INFO_CHANGED_V2_PAD_SIZE]; /* for future use */
> +};
> +
> +enum gpioline_event_id {
> + GPIOLINE_EVENT_RISING_EDGE = 1,
> + GPIOLINE_EVENT_FALLING_EDGE = 2,
> +};
> +
> +/**
> + * struct gpioline_event - The actual event being pushed to userspace
> + * @timestamp: best estimate of time of event occurrence, in nanoseconds.
> + * The timestamp is read from CLOCK_MONOTONIC and is intended to allow the
> + * accurate measurement of the time between events. It does not provide
> + * the wall-clock time.
> + * @id: event identifier with value from enum gpioline_event_id
> + * @offset: the offset of the line that triggered the event
> + * @seqno: the sequence number for this event in the sequence of events for
> + * all the lines in this line request
> + * @line_seqno: the sequence number for this event in the sequence of
> + * events on this particular line
> + * @padding: reserved for future use
> + */
> +struct gpioline_event {
> + __u64 timestamp;
> + __u32 id;
> + __u32 offset;
> + __u32 seqno;
> + __u32 line_seqno;
> + __u32 padding[GPIOLINE_EVENT_PAD_SIZE]; /* for future use */
Any reason for only having 64 bits of padding? 128 wouldn't change much, right?
> +};
> +
> +/*
> + * ABI V1
> + */
> +
> /* Informational flags */
> #define GPIOLINE_FLAG_KERNEL (1UL << 0) /* Line used by the kernel */
> #define GPIOLINE_FLAG_IS_OUT (1UL << 1)
> @@ -149,8 +357,6 @@ struct gpiohandle_config {
> __u32 padding[4]; /* padding for future use */
> };
>
> -#define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0a, struct gpiohandle_config)
> -
> /**
> * struct gpiohandle_data - Information of values on a GPIO handle
> * @values: when getting the state of lines this contains the current
> @@ -161,9 +367,6 @@ struct gpiohandle_data {
> __u8 values[GPIOHANDLES_MAX];
> };
>
> -#define GPIOHANDLE_GET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x08, struct gpiohandle_data)
> -#define GPIOHANDLE_SET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x09, struct gpiohandle_data)
> -
> /* Eventrequest flags */
> #define GPIOEVENT_REQUEST_RISING_EDGE (1UL << 0)
> #define GPIOEVENT_REQUEST_FALLING_EDGE (1UL << 1)
> @@ -207,11 +410,31 @@ struct gpioevent_data {
> __u32 id;
> };
>
> +/*
> + * V1 and V2 ioctl()s
> + */
> #define GPIO_GET_CHIPINFO_IOCTL _IOR(0xB4, 0x01, struct gpiochip_info)
> +#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0C, __u32)
> +
> +/*
> + * V2 ioctl()s
> + */
> +#define GPIO_GET_LINEINFO_V2_IOCTL _IOWR(0xB4, 0x05, struct gpioline_info_v2)
> +#define GPIO_GET_LINEINFO_WATCH_V2_IOCTL _IOWR(0xB4, 0x06, struct gpioline_info_v2)
> +#define GPIO_GET_LINE_IOCTL _IOWR(0xB4, 0x07, struct gpioline_request)
> +#define GPIOLINE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0D, struct gpioline_config)
> +#define GPIOLINE_GET_VALUES_IOCTL _IOWR(0xB4, 0x0E, struct gpioline_values)
> +#define GPIOLINE_SET_VALUES_IOCTL _IOWR(0xB4, 0x0F, struct gpioline_values)
> +
> +/*
> + * V1 ioctl()s
> + */
> #define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
> -#define GPIO_GET_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0b, struct gpioline_info)
> -#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0c, __u32)
> #define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
> #define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request)
> +#define GPIOHANDLE_GET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x08, struct gpiohandle_data)
> +#define GPIOHANDLE_SET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x09, struct gpiohandle_data)
> +#define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0A, struct gpiohandle_config)
> +#define GPIO_GET_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0B, struct gpioline_info)
>
> #endif /* _UAPI_GPIO_H_ */
> --
> 2.27.0
>
Looks nice! Thanks again Kent!
Bart
On Tue, Jun 23, 2020 at 7:03 AM Kent Gibson <[email protected]> wrote:
>
> Merge separate usage of test_bit/set_bit into test_and_set_bit to remove
> the possibility of a race between the test and set.
>
> Similarly test_bit and clear_bit.
>
> In the existing code it is possible for two threads to race past the
> test_bit and then set or clear the watch bit, and neither return EBUSY.
I stumbled over this myself, but...
> - if (test_bit(hwgpio, gcdev->watched_lines))
> + if (test_and_set_bit(hwgpio, gcdev->watched_lines))
> return -EBUSY;
>
> gpio_desc_to_lineinfo(desc, &lineinfo);
> @@ -897,7 +897,6 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
> return -EFAULT;
>
> - set_bit(hwgpio, gcdev->watched_lines);
> return 0;
...I think it's not an equivalent despite races involved. If you set
bit and return error code, you will have the wrong state.
...
> - if (!test_bit(hwgpio, gcdev->watched_lines))
> + if (!test_and_clear_bit(hwgpio, gcdev->watched_lines))
> return -EBUSY;
>
> - clear_bit(hwgpio, gcdev->watched_lines);
> return 0;
OTOH, this is okay as long as we have no code in between. So, I really
prefer something better to do such checks.
(Alas, I can't come up with a proposal right now)
--
With Best Regards,
Andy Shevchenko
On Wed, Jun 24, 2020 at 05:33:26PM +0300, Andy Shevchenko wrote:
> On Tue, Jun 23, 2020 at 7:04 AM Kent Gibson <[email protected]> wrote:
> >
> > Add a new version of the uAPI to address existing 32/64bit alignment
>
> I think using - would be nice, like 32/64-bit (or at least space like
> 32/64 bit) as a common practice.
>
Fair enough. And you don't need to point out every single case - a
catch all is sufficient.
[ snip ]
>
> > +#include <linux/kernel.h>
>
> Perhaps keep it in order?
>
Haha, I didn't notice that one.
> ...
>
> > + * Must be a multiple of 8 to ensure 32/64bit alignment of structs.
>
> Dash. And same for all cases like this.
>
And you are repeating yourself. Again ;-).
> ...
>
> > +/* the number of __u64 required for a bitmap for GPIOLINES_MAX lines */
>
> the -> The ?
>
> > +#define GPIOLINES_BITMAP_SIZE __KERNEL_DIV_ROUND_UP(GPIOLINES_MAX, 64)
>
> Not sure we need this definition. The problem is that definitions in
> the uAPI header are also part of uAPI. Here is just a calculus which
> can be done manually since if anybody changes MAX, they will anyway
> have to check if everything else is consistent. And yes, I'm not in
> favour of including kernel.h here and there.
>
We are talking about include/uapi/linux/kernel.h, right?
I thought headers in uapi/linux were fair game for other uapi headers.
That is what they are there for right?
Similarly, I thought those macros were there to avoid having the recalc
manually.
> ...
>
> > +/*
> > + * Struct padding sizes.
> > + *
> > + * These are sized to pad structs to 64bit boundaries.
> > + * To maintain 32/64bit alignment, any arbitrary change must be even, as
> > + * the pad elements are __u32.
> > + */
> > +#define GPIOLINE_CONFIG_PAD_SIZE 7
> > +#define GPIOLINE_REQUEST_PAD_SIZE 5
> > +#define GPIOLINE_INFO_V2_PAD_SIZE 5
> > +#define GPIOLINE_INFO_CHANGED_V2_PAD_SIZE 5
> > +#define GPIOLINE_EVENT_PAD_SIZE 2
>
> I'm not sure they (definitions) should be part of UAPI. Can't you
> simple hard code numbers per case?
>
Ironically they are there as you wanted the padding sizes, and their
impact on alignment, documented and it made sense to me to group them
like this.
> ...
>
> > +/**
> > + * struct gpioline_config - Configuration for GPIO lines
> > + */
> > +struct gpioline_config {
> > + struct gpioline_values values;
> > + __u32 flags;
> > + /* Note that the following four fields are equivalent to a single u32. */
> > + __u8 direction;
> > + __u8 drive;
> > + __u8 bias;
> > + __u8 edge_detection;
> > + __u32 debounce_period;
>
> > + __u32 padding[GPIOLINE_CONFIG_PAD_SIZE]; /* for future use */
>
> I would just put minimum here. If you need to extend you have to use
> sizeof(struct) as a version of ABI.
>
That is doable, but gets complicated when you have embedded structs, as
this one is in gpioline_request and gpioline_info_v2.
To keep decoding simple on the kernel side, we would have to explode all
the struct definitions so new fields are always added to the end.
Or we would end up with rather complex decoding on the kernel side.
We can always use that as a fallback if we run out of padding.
> > +};
>
> I'm wondering how many lines (in average) the user usually changes at
> once? One? Two?
>
> Perhaps we need to be better with this, something like single line /
> multiple lines?
>
> So, having a struct for single line change being embedded multiple
> times would allow to configure atomically several lines with different
> requirements.
> For example you can turn directions of the two lines for some kind of
> half-duplex bit banging protocol.
>
> I'm not sure about the rest, but to me it seems reasonable to have
> single vs. multiple separation in some of the structures.
>
I agree in general, but not sure where to draw the line (pun not
indended).
Or how to provide a coherent API at a higher layer for that matter.
Do you have a concrete use case you need a solution for?
> ...
>
> > +/*
> > + * ABI V1
>
> V1 -> v1
>
> And below V2 -> v2.
>
Fair enough.
Cheers,
Kent.
On Wed, Jun 24, 2020 at 05:46:33PM +0300, Andy Shevchenko wrote:
> On Tue, Jun 23, 2020 at 7:03 AM Kent Gibson <[email protected]> wrote:
> >
> > Merge separate usage of test_bit/set_bit into test_and_set_bit to remove
> > the possibility of a race between the test and set.
> >
> > Similarly test_bit and clear_bit.
> >
> > In the existing code it is possible for two threads to race past the
> > test_bit and then set or clear the watch bit, and neither return EBUSY.
>
> I stumbled over this myself, but...
>
> > - if (test_bit(hwgpio, gcdev->watched_lines))
> > + if (test_and_set_bit(hwgpio, gcdev->watched_lines))
> > return -EBUSY;
> >
> > gpio_desc_to_lineinfo(desc, &lineinfo);
> > @@ -897,7 +897,6 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
> > return -EFAULT;
> >
> > - set_bit(hwgpio, gcdev->watched_lines);
> > return 0;
>
> ...I think it's not an equivalent despite races involved. If you set
> bit and return error code, you will have the wrong state.
>
Not quite sure what you mean. There is only an error if the bit is
already set, so you've changed nothing.
And the watched state is not part of the lineinfo, so the state returned is
the same either way.
Cheers,
Kent.
On Wed, Jun 24, 2020 at 11:57:14PM +0800, Kent Gibson wrote:
> On Wed, Jun 24, 2020 at 05:46:33PM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 23, 2020 at 7:03 AM Kent Gibson <[email protected]> wrote:
> > >
> > > Merge separate usage of test_bit/set_bit into test_and_set_bit to remove
> > > the possibility of a race between the test and set.
> > >
> > > Similarly test_bit and clear_bit.
> > >
> > > In the existing code it is possible for two threads to race past the
> > > test_bit and then set or clear the watch bit, and neither return EBUSY.
> >
> > I stumbled over this myself, but...
> >
> > > - if (test_bit(hwgpio, gcdev->watched_lines))
> > > + if (test_and_set_bit(hwgpio, gcdev->watched_lines))
> > > return -EBUSY;
> > >
> > > gpio_desc_to_lineinfo(desc, &lineinfo);
> > > @@ -897,7 +897,6 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > > if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
> > > return -EFAULT;
> > >
> > > - set_bit(hwgpio, gcdev->watched_lines);
> > > return 0;
> >
> > ...I think it's not an equivalent despite races involved. If you set
> > bit and return error code, you will have the wrong state.
> >
>
> Not quite sure what you mean. There is only an error if the bit is
> already set, so you've changed nothing.
>
> And the watched state is not part of the lineinfo, so the state returned is
> the same either way.
>
Perhaps you are referring to the case where the copy_to_user fails?
To be honest I considered that to be so unlikely that I ignored it.
Is there a relevant failure mode that I'm missing?
Cheers,
Kent.
On Wed, Jun 24, 2020 at 04:20:49PM +0200, Bartosz Golaszewski wrote:
> śr., 24 cze 2020 o 16:19 Kent Gibson <[email protected]> napisał(a):
> >
> > On Wed, Jun 24, 2020 at 04:04:09PM +0200, Bartosz Golaszewski wrote:
> > > wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
> > > >
> > > > Rename priv to gcdev to improve readability.
> > > >
> > > > The name "priv" indicates that the object is pointed to by
> > > > file->private_data, not what the object is actually is.
> > > > It is always used to point to a struct gpio_chardev_data so renaming
> > > > it to gcdev seemed as good as anything, and certainly clearer than "priv".
> > > >
> > > > Signed-off-by: Kent Gibson <[email protected]>
> > > >
> > >
> > > Ugh now it's gcdev and gdev everywhere and it doesn't really make it
> > > more readable. Maybe chardev_data or cdev_data?
> > >
> >
> > Agreed, it isn't ideal visually, but is at least more unique than priv.
> > Linus was going for short names recently (e.g. gc for gpiochip), so I was
> > going for something short.
> >
> > And I try avoid names ending in _data or _state or similar where they
> > don't really add anything.
> >
> > Would chardev or gchardev work for you?
> >
>
> Yes, chardev is fine. Even cdev is fine for me: gdev vs gcdev is
> confusing but gdev vs cdev looks better IMO.
>
OK, I was avoiding cdev to try to make the name more likely to be
globally unique, hence the leading "g".
To try to keep it short, how about attacking it from the other end and
reducing it to gcd?
That would also be in keeping with the naming convention I use in later
patches, e.g. glv for gpioline_values.
So gcd for gpio_chardev_data. Hmmm, or maybe gcdd?
Why is it that naming things is always the hardest part ;-)?
Cheers,
Kent.
On Wed, Jun 24, 2020 at 04:36:22PM +0200, Bartosz Golaszewski wrote:
> wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
> >
[ snip ]
> > +
> > +/*
> > + * Struct padding sizes.
> > + *
> > + * These are sized to pad structs to 64bit boundaries.
> > + * To maintain 32/64bit alignment, any arbitrary change must be even, as
> > + * the pad elements are __u32.
> > + */
> > +#define GPIOLINE_CONFIG_PAD_SIZE 7
> > +#define GPIOLINE_REQUEST_PAD_SIZE 5
> > +#define GPIOLINE_INFO_V2_PAD_SIZE 5
> > +#define GPIOLINE_INFO_CHANGED_V2_PAD_SIZE 5
> > +#define GPIOLINE_EVENT_PAD_SIZE 2
> > +
>
> Did someone request such definitions? IMO it's one of those rare
> instances where I'd prefer to see magic numbers used in the structs.
> I'm not sure if we need this indirection.
>
Not explicitly. Andy requested documentation on the pad sizes, which
seemed reasonable, and it also seemed reasonable to me to group them in
the one place rather than repeat the doco for each struct.
But having the size of the pad visible in the struct definition is
preferable, as it is only really relevant to that struct.
And everyone wants to keep the uAPI definitions as short possible, so
will drop these in v2.
> > +/**
> > + * struct gpioline_values - Values of GPIO lines
> > + * @bitmap: a bitmap containing the value of the lines, set to 1 for active
> > + * and 0 for inactive. Note that this is the logical value, which will be
> > + * the opposite of the physical value if GPIOLINE_FLAG_V2_ACTIVE_LOW is
> > + * set in flags.
> > + */
> > +struct gpioline_values {
> > + __u64 bitmap[GPIOLINES_BITMAP_SIZE];
> > +};
>
> Ok so now when embedding this structure we get something like
> "config.values.bitmap". This looks fine with the exception that "bits"
> would be even shorter and the information about this field being a
> bitmap is not necessary. I hope this isn't too much bikeshedding. :)
>
Fair enough - bits it is.
> > +
> > +/**
> > + * struct gpioline_config - Configuration for GPIO lines
> > + * @values: if the direction is GPIOLINE_DIRECTION_OUTPUT, the values that
> > + * the lines will be set to. This field is write-only and is zeroed when
> > + * returned within struct gpioline_info.
> > + * @flags: flags for the GPIO lines, such as GPIOLINE_FLAG_V2_ACTIVE_LOW,
> > + * GPIOLINE_FLAG_V2_DIRECTION etc, OR:ed together
> > + * @direction: if GPIOLINE_FLAG_V2_DIRECTION is set in flags, the desired
> > + * direction for the requested lines, with a value from enum
> > + * gpioline_direction
> > + * @drive: if GPIOLINE_FLAG_V2_DRIVE is set in flags, the desired drive for
> > + * the requested lines, with a value from enum gpioline_drive
> > + * @bias: if GPIOLINE_FLAG_V2_BIAS is set in flags, the desired bias for
> > + * the requested lines, with a value from enum gpioline_bias
> > + * @edge_detection: if GPIOLINE_FLAG_V2_EDGE_DETECTION is set in flags, the
> > + * desired edge_detection for the requested lines, with a value from enum
> > + * gpioline_edge
> > + * @debounce_period: if GPIOLINE_FLAG_V2_DEBOUNCE is set in flags, the
> > + * desired debounce period for the requested lines, in microseconds
> > + * @padding: reserved for future use and must be zero filled
> > + */
> > +struct gpioline_config {
> > + struct gpioline_values values;
> > + __u32 flags;
> > + /* Note that the following four fields are equivalent to a single u32. */
> > + __u8 direction;
> > + __u8 drive;
> > + __u8 bias;
> > + __u8 edge_detection;
>
> Limiting the size of these enum fields doesn't improve performance in
> any measurable way. We already have 4 possible values for events. I'm
> afraid that at some point in the future we'll add support for level
> events or something else etc. and we'll run into a problem because we
> only have 8 bits available. If there are no objections, I'd make it 32
> bits wide.
>
Agreed - the reduction in size isn't worth the effort.
> For the same reason I was thinking that making flags 64 bits wouldn't
> hurt either.
>
OK
[ snip ]
> > + * struct gpioline_event - The actual event being pushed to userspace
> > + * @timestamp: best estimate of time of event occurrence, in nanoseconds.
> > + * The timestamp is read from CLOCK_MONOTONIC and is intended to allow the
> > + * accurate measurement of the time between events. It does not provide
> > + * the wall-clock time.
> > + * @id: event identifier with value from enum gpioline_event_id
> > + * @offset: the offset of the line that triggered the event
> > + * @seqno: the sequence number for this event in the sequence of events for
> > + * all the lines in this line request
> > + * @line_seqno: the sequence number for this event in the sequence of
> > + * events on this particular line
> > + * @padding: reserved for future use
> > + */
> > +struct gpioline_event {
> > + __u64 timestamp;
> > + __u32 id;
> > + __u32 offset;
> > + __u32 seqno;
> > + __u32 line_seqno;
> > + __u32 padding[GPIOLINE_EVENT_PAD_SIZE]; /* for future use */
>
> Any reason for only having 64 bits of padding? 128 wouldn't change much, right?
>
No, as I mentioned in the commentary, I'm concerned that I've gone
too small with the padding, so happy to bump that.
And I'm open to suggestions on the other pads as well.
Cheers,
Kent.
On Thu, Jun 25, 2020 at 1:58 AM Kent Gibson <[email protected]> wrote:
> On Wed, Jun 24, 2020 at 11:57:14PM +0800, Kent Gibson wrote:
> > On Wed, Jun 24, 2020 at 05:46:33PM +0300, Andy Shevchenko wrote:
> > > On Tue, Jun 23, 2020 at 7:03 AM Kent Gibson <[email protected]> wrote:
...
> > > I stumbled over this myself, but...
> > >
> > > > - if (test_bit(hwgpio, gcdev->watched_lines))
> > > > + if (test_and_set_bit(hwgpio, gcdev->watched_lines))
> > > > return -EBUSY;
> > > >
> > > > gpio_desc_to_lineinfo(desc, &lineinfo);
> > > > @@ -897,7 +897,6 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > > > if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
> > > > return -EFAULT;
> > > >
> > > > - set_bit(hwgpio, gcdev->watched_lines);
> > > > return 0;
> > >
> > > ...I think it's not an equivalent despite races involved. If you set
> > > bit and return error code, you will have the wrong state.
> Perhaps you are referring to the case where the copy_to_user fails?
Yes.
> To be honest I considered that to be so unlikely that I ignored it.
> Is there a relevant failure mode that I'm missing?
The traditional question for such cases is "what can possibly go wrong?"
I wouldn't underestimate the probability of failure.
--
With Best Regards,
Andy Shevchenko
On Thu, Jun 25, 2020 at 11:44:21AM +0300, Andy Shevchenko wrote:
> On Thu, Jun 25, 2020 at 1:58 AM Kent Gibson <[email protected]> wrote:
> > On Wed, Jun 24, 2020 at 11:57:14PM +0800, Kent Gibson wrote:
> > > On Wed, Jun 24, 2020 at 05:46:33PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Jun 23, 2020 at 7:03 AM Kent Gibson <[email protected]> wrote:
>
> ...
>
> > > > I stumbled over this myself, but...
> > > >
> > > > > - if (test_bit(hwgpio, gcdev->watched_lines))
> > > > > + if (test_and_set_bit(hwgpio, gcdev->watched_lines))
> > > > > return -EBUSY;
> > > > >
> > > > > gpio_desc_to_lineinfo(desc, &lineinfo);
> > > > > @@ -897,7 +897,6 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > > > > if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
> > > > > return -EFAULT;
> > > > >
> > > > > - set_bit(hwgpio, gcdev->watched_lines);
> > > > > return 0;
> > > >
> > > > ...I think it's not an equivalent despite races involved. If you set
> > > > bit and return error code, you will have the wrong state.
>
> > Perhaps you are referring to the case where the copy_to_user fails?
>
> Yes.
>
> > To be honest I considered that to be so unlikely that I ignored it.
> > Is there a relevant failure mode that I'm missing?
>
> The traditional question for such cases is "what can possibly go wrong?"
> I wouldn't underestimate the probability of failure.
>
The worst case is the watch is enabled and the userspace gets an
EFAULT so it thinks it failed. If userspace retries then they get
EBUSY, so userspace accounting gets muddled.
We can clear the watch bit if the copy_to_user fails - before
returning the EFAULT. Would that be satisfactory?
Back to the failure, is it possible for the copy_to_user fail here,
given that the corresponding copy_from_user has succeeded?
If so, can that be manually triggered for test purposes?
Cheers,
Kent.
On Thu, Jun 25, 2020 at 12:13 PM Kent Gibson <[email protected]> wrote:
> On Thu, Jun 25, 2020 at 11:44:21AM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 25, 2020 at 1:58 AM Kent Gibson <[email protected]> wrote:
> > > On Wed, Jun 24, 2020 at 11:57:14PM +0800, Kent Gibson wrote:
...
> > > Perhaps you are referring to the case where the copy_to_user fails?
> >
> > Yes.
> >
> > > To be honest I considered that to be so unlikely that I ignored it.
> > > Is there a relevant failure mode that I'm missing?
> >
> > The traditional question for such cases is "what can possibly go wrong?"
> > I wouldn't underestimate the probability of failure.
> >
>
> The worst case is the watch is enabled and the userspace gets an
> EFAULT so it thinks it failed. If userspace retries then they get
> EBUSY, so userspace accounting gets muddled.
>
> We can clear the watch bit if the copy_to_user fails - before
> returning the EFAULT. Would that be satisfactory?
Perhaps. I didn't check that scenario.
> Back to the failure, is it possible for the copy_to_user fail here,
> given that the corresponding copy_from_user has succeeded?
Of course. The general rule is if on SMP system you have not strongly
serialized sequence of calls (means no preemption, no interrupts, etc)
anything can happen in between.
> If so, can that be manually triggered for test purposes?
Unfortunately not an expert in mm, no idea, sorry.
--
With Best Regards,
Andy Shevchenko
On Thu, Jun 25, 2020 at 12:23:49PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 25, 2020 at 12:13 PM Kent Gibson <[email protected]> wrote:
> > On Thu, Jun 25, 2020 at 11:44:21AM +0300, Andy Shevchenko wrote:
> > > On Thu, Jun 25, 2020 at 1:58 AM Kent Gibson <[email protected]> wrote:
> > > > On Wed, Jun 24, 2020 at 11:57:14PM +0800, Kent Gibson wrote:
>
> ...
>
> > > > Perhaps you are referring to the case where the copy_to_user fails?
> > >
> > > Yes.
> > >
> > > > To be honest I considered that to be so unlikely that I ignored it.
> > > > Is there a relevant failure mode that I'm missing?
> > >
> > > The traditional question for such cases is "what can possibly go wrong?"
> > > I wouldn't underestimate the probability of failure.
> > >
> >
> > The worst case is the watch is enabled and the userspace gets an
> > EFAULT so it thinks it failed. If userspace retries then they get
> > EBUSY, so userspace accounting gets muddled.
> >
> > We can clear the watch bit if the copy_to_user fails - before
> > returning the EFAULT. Would that be satisfactory?
>
> Perhaps. I didn't check that scenario.
>
To be clear I'm suggesting this:
gpio_desc_to_lineinfo(desc, &lineinfo);
- if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
+ if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) {
+ clear_bit(lineinfo.offset, gcdev->watched_lines);
return -EFAULT;
+ }
That undoes the set, returning the watch state to what it was before the
call.
Cheers,
Kent.
On Wed, Jun 24, 2020 at 4:08 PM Kent Gibson <[email protected]> wrote:
>
> On Wed, Jun 24, 2020 at 04:00:42PM +0200, Bartosz Golaszewski wrote:
> > wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
> > >
> > > Reset the timestamp field to 0 after using it in lineevent_irq_thread.
> > >
> > > The timestamp is set by lineevent_irq_handler and is tested by
> > > lineevent_irq_thread to determine if it is called from a nested theaded
> > > interrupt.
> > > lineevent_irq_thread is assuming that the nested, or otherwise, status
> > > of the IRQ is static, i.e. it is either always nested or never nested.
> > > This change removes that assumption, resetting the timestamp so it can
> > > be re-used to determine the nested state of subsequent interrupts.
> > >
> > > Signed-off-by: Kent Gibson <[email protected]>
> > >
> >
> > This change makes sense to me but I'm having a hard time processing
> > the explanation. If we're requesting the interrupt and allocating the
> > lineevent state in the same function - how can we run into a situation
> > here the status of the irq would change like what you describe?
> >
>
> I'm not totally sure myself, as my understanding of how interrupts are
> shared in the kernel is pretty sketchy, but my concern is that if we
> are sharing the irq then whoever we are sharing with may release the irq
> and we go from nested to unnested. Or vice versa. Not sure if that is
> valid, but that was my concern, and it seemed like a minor change to
> cover it just in case.
>
It's my understanding that a shared interrupt must be explicitly
requested as shared by all previous users or request_irq() will fail.
In this case: we call request_threaded_irq() without the IRQF_SHARED
flag so it's never a shared interrupt. Even if someone previously
requested it as shared - our call will simply fail.
I still think that resetting the timestamp is fine because it's not
being set to 0 in hardirq context. We just need a different
explanation.
Bart
On Thu, Jun 25, 2020 at 11:44:30AM +0200, Bartosz Golaszewski wrote:
> On Wed, Jun 24, 2020 at 4:08 PM Kent Gibson <[email protected]> wrote:
> >
> > On Wed, Jun 24, 2020 at 04:00:42PM +0200, Bartosz Golaszewski wrote:
> > > wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
[ snip ]
> >
> > I'm not totally sure myself, as my understanding of how interrupts are
> > shared in the kernel is pretty sketchy, but my concern is that if we
> > are sharing the irq then whoever we are sharing with may release the irq
> > and we go from nested to unnested. Or vice versa. Not sure if that is
> > valid, but that was my concern, and it seemed like a minor change to
> > cover it just in case.
> >
>
> It's my understanding that a shared interrupt must be explicitly
> requested as shared by all previous users or request_irq() will fail.
> In this case: we call request_threaded_irq() without the IRQF_SHARED
> flag so it's never a shared interrupt. Even if someone previously
> requested it as shared - our call will simply fail.
>
OK. Is there a reason not to share the interrupt?
> I still think that resetting the timestamp is fine because it's not
> being set to 0 in hardirq context. We just need a different
> explanation.
>
Or just drop it?
Cheers,
Kent.
On Thu, Jun 25, 2020 at 1:16 AM Kent Gibson <[email protected]> wrote:
>
> On Wed, Jun 24, 2020 at 04:20:49PM +0200, Bartosz Golaszewski wrote:
> > śr., 24 cze 2020 o 16:19 Kent Gibson <[email protected]> napisał(a):
> > >
> > > On Wed, Jun 24, 2020 at 04:04:09PM +0200, Bartosz Golaszewski wrote:
> > > > wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
> > > > >
> > > > > Rename priv to gcdev to improve readability.
> > > > >
> > > > > The name "priv" indicates that the object is pointed to by
> > > > > file->private_data, not what the object is actually is.
> > > > > It is always used to point to a struct gpio_chardev_data so renaming
> > > > > it to gcdev seemed as good as anything, and certainly clearer than "priv".
> > > > >
> > > > > Signed-off-by: Kent Gibson <[email protected]>
> > > > >
> > > >
> > > > Ugh now it's gcdev and gdev everywhere and it doesn't really make it
> > > > more readable. Maybe chardev_data or cdev_data?
> > > >
> > >
> > > Agreed, it isn't ideal visually, but is at least more unique than priv.
> > > Linus was going for short names recently (e.g. gc for gpiochip), so I was
> > > going for something short.
> > >
> > > And I try avoid names ending in _data or _state or similar where they
> > > don't really add anything.
> > >
> > > Would chardev or gchardev work for you?
> > >
> >
> > Yes, chardev is fine. Even cdev is fine for me: gdev vs gcdev is
> > confusing but gdev vs cdev looks better IMO.
> >
>
> OK, I was avoiding cdev to try to make the name more likely to be
> globally unique, hence the leading "g".
>
> To try to keep it short, how about attacking it from the other end and
> reducing it to gcd?
> That would also be in keeping with the naming convention I use in later
> patches, e.g. glv for gpioline_values.
> So gcd for gpio_chardev_data. Hmmm, or maybe gcdd?
>
> Why is it that naming things is always the hardest part ;-)?
>
I prefer cdev here but it's just a personal preference. If anyone has
a better idea I'm happy to switch to it.
Bart
On Wed, Jun 24, 2020 at 11:40:57PM +0800, Kent Gibson wrote:
> On Wed, Jun 24, 2020 at 05:33:26PM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 23, 2020 at 7:04 AM Kent Gibson <[email protected]> wrote:
> > >
> > > Add a new version of the uAPI to address existing 32/64bit alignment
> >
[ snip ]
> >
> > I'm wondering how many lines (in average) the user usually changes at
> > once? One? Two?
> >
> > Perhaps we need to be better with this, something like single line /
> > multiple lines?
> >
> > So, having a struct for single line change being embedded multiple
> > times would allow to configure atomically several lines with different
> > requirements.
> > For example you can turn directions of the two lines for some kind of
> > half-duplex bit banging protocol.
> >
> > I'm not sure about the rest, but to me it seems reasonable to have
> > single vs. multiple separation in some of the structures.
> >
>
I think you are right about this - we should be taking the opportunity
to remove the homogeneous config restriction, so we can have a mixture
of lines with different configs in the one request.
e.g. I've had a request to have lines with different active low settings.
Your example requires both inputs and outputs.
And for a recent rotary encoder example I would've liked to be able to
have edge detection on one line, but return a snapshot with the state
of several lines in the edge event.
So what I'm thinking is to replace the config that I had proposed with
something like this:
struct attrib {
/*
* the set of lines from request.offsets that this attrib DOESN'T
* apply to. A zero value means it applies to all lines.
* A set bit means the line request.offsets[bit] does NOT get this
* attribute.
*/
__u64 mask[GPIOLINES_BITMAP_SIZE];
/*
* an attribute identifier that dictates the which of the union
* fields is valid and how it is interpreted.
*/
__u32 id;
union {
__u64 flags; /* similar to v1 handleflags + eventflags */
__u32 debounce_period;
__u64 values[GPIOLINES_BITMAP_SIZE]; /* for id == output */
/* ... */
/* future config values go here */
/*
* padding to ensure 32/64-bit alignment of attrib
*
* This must be the largest sized value.
*/
__u32 padding[3];
};
};
/* config is a stack of attribs associated with requested lines. */
struct config {
/* the number of populated attribs */
__u32 num_attribs;
/* for 32/64-bit alignment */
__u32 padding;
struct attrib attribs[MAX_CONFIG_ATTRIBS];
};
The idea is a stack of attributes, each of which can be applied to a
subset of the requested lines, as determined by the attrib.mask.
The config for a given attribute on a given line is determined by
finding the first match while walking down the stack, or falling
back to the sensible default if no match is found.
To reduce the number of attributes required, a number of boolean or
boolean-ish fields can be combined into flags, similar to v1.
I'm guessing a handful of attribs would suffice for the vast majority of
cases, so MAX_CONFIG_ATTRIBS would be in the 5-10 range.
(Are we concerned about struct sizes?)
Adding new config attribs in the future would involve adding a new id
and a corresponding value in the union. So we can potentially add as
many new config attribs as we like - though the user would still be
limited to MAX_CONFIG_ATTRIBS and may have to trade-off attribs in
their particular application.
Does that make any sense?
Cheers,
Kent.
On Tue, Jun 23, 2020 at 6:03 AM Kent Gibson <[email protected]> wrote:
>
> Add a build option to allow the removal of the CDEV v1 ABI.
>
> Suggested-by: Bartosz Golaszewski <[email protected]>
> Signed-off-by: Kent Gibson <[email protected]>
>
> ---
>
> This patch is before the V2 implementation, and is non-functional until
> that patch, as some parts of that patch would be written slightly
> differently if removing V1 was not considered.
> Adding this patch after that would necessitate revisiting the V2 changes,
> so this ordering results in two simpler patches.
>
> drivers/gpio/Kconfig | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index affc1524bc2c..b966a7dc1c9a 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -81,6 +81,18 @@ config GPIO_CDEV
>
> If unsure, say Y.
>
> +config GPIO_CDEV_V1
> + bool "Support GPIO ABI Version 1"
> + default y
> + depends on GPIO_CDEV
> + help
> + Say Y here to support version 1 of the GPIO CDEV ABI.
> +
> + This ABI version is deprecated and will be removed in the future.
> + Please use the latest ABI for new developments.
> +
> + If unsure, say Y.
> +
> config GPIO_GENERIC
> depends on HAS_IOMEM # Only for IOMEM drivers
> tristate
> --
> 2.27.0
>
As long as the series remains bisectable:
Reviewed-by: Bartosz Golaszewski <[email protected]>
On Tue, Jun 23, 2020 at 6:02 AM Kent Gibson <[email protected]> wrote:
>
> Make the gpiolib-cdev module a build option. This allows the CDEV
> interface to be removed from the kernel to reduce kernel size in
> applications where is it not required, and provides the parent for
> other other CDEV interface specific build options to follow.
>
> Suggested-by: Bartosz Golaszewski <[email protected]>
> Signed-off-by: Kent Gibson <[email protected]>
>
> ---
> drivers/gpio/Kconfig | 16 ++++++++++++++--
> drivers/gpio/Makefile | 2 +-
> drivers/gpio/gpiolib-cdev.h | 15 +++++++++++++++
> 3 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index c6b5c65c8405..affc1524bc2c 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -66,8 +66,20 @@ config GPIO_SYSFS
>
> This ABI is deprecated. If you want to use GPIO from userspace,
> use the character device /dev/gpiochipN with the appropriate
> - ioctl() operations instead. The character device is always
> - available.
> + ioctl() operations instead.
> +
> +config GPIO_CDEV
> + bool "/dev/gpiochipN (character device interface)"
> + default y
> + help
> + Say Y here to add the character device /dev/gpiochipN interface
> + for GPIOs. The character device allows userspace to control GPIOs
> + using ioctl() operations.
> +
> + Only say N is you are sure that the GPIO character device is not
> + required.
> +
> + If unsure, say Y.
>
> config GPIO_GENERIC
> depends on HAS_IOMEM # Only for IOMEM drivers
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index ef666cfef9d0..45eb09808d12 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -7,8 +7,8 @@ obj-$(CONFIG_GPIOLIB) += gpiolib.o
> obj-$(CONFIG_GPIOLIB) += gpiolib-devres.o
> obj-$(CONFIG_GPIOLIB) += gpiolib-legacy.o
> obj-$(CONFIG_GPIOLIB) += gpiolib-devprop.o
> -obj-$(CONFIG_GPIOLIB) += gpiolib-cdev.o
> obj-$(CONFIG_OF_GPIO) += gpiolib-of.o
> +obj-$(CONFIG_GPIO_CDEV) += gpiolib-cdev.o
> obj-$(CONFIG_GPIO_SYSFS) += gpiolib-sysfs.o
> obj-$(CONFIG_GPIO_ACPI) += gpiolib-acpi.o
>
> diff --git a/drivers/gpio/gpiolib-cdev.h b/drivers/gpio/gpiolib-cdev.h
> index 973578e7ad10..19a4e3d57120 100644
> --- a/drivers/gpio/gpiolib-cdev.h
> +++ b/drivers/gpio/gpiolib-cdev.h
> @@ -5,7 +5,22 @@
>
> #include <linux/device.h>
>
> +#ifdef CONFIG_GPIO_CDEV
> +
> int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt);
> void gpiolib_cdev_unregister(struct gpio_device *gdev);
>
> +#else
> +
> +static inline int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
> +{
> + return 0;
> +}
> +
> +static inline void gpiolib_cdev_unregister(struct gpio_device *gdev)
> +{
> +}
> +
> +#endif /* CONFIG_GPIO_CDEV */
> +
> #endif /* GPIOLIB_CDEV_H */
> --
> 2.27.0
>
I know Linus doesn't like this, but I'm personally in favor of adding
this as long as it's enabled by default.
Reviewed-by: Bartosz Golaszewski <[email protected]>
On Tue, Jun 23, 2020 at 6:02 AM Kent Gibson <[email protected]> wrote:
>
> Remove recalculation of offset from desc, where desc itself was calculated
> from offset.
>
> There is no benefit from for the desc -> hwgpio conversion in this
> context. The only implicit benefit of the offset -> desc -> hwgpio is
> the range check in the offset -> desc, but where desc is required you
> still get that, and where desc isn't required it is simpler to perform
> the range check directly.
>
> Signed-off-by: Kent Gibson <[email protected]>
Reviewed-by: Bartosz Golaszewski <[email protected]>
On Thu, Jun 25, 2020 at 12:01 PM Kent Gibson <[email protected]> wrote:
>
> On Thu, Jun 25, 2020 at 11:44:30AM +0200, Bartosz Golaszewski wrote:
> > On Wed, Jun 24, 2020 at 4:08 PM Kent Gibson <[email protected]> wrote:
> > >
> > > On Wed, Jun 24, 2020 at 04:00:42PM +0200, Bartosz Golaszewski wrote:
> > > > wt., 23 cze 2020 o 06:02 Kent Gibson <[email protected]> napisał(a):
> [ snip ]
> > >
> > > I'm not totally sure myself, as my understanding of how interrupts are
> > > shared in the kernel is pretty sketchy, but my concern is that if we
> > > are sharing the irq then whoever we are sharing with may release the irq
> > > and we go from nested to unnested. Or vice versa. Not sure if that is
> > > valid, but that was my concern, and it seemed like a minor change to
> > > cover it just in case.
> > >
> >
> > It's my understanding that a shared interrupt must be explicitly
> > requested as shared by all previous users or request_irq() will fail.
> > In this case: we call request_threaded_irq() without the IRQF_SHARED
> > flag so it's never a shared interrupt. Even if someone previously
> > requested it as shared - our call will simply fail.
> >
>
> OK. Is there a reason not to share the interrupt?
>
If nobody requested this yet, I'd say: let's not touch it. :)
In theory, we check if the line state changed so we should be fine but
in practice this sounds like a can of worms. That being said: I don't
have a reason not to do it. Just a feeling.
> > I still think that resetting the timestamp is fine because it's not
> > being set to 0 in hardirq context. We just need a different
> > explanation.
> >
>
> Or just drop it?
Yes, I think dropping this patch for now is fine.
Bart
On Tue, Jun 23, 2020 at 6:03 AM Kent Gibson <[email protected]> wrote:
>
> Extend gpio-event-mon to support monitoring multiple lines.
> This would require multiple lineevent requests to implement using uAPI V1,
> but can be performed with a single line request using uAPI V2.
>
> Signed-off-by: Kent Gibson <[email protected]>
Reviewed-by: Bartosz Golaszewski <[email protected]>