* [PATCH v1 2/2] gpiolib: Make use of assign_bit() API
2019-12-04 19:42 [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode Andy Shevchenko
@ 2019-12-04 19:42 ` Andy Shevchenko
2019-12-13 10:03 ` Linus Walleij
2019-12-06 10:57 ` [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode Bartosz Golaszewski
2019-12-10 9:06 ` Bartosz Golaszewski
2 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2019-12-04 19:42 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, linux-gpio; +Cc: Andy Shevchenko
We have for some time the assign_bit() API to replace open coded
if (foo)
set_bit(n, bar);
else
clear_bit(n, bar);
Use this API in GPIO library code.
No functional change intended.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/gpio/gpiolib.c | 59 ++++++++++++++----------------------------
1 file changed, 20 insertions(+), 39 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 134985210619..b332121da4b5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -224,15 +224,15 @@ int gpiod_get_direction(struct gpio_desc *desc)
return -ENOTSUPP;
ret = chip->get_direction(chip, offset);
- if (ret > 0) {
- /* GPIOF_DIR_IN, or other positive */
+ if (ret < 0)
+ return ret;
+
+ /* GPIOF_DIR_IN or other positive, otherwise GPIOF_DIR_OUT */
+ if (ret > 0)
ret = 1;
- clear_bit(FLAG_IS_OUT, &desc->flags);
- }
- if (ret == 0) {
- /* GPIOF_DIR_OUT */
- set_bit(FLAG_IS_OUT, &desc->flags);
- }
+
+ assign_bit(FLAG_IS_OUT, &desc->flags, !ret);
+
return ret;
}
EXPORT_SYMBOL_GPL(gpiod_get_direction);
@@ -484,15 +484,6 @@ static int linehandle_validate_flags(u32 flags)
return 0;
}
-static void linehandle_configure_flag(unsigned long *flagsp,
- u32 bit, bool active)
-{
- if (active)
- set_bit(bit, flagsp);
- else
- clear_bit(bit, flagsp);
-}
-
static long linehandle_set_config(struct linehandle_state *lh,
void __user *ip)
{
@@ -514,22 +505,22 @@ static long linehandle_set_config(struct linehandle_state *lh,
desc = lh->descs[i];
flagsp = &desc->flags;
- linehandle_configure_flag(flagsp, FLAG_ACTIVE_LOW,
+ assign_bit(FLAG_ACTIVE_LOW, flagsp,
lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW);
- linehandle_configure_flag(flagsp, FLAG_OPEN_DRAIN,
+ assign_bit(FLAG_OPEN_DRAIN, flagsp,
lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN);
- linehandle_configure_flag(flagsp, FLAG_OPEN_SOURCE,
+ assign_bit(FLAG_OPEN_SOURCE, flagsp,
lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE);
- linehandle_configure_flag(flagsp, FLAG_PULL_UP,
+ assign_bit(FLAG_PULL_UP, flagsp,
lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP);
- linehandle_configure_flag(flagsp, FLAG_PULL_DOWN,
+ assign_bit(FLAG_PULL_DOWN, flagsp,
lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN);
- linehandle_configure_flag(flagsp, FLAG_BIAS_DISABLE,
+ assign_bit(FLAG_BIAS_DISABLE, flagsp,
lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE);
/*
@@ -1561,15 +1552,11 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
struct gpio_desc *desc = &gdev->descs[i];
if (chip->get_direction && gpiochip_line_is_valid(chip, i)) {
- if (!chip->get_direction(chip, i))
- set_bit(FLAG_IS_OUT, &desc->flags);
- else
- clear_bit(FLAG_IS_OUT, &desc->flags);
+ assign_bit(FLAG_IS_OUT,
+ &desc->flags, !chip->get_direction(chip, i));
} else {
- if (!chip->direction_input)
- set_bit(FLAG_IS_OUT, &desc->flags);
- else
- clear_bit(FLAG_IS_OUT, &desc->flags);
+ assign_bit(FLAG_IS_OUT,
+ &desc->flags, !chip->direction_input);
}
}
@@ -3371,10 +3358,7 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
* Handle FLAG_TRANSITORY first, enabling queries to gpiolib for
* persistence state.
*/
- if (transitory)
- set_bit(FLAG_TRANSITORY, &desc->flags);
- else
- clear_bit(FLAG_TRANSITORY, &desc->flags);
+ assign_bit(FLAG_TRANSITORY, &desc->flags, transitory);
/* If the driver supports it, set the persistence state now */
chip = desc->gdev->chip;
@@ -3830,10 +3814,7 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
gpio_set_open_source_value_commit(desc, value);
} else {
__set_bit(hwgpio, mask);
- if (value)
- __set_bit(hwgpio, bits);
- else
- __clear_bit(hwgpio, bits);
+ __assign_bit(hwgpio, bits, value);
count++;
}
i++;
--
2.24.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v1 2/2] gpiolib: Make use of assign_bit() API
2019-12-04 19:42 ` [PATCH v1 2/2] gpiolib: Make use of assign_bit() API Andy Shevchenko
@ 2019-12-13 10:03 ` Linus Walleij
0 siblings, 0 replies; 12+ messages in thread
From: Linus Walleij @ 2019-12-13 10:03 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM
On Wed, Dec 4, 2019 at 8:42 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> We have for some time the assign_bit() API to replace open coded
>
> if (foo)
> set_bit(n, bar);
> else
> clear_bit(n, bar);
>
> Use this API in GPIO library code.
>
> No functional change intended.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Patch applied to the GPIO tree!
Thanks,
Linus Walleij
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode
2019-12-04 19:42 [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode Andy Shevchenko
2019-12-04 19:42 ` [PATCH v1 2/2] gpiolib: Make use of assign_bit() API Andy Shevchenko
@ 2019-12-06 10:57 ` Bartosz Golaszewski
2019-12-10 9:06 ` Bartosz Golaszewski
2 siblings, 0 replies; 12+ messages in thread
From: Bartosz Golaszewski @ 2019-12-06 10:57 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Linus Walleij, linux-gpio
śr., 4 gru 2019 o 20:42 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> The introduced line even handling ABI in the commit
>
> 61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
>
> missed the fact that 64-bit kernel may serve for 32-bit applications.
> In such case the very first check in the lineevent_read() will fail
> due to alignment differences.
>
> To workaround this we do several things here:
> - put warning comment to UAPI header near to the structure description
> - derive the size of the structure in the compatible mode from its members
> - check for the size of this structure in the ->read() callback
> - return only one event in the compatible mode at a time
>
> Above mitigation will work at least with libgpiod which does one event
> at a time.
>
> Since the bug hasn't been reported earlier we assume that there is close
> to zero actual users of the compatible mode to monitor GPIO events and thus
> we might consider to rework this ABI in the future.
>
> Fixes: 61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/gpio/gpiolib.c | 51 ++++++++++++++++++++++++++++++++++++---
> include/uapi/linux/gpio.h | 6 +++++
> 2 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 7340e4d0e873..134985210619 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -825,17 +825,26 @@ static __poll_t lineevent_poll(struct file *filep,
> return events;
> }
>
> -
> static ssize_t lineevent_read(struct file *filep,
> char __user *buf,
> size_t count,
> loff_t *f_ps)
> {
> struct lineevent_state *le = filep->private_data;
> + struct gpioevent_data event, *e = &event;
> + /* We need size of each member to avoid endianess issues below */
> + size_t ts_sz = sizeof(e->timestamp), id_sz = sizeof(e->id), e_sz;
> unsigned int copied;
> int ret;
>
> - if (count < sizeof(struct gpioevent_data))
> + /*
> + * In compatible mode, when kernel is 64-bit and user space is 32-bit,
> + * we may not tell what user wanted here when count is bigger than size
> + * of one event, so, we just assume that user asks for precisely one
> + * event.
> + */
> + e_sz = in_compat_syscall() ? ts_sz + id_sz : sizeof(*e);
> + if (count < e_sz)
> return -EINVAL;
>
> do {
> @@ -851,7 +860,43 @@ static ssize_t lineevent_read(struct file *filep,
>
> if (mutex_lock_interruptible(&le->read_lock))
> return -ERESTARTSYS;
> - ret = kfifo_to_user(&le->events, buf, count, &copied);
> + if (in_compat_syscall()) {
> + /*
> + * First we peek the one event and, if there is
> + * no error during copying to user space, skip it
> + * later.
> + */
> + if (kfifo_peek(&le->events, e))
> + copied = e_sz;
> + else
> + copied = 0;
> +
> + /* Do not try to copy garbage to the user */
> + ret = copied ? 0 : -EFAULT;
> +
> + /*
> + * Due to endianess concerns we have to copy this
> + * member-by-member. Luckily there are no members
> + * less than 32-bit.
> + */
> + if (!ret)
> + ret = copy_to_user(buf, &e->timestamp, ts_sz);
> + if (!ret)
> + ret = copy_to_user(buf + ts_sz, &e->id, id_sz);
> +
> + if (ret) {
> + /*
> + * Either we have got nothing from the FIFO or
> + * one of copy_to_user() calls failed.
> + */
> + ret = -EFAULT;
> + } else {
> + /* Skip peeked event if no error happened */
> + kfifo_skip(&le->events);
> + }
> + } else {
> + ret = kfifo_to_user(&le->events, buf, count, &copied);
> + }
> mutex_unlock(&le->read_lock);
>
> if (ret)
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 799cf823d493..054756bf6991 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -168,6 +168,12 @@ 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
> + *
> + * Warning! This structure has issues in the compatible mode, when
> + * kernel is 64-bit and user space is 32-bit, due to alignment
> + * differences.
> + *
> + * It's not recommended to retrieve more than one event at a time.
> */
> struct gpioevent_data {
> __u64 timestamp;
> --
> 2.24.0
>
Linus,
please don't apply the patches from this series - I will incorporate
them into my LINEINFO_WATCH ioctl() series.
Best regards,
Bartosz Golaszewski
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode
2019-12-04 19:42 [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode Andy Shevchenko
2019-12-04 19:42 ` [PATCH v1 2/2] gpiolib: Make use of assign_bit() API Andy Shevchenko
2019-12-06 10:57 ` [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode Bartosz Golaszewski
@ 2019-12-10 9:06 ` Bartosz Golaszewski
2019-12-10 13:44 ` Andy Shevchenko
2019-12-10 14:39 ` Kent Gibson
2 siblings, 2 replies; 12+ messages in thread
From: Bartosz Golaszewski @ 2019-12-10 9:06 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM
śr., 4 gru 2019 o 20:42 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> The introduced line even handling ABI in the commit
>
> 61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
>
> missed the fact that 64-bit kernel may serve for 32-bit applications.
> In such case the very first check in the lineevent_read() will fail
> due to alignment differences.
>
> To workaround this we do several things here:
> - put warning comment to UAPI header near to the structure description
> - derive the size of the structure in the compatible mode from its members
> - check for the size of this structure in the ->read() callback
> - return only one event in the compatible mode at a time
>
> Above mitigation will work at least with libgpiod which does one event
> at a time.
>
> Since the bug hasn't been reported earlier we assume that there is close
> to zero actual users of the compatible mode to monitor GPIO events and thus
> we might consider to rework this ABI in the future.
>
How come this only affects the read operation but not the structures
passed as arguments to ioctl() calls?
Bart
> Fixes: 61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/gpio/gpiolib.c | 51 ++++++++++++++++++++++++++++++++++++---
> include/uapi/linux/gpio.h | 6 +++++
> 2 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 7340e4d0e873..134985210619 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -825,17 +825,26 @@ static __poll_t lineevent_poll(struct file *filep,
> return events;
> }
>
> -
> static ssize_t lineevent_read(struct file *filep,
> char __user *buf,
> size_t count,
> loff_t *f_ps)
> {
> struct lineevent_state *le = filep->private_data;
> + struct gpioevent_data event, *e = &event;
> + /* We need size of each member to avoid endianess issues below */
> + size_t ts_sz = sizeof(e->timestamp), id_sz = sizeof(e->id), e_sz;
> unsigned int copied;
> int ret;
>
> - if (count < sizeof(struct gpioevent_data))
> + /*
> + * In compatible mode, when kernel is 64-bit and user space is 32-bit,
> + * we may not tell what user wanted here when count is bigger than size
> + * of one event, so, we just assume that user asks for precisely one
> + * event.
> + */
> + e_sz = in_compat_syscall() ? ts_sz + id_sz : sizeof(*e);
> + if (count < e_sz)
> return -EINVAL;
>
> do {
> @@ -851,7 +860,43 @@ static ssize_t lineevent_read(struct file *filep,
>
> if (mutex_lock_interruptible(&le->read_lock))
> return -ERESTARTSYS;
> - ret = kfifo_to_user(&le->events, buf, count, &copied);
> + if (in_compat_syscall()) {
> + /*
> + * First we peek the one event and, if there is
> + * no error during copying to user space, skip it
> + * later.
> + */
> + if (kfifo_peek(&le->events, e))
> + copied = e_sz;
> + else
> + copied = 0;
> +
> + /* Do not try to copy garbage to the user */
> + ret = copied ? 0 : -EFAULT;
> +
> + /*
> + * Due to endianess concerns we have to copy this
> + * member-by-member. Luckily there are no members
> + * less than 32-bit.
> + */
> + if (!ret)
> + ret = copy_to_user(buf, &e->timestamp, ts_sz);
> + if (!ret)
> + ret = copy_to_user(buf + ts_sz, &e->id, id_sz);
> +
> + if (ret) {
> + /*
> + * Either we have got nothing from the FIFO or
> + * one of copy_to_user() calls failed.
> + */
> + ret = -EFAULT;
> + } else {
> + /* Skip peeked event if no error happened */
> + kfifo_skip(&le->events);
> + }
> + } else {
> + ret = kfifo_to_user(&le->events, buf, count, &copied);
> + }
> mutex_unlock(&le->read_lock);
>
> if (ret)
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 799cf823d493..054756bf6991 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -168,6 +168,12 @@ 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
> + *
> + * Warning! This structure has issues in the compatible mode, when
> + * kernel is 64-bit and user space is 32-bit, due to alignment
> + * differences.
> + *
> + * It's not recommended to retrieve more than one event at a time.
> */
> struct gpioevent_data {
> __u64 timestamp;
> --
> 2.24.0
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode
2019-12-10 9:06 ` Bartosz Golaszewski
@ 2019-12-10 13:44 ` Andy Shevchenko
2019-12-10 14:39 ` Kent Gibson
1 sibling, 0 replies; 12+ messages in thread
From: Andy Shevchenko @ 2019-12-10 13:44 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM
On Tue, Dec 10, 2019 at 10:06:04AM +0100, Bartosz Golaszewski wrote:
> śr., 4 gru 2019 o 20:42 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> napisał(a):
> >
> > The introduced line even handling ABI in the commit
> >
> > 61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
> >
> > missed the fact that 64-bit kernel may serve for 32-bit applications.
> > In such case the very first check in the lineevent_read() will fail
> > due to alignment differences.
> >
> > To workaround this we do several things here:
> > - put warning comment to UAPI header near to the structure description
> > - derive the size of the structure in the compatible mode from its members
> > - check for the size of this structure in the ->read() callback
> > - return only one event in the compatible mode at a time
> >
> > Above mitigation will work at least with libgpiod which does one event
> > at a time.
> >
> > Since the bug hasn't been reported earlier we assume that there is close
> > to zero actual users of the compatible mode to monitor GPIO events and thus
> > we might consider to rework this ABI in the future.
> >
>
> How come this only affects the read operation but not the structures
> passed as arguments to ioctl() calls?
On x86 the rest of the structures is naturally aligned by 8 bytes.
But you are right, the ABI is broken more widely than simple ->read().
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode
2019-12-10 9:06 ` Bartosz Golaszewski
2019-12-10 13:44 ` Andy Shevchenko
@ 2019-12-10 14:39 ` Kent Gibson
2019-12-10 16:55 ` Andy Shevchenko
1 sibling, 1 reply; 12+ messages in thread
From: Kent Gibson @ 2019-12-10 14:39 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Andy Shevchenko, Linus Walleij, Bartosz Golaszewski,
open list:GPIO SUBSYSTEM
On Tue, Dec 10, 2019 at 10:06:04AM +0100, Bartosz Golaszewski wrote:
> śr., 4 gru 2019 o 20:42 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> napisał(a):
> >
> > The introduced line even handling ABI in the commit
> >
> > 61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
> >
> > missed the fact that 64-bit kernel may serve for 32-bit applications.
> > In such case the very first check in the lineevent_read() will fail
> > due to alignment differences.
> >
> > To workaround this we do several things here:
> > - put warning comment to UAPI header near to the structure description
> > - derive the size of the structure in the compatible mode from its members
> > - check for the size of this structure in the ->read() callback
> > - return only one event in the compatible mode at a time
> >
> > Above mitigation will work at least with libgpiod which does one event
> > at a time.
> >
> > Since the bug hasn't been reported earlier we assume that there is close
> > to zero actual users of the compatible mode to monitor GPIO events and thus
> > we might consider to rework this ABI in the future.
> >
>
> How come this only affects the read operation but not the structures
> passed as arguments to ioctl() calls?
>
For Go the structs are aligned based on the size of their components so
that arrays of struct are naturally aligned. The struct is given a
hidden trailing pad so that a subsequent struct will be correctly aligned.
The sizeof the struct includes this hidden pad.
I'm pretty sure the same is true for gcc.
The gpioevent_data contains a __u64 which causes the whole struct to be
64 bit aligned on 64 bit, so it actually looks like this internally:
struct gpioevent_data {
__u64 timestamp;
__u32 id;
__u32 pad; // hidden
};
so 16 bytes.
On 32 bit the struct is 32 bit aligned and the trailing pad is missing,
so 12 bytes. This causes grief for the read due to the size mismatch.
(I'm sorry to say I had to add the pad to my Go gpiod library to get it
to read event data - but forgot to go back later and work out why -
until now :-()
Your new info change struct has the same problem, as it also contains a
__u64 and ends up with an odd number of __u32s, so gets a trailing pad
on 64 bit. Using __packed seems to inhibit the trailing pad.
Or you could explicitly add the pad so the struct will be 64bit aligned
even on 32bit. Neither of those options are available for the
gpioevent_data, as that would break the ABI.
The ioctl structs only contain __u32s (or smaller) and so get aligned to
32 bit boundaries on both 32 and 64 bit. So just lucky.
It is also lucky that the event_data happens to have the __u64 at the
beginning of the struct or there could be padding inserted between
fields, not just at the end. Similarly the byte array lengths in the
ioctl structs are all multiples of 4, so all the components happen to
align to 32 bit boundaries.
Kent.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode
2019-12-10 14:39 ` Kent Gibson
@ 2019-12-10 16:55 ` Andy Shevchenko
2019-12-11 9:18 ` Bartosz Golaszewski
0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2019-12-10 16:55 UTC (permalink / raw)
To: Kent Gibson
Cc: Bartosz Golaszewski, Linus Walleij, Bartosz Golaszewski,
open list:GPIO SUBSYSTEM
On Tue, Dec 10, 2019 at 10:39:02PM +0800, Kent Gibson wrote:
> On Tue, Dec 10, 2019 at 10:06:04AM +0100, Bartosz Golaszewski wrote:
> > śr., 4 gru 2019 o 20:42 Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> napisał(a):
> > >
> > > The introduced line even handling ABI in the commit
> > >
> > > 61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
> > >
> > > missed the fact that 64-bit kernel may serve for 32-bit applications.
> > > In such case the very first check in the lineevent_read() will fail
> > > due to alignment differences.
> > >
> > > To workaround this we do several things here:
> > > - put warning comment to UAPI header near to the structure description
> > > - derive the size of the structure in the compatible mode from its members
> > > - check for the size of this structure in the ->read() callback
> > > - return only one event in the compatible mode at a time
> > >
> > > Above mitigation will work at least with libgpiod which does one event
> > > at a time.
> > >
> > > Since the bug hasn't been reported earlier we assume that there is close
> > > to zero actual users of the compatible mode to monitor GPIO events and thus
> > > we might consider to rework this ABI in the future.
> > >
> >
> > How come this only affects the read operation but not the structures
> > passed as arguments to ioctl() calls?
> >
>
> For Go the structs are aligned based on the size of their components so
> that arrays of struct are naturally aligned. The struct is given a
> hidden trailing pad so that a subsequent struct will be correctly aligned.
> The sizeof the struct includes this hidden pad.
> I'm pretty sure the same is true for gcc.
>
> The gpioevent_data contains a __u64 which causes the whole struct to be
> 64 bit aligned on 64 bit, so it actually looks like this internally:
>
> struct gpioevent_data {
> __u64 timestamp;
> __u32 id;
> __u32 pad; // hidden
> };
>
> so 16 bytes.
>
> On 32 bit the struct is 32 bit aligned and the trailing pad is missing,
> so 12 bytes. This causes grief for the read due to the size mismatch.
Exactly.
> (I'm sorry to say I had to add the pad to my Go gpiod library to get it
> to read event data - but forgot to go back later and work out why -
> until now :-()
>
> Your new info change struct has the same problem, as it also contains a
> __u64 and ends up with an odd number of __u32s, so gets a trailing pad
> on 64 bit. Using __packed seems to inhibit the trailing pad.
> Or you could explicitly add the pad so the struct will be 64bit aligned
> even on 32bit.
I spoke to colleague of mine and has been told that best option is to fill all
gaps explicitly to have all members in the struct + 8 bytes alignment at the
end (also with explicit member).
> Neither of those options are available for the
> gpioevent_data, as that would break the ABI.
ABI needs v2 actually.
So, it must be
struct gpioevent_data_v2 {
__u64 timestamp;
__u32 id;
__u32 padding;
};
And so on...
> The ioctl structs only contain __u32s (or smaller) and so get aligned to
> 32 bit boundaries on both 32 and 64 bit. So just lucky.
Right.
> It is also lucky that the event_data happens to have the __u64 at the
> beginning of the struct or there could be padding inserted between
> fields, not just at the end. Similarly the byte array lengths in the
> ioctl structs are all multiples of 4, so all the components happen to
> align to 32 bit boundaries.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode
2019-12-10 16:55 ` Andy Shevchenko
@ 2019-12-11 9:18 ` Bartosz Golaszewski
2019-12-11 9:29 ` Kent Gibson
0 siblings, 1 reply; 12+ messages in thread
From: Bartosz Golaszewski @ 2019-12-11 9:18 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Kent Gibson, Bartosz Golaszewski, Linus Walleij,
open list:GPIO SUBSYSTEM
wt., 10 gru 2019 o 17:55 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> > For Go the structs are aligned based on the size of their components so
> > that arrays of struct are naturally aligned. The struct is given a
> > hidden trailing pad so that a subsequent struct will be correctly aligned.
> > The sizeof the struct includes this hidden pad.
> > I'm pretty sure the same is true for gcc.
> >
> > The gpioevent_data contains a __u64 which causes the whole struct to be
> > 64 bit aligned on 64 bit, so it actually looks like this internally:
> >
> > struct gpioevent_data {
> > __u64 timestamp;
> > __u32 id;
> > __u32 pad; // hidden
> > };
> >
> > so 16 bytes.
> >
> > On 32 bit the struct is 32 bit aligned and the trailing pad is missing,
> > so 12 bytes. This causes grief for the read due to the size mismatch.
>
> Exactly.
>
> > (I'm sorry to say I had to add the pad to my Go gpiod library to get it
> > to read event data - but forgot to go back later and work out why -
> > until now :-()
> >
> > Your new info change struct has the same problem, as it also contains a
> > __u64 and ends up with an odd number of __u32s, so gets a trailing pad
> > on 64 bit. Using __packed seems to inhibit the trailing pad.
> > Or you could explicitly add the pad so the struct will be 64bit aligned
> > even on 32bit.
>
> I spoke to colleague of mine and has been told that best option is to fill all
> gaps explicitly to have all members in the struct + 8 bytes alignment at the
> end (also with explicit member).
>
> > Neither of those options are available for the
> > gpioevent_data, as that would break the ABI.
>
> ABI needs v2 actually.
>
I finally sat down to integrate this with my series and figured that
this can't go on top of it. It's a bug-fix actually and maybe even
stable material.
On the other hand - if we have so few users of GPIO chardev with
32-bit user-space and 64-bit kernel - maybe we should just bite the
bullet, not fix this one, deprecate it and introduce a proper v2 of
the API?
Bart
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode
2019-12-11 9:18 ` Bartosz Golaszewski
@ 2019-12-11 9:29 ` Kent Gibson
2019-12-11 10:47 ` Andy Shevchenko
0 siblings, 1 reply; 12+ messages in thread
From: Kent Gibson @ 2019-12-11 9:29 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Andy Shevchenko, Bartosz Golaszewski, Linus Walleij,
open list:GPIO SUBSYSTEM
On Wed, Dec 11, 2019 at 10:18:39AM +0100, Bartosz Golaszewski wrote:
> wt., 10 gru 2019 o 17:55 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> napisał(a):
> >
> > > For Go the structs are aligned based on the size of their components so
> > > that arrays of struct are naturally aligned. The struct is given a
> > > hidden trailing pad so that a subsequent struct will be correctly aligned.
> > > The sizeof the struct includes this hidden pad.
> > > I'm pretty sure the same is true for gcc.
> > >
> > > The gpioevent_data contains a __u64 which causes the whole struct to be
> > > 64 bit aligned on 64 bit, so it actually looks like this internally:
> > >
> > > struct gpioevent_data {
> > > __u64 timestamp;
> > > __u32 id;
> > > __u32 pad; // hidden
> > > };
> > >
> > > so 16 bytes.
> > >
> > > On 32 bit the struct is 32 bit aligned and the trailing pad is missing,
> > > so 12 bytes. This causes grief for the read due to the size mismatch.
> >
> > Exactly.
> >
> > > (I'm sorry to say I had to add the pad to my Go gpiod library to get it
> > > to read event data - but forgot to go back later and work out why -
> > > until now :-()
> > >
> > > Your new info change struct has the same problem, as it also contains a
> > > __u64 and ends up with an odd number of __u32s, so gets a trailing pad
> > > on 64 bit. Using __packed seems to inhibit the trailing pad.
> > > Or you could explicitly add the pad so the struct will be 64bit aligned
> > > even on 32bit.
> >
> > I spoke to colleague of mine and has been told that best option is to fill all
> > gaps explicitly to have all members in the struct + 8 bytes alignment at the
> > end (also with explicit member).
> >
> > > Neither of those options are available for the
> > > gpioevent_data, as that would break the ABI.
> >
> > ABI needs v2 actually.
> >
>
> I finally sat down to integrate this with my series and figured that
> this can't go on top of it. It's a bug-fix actually and maybe even
> stable material.
>
> On the other hand - if we have so few users of GPIO chardev with
> 32-bit user-space and 64-bit kernel - maybe we should just bite the
> bullet, not fix this one, deprecate it and introduce a proper v2 of
> the API?
>
Fixing it in API v2 makes the most sense to me.
Kent.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode
2019-12-11 9:29 ` Kent Gibson
@ 2019-12-11 10:47 ` Andy Shevchenko
2019-12-11 13:15 ` Bartosz Golaszewski
0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2019-12-11 10:47 UTC (permalink / raw)
To: Kent Gibson
Cc: Bartosz Golaszewski, Bartosz Golaszewski, Linus Walleij,
open list:GPIO SUBSYSTEM
On Wed, Dec 11, 2019 at 05:29:21PM +0800, Kent Gibson wrote:
> On Wed, Dec 11, 2019 at 10:18:39AM +0100, Bartosz Golaszewski wrote:
> > wt., 10 gru 2019 o 17:55 Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> napisał(a):
> > > > Neither of those options are available for the
> > > > gpioevent_data, as that would break the ABI.
> > >
> > > ABI needs v2 actually.
> > >
> >
> > I finally sat down to integrate this with my series and figured that
> > this can't go on top of it. It's a bug-fix actually and maybe even
> > stable material.
> >
> > On the other hand - if we have so few users of GPIO chardev with
> > 32-bit user-space and 64-bit kernel - maybe we should just bite the
> > bullet, not fix this one, deprecate it and introduce a proper v2 of
> > the API?
> >
>
> Fixing it in API v2 makes the most sense to me.
I agree. Please, use only second patch in my series (if needed I can resend
it separately) and drop this one.
P.S. Actually it's not a bugfix since it's never worked from the day 1.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode
2019-12-11 10:47 ` Andy Shevchenko
@ 2019-12-11 13:15 ` Bartosz Golaszewski
0 siblings, 0 replies; 12+ messages in thread
From: Bartosz Golaszewski @ 2019-12-11 13:15 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Kent Gibson, Bartosz Golaszewski, Linus Walleij,
open list:GPIO SUBSYSTEM
śr., 11 gru 2019 o 11:47 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> On Wed, Dec 11, 2019 at 05:29:21PM +0800, Kent Gibson wrote:
> > On Wed, Dec 11, 2019 at 10:18:39AM +0100, Bartosz Golaszewski wrote:
> > > wt., 10 gru 2019 o 17:55 Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> napisał(a):
>
> > > > > Neither of those options are available for the
> > > > > gpioevent_data, as that would break the ABI.
> > > >
> > > > ABI needs v2 actually.
> > > >
> > >
> > > I finally sat down to integrate this with my series and figured that
> > > this can't go on top of it. It's a bug-fix actually and maybe even
> > > stable material.
> > >
> > > On the other hand - if we have so few users of GPIO chardev with
> > > 32-bit user-space and 64-bit kernel - maybe we should just bite the
> > > bullet, not fix this one, deprecate it and introduce a proper v2 of
> > > the API?
> > >
> >
> > Fixing it in API v2 makes the most sense to me.
>
> I agree. Please, use only second patch in my series (if needed I can resend
> it separately) and drop this one.
>
> P.S. Actually it's not a bugfix since it's never worked from the day 1.
>
Yeah I guess so. Anyway, in this case, I won't be adding the event
watch feature to v1 - I'll go ahead and just add it to v2 then.
Bart
^ permalink raw reply [flat|nested] 12+ messages in thread