Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Darren Hart]

On Thu, Jun 22, 2017 at 09:20:21AM +0930, Jonathan Woithe wrote:
> On Wed, Jun 21, 2017 at 11:15:43AM -0700, Darren Hart wrote:
> > On Fri, Jun 16, 2017 at 06:40:52AM +0200, Micha?? K??pie?? wrote:
> > > -#define RINGBUFFERSIZE 40
> > >  
> > >  /* Debugging */
> > >  #define FUJLAPTOP_DBG_ERROR	  0x0001
> > > @@ -146,8 +144,8 @@ struct fujitsu_laptop {
> > >  	struct input_dev *input;
> > >  	char phys[32];
> > >  	struct platform_device *pf_device;
> > > -	struct kfifo fifo;
> > > -	spinlock_t fifo_lock;
> > > +	int scancode_buf[40];
> > 
> > Do we know why 40 was used here? A single use magic number is fine, but it
> > would be good to document why it is what it is if we know.
> 
> I am unsure as to why 40 was chosen as the size.  The support for the
> hotkeys was contributed by Peter Gruber in 2008 who had a Fujitsu laptop
> which featured this functionality (mine doesn't).  I obviously can't speak
> for Peter but I suspect that he simply picked a number which seemed
> reasonable at the time.  I am unaware of any specific reason why the size
> was set at 40, and to be honest never throught to ask at the time the patch
> was proposed.
> 
> If we need a reason to justify the value of 40, I guess the best option is
> "because it seems reasonable".

That's not surprising, and it's fine. If we know, it's worth documenting. If
not, well, we do what we can :-)

> 
> I think the buffer size could probably be reduced a little without impacting
> on functionality.  I suspect the value was chosen so as to be well above the
> number of events which could be generated ahead of a button release (which
> effectively releases all buttons held at that stage).  The number of hot
> keys is limited (I don't think any model has more than 5), so reducing the
> buffer somewhat appears safe (perhaps to 32 if there were any advantages to
> having the size be a power of two).  There seems little point doing this in
> the name of memory saving since the amount saved is trivially small.
> 

It's not a problem as far as I'm concerned. Plenty more lower hanging fruit if
people want to reduce memory footprint.

> Looking through my correspondance with Peter from 2008 (which was largely
> off-list for reasons I do not recall), it seems the reason the kfifo was
> used was due to Peter's concern about concurrent processing of mulitple
> events across different CPUs.  Since at the time we couldn't determine
> whether this was a real issue Peter took the safe route and used a kfifo. 
> If the assumption that it cannot happen is known to hold in all cases there
> is clearly no need for the more complex construct (at least on the basis of
> concurrency arguments).
> 
> I've given some more thought to the following point (from the original patch
> submission):
> > In order to simplify implementation, hotkey input device behavior is
> > slightly changed (from FIFO to LIFO).  The order of release events
> > generated by the input device should not matter from end user
> > perspective as upon releasing any hotkey the firmware only produces a
> > single event which means "all hotkeys were released".
> 
> This will effectively alter the order the button events are seen by
> userspace though, won't it?  It would mean that (for example) a user who
> presses (and holds) the "media" key before "email" will end up with "email"
> being acted on first.  This may surprise them.  Then again, I suppose it
> could be argued that such usage is unusual and therefore is likely to be
> rare - and therefore not worth bothering about.

Michal noted there is only one event to release all keys so the order wouldn't
be noticed in userspace. Has this been confirmed with testing?

-- 
Darren Hart
VMware Open Source Technology Center