Date: Thu, 22 Jun 2017 22:46:19 +0200
From: =?utf-8?B?TWljaGHFgiBLxJlwaWXFhA==?= <kernel@kempniu.pl>
To: Jonathan Woithe <jwoithe@just42.net>
Cc: Darren Hart <dvhart@infradead.org>, 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: <20170622204619.GB4194@kmp-mobile.hq.kempniu.pl>
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>
 <20170622030120.GF32131@marvin.atrad.com.au>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20170622030120.GF32131@marvin.atrad.com.au>
User-Agent: Mutt/1.8.3 (2017-05-23)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3771
Lines: 75

> 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 may have worded this in a confusing way, sorry for that.  What I
wanted to convey is that the driver currently produces release events in
FIFO order, but that order is not imposed by the firmware.

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

All you wrote above is correct and this patch does change the order of
release events sent to userspace when multiple hotkeys are pressed
simultaneously.  The question is: is it relevant for any practical
scenario?

Anyway, I find this matter to be of secondary importance.  The primary
objective of this patch is to get rid of the kfifo.  If anyone has
strong feelings about the change in event ordering, I will be happy to
revert to FIFO in v2.

-- 
Best regards,
Michał Kępień