Re: [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode

[Arnd Bergmann <arnd@xxxxxxxx>]

> +static ssize_t lineevent_to_user(char __user *buf, struct gpioevent_data *ge)
> +{
> +#ifdef __x86_64__

I would make this "#ifdef CONFIG_IA32_COMPAT" to clarify what this
is actually checking for. In theory we could add support for
CONFIG_OABI_COMPAT here as well, not sure if there is a point.
I recently came across a couple of things that all need the same
hacks for arm-oabi and x86-32 in principle.

> +       /* i386 has no padding after 'id' */
> +       if (in_ia32_syscall()) {
> +               struct compat_ge {
> +                       compat_u64      timestamp __packed;

No need for marking this __packed, it already is.

> +                       u32             id;
> +               } cge;
> +
> +               if (buf && ge) {

I think I'd leave out the 'if()' checks here, and require the function
to be called with valid pointers. It seems odd otherwise to return
sizeof(cge) from the read() function without having written data.

Note also that user space may pass a NULL pointer and should
get back -EFAULT instead of 12 or 16.

> -       if (count < sizeof(ge))
> +       /* When argument is NULL it returns size of the structure in user space */
> +       ge_size = lineevent_to_user(NULL, NULL);
> +       if (count < ge_size)
>                 return -EINVAL;

Right, I see this is how it's being used, and I'd tend to agree with Kent:
if you just determine the size dynamically and add a good comment,
then the rest of the code gets simpler and more logical.

      Arnd