Re: [PATCH v1 2/2] HID: logitech-hidpp: Add Bluetooth Mouse M336/M337/M535 to unhandled_hidpp_devices[]

[Benjamin Tissoires]

On Wed, Dec 7, 2022 at 3:24 PM Bastien Nocera <hadess@xxxxxxxxxx> wrote:
>
> On Wed, 2022-12-07 at 13:43 +0100, Bastien Nocera wrote:
> > On Wed, 2022-12-07 at 11:19 +0100, Jiri Kosina wrote:
> > > On Wed, 7 Dec 2022, Benjamin Tissoires wrote:
> > >
> > > > Agree, but OTOH, Rafael, your mouse is not brand new AFAICT, so I
> > > > am
> > > > worried that you won't be the only one complaining we just killed
> > > > their
> > > > mouse. So I think the even wiser solution would be to delay (and
> > > > so
> > > > revert in 6.1 or 6.2) the 2 patches that enable hid++ on all
> > > > logitech
> > > > mice (8544c812e43ab7bdf40458411b83987b8cba924d and
> > > > 532223c8ac57605a10e46dc0ab23dcf01c9acb43).
> > >
> > > If we were not at -rc8 timeframe, I'd be in favor to coming up with
> > > proper
> > > fix still for 6.1. But as things currently are, let's just revert
> > > those
> > > and reschedule them with proper fix for 6.2+.
> >
> > Has anyone seen any other reports?
> >
> > Because, honestly, seeing work that adds support for dozens of
> > devices
> > getting tossed out at the last minute based on a single report with
> > no
> > opportunity to fix the problem is really frustrating.
>
> FWIW, I went out to buy a Logitech device that uses Bluetooth Classic,
> the only one I could find in 2 different shops among dozens of Logitech
> devices, tested it, and it worked correctly.

Again, I understand the frustration. But the problem is not so much
that we might or might not ever need another entry in that list. The
problem is that some devices were supported previously (not in a fancy
way), and now we have a chance to just disable those devices. Of
course, we could say "just rmmod hid-logitech-hidpp". I have already
been through that as well, and then you fight for 10 years on some
forums where everybody says that if you have an issue with your
touchscreen, just disable <insert any driver here> when the particular
touchscreen is *not* using that driver at all.

Anyway, let me write down my thoughts since yesterday:
1. Rafael already realized that the ->match() function was not working
outside any other driver than hid-generic (and this was the design at
the time)
2. We have an issue in hid-logitech-hidpp where during probe calling
hidpp_root_get_protocol_version() returns an error, when userspace
tools are working fine for the exact same command
3. IMO, the way hid-logitech-hidpp probe function is behaving is not
resilient enough to be able to have a generic catch-all, because there
is a non-zero chance the probe returns -ENODEV (see all the exit paths
that return -ENODEV in probe).

To solve 1, it needs a little bit of tinkering and Rafael already sent
a v1 for that. IMO we should refine it, but that's an already ongoing
process

To solve 2, Bastien already mentioned one piece of the puzzle (the
error code not being correctly reported and the signification changed
between HID++ 1.0 and 2.0). But I am still yet to understand why there
is a difference between userspace call of the function, and kernel
space.

To solve 3, I initially started to work on a simple, more resilient
probe in hid-logitech-hidpp. I thought that we could regroup all
device initialization we do in a hidpp_preinit() call, and if that
call fails, revert to the generic hid processing.

But then, looking at the bigger picture, it would make sense to not do
that exactly. Instead of returning 0 and handling the device through
hid-logitech-hidpp, maybe we should actually return -ENODEV, and have
a fallback mechanism in hid-core that says "it seems I have tried all
possible drivers, all of them failed, let's force hid-generic for this
one".

And as I type those lines, how about the cases when we actually want
to disable a USB interface from HID because it is legitimate to do so?

I'll need to think about this a little bit more.

To be able to reintroduce the bluetooth catch-all, I think we need to
solve 1 and 3. 2 would be nice to understand but is not preventing the
series from being merged back.

Cheers,
Benjamin