| Message ID | 20200922023151.387447-9-warthog618@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: cdev: add uAPI v2 | expand |
On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@gmail.com> wrote: > > Add support for GPIO_V2_GET_LINEINFO_IOCTL and > GPIO_V2_GET_LINEINFO_WATCH_IOCTL. > > The core of this change is the event kfifo switching to contain > struct gpioline_info_changed_v2, instead of v1 as v2 is richer. > > 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_V2_GET_LINEINFO_WATCH_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. > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > --- > > Changes for v5: > - as per cover letter > > Changes for v4: > - replace strncpy with memcpy in gpio_v2_line_info_to_v1 > > drivers/gpio/gpiolib-cdev.c | 197 +++++++++++++++++++++++++++++++----- > 1 file changed, 169 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > index 7a3ed2617f74..d3857113f58c 100644 > --- a/drivers/gpio/gpiolib-cdev.c > +++ b/drivers/gpio/gpiolib-cdev.c > @@ -181,7 +181,8 @@ static long linehandle_set_config(struct linehandle_state *lh, > } > > blocking_notifier_call_chain(&desc->gdev->notifier, > - GPIOLINE_CHANGED_CONFIG, desc); > + GPIO_V2_LINE_CHANGED_CONFIG, > + desc); > } > return 0; > } > @@ -353,7 +354,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) > } > > blocking_notifier_call_chain(&desc->gdev->notifier, > - GPIOLINE_CHANGED_REQUESTED, desc); > + GPIO_V2_LINE_CHANGED_REQUESTED, desc); > > dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", > offset); > @@ -747,7 +748,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip) > } > > blocking_notifier_call_chain(&desc->gdev->notifier, > - GPIOLINE_CHANGED_REQUESTED, desc); > + GPIO_V2_LINE_CHANGED_REQUESTED, desc); > > dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", > offset); > @@ -1094,7 +1095,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip) > goto out_free_le; > > blocking_notifier_call_chain(&desc->gdev->notifier, > - GPIOLINE_CHANGED_REQUESTED, desc); > + GPIO_V2_LINE_CHANGED_REQUESTED, desc); > > irq = gpiod_to_irq(desc); > if (irq <= 0) { > @@ -1161,17 +1162,59 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip) > return ret; > } > > +static void gpio_v2_line_info_to_v1(struct gpio_v2_line_info *info_v2, > + struct gpioline_info *info_v1) > +{ > + u64 flagsv2 = info_v2->flags; > + > + memcpy(info_v1->name, info_v2->name, sizeof(info_v1->name)); > + memcpy(info_v1->consumer, info_v2->consumer, > + sizeof(info_v1->consumer)); One line? > + info_v1->line_offset = info_v2->offset; > + info_v1->flags = 0; > + > + if (flagsv2 & GPIO_V2_LINE_FLAG_USED) > + info_v1->flags |= GPIOLINE_FLAG_KERNEL; > + > + if (flagsv2 & GPIO_V2_LINE_FLAG_OUTPUT) > + info_v1->flags |= GPIOLINE_FLAG_IS_OUT; > + > + if (flagsv2 & GPIO_V2_LINE_FLAG_ACTIVE_LOW) > + info_v1->flags |= GPIOLINE_FLAG_ACTIVE_LOW; > + > + if (flagsv2 & GPIO_V2_LINE_FLAG_OPEN_DRAIN) > + info_v1->flags |= GPIOLINE_FLAG_OPEN_DRAIN; > + if (flagsv2 & GPIO_V2_LINE_FLAG_OPEN_SOURCE) > + info_v1->flags |= GPIOLINE_FLAG_OPEN_SOURCE; > + > + if (flagsv2 & GPIO_V2_LINE_FLAG_BIAS_PULL_UP) > + info_v1->flags |= GPIOLINE_FLAG_BIAS_PULL_UP; > + if (flagsv2 & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN) > + info_v1->flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN; > + if (flagsv2 & GPIO_V2_LINE_FLAG_BIAS_DISABLED) > + info_v1->flags |= GPIOLINE_FLAG_BIAS_DISABLE; > +} > + > +static void gpio_v2_line_info_changed_to_v1( > + struct gpio_v2_line_info_changed *lic_v2, > + struct gpioline_info_changed *lic_v1) > +{ > + gpio_v2_line_info_to_v1(&lic_v2->info, &lic_v1->info); > + lic_v1->timestamp = lic_v2->timestamp_ns; > + 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 gpio_v2_line_info *info) > { > struct gpio_chip *gc = desc->gdev->chip; > bool ok_for_pinctrl; > unsigned long flags; > > memset(info, 0, sizeof(*info)); > - info->line_offset = gpio_chip_hwgpio(desc); > + info->offset = gpio_chip_hwgpio(desc); > > /* > * This function takes a mutex so we must check this before taking > @@ -1181,7 +1224,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); > > @@ -1202,23 +1245,27 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc, > test_bit(FLAG_EXPORT, &desc->flags) || > test_bit(FLAG_SYSFS, &desc->flags) || > !ok_for_pinctrl) > - info->flags |= GPIOLINE_FLAG_KERNEL; > + info->flags |= GPIO_V2_LINE_FLAG_USED; > + > if (test_bit(FLAG_IS_OUT, &desc->flags)) > - info->flags |= GPIOLINE_FLAG_IS_OUT; > + info->flags |= GPIO_V2_LINE_FLAG_OUTPUT; > + else > + info->flags |= GPIO_V2_LINE_FLAG_INPUT; > + > if (test_bit(FLAG_ACTIVE_LOW, &desc->flags)) > - info->flags |= GPIOLINE_FLAG_ACTIVE_LOW; > + info->flags |= GPIO_V2_LINE_FLAG_ACTIVE_LOW; > + > if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) > - info->flags |= (GPIOLINE_FLAG_OPEN_DRAIN | > - GPIOLINE_FLAG_IS_OUT); > + info->flags |= GPIO_V2_LINE_FLAG_OPEN_DRAIN; > if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) > - info->flags |= (GPIOLINE_FLAG_OPEN_SOURCE | > - GPIOLINE_FLAG_IS_OUT); > + info->flags |= GPIO_V2_LINE_FLAG_OPEN_SOURCE; > + > if (test_bit(FLAG_BIAS_DISABLE, &desc->flags)) > - info->flags |= GPIOLINE_FLAG_BIAS_DISABLE; > + info->flags |= GPIO_V2_LINE_FLAG_BIAS_DISABLED; > if (test_bit(FLAG_PULL_DOWN, &desc->flags)) > - info->flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN; > + info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN; > if (test_bit(FLAG_PULL_UP, &desc->flags)) > - info->flags |= GPIOLINE_FLAG_BIAS_PULL_UP; > + info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_UP; > > spin_unlock_irqrestore(&gpio_lock, flags); > } > @@ -1226,11 +1273,65 @@ 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 gpio_v2_line_info_changed, 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); atomic_cmpxchng() returns a value. Also there are no barriers here... > + } > + if (abiv != version) > + return -EPERM; I'm not sure I understand why this is atomic. Also this seems to be racy if cdata changed in background. Shouldn't be rather if (atomic_cmpxchg() == 0) { if (atomic_read() != version) return ...; } But here is still the question: why do you expect the version to be changed on background? And what about barriers? > + return 0; > +} > +#endif > + > +static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip, > + bool watch) > +{ > + struct gpio_desc *desc; > + struct gpio_v2_line_info lineinfo; > + > + if (copy_from_user(&lineinfo, ip, sizeof(lineinfo))) > + return -EFAULT; > + > + if (memchr_inv(lineinfo.padding, 0, sizeof(lineinfo.padding))) > + return -EINVAL; > + > + desc = gpiochip_get_desc(cdev->gdev->chip, lineinfo.offset); > + if (IS_ERR(desc)) > + return PTR_ERR(desc); > + > + if (watch) { > +#ifdef CONFIG_GPIO_CDEV_V1 > + if (lineinfo_ensure_abi_version(cdev, 2)) > + return -EPERM; Can't you propagate error code from the function? > +#endif > + if (test_and_set_bit(lineinfo.offset, cdev->watched_lines)) > + return -EBUSY; > + } > + gpio_desc_to_lineinfo(desc, &lineinfo); > + > + if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) { > + if (watch) > + clear_bit(lineinfo.offset, cdev->watched_lines); > + return -EFAULT; > + } > + > + return 0; > +} > + > /* > * gpio_ioctl() - ioctl handler for the GPIO chardev > */ > @@ -1240,7 +1341,6 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > struct gpio_device *gdev = cdev->gdev; > struct gpio_chip *gc = gdev->chip; > void __user *ip = (void __user *)arg; > - struct gpio_desc *desc; > __u32 offset; > > /* We fail any subsequent ioctl():s when the chip is gone */ > @@ -1263,7 +1363,9 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > return 0; > #ifdef CONFIG_GPIO_CDEV_V1 > } else if (cmd == GPIO_GET_LINEINFO_IOCTL) { > + struct gpio_desc *desc; > struct gpioline_info lineinfo; > + struct gpio_v2_line_info lineinfo_v2; > > if (copy_from_user(&lineinfo, ip, sizeof(lineinfo))) > return -EFAULT; > @@ -1273,7 +1375,8 @@ 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); > + gpio_desc_to_lineinfo(desc, &lineinfo_v2); > + gpio_v2_line_info_to_v1(&lineinfo_v2, &lineinfo); > > if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) > return -EFAULT; > @@ -1283,7 +1386,9 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > } else if (cmd == GPIO_GET_LINEEVENT_IOCTL) { > return lineevent_create(gdev, ip); > } else if (cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) { > + struct gpio_desc *desc; > struct gpioline_info lineinfo; > + struct gpio_v2_line_info lineinfo_v2; > > if (copy_from_user(&lineinfo, ip, sizeof(lineinfo))) > return -EFAULT; > @@ -1293,10 +1398,14 @@ 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(cdev, 1)) > + return -EPERM; > + > if (test_and_set_bit(lineinfo.line_offset, cdev->watched_lines)) > return -EBUSY; > > - gpio_desc_to_lineinfo(desc, &lineinfo); > + gpio_desc_to_lineinfo(desc, &lineinfo_v2); > + gpio_v2_line_info_to_v1(&lineinfo_v2, &lineinfo); > > if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) { > clear_bit(lineinfo.line_offset, cdev->watched_lines); > @@ -1305,6 +1414,10 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > > return 0; > #endif /* CONFIG_GPIO_CDEV_V1 */ > + } else if (cmd == GPIO_V2_GET_LINEINFO_IOCTL || > + cmd == GPIO_V2_GET_LINEINFO_WATCH_IOCTL) { > + return lineinfo_get(cdev, ip, > + cmd == GPIO_V2_GET_LINEINFO_WATCH_IOCTL); > } else if (cmd == GPIO_V2_GET_LINE_IOCTL) { > return linereq_create(gdev, ip); > } else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) { > @@ -1340,7 +1453,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb, > unsigned long action, void *data) > { > struct gpio_chardev_data *cdev = to_gpio_chardev_data(nb); > - struct gpioline_info_changed chg; > + struct gpio_v2_line_info_changed chg; > struct gpio_desc *desc = data; > int ret; > > @@ -1349,7 +1462,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb, > > memset(&chg, 0, sizeof(chg)); > chg.event_type = action; > - chg.timestamp = ktime_get_ns(); > + chg.timestamp_ns = ktime_get_ns(); > gpio_desc_to_lineinfo(desc, &chg.info); > > ret = kfifo_in_spinlocked(&cdev->events, &chg, 1, &cdev->wait.lock); > @@ -1380,12 +1493,16 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf, > size_t count, loff_t *off) > { > struct gpio_chardev_data *cdev = file->private_data; > - struct gpioline_info_changed event; > + struct gpio_v2_line_info_changed 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 gpio_v2_line_info_changed); > + if (count < event_size) > return -EINVAL; > +#endif > > do { > spin_lock(&cdev->wait.lock); > @@ -1407,7 +1524,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(&cdev->watch_abi_version) == 2) > + event_size = sizeof(struct gpio_v2_line_info_changed); > + else > + event_size = sizeof(struct gpioline_info_changed); > + if (count < event_size) { > + spin_unlock(&cdev->wait.lock); > + return -EINVAL; > + } > +#endif > ret = kfifo_out(&cdev->events, &event, 1); > spin_unlock(&cdev->wait.lock); > if (ret != 1) { > @@ -1416,9 +1543,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 (event_size == sizeof(struct gpio_v2_line_info_changed)) { > + if (copy_to_user(buf + bytes_read, &event, event_size)) > + return -EFAULT; > + } else { > + struct gpioline_info_changed event_v1; > + > + gpio_v2_line_info_changed_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; > -- > 2.28.0 > -- With Best Regards, Andy Shevchenko
On Wed, Sep 23, 2020 at 06:41:45PM +0300, Andy Shevchenko wrote: > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > Add support for GPIO_V2_GET_LINEINFO_IOCTL and > > GPIO_V2_GET_LINEINFO_WATCH_IOCTL. > > [snip] > > > > +static void gpio_v2_line_info_to_v1(struct gpio_v2_line_info *info_v2, > > + struct gpioline_info *info_v1) > > +{ > > + u64 flagsv2 = info_v2->flags; > > + > > + memcpy(info_v1->name, info_v2->name, sizeof(info_v1->name)); > > > + memcpy(info_v1->consumer, info_v2->consumer, > > + sizeof(info_v1->consumer)); > > One line? > It can be now the line length limit has been raised - it just breaks the old 80 character limit. [snip] > > > > +#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); > > atomic_cmpxchng() returns a value. Yep, it returns the old value - which we don't care about - see below. > Also there are no barriers here... > No barriers required - the atomic_cmpxchg() is sufficient. > > + } > > + if (abiv != version) > > + return -EPERM; > > I'm not sure I understand why this is atomic. > The algorithm requires some level of protection and atomic is sufficient. > Also this seems to be racy if cdata changed in background. > Can you provide a case? The atomic_cmpxchg() ensures cdata->watch_abi_version is only set once - first in wins. The atomic_read() is so we can check that the set version matches what the caller wants. Note that multiple callers may request the same version - and all should succeed. > Shouldn't be rather > > if (atomic_cmpxchg() == 0) { > if (atomic_read() != version) > return ...; > } > My algorithm allows for multiple callers requesting the same version to all succeed. Yours would fail the first conditional for all but the first, and you haven't provided an else for that case... ... but it would probably look the same so the conditional is pointless, or it would reject the request - which would be wrong. > But here is still the question: why do you expect the version to be > changed on background? And what about barriers? > While it is unlikely that userspace will be attempting to use both ABI versions simultaneously on the same chip request, it is a possiblity and so needs to be protected against. And better to have the watch request fail than the read fail or worse - return the wrong struct version. The atomic_cmpxchg() is sufficient for this algorithm - no barriers required. It could also be written with a spinlock but I was trying to avoid locks unless they were absolutely necessary. A spinlock version may arguably be more readable, but it would certainly be more verbose, larger and slower. I'm happy to add some documentation to the function if that would help. > > + return 0; > > +} > > +#endif > > + > > +static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip, > > + bool watch) > > +{ > > + struct gpio_desc *desc; > > + struct gpio_v2_line_info lineinfo; > > + > > + if (copy_from_user(&lineinfo, ip, sizeof(lineinfo))) > > + return -EFAULT; > > + > > + if (memchr_inv(lineinfo.padding, 0, sizeof(lineinfo.padding))) > > + return -EINVAL; > > + > > + desc = gpiochip_get_desc(cdev->gdev->chip, lineinfo.offset); > > + if (IS_ERR(desc)) > > + return PTR_ERR(desc); > > + > > + if (watch) { > > +#ifdef CONFIG_GPIO_CDEV_V1 > > > + if (lineinfo_ensure_abi_version(cdev, 2)) > > + return -EPERM; > > Can't you propagate error code from the function? > You mean: + ret = lineinfo_ensure_abi_version(cdev, 2) + if (ret) + return ret; That seems more verbose and less clear. And I'd need to conditionally declare a ret - as this test is compiled out if CDEV_V1 is not defined. I did flip-flop on what lineinfo_ensure_abi_version() should return - either a bool or an error code. If a bool then the code would include the dreaded negative conditional ;-(: + if (!lineinfo_is_abi_version(cdev, 2)) + return -EPERM; so I eventually settled for the error code. But I'm on the fence on this one and happy to change it if you think the bool form is clearer. Cheers, Kent.
On Thu, Sep 24, 2020 at 5:39 AM Kent Gibson <warthog618@gmail.com> wrote: > On Wed, Sep 23, 2020 at 06:41:45PM +0300, Andy Shevchenko wrote: > > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@gmail.com> wrote: ... > > > + memcpy(info_v1->consumer, info_v2->consumer, > > > + sizeof(info_v1->consumer)); > > > > One line? > > > > It can be now the line length limit has been raised - it just breaks the > old 80 character limit. I really wouldn't care about this if it's only for a couple of characters. ... > > > +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); > > > > atomic_cmpxchng() returns a value. > > Yep, it returns the old value - which we don't care about - see below. Then what's the point to read back?.. > > Also there are no barriers here... > > > > No barriers required - the atomic_cmpxchg() is sufficient. > > > > + } > > > + if (abiv != version) > > > + return -EPERM; > > > > I'm not sure I understand why this is atomic. > > > > The algorithm requires some level of protection and atomic is > sufficient. > > > Also this seems to be racy if cdata changed in background. > > > > Can you provide a case? CPU0: CPU1: xchg() ... ... xchg() ... read() -> OK read() ->NOK > The atomic_cmpxchg() ensures cdata->watch_abi_version is only set > once - first in wins. The atomic_read() is so we can check that > the set version matches what the caller wants. > Note that multiple callers may request the same version - and all > should succeed. So, that's basically what you need when using _old_ value. 0 means you were first, right? Anything else you simply compare and bail out if it's not the same as what has been asked. > > > Shouldn't be rather > > > > if (atomic_cmpxchg() == 0) { > > if (atomic_read() != version) > > return ...; > > } > > > > My algorithm allows for multiple callers requesting the same version > to all succeed. Yours would fail the first conditional for all but > the first, and you haven't provided an else for that case... > > ... but it would probably look the same so the conditional is pointless, > or it would reject the request - which would be wrong. > > > But here is still the question: why do you expect the version to be > > changed on background? And what about barriers? > > > > While it is unlikely that userspace will be attempting to use both ABI > versions simultaneously on the same chip request, it is a possiblity and > so needs to be protected against. And better to have the watch request > fail than the read fail or worse - return the wrong struct version. > > The atomic_cmpxchg() is sufficient for this algorithm - no barriers > required. It could also be written with a spinlock but I was trying to > avoid locks unless they were absolutely necessary. A spinlock version > may arguably be more readable, but it would certainly be more verbose, > larger and slower. > > I'm happy to add some documentation to the function if that would help. Yes, I guess documentation is what is eagerly needed here. > > > + return 0; > > > +} > > > +#endif > > > + > > > +static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip, > > > + bool watch) > > > +{ > > > + struct gpio_desc *desc; > > > + struct gpio_v2_line_info lineinfo; > > > + > > > + if (copy_from_user(&lineinfo, ip, sizeof(lineinfo))) > > > + return -EFAULT; > > > + > > > + if (memchr_inv(lineinfo.padding, 0, sizeof(lineinfo.padding))) > > > + return -EINVAL; > > > + > > > + desc = gpiochip_get_desc(cdev->gdev->chip, lineinfo.offset); > > > + if (IS_ERR(desc)) > > > + return PTR_ERR(desc); > > > + > > > + if (watch) { > > > +#ifdef CONFIG_GPIO_CDEV_V1 > > > > > + if (lineinfo_ensure_abi_version(cdev, 2)) > > > + return -EPERM; > > > > Can't you propagate error code from the function? > > > > You mean: > + ret = lineinfo_ensure_abi_version(cdev, 2) > + if (ret) > + return ret; > > That seems more verbose and less clear. And I'd need to conditionally > declare a ret - as this test is compiled out if CDEV_V1 is not defined. > > I did flip-flop on what lineinfo_ensure_abi_version() should return - > either a bool or an error code. > > If a bool then the code would include the dreaded negative conditional > ;-(: > > + if (!lineinfo_is_abi_version(cdev, 2)) > + return -EPERM; > > so I eventually settled for the error code. But I'm on the fence on > this one and happy to change it if you think the bool form is clearer. > > Cheers, > Kent.
On Thu, Sep 24, 2020 at 11:39:03AM +0300, Andy Shevchenko wrote: > On Thu, Sep 24, 2020 at 5:39 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Wed, Sep 23, 2020 at 06:41:45PM +0300, Andy Shevchenko wrote: > > > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@gmail.com> wrote: > > ... > [snip] > > > > > +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); > > > > > > atomic_cmpxchng() returns a value. > > > > Yep, it returns the old value - which we don't care about - see below. > > Then what's the point to read back?.. > > > > Also there are no barriers here... > > > > > > > No barriers required - the atomic_cmpxchg() is sufficient. > > > > > > + } > > > > + if (abiv != version) > > > > + return -EPERM; > > > > > > I'm not sure I understand why this is atomic. > > > > > > > The algorithm requires some level of protection and atomic is > > sufficient. > > > > > Also this seems to be racy if cdata changed in background. > > > > > > > Can you provide a case? > > CPU0: CPU1: > xchg() ... > ... xchg() > ... read() -> OK > read() ->NOK > Lets say CPU0 is setting 1 and CPU1 setting 2, and assuming the xchg() completes... Your case is not possible - CPU1 would see the value 1 set by CPU0 in the read() and so NOK. Its xchg() would fail as it compares against 0 and that also sees the 1 and so fails. What am I missing? > > The atomic_cmpxchg() ensures cdata->watch_abi_version is only set > > once - first in wins. The atomic_read() is so we can check that > > the set version matches what the caller wants. > > Note that multiple callers may request the same version - and all > > should succeed. > > So, that's basically what you need when using _old_ value. > > 0 means you were first, right? > Anything else you simply compare and bail out if it's not the same as > what has been asked. > Could you provide a complete implementation that behaves as I expect, rather than snippets and verbage? > > > > > Shouldn't be rather > > > > > > if (atomic_cmpxchg() == 0) { > > > if (atomic_read() != version) > > > return ...; > > > } > > > > > > > My algorithm allows for multiple callers requesting the same version > > to all succeed. Yours would fail the first conditional for all but > > the first, and you haven't provided an else for that case... > > > > ... but it would probably look the same so the conditional is pointless, > > or it would reject the request - which would be wrong. > > > > > But here is still the question: why do you expect the version to be > > > changed on background? And what about barriers? > > > > > > > While it is unlikely that userspace will be attempting to use both ABI > > versions simultaneously on the same chip request, it is a possiblity and > > so needs to be protected against. And better to have the watch request > > fail than the read fail or worse - return the wrong struct version. > > > > The atomic_cmpxchg() is sufficient for this algorithm - no barriers > > required. It could also be written with a spinlock but I was trying to > > avoid locks unless they were absolutely necessary. A spinlock version > > may arguably be more readable, but it would certainly be more verbose, > > larger and slower. > > > > I'm happy to add some documentation to the function if that would help. > > Yes, I guess documentation is what is eagerly needed here. > No problem. Cheers, Kent.
On Wed, Sep 23, 2020 at 06:41:45PM +0300, Andy Shevchenko wrote: > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > Add support for GPIO_V2_GET_LINEINFO_IOCTL and > > GPIO_V2_GET_LINEINFO_WATCH_IOCTL. > > > > +#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); > > atomic_cmpxchng() returns a value. > Also there are no barriers here... > > > + } > > + if (abiv != version) > > + return -EPERM; > > I'm not sure I understand why this is atomic. > > Also this seems to be racy if cdata changed in background. > > Shouldn't be rather > > if (atomic_cmpxchg() == 0) { > if (atomic_read() != version) > return ...; > } > Perhaps this is what you had in mind: static bool lineinfo_is_abi_version(struct gpio_chardev_data *cdata, unsigned int version) { int abiv = atomic_cmpxchg(&cdata->watch_abi_version, 0, version); if (abiv == version) return true; if (abiv == 0) /* first (and only) setter sees initial 0 value */ return true; return false; } That is functionally equivalent, but slightly shorter. I was thinking the atomic_cmpxchg() is more expensive than an atomic_read(), and so initially checked the value with that, but for the cmp failure case it is probably the same, or at least not worth the bother - this isn't even vaguely near a hot path. I've also switched the return value to bool in response to your other comments. Cheers, Kent.
On Thu, Sep 24, 2020 at 12:48 PM Kent Gibson <warthog618@gmail.com> wrote: > On Thu, Sep 24, 2020 at 11:39:03AM +0300, Andy Shevchenko wrote: > > On Thu, Sep 24, 2020 at 5:39 AM Kent Gibson <warthog618@gmail.com> wrote: > > > On Wed, Sep 23, 2020 at 06:41:45PM +0300, Andy Shevchenko wrote: > > > > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@gmail.com> wrote: ... > > > > > +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); > > > > > > > > atomic_cmpxchng() returns a value. > > > > > > Yep, it returns the old value - which we don't care about - see below. > > > > Then what's the point to read back?.. > > > > > > Also there are no barriers here... > > > > > > > > > > No barriers required - the atomic_cmpxchg() is sufficient. > > > > > > > > + } > > > > > + if (abiv != version) > > > > > + return -EPERM; > > > > > > > > I'm not sure I understand why this is atomic. > > > > > > > > > > The algorithm requires some level of protection and atomic is > > > sufficient. > > > > > > > Also this seems to be racy if cdata changed in background. > > > > > > > > > > Can you provide a case? > > > > CPU0: CPU1: > > xchg() ... > > ... xchg() > > ... read() -> OK > > read() ->NOK > > > > Lets say CPU0 is setting 1 and CPU1 setting 2, and assuming the xchg() > completes... > Your case is not possible - CPU1 would see the value 1 set by CPU0 in the > read() and so NOK. Its xchg() would fail as it compares against 0 > and that also sees the 1 and so fails. > > What am I missing? Barriers? That's what documentation says about xchg(). https://stackoverflow.com/q/20950603/2511795 > > > The atomic_cmpxchg() ensures cdata->watch_abi_version is only set > > > once - first in wins. The atomic_read() is so we can check that > > > the set version matches what the caller wants. > > > Note that multiple callers may request the same version - and all > > > should succeed. > > > > So, that's basically what you need when using _old_ value. > > > > 0 means you were first, right? > > Anything else you simply compare and bail out if it's not the same as > > what has been asked. > > > > Could you provide a complete implementation that behaves as I expect, > rather than snippets and verbage? if (atomic_cmpxchg(&cdata..., version) == 0) return 0; // we were first! return -EPERM; // somebody has changed the version before us! > > > > Shouldn't be rather > > > > > > > > if (atomic_cmpxchg() == 0) { > > > > if (atomic_read() != version) > > > > return ...; > > > > } > > > > > > > > > > My algorithm allows for multiple callers requesting the same version > > > to all succeed. Yours would fail the first conditional for all but > > > the first, and you haven't provided an else for that case... > > > > > > ... but it would probably look the same so the conditional is pointless, > > > or it would reject the request - which would be wrong. > > > > > > > But here is still the question: why do you expect the version to be > > > > changed on background? And what about barriers? > > > > > > > > > > While it is unlikely that userspace will be attempting to use both ABI > > > versions simultaneously on the same chip request, it is a possiblity and > > > so needs to be protected against. And better to have the watch request > > > fail than the read fail or worse - return the wrong struct version. > > > > > > The atomic_cmpxchg() is sufficient for this algorithm - no barriers > > > required. It could also be written with a spinlock but I was trying to > > > avoid locks unless they were absolutely necessary. A spinlock version > > > may arguably be more readable, but it would certainly be more verbose, > > > larger and slower. > > > > > > I'm happy to add some documentation to the function if that would help. > > > > Yes, I guess documentation is what is eagerly needed here. > > > > No problem.
On Fri, Sep 25, 2020 at 01:12:14PM +0300, Andy Shevchenko wrote: > On Thu, Sep 24, 2020 at 12:48 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Thu, Sep 24, 2020 at 11:39:03AM +0300, Andy Shevchenko wrote: > > > On Thu, Sep 24, 2020 at 5:39 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Wed, Sep 23, 2020 at 06:41:45PM +0300, Andy Shevchenko wrote: > > > > > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@gmail.com> wrote: > [snip] > > > > Lets say CPU0 is setting 1 and CPU1 setting 2, and assuming the xchg() > > completes... > > Your case is not possible - CPU1 would see the value 1 set by CPU0 in the > > read() and so NOK. Its xchg() would fail as it compares against 0 > > and that also sees the 1 and so fails. > > > > What am I missing? > > Barriers? That's what documentation says about xchg(). > https://stackoverflow.com/q/20950603/2511795 > Firstly, the answer in Stackoverflow is from someone who explicitly acknowledges not being a kernel developer, so they aren't sure. Secondly, the latest version of the kernel doc [1] says differently than what is quoted on Stackoverlow - it indicates implementations of atomic_cmpxchg() must provide its own memory barriers. The relevant section says "This performs an atomic compare exchange operation on the atomic value v, with the given old and new values. Like all atomic_xxx operations, atomic_cmpxchg will only satisfy its atomicity semantics as long as all other accesses of *v are performed through atomic_xxx operations. atomic_cmpxchg must provide explicit memory barriers around the operation, although if the comparison fails then no memory ordering guarantees are required." Note that this doc is aimed at atomic_cmpxchg() implementors, so I took that to mean the operation itself must provide the barriers - not the caller. Also, the sentence only makes sense wrt the atomic_cmpxchg() implementation - the caller can't decide on memory barriers if the comparison fails or not. The memory-barriers.txt they quote is also dated - the atomic section they quote is moved to atomic_t.txt[2]? That says that cmpxchg is a RMW op, and that it will perform an ACQUIRE and RELEASE - for the non-failure case anyway. Again, I took that to mean it will provide the barriers itself. And even the old text they quote says those operations IMPLY a memory barrier, "Any atomic operation that modifies some state in memory and returns information about the state (old or new) implies an SMP-conditional general memory barrier (smp_mb()) on each side of the actual operation" and that "the implicit memory barrier effects are necessary". Again that indicates the barrier is a part of the op, as it is implicit, and not necessary to be added separately. > > > > The atomic_cmpxchg() ensures cdata->watch_abi_version is only set > > > > once - first in wins. The atomic_read() is so we can check that > > > > the set version matches what the caller wants. > > > > Note that multiple callers may request the same version - and all > > > > should succeed. > > > > > > So, that's basically what you need when using _old_ value. > > > > > > 0 means you were first, right? > > > Anything else you simply compare and bail out if it's not the same as > > > what has been asked. > > > > > > > Could you provide a complete implementation that behaves as I expect, > > rather than snippets and verbage? > > if (atomic_cmpxchg(&cdata..., version) == 0) > return 0; // we were first! > return -EPERM; // somebody has changed the version before us! > Which can fail if two callers are requesting the same version - in a race the second one will get a fail - independent of the version they are requesting. I keep flip-flopping and twiddling with the implementation of this - my current one is: /* * returns 0 if the versions match, else the previously selected ABI version */ static int lineinfo_ensure_abi_version(struct gpio_chardev_data *cdata, unsigned int version) { int abiv = atomic_cmpxchg(&cdata->watch_abi_version, 0, version); if (abiv == version) return 0; return abiv; } Does that work for you? (assuming no explicit barriers are necessary) Cheers, Kent. [1] https://www.kernel.org/doc/html/v5.8/core-api/atomic_ops.html [2] https://www.kernel.org/doc/Documentation/atomic_t.txt
On Fri, Sep 25, 2020 at 2:56 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Fri, Sep 25, 2020 at 01:12:14PM +0300, Andy Shevchenko wrote: > > On Thu, Sep 24, 2020 at 12:48 PM Kent Gibson <warthog618@gmail.com> wrote: > > > On Thu, Sep 24, 2020 at 11:39:03AM +0300, Andy Shevchenko wrote: > > > > On Thu, Sep 24, 2020 at 5:39 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > On Wed, Sep 23, 2020 at 06:41:45PM +0300, Andy Shevchenko wrote: > > > > > > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > [snip] > > > > > > Lets say CPU0 is setting 1 and CPU1 setting 2, and assuming the xchg() > > > completes... > > > Your case is not possible - CPU1 would see the value 1 set by CPU0 in the > > > read() and so NOK. Its xchg() would fail as it compares against 0 > > > and that also sees the 1 and so fails. > > > > > > What am I missing? > > > > Barriers? That's what documentation says about xchg(). > > https://stackoverflow.com/q/20950603/2511795 > > > > Firstly, the answer in Stackoverflow is from someone who explicitly > acknowledges not being a kernel developer, so they aren't sure. > > Secondly, the latest version of the kernel doc [1] says differently than what > is quoted on Stackoverlow - it indicates implementations of atomic_cmpxchg() > must provide its own memory barriers. > > The relevant section says "This performs an atomic compare exchange operation > on the atomic value v, with the given old and new values. Like all atomic_xxx > operations, atomic_cmpxchg will only satisfy its atomicity semantics as long > as all other accesses of *v are performed through atomic_xxx operations. > > atomic_cmpxchg must provide explicit memory barriers around the operation, > although if the comparison fails then no memory ordering guarantees are required." > > Note that this doc is aimed at atomic_cmpxchg() implementors, so I took > that to mean the operation itself must provide the barriers - not > the caller. Also, the sentence only makes sense wrt the > atomic_cmpxchg() implementation - the caller can't decide on memory barriers > if the comparison fails or not. > > The memory-barriers.txt they quote is also dated - the atomic section they quote > is moved to atomic_t.txt[2]? > That says that cmpxchg is a RMW op, and that it will perform an > ACQUIRE and RELEASE - for the non-failure case anyway. > > Again, I took that to mean it will provide the barriers itself. > > And even the old text they quote says those operations IMPLY a memory barrier, > "Any atomic operation that modifies some state in memory and returns > information about the state (old or new) implies an SMP-conditional > general memory barrier (smp_mb()) on each side of the actual operation" > and that "the implicit memory barrier effects are necessary". > > Again that indicates the barrier is a part of the op, as it is implicit, > and not necessary to be added separately. Okay! Thanks for digging into it. > > > > > The atomic_cmpxchg() ensures cdata->watch_abi_version is only set > > > > > once - first in wins. The atomic_read() is so we can check that > > > > > the set version matches what the caller wants. > > > > > Note that multiple callers may request the same version - and all > > > > > should succeed. > > > > > > > > So, that's basically what you need when using _old_ value. > > > > > > > > 0 means you were first, right? > > > > Anything else you simply compare and bail out if it's not the same as > > > > what has been asked. > > > > > > > > > > Could you provide a complete implementation that behaves as I expect, > > > rather than snippets and verbage? > > > > if (atomic_cmpxchg(&cdata..., version) == 0) > > return 0; // we were first! > > return -EPERM; // somebody has changed the version before us! > > > > Which can fail if two callers are requesting the same version - in a > race the second one will get a fail - independent of the version they > are requesting. > > I keep flip-flopping and twiddling with the implementation of this - > my current one is: > > /* > * returns 0 if the versions match, else the previously selected ABI version > */ > static int lineinfo_ensure_abi_version(struct gpio_chardev_data *cdata, > unsigned int version) > { > int abiv = atomic_cmpxchg(&cdata->watch_abi_version, 0, version); > > if (abiv == version) > return 0; > > return abiv; > } > > > Does that work for you? (assuming no explicit barriers are necessary) Perfectly! > [1] https://www.kernel.org/doc/html/v5.8/core-api/atomic_ops.html > [2] https://www.kernel.org/doc/Documentation/atomic_t.txt
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index 7a3ed2617f74..d3857113f58c 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -181,7 +181,8 @@ static long linehandle_set_config(struct linehandle_state *lh, } blocking_notifier_call_chain(&desc->gdev->notifier, - GPIOLINE_CHANGED_CONFIG, desc); + GPIO_V2_LINE_CHANGED_CONFIG, + desc); } return 0; } @@ -353,7 +354,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) } blocking_notifier_call_chain(&desc->gdev->notifier, - GPIOLINE_CHANGED_REQUESTED, desc); + GPIO_V2_LINE_CHANGED_REQUESTED, desc); dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", offset); @@ -747,7 +748,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip) } blocking_notifier_call_chain(&desc->gdev->notifier, - GPIOLINE_CHANGED_REQUESTED, desc); + GPIO_V2_LINE_CHANGED_REQUESTED, desc); dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", offset); @@ -1094,7 +1095,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip) goto out_free_le; blocking_notifier_call_chain(&desc->gdev->notifier, - GPIOLINE_CHANGED_REQUESTED, desc); + GPIO_V2_LINE_CHANGED_REQUESTED, desc); irq = gpiod_to_irq(desc); if (irq <= 0) { @@ -1161,17 +1162,59 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip) return ret; } +static void gpio_v2_line_info_to_v1(struct gpio_v2_line_info *info_v2, + struct gpioline_info *info_v1) +{ + u64 flagsv2 = info_v2->flags; + + memcpy(info_v1->name, info_v2->name, sizeof(info_v1->name)); + memcpy(info_v1->consumer, info_v2->consumer, + sizeof(info_v1->consumer)); + info_v1->line_offset = info_v2->offset; + info_v1->flags = 0; + + if (flagsv2 & GPIO_V2_LINE_FLAG_USED) + info_v1->flags |= GPIOLINE_FLAG_KERNEL; + + if (flagsv2 & GPIO_V2_LINE_FLAG_OUTPUT) + info_v1->flags |= GPIOLINE_FLAG_IS_OUT; + + if (flagsv2 & GPIO_V2_LINE_FLAG_ACTIVE_LOW) + info_v1->flags |= GPIOLINE_FLAG_ACTIVE_LOW; + + if (flagsv2 & GPIO_V2_LINE_FLAG_OPEN_DRAIN) + info_v1->flags |= GPIOLINE_FLAG_OPEN_DRAIN; + if (flagsv2 & GPIO_V2_LINE_FLAG_OPEN_SOURCE) + info_v1->flags |= GPIOLINE_FLAG_OPEN_SOURCE; + + if (flagsv2 & GPIO_V2_LINE_FLAG_BIAS_PULL_UP) + info_v1->flags |= GPIOLINE_FLAG_BIAS_PULL_UP; + if (flagsv2 & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN) + info_v1->flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN; + if (flagsv2 & GPIO_V2_LINE_FLAG_BIAS_DISABLED) + info_v1->flags |= GPIOLINE_FLAG_BIAS_DISABLE; +} + +static void gpio_v2_line_info_changed_to_v1( + struct gpio_v2_line_info_changed *lic_v2, + struct gpioline_info_changed *lic_v1) +{ + gpio_v2_line_info_to_v1(&lic_v2->info, &lic_v1->info); + lic_v1->timestamp = lic_v2->timestamp_ns; + 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 gpio_v2_line_info *info) { struct gpio_chip *gc = desc->gdev->chip; bool ok_for_pinctrl; unsigned long flags; memset(info, 0, sizeof(*info)); - info->line_offset = gpio_chip_hwgpio(desc); + info->offset = gpio_chip_hwgpio(desc); /* * This function takes a mutex so we must check this before taking @@ -1181,7 +1224,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); @@ -1202,23 +1245,27 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc, test_bit(FLAG_EXPORT, &desc->flags) || test_bit(FLAG_SYSFS, &desc->flags) || !ok_for_pinctrl) - info->flags |= GPIOLINE_FLAG_KERNEL; + info->flags |= GPIO_V2_LINE_FLAG_USED; + if (test_bit(FLAG_IS_OUT, &desc->flags)) - info->flags |= GPIOLINE_FLAG_IS_OUT; + info->flags |= GPIO_V2_LINE_FLAG_OUTPUT; + else + info->flags |= GPIO_V2_LINE_FLAG_INPUT; + if (test_bit(FLAG_ACTIVE_LOW, &desc->flags)) - info->flags |= GPIOLINE_FLAG_ACTIVE_LOW; + info->flags |= GPIO_V2_LINE_FLAG_ACTIVE_LOW; + if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) - info->flags |= (GPIOLINE_FLAG_OPEN_DRAIN | - GPIOLINE_FLAG_IS_OUT); + info->flags |= GPIO_V2_LINE_FLAG_OPEN_DRAIN; if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) - info->flags |= (GPIOLINE_FLAG_OPEN_SOURCE | - GPIOLINE_FLAG_IS_OUT); + info->flags |= GPIO_V2_LINE_FLAG_OPEN_SOURCE; + if (test_bit(FLAG_BIAS_DISABLE, &desc->flags)) - info->flags |= GPIOLINE_FLAG_BIAS_DISABLE; + info->flags |= GPIO_V2_LINE_FLAG_BIAS_DISABLED; if (test_bit(FLAG_PULL_DOWN, &desc->flags)) - info->flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN; + info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN; if (test_bit(FLAG_PULL_UP, &desc->flags)) - info->flags |= GPIOLINE_FLAG_BIAS_PULL_UP; + info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_UP; spin_unlock_irqrestore(&gpio_lock, flags); } @@ -1226,11 +1273,65 @@ 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 gpio_v2_line_info_changed, 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 *cdev, void __user *ip, + bool watch) +{ + struct gpio_desc *desc; + struct gpio_v2_line_info lineinfo; + + if (copy_from_user(&lineinfo, ip, sizeof(lineinfo))) + return -EFAULT; + + if (memchr_inv(lineinfo.padding, 0, sizeof(lineinfo.padding))) + return -EINVAL; + + desc = gpiochip_get_desc(cdev->gdev->chip, lineinfo.offset); + if (IS_ERR(desc)) + return PTR_ERR(desc); + + if (watch) { +#ifdef CONFIG_GPIO_CDEV_V1 + if (lineinfo_ensure_abi_version(cdev, 2)) + return -EPERM; +#endif + if (test_and_set_bit(lineinfo.offset, cdev->watched_lines)) + return -EBUSY; + } + gpio_desc_to_lineinfo(desc, &lineinfo); + + if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) { + if (watch) + clear_bit(lineinfo.offset, cdev->watched_lines); + return -EFAULT; + } + + return 0; +} + /* * gpio_ioctl() - ioctl handler for the GPIO chardev */ @@ -1240,7 +1341,6 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg) struct gpio_device *gdev = cdev->gdev; struct gpio_chip *gc = gdev->chip; void __user *ip = (void __user *)arg; - struct gpio_desc *desc; __u32 offset; /* We fail any subsequent ioctl():s when the chip is gone */ @@ -1263,7 +1363,9 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg) return 0; #ifdef CONFIG_GPIO_CDEV_V1 } else if (cmd == GPIO_GET_LINEINFO_IOCTL) { + struct gpio_desc *desc; struct gpioline_info lineinfo; + struct gpio_v2_line_info lineinfo_v2; if (copy_from_user(&lineinfo, ip, sizeof(lineinfo))) return -EFAULT; @@ -1273,7 +1375,8 @@ 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); + gpio_desc_to_lineinfo(desc, &lineinfo_v2); + gpio_v2_line_info_to_v1(&lineinfo_v2, &lineinfo); if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) return -EFAULT; @@ -1283,7 +1386,9 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg) } else if (cmd == GPIO_GET_LINEEVENT_IOCTL) { return lineevent_create(gdev, ip); } else if (cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) { + struct gpio_desc *desc; struct gpioline_info lineinfo; + struct gpio_v2_line_info lineinfo_v2; if (copy_from_user(&lineinfo, ip, sizeof(lineinfo))) return -EFAULT; @@ -1293,10 +1398,14 @@ 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(cdev, 1)) + return -EPERM; + if (test_and_set_bit(lineinfo.line_offset, cdev->watched_lines)) return -EBUSY; - gpio_desc_to_lineinfo(desc, &lineinfo); + gpio_desc_to_lineinfo(desc, &lineinfo_v2); + gpio_v2_line_info_to_v1(&lineinfo_v2, &lineinfo); if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) { clear_bit(lineinfo.line_offset, cdev->watched_lines); @@ -1305,6 +1414,10 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg) return 0; #endif /* CONFIG_GPIO_CDEV_V1 */ + } else if (cmd == GPIO_V2_GET_LINEINFO_IOCTL || + cmd == GPIO_V2_GET_LINEINFO_WATCH_IOCTL) { + return lineinfo_get(cdev, ip, + cmd == GPIO_V2_GET_LINEINFO_WATCH_IOCTL); } else if (cmd == GPIO_V2_GET_LINE_IOCTL) { return linereq_create(gdev, ip); } else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) { @@ -1340,7 +1453,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb, unsigned long action, void *data) { struct gpio_chardev_data *cdev = to_gpio_chardev_data(nb); - struct gpioline_info_changed chg; + struct gpio_v2_line_info_changed chg; struct gpio_desc *desc = data; int ret; @@ -1349,7 +1462,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb, memset(&chg, 0, sizeof(chg)); chg.event_type = action; - chg.timestamp = ktime_get_ns(); + chg.timestamp_ns = ktime_get_ns(); gpio_desc_to_lineinfo(desc, &chg.info); ret = kfifo_in_spinlocked(&cdev->events, &chg, 1, &cdev->wait.lock); @@ -1380,12 +1493,16 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf, size_t count, loff_t *off) { struct gpio_chardev_data *cdev = file->private_data; - struct gpioline_info_changed event; + struct gpio_v2_line_info_changed 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 gpio_v2_line_info_changed); + if (count < event_size) return -EINVAL; +#endif do { spin_lock(&cdev->wait.lock); @@ -1407,7 +1524,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(&cdev->watch_abi_version) == 2) + event_size = sizeof(struct gpio_v2_line_info_changed); + else + event_size = sizeof(struct gpioline_info_changed); + if (count < event_size) { + spin_unlock(&cdev->wait.lock); + return -EINVAL; + } +#endif ret = kfifo_out(&cdev->events, &event, 1); spin_unlock(&cdev->wait.lock); if (ret != 1) { @@ -1416,9 +1543,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 (event_size == sizeof(struct gpio_v2_line_info_changed)) { + if (copy_to_user(buf + bytes_read, &event, event_size)) + return -EFAULT; + } else { + struct gpioline_info_changed event_v1; + + gpio_v2_line_info_changed_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;
Add support for GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL. The core of this change is the event kfifo switching to contain struct gpioline_info_changed_v2, instead of v1 as v2 is richer. 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_V2_GET_LINEINFO_WATCH_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. Signed-off-by: Kent Gibson <warthog618@gmail.com> --- Changes for v5: - as per cover letter Changes for v4: - replace strncpy with memcpy in gpio_v2_line_info_to_v1 drivers/gpio/gpiolib-cdev.c | 197 +++++++++++++++++++++++++++++++----- 1 file changed, 169 insertions(+), 28 deletions(-)