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

[Kent Gibson <warthog618@xxxxxxxxx>]

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.