Re: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s

[Dmitry Torokhov]

On Thu, Jul 28, 2016 at 03:52:11PM +0000, Mario_Limonciello@xxxxxxxx wrote:
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx]
> > Sent: Wednesday, July 27, 2016 6:08 PM
> > To: Darren Hart <dvhart@xxxxxxxxxxxxx>
> > Cc: Pali Rohár <pali.rohar@xxxxxxxxx>; Limonciello, Mario
> > <Mario_Limonciello@xxxxxxxx>; lkml <linux-kernel@xxxxxxxxxxxxxxx>;
> > platform-driver-x86@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s
> > 
> > Hi Darren,
> > 
> > On Wed, Jul 27, 2016 at 3:43 PM, Darren Hart <dvhart@xxxxxxxxxxxxx> wrote:
> > > Dmitry, we'd appreciate your thoughts as the input maintainer on a
> > question
> > > below (left the thread in tact so you have all the context).
> > >
> > > On Wed, Jul 27, 2016 at 10:15:12PM +0200, Pali Rohár wrote:
> > >> On Wednesday 27 July 2016 21:03:57 Mario_Limonciello@xxxxxxxx wrote:
> > >> > > -----Original Message-----
> > >> > > From: Pali Rohár [mailto:pali.rohar@xxxxxxxxx]
> > >> > > Sent: Wednesday, July 27, 2016 11:06 AM
> > >> > > To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>
> > >> > > Cc: dvhart@xxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > >> > > platform-driver- x86@xxxxxxxxxxxxxxx
> > >> > > Subject: Re: [PATCH] dell-wmi: Add events created by Dell Rugged
> > >> > > 2-in-1s
> > >> > >
> > >> > > On Wednesday 27 July 2016 17:55:09 Mario_Limonciello@xxxxxxxx
> > wrote:
> > >> > > > > Hi! I'm not sure if KEY_PROG1/2/3 are good names here as we
> > >> > > > > already use
> > >> > > > >
> > >> > > > >those for other actions (see bios_to_linux_keycode). Also there
> > >> > > > >is:
> > >> > > > I had missed this, do you have some recommendations on what
> > would
> > >> > > > be better codes to map this to?
> > >> > >
> > >> > > Problem is that I do not know when those KEY_PROG keys from
> > >> > > bios_to_linux_keycode table are emitted. There are missing
> > comments
> > >> > > or description.
> > >> > >
> > >> > > Are you able to find out description for all those keys in that
> > >> > > table? Maybe now (when linux key constants are extended), there
> > >> > > could be better candidates...
> > >> >
> > >> > I'll ask around.  It's not immediately obvious to me though, maybe
> > >> > from old specs?
> > >>
> > >> Do not know if it is old. At least some codes from it are used on my
> > >> E6440 machine. Table itself was added by Rezwanul Kabir in this commit:
> > >>
> > >>
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5e
> > a2559726b786283236835dc2905c23b36ac91c
> > >>
> > >> Maybe commit message could help you to indentify what it is...
> > >>
> > >> > > > I'll double check what the things that "were" mapping to
> > >> > > > KEY_PROG1 etc actually were.  This might be a case of an
> > >> > > > expected clash if the functions aren't in current generations.
> > >> > > >
> > >> > > > >/* Wifi Catcher */
> > >> > > > >
> > >> > > > > { KE_KEY,    0xe011, { KEY_PROG2 } },
> > >> > > >
> > >> > > > It's worth mentioning that Wifi Catcher hasn't been used for any
> > >> > > > Dell laptops for a handful generations.  The rugged 2in1's are
> > >> > > > current generation that have these programmable buttons and
> > >> > > > don't have wifi catcher.
> > >> > >
> > >> > > Anyway, what is "Wifi Catcher"? HW switch buttton which
> > >> > > enable/disable wifi? Or something else?
> > >> >
> > >> > Wifi catcher was a special hardware switch slider switch.  When the
> > >> > machine was turned in S3 the sliding the switch beyond the on
> > >> > position would scan for available wifi networks and light up an LED
> > >> > if some from your predefined list were found.
> > >> >
> > >> > When the machine was on, it would open up the applet that let you
> > >> > configure the behavior for the switch in S3.
> > >>
> > >> Hm... maybe it better fit KEY_WLAN then? Just speculation.
> > >>
> > >> > > > So there should be no "real" clash here.
> > >> > >
> > >> > > Problem can be in future. This driver is unified for all Dell
> > >> > > products with wmi interface and so future product could do some
> > >> > > nasty things...
> > >> >
> > >> > I agree with Darren here.
> > >> >
> > >> > At least on Dell side we're creating new codes for "new" buttons and
> > >> > the limitation is really on the kernel side for how many KEY_PROG#
> > >> > or similar functions they can be re-assigned to.
> > >>
> > >> Ok.
> > >>
> > >> > Maybe it's time to think of another way to get this information to
> > >> > userspace rather that always translating them into key codes?
> > >>
> > >> If event is generated by pressing key, button or hw switch, then input
> > >> key is correct way. If there is no KEY define which fit for new vendor
> > >> hw button, then I think we should start discussion with input subsystem
> > >> how to handle such situation.
> > >>
> > >> > There's a lot that are marked as KE_IGNORE because the kernel can't
> > >> > do anything interesting with them as no 1-1 mapping exists to a
> > >> > keycode.
> > >>
> > >> Most marked as KE_IGNORE are because duplicate event is sent via
> > >> keyboard controller or via other subsystem (e.g. rfkill).
> > >>
> > >> But events which are not keys or buttons should not be sent via input
> > >> devices. At least I think it is against usage of input devices.
> > >>
> > >> Events like "battery was removed" or "keyboard backlight was changed"
> > or
> > >> "battery lifetime decreased under X %" can be useful for userspace
> > >> applications. I agree. But I think we do not have any subsystem or
> > >> interface for sending them from kernel to userspace.
> > >>
> > >> If we start talking about creating interface for it, I would rather see
> > >> something more generic, not Dell-only specific or created specially for
> > >> Dell machines which will not fit for others...
> > >>
> > >> Darren, what do you think about it?
> > >
> > > It sounds like a good Kernel Summit topic... and they just happen to have
> > the
> > > call for topics going on right now... first step would be to get the input
> > > maintainers thoughts...
> > >
> > > +Dmitry Torokhov
> > >
> > > Dmitry, what are your thoughts with respect to handing events up to
> > userspace
> > > which are not strictly key presses via the input system? Pali's suggestion
> > makes
> > > sense to me - do you think this is something we should discuss at KS... or is
> > > this already decided and can you set us straight?
> > 
> > So here is what I believe:
> > 
> > - Stuff that are not keys/buttons/switches/other bits of hardware
> > directly manipulated by user, but rather state change notifications
> > should not be delivered via input subsystem
> 
> Yeah this matches existing kernel documentation too.
> 
> > 
> > - Even if something is a key/button/switch it does not necessarily
> > need to be sent through input subsystem or we may want to wait until
> > we add a new input event. This is because vendors love to come up with
> > "value add" features that require vendor specific driver that noone
> > ends up using. I mean, for the "WiFi catcher" do you have any kind of
> > numbers for the users of the feature?
> 
> Like I said, this particular feature hasn't been in use for quite a few generations.  
> In its time it was deemed "beneficial" in that day because of the amount of time it 
> took for waking up the machine only to determine there was no available wifi nearby.
> It's been superseded by other technology improvements.
> 
> There are other types of "notification" only events that could be useful to userspace.
> I don't think they need to all be generally demonized with the connotation as a negative 
> vendor specific value add, there are plenty of generic things that userspace could notify on

Generic things should find a specific system they are part of (i.e. wifi
notifications -> rflill, wifi switching -> input, etc), it is one-off
that "sounded good" but are hard to discover by user and nobody ends up
using that I am demonizing.

> that are essentially "killed" at the WMI driver today.
> 
> Here's a few I find:
> "Keyboard illumination toggle"
> "Mic mute toggle"
> "ALS sensor toggle"
> " Rotation lock toggle"
> "Airplane mode toggle"

These all seem valid key events (unless they are not requests to execute
said actions but rather notifiers of events already happened).

Are they "killed" because they are also delivered via i8042?

> 
> > 
> > - In the same vein, we can come up with something generic to shuffle
> > the crap over to userspace ("com.vendor.crap.morecrap..." over
> > connector? I think I just threw into my mouth a little), but do we
> > have consumer for this?
> > 
> 
> I think it would be most equitable to shuffle all notification events over
> to userspace with a generic connector and let userspace maintainers 
> decide which ones make sense to show to the user.
> 
> The most logical one to me will be adding patches into gnome-settings-daemon
> and similar UI interfaces that already display things like brightness and volume
> changes.

Well, it is really easy to shove stuff up into userspace and say "let
them deal with it", but we if stuff is useful and applicable to many
devices, then I am sure there are more specific interfaces that would be
better suited for such events, and one off will require writing "tray
notification apps" that we all so love.

Thanks.

-- 
Dmitry