Message-ID: <20170621235021.GA32131@marvin.atrad.com.au>
Date:   Thu, 22 Jun 2017 09:20:21 +0930
From:   Jonathan Woithe <jwoithe@...t42.net>
To:     Darren Hart <dvhart@...radead.org>
Cc:     Micha?? K??pie?? <kernel@...pniu.pl>,
        Rafael Wysocki <rjw@...ysocki.net>,
        Andy Shevchenko <andy@...radead.org>,
        platform-driver-x86@...r.kernel.org, linux-acpi@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for
 storing hotkey scancodes

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

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.

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.

Regards
  jonathan

Powered by blists - more mailing lists