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

[Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>]

On Sat, Sep 12, 2020 at 4:21 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Fri, Sep 11, 2020 at 05:28:46PM +0300, Andy Shevchenko wrote:
> > On Fri, Sep 11, 2020 at 06:17:14PM +0800, Kent Gibson wrote:
> > > On Fri, Sep 11, 2020 at 12:53:55PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Sep 11, 2020 at 05:12:49PM +0800, Kent Gibson wrote:
> > > > > On Fri, Sep 11, 2020 at 11:31:09AM +0300, Andy Shevchenko wrote:
> > > > > > On Fri, Sep 11, 2020 at 11:05:39AM +0800, Kent Gibson wrote:
> > > > > > > On Thu, Sep 10, 2020 at 01:19:34PM +0300, Andy Shevchenko wrote:
> > > >
>
> [snip]
> > >
> > > typedef u64 __attribute__((aligned(4))) compat_u64;
> > >
> > > which is bitwise identical - only allowed to 32-bit align.
> >
> > Yes. That's what I meant under "not the same".
> >
> > As far as I understand the alignment makes sense if this type is a part of
> > the uAPI definition. But here we have it completely local. copy_to_user() takes
> > a pointer to a memory without any specific alignment implied.
> >
> > So, what you proposing is basically something like
> >
> > ret = copy_to_user(buf, &ge, compat ?  sizeof(compat) : sizeof(ge));
> >
> > Correct?
> >
>
> That isn't how I would write the copy_to_user(). The size would be
> calculated once, using the linevent_user_size() helper, with
> appropriate documentation as to why this is necessary, and then
> used throughout lineevent_read().
>
> The documentation would mainly be on the lineevent_user_size() function
> itself.
>
> > I don't like the difference between 2nd and 3rd argument. This what looks to me
> > hackish. Variant with explicit compat structure I like more.
> >
>
> Agreed - writing it that way does look pretty nasty.
>
> But my suggestion is actually this:
>
> ret = copy_to_user(buf, &ge, event_size);
>
> I suggested ge_size previously, but event_size might help highlight that
> it isn't always sizeof(ge).
>
> > But if you think it's okay, I will update your way.
> >
>
> I would defer to Bart or Linus, but I think just calculating the
> appropriate size is preferable for this case.
>
> Cheers,
> Kent.

Kent has been producing clean and elegant code so far so I trust him
on code quality issues TBH. Personally in this case however I'd go
with an explicit compat structure as Andy prefers.

I don't have a strong opinion on this so I really am fine either way.

Bartosz