Date: Thu, 22 Jun 2017 12:31:21 +0930
From: Jonathan Woithe <jwoithe@just42.net>
To: Darren Hart <dvhart@infradead.org>
Cc: Micha?? K??pie?? <kernel@kempniu.pl>,
        Rafael Wysocki <rjw@rjwysocki.net>,
        Andy Shevchenko <andy@infradead.org>,
        platform-driver-x86@vger.kernel.org, linux-acpi@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for
 storing hotkey scancodes
Message-ID: <20170622030120.GF32131@marvin.atrad.com.au>
References: <20170616044058.30443-1-kernel@kempniu.pl>
 <20170616044058.30443-2-kernel@kempniu.pl>
 <20170621181543.GB25900@fury>
 <20170621235021.GA32131@marvin.atrad.com.au>
 <20170622024413.GE25900@fury>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170622024413.GE25900@fury>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2994
Lines: 59

Hi Darren

On Wed, Jun 21, 2017 at 07:44:13PM -0700, Darren Hart wrote:
> > 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.

Indeed.  I'm happy to leave it at 40.

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

I can't test this because my Fujitsu doesn't have these hotkeys.

Regarding the order, the original code sent "press" events to userspace in
the order that the keys were pressed.  When the single "release all keys"
event is received, a "release" event ws sent to userspace for each button
which was pressed, in the order they were pressed.

If userspace acted on the button release event then the order of the
synthesised "release" events could be significant to userspace, even if they
are all generated at the same time in response to the first "release" event.

By way of example, let's say there are two hotkeys, "A" and "B".  Let us also
say that the user does the following:
 - Press and hold hotkey "A"
 - Press and hold hotkey "B"
 - Release one of these hotkeys

The events seen by userspace with the original code would be "A-press",
"B-press", "A-release", "B-release".  With the revised code the order of the
release events would be reversed compared to the previous behaviour.  That
is, unless I've misunderstood how sparse_keymap_report_event() works.

Regards
  jonathan