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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Fri, Sep 11, 2020 at 06:20:49PM +0200, Arnd Bergmann wrote:
> > +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.

There is no such option available right now, I prefer to leave as is to make
backporting easier.

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

Yeah, due to a special alignment for compat_u64. I blindly copied from your
proposal.

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

OK!

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

Okay, I will re-do this.

-- 
With Best Regards,
Andy Shevchenko