[GIT PULL] HID for 5.7

[GIT PULL] HID for 5.7

[Jiri Kosina]

Linus,

please pull from

  git://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-linus

to receive HID subsytem patches for 5.7 merge window.

=====
- Logitech HID++ protocol support improvement from Filipe Laíns
- probe fix for Logitech-G* devices from Hans de Goede
- a few other small code cleanups and support for new device IDs
=====

Thanks.

----------------------------------------------------------------
Christophe JAILLET (1):
      HID: rmi: Simplify an error handling path in 'rmi_hid_read_block()'

Filipe Laíns (2):
      HID: logitech-dj: add debug msg when exporting a HID++ report descriptors
      HID: logitech-dj: add support for the static device in the Powerplay mat/receiver

Gustavo A. R. Silva (2):
      HID: intel-ish-hid: ishtp-dev.h: Replace zero-length array with flexible-array member
      HID: intel-ish-hid: hbm.h: Replace zero-length array with flexible-array member

Hans de Goede (2):
      HID: quirks: Remove ITE 8595 entry from hid_have_special_driver
      HID: lg-g15: Do not fail the probe when we fail to disable F# emulation

Lucas Tanure (2):
      HID: appleir: Remove unnecessary goto label
      HID: appleir: Use devm_kzalloc() instead of kzalloc()

Rishi Gupta (1):
      HID: mcp2221: add usb to i2c-smbus host bridge

Samuel Čavoj (1):
      HID: Add driver fixing Glorious PC Gaming Race mouse report descriptor

 MAINTAINERS                                 |   7 +
 drivers/hid/Kconfig                         |  17 +
 drivers/hid/Makefile                        |   2 +
 drivers/hid/hid-appleir.c                   |  12 +-
 drivers/hid/hid-glorious.c                  |  86 ++++
 drivers/hid/hid-ids.h                       |   5 +
 drivers/hid/hid-lg-g15.c                    |   6 +-
 drivers/hid/hid-logitech-dj.c               |  11 +-
 drivers/hid/hid-mcp2221.c                   | 742 ++++++++++++++++++++++++++++
 drivers/hid/hid-quirks.c                    |   3 -
 drivers/hid/hid-rmi.c                       |   1 -
 drivers/hid/intel-ish-hid/ishtp/hbm.h       |   2 +-
 drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h |   2 +-
 13 files changed, 878 insertions(+), 18 deletions(-)
 create mode 100644 drivers/hid/hid-glorious.c
 create mode 100644 drivers/hid/hid-mcp2221.c

-- 
Jiri Kosina
SUSE Labs

Re: [GIT PULL] HID for 5.7

[pr-tracker-bot]

The pull request you sent on Wed, 1 Apr 2020 14:11:24 +0200 (CEST):

> git://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-linus

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/c101e9bbce4ae2947b35a660f17d617fc3827595

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

Re: [GIT PULL] HID for 5.7

[Linus Torvalds]

On Wed, Apr 1, 2020 at 5:11 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> Samuel Čavoj (1):
>       HID: Add driver fixing Glorious PC Gaming Race mouse report descriptor

What a glorious name for a piece of hardware. Even if it's apparently
buggy and needs help to work right.

I felt bad saying I don't need that glorious driver when doing my oldconfig.

Anyway, because I noticed this due to the name, it does strike me that
clearly Windows must be ignoring - or otherwise reacting differently
to - the HID_MAIN_ITEM_CONSTANT flag. Because presumably those mice
work under Windows without special drivers?

In fact, reading that driver, it looks like they report being *both*
constant *and* variable in their report descriptors. Which sounds odd.
Maybe we should do whatever Windows does, and not need a special
driver for this maybe-bot-so-glorious-after-all mouse hardware?

Hmm?

              Linus

Re: [GIT PULL] HID for 5.7

[Jiri Kosina]

On Wed, 1 Apr 2020, Linus Torvalds wrote:

> > Samuel Čavoj (1):
> >       HID: Add driver fixing Glorious PC Gaming Race mouse report descriptor
> 
> What a glorious name for a piece of hardware. Even if it's apparently
> buggy and needs help to work right.

Yeah, the vendor apparently was not of the humble kind :)

> I felt bad saying I don't need that glorious driver when doing my 
> oldconfig.

:))

> Anyway, because I noticed this due to the name, it does strike me that 
> clearly Windows must be ignoring - or otherwise reacting differently to 
> - the HID_MAIN_ITEM_CONSTANT flag. Because presumably those mice work 
> under Windows without special drivers?
> 
> In fact, reading that driver, it looks like they report being *both* 
> constant *and* variable in their report descriptors. Which sounds odd. 
> Maybe we should do whatever Windows does, and not need a special driver 
> for this maybe-bot-so-glorious-after-all mouse hardware?

Adding Samuel to CC.

From what I understood is that in order to access the buttons reported in 
report #2 (the one marked with HID_MAIN_ITEM_CONSTANT), you actually *do* 
need a special software on windows anyway.

What we do is that we ignore any changes in reports with 
HID_MAIN_ITEM_CONSTANT in the HID core.

It would still be possible to access the report via hidraw, and maybe 
that's analogy of what the Windows driver/special Glorious software :) 
does, I don't know. It's hard to believe that Windows would be actually 
willing to report any changes coming through HID_MAIN_ITEM_CONSTANT 
reports, but who knows.

-- 
Jiri Kosina
SUSE Labs

Re: [GIT PULL] HID for 5.7

[Benjamin Tissoires]

On Fri, Apr 3, 2020 at 12:05 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Wed, 1 Apr 2020, Linus Torvalds wrote:
>
> > > Samuel Čavoj (1):
> > >       HID: Add driver fixing Glorious PC Gaming Race mouse report descriptor
> >
> > What a glorious name for a piece of hardware. Even if it's apparently
> > buggy and needs help to work right.
>
> Yeah, the vendor apparently was not of the humble kind :)
>
> > I felt bad saying I don't need that glorious driver when doing my
> > oldconfig.
>
> :))
>
> > Anyway, because I noticed this due to the name, it does strike me that
> > clearly Windows must be ignoring - or otherwise reacting differently to
> > - the HID_MAIN_ITEM_CONSTANT flag. Because presumably those mice work
> > under Windows without special drivers?
> >
> > In fact, reading that driver, it looks like they report being *both*
> > constant *and* variable in their report descriptors. Which sounds odd.
> > Maybe we should do whatever Windows does, and not need a special driver
> > for this maybe-bot-so-glorious-after-all mouse hardware?
>
> Adding Samuel to CC.
>
> From what I understood is that in order to access the buttons reported in
> report #2 (the one marked with HID_MAIN_ITEM_CONSTANT), you actually *do*
> need a special software on windows anyway.
>
> What we do is that we ignore any changes in reports with
> HID_MAIN_ITEM_CONSTANT in the HID core.

Funny enough, I think most hardware vendors are actually correct with
the CONSTANT implementation but Microsoft itself with the Surface
touchpad line :)

>
> It would still be possible to access the report via hidraw, and maybe
> that's analogy of what the Windows driver/special Glorious software :)
> does, I don't know. It's hard to believe that Windows would be actually
> willing to report any changes coming through HID_MAIN_ITEM_CONSTANT
> reports, but who knows.

I'll need to check whether we have too many drivers that replace
constant by variable. But I know it bit us in the past a few times
(and the Surface covers are the ones I remember). But again, Windows
is weird in a lot of ways, and I believe that they do not have one
unified driver for everything, but some behaviour that depends on the
application. So I am a little bit hesitant to toggle the switch to
consider constant usages as variable ones. Luckily, we now have a
regression test suite. It's not complete, but it can prevent such
regressions with the devices we have in there.

Cheers,
Benjamin

>
> --
> Jiri Kosina
> SUSE Labs
>

Re: [GIT PULL] HID for 5.7

[Samuel Čavoj]

On 03.04.2020 12:05, Jiri Kosina wrote:
> On Wed, 1 Apr 2020, Linus Torvalds wrote:
> 
> > Anyway, because I noticed this due to the name, it does strike me that 
> > clearly Windows must be ignoring - or otherwise reacting differently to 
> > - the HID_MAIN_ITEM_CONSTANT flag. Because presumably those mice work 
> > under Windows without special drivers?
> > 
> > In fact, reading that driver, it looks like they report being *both* 
> > constant *and* variable in their report descriptors. Which sounds odd. 
> > Maybe we should do whatever Windows does, and not need a special driver 
> > for this maybe-bot-so-glorious-after-all mouse hardware?
> 
> Adding Samuel to CC.
> 
> From what I understood is that in order to access the buttons reported in 
> report #2 (the one marked with HID_MAIN_ITEM_CONSTANT), you actually *do* 
> need a special software on windows anyway.

The Glorious software is merely used to map the actions to the buttons.
The mouse comes with a common default configuration (forward and back on
the side buttons) but every button can be remapped to any action. I am
used to having a Play/Pause button on my mouse and that's where I
encountered the problem in the first place.

The configuration of the mouse is then stored in the firmware. The
Glorious software is not required to be running or even to be installed
for the mouse to function. The vanilla Windows HID driver is in use.
Sorry if that wasn't clear.

> What we do is that we ignore any changes in reports with 
> HID_MAIN_ITEM_CONSTANT in the HID core.
> 
> It would still be possible to access the report via hidraw, and maybe 
> that's analogy of what the Windows driver/special Glorious software :) 
> does, I don't know. It's hard to believe that Windows would be actually 
> willing to report any changes coming through HID_MAIN_ITEM_CONSTANT 
> reports, but who knows.

It unfortunately appears to be the case. Just for reference, here is the
relevant part of the original descriptor again:
  
  INPUT(2)[INPUT]
    Field(0)
      Flags( Constant Variable Absolute )
    Field(1)
      Flags( Constant Variable Absolute )
    Field(2)
      Flags( Constant Variable Absolute )

Windows accepts the reports just fine. Unless there is something else at
play here, but I don't see anything that could be since the default HID
driver is used on Windows.

I also set the Relative flag instead of Absolute in the driver, in order
to drop repeat events when holding down the button. These are not
desirable in the case of consumer control events, such as 'Next Track'.
This is not a big problem, though.

Regards,
Samuel

Re: [GIT PULL] HID for 5.7

[Samuel Čavoj]

On 03.04.2020 12:05, Jiri Kosina wrote:
> It would still be possible to access the report via hidraw, and maybe 
> that's analogy of what the Windows driver/special Glorious software :) 
> does, I don't know. It's hard to believe that Windows would be actually 
> willing to report any changes coming through HID_MAIN_ITEM_CONSTANT 
> reports, but who knows.

I did some research of what other HID implementations do in this
situation and would like to share it here.

Windows, as we already know, does not seem to mind the CONST flag and
accepts the reports just fine. Of course, whether this is the general
behaviour or only a special case, we can only speculate, short of
emulating devices with the descriptors incorrect in some way or another,
either in software or with some sort of microcontroller. I haven't yet
set out to do this, but I might eventually.

macOS (tested a 10.13 hackintosh) accepts the reports just fine. This
platform is an interesting case, because Apple's HID stack is
open-source. Assuming I understand the code correctly, the logic which
filters out padding is found in HIDIsButtonOrValue.c of the IOHIDFamily
component. The file can be found here[1]. The author(?) helpfully
provides a description in the changelog:

11/1/99    BWS     [2405720]
    We need a better check for 'bit padding' items,                                                                                                                   
    rather than just is constant. We will check to make sure the
    item is constant, and has no usage, or zero usage.

I am not particularly well-versed in HID, but this sounds like a
reasonable solution. Is there anything preventing this approach in
Linux? While doing the initial research when I was working on the
original patch, I noticed some code was purposefully setting the CONST
flag in order to get reports ignored. Food for thought, especially for
someone who knows what they are doing, unlike me :D

FreeBSD, to my limited knowledge, only includes a basic HID driver
in the kernel, capable of boot protocol mice and keyboards. There is a
userspace daemon, uhidd, which grabs the raw ugen device and submits
keycodes to a virtual keyboard (or mouse) with more comprehensive
support for consumer control and such. It ignores the reports as can be
seen on L318 of uhidd_cc.c [2].

I don't currently have access to other platforms, although I don't even
know of any with a comprehensive HID implementation. Maybe game consoles?

Of course, I am not sure this is worth the effort in the first place, I
was just curious. Also, not sure if I mentioned this before, I reached
out to the hardware manufacturer about this issue, they haven't
responded. Not a surprise.

Sam

[1]: https://opensource.apple.com/source/IOHIDFamily/IOHIDFamily-1446.61.2/IOHIDSystem/IOHIDDescriptorParser/
[2]: https://github.com/kaiwang27/uhidd/blob/master/uhidd/uhidd_cc.c#L318