On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Evidently, Logitech Bluetooth Mouse M336/M337/M535 (0xb016) does not
> work when HID++ is enabled for it,
This needs the output of the hidpp-list-features tool mentioned earlier
in the thread so we can avoid words like "evidently" and provide
concrete proof.
But why is it needed in this case? We purposefully try to avoid blanket
blocklists. The lack of HID++ can be probed, so the device should work
just as it used to (if the fallback code works).
We should only list devices that need special handling, and the ones
that don't work once HID++ was probed unsuccessfully.
> so add it to the list of devices
> that are not handled by logitech-hidpp.
>
> Fixes: 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the
> Logitech Bluetooth devices")
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/hid/hid-logitech-hidpp.c | 1 +
> 1 file changed, 1 insertion(+)
>
> Index: linux-pm/drivers/hid/hid-logitech-hidpp.c
> ===================================================================
> --- linux-pm.orig/drivers/hid/hid-logitech-hidpp.c
> +++ linux-pm/drivers/hid/hid-logitech-hidpp.c
> @@ -4274,6 +4274,7 @@ static const struct hid_device_id unhand
> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> USB_DEVICE_ID_LOGITECH_HARMONY_PS3) },
> /* Handled in hid-generic */
> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> USB_DEVICE_ID_LOGITECH_DINOVO_EDGE_KBD) },
> + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb016) },
> {}
> };
>
>
>
>
On Wed, Dec 7, 2022 at 10:29 AM Bastien Nocera <[email protected]> wrote:
>
> On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Evidently, Logitech Bluetooth Mouse M336/M337/M535 (0xb016) does not
> > work when HID++ is enabled for it,
>
> This needs the output of the hidpp-list-features tool mentioned earlier
> in the thread so we can avoid words like "evidently" and provide
> concrete proof.
Well, so point me to a binary of this, please.
> But why is it needed in this case?
Because it doesn't work otherwise.
> We purposefully try to avoid blanket
> blocklists. The lack of HID++ can be probed, so the device should work
> just as it used to (if the fallback code works).
No, because the hid-generic driver has no way to check that the probe
function of your driver fails for this particular device. The probing
of hid-generic will fail so long as the device matches the device ID
list of any specific HID driver. With patch [1/2] from this series
applied this is unless that specific driver has a ->match() callback
rejecting the given device.
You'd need a list of drivers that have been tried and failed somewhere
for that and AFAICS no such list is present in the code.
So a minimum fix for 6.1 that actually works for me is to add the
non-working device to the blocklist. More sophisticated stuff can be
done later.
> We should only list devices that need special handling, and the ones
> that don't work once HID++ was probed unsuccessfully.
>
> > so add it to the list of devices
> > that are not handled by logitech-hidpp.
> >
> > Fixes: 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the
> > Logitech Bluetooth devices")
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > drivers/hid/hid-logitech-hidpp.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > Index: linux-pm/drivers/hid/hid-logitech-hidpp.c
> > ===================================================================
> > --- linux-pm.orig/drivers/hid/hid-logitech-hidpp.c
> > +++ linux-pm/drivers/hid/hid-logitech-hidpp.c
> > @@ -4274,6 +4274,7 @@ static const struct hid_device_id unhand
> > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> > USB_DEVICE_ID_LOGITECH_HARMONY_PS3) },
> > /* Handled in hid-generic */
> > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> > USB_DEVICE_ID_LOGITECH_DINOVO_EDGE_KBD) },
> > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb016) },
> > {}
> > };
On Wed, Dec 7, 2022 at 10:29 AM Bastien Nocera <[email protected]> wrote:
>
> On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Evidently, Logitech Bluetooth Mouse M336/M337/M535 (0xb016) does not
> > work when HID++ is enabled for it,
>
> This needs the output of the hidpp-list-features tool mentioned earlier
> in the thread so we can avoid words like "evidently" and provide
> concrete proof.
>
> But why is it needed in this case? We purposefully try to avoid blanket
> blocklists. The lack of HID++ can be probed, so the device should work
> just as it used to (if the fallback code works).
If I read the probe function correctly, we should have the HID++
reports present, so a static analysis will not allow us to detect that
information.
However, this reminds me of the Litra Glow[0]. On this device,
hidpp_root_get_protocol_version() also reports an error when it is
obvious it is connected.
And AFAICT, a BLE device is supposed to always be connected when it is
presented to the kernel (because disconnect is handled in the BLE
layer, in bluez).
Apparently Rafael's mouse is Bluetooth classic, so I have a doubt on
what happens when it goes into low power.
>
> We should only list devices that need special handling, and the ones
> that don't work once HID++ was probed unsuccessfully.
>
Given the current state of Rafael's mouse it goes into the second
category. But I suspect we should be smarter in the probe's decision
to consider if a device is connected or not.
Cheers,
Benjamin
[0] https://lore.kernel.org/linux-input/CABfF9mO3SQZvkQGOC09H5s7EEd2UGhpE=GYB46g_zF3aEOVn=Q@mail.gmail.com/
On Wed, 2022-12-07 at 10:47 +0100, Rafael J. Wysocki wrote:
> On Wed, Dec 7, 2022 at 10:29 AM Bastien Nocera <[email protected]>
> wrote:
> >
> > On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > Evidently, Logitech Bluetooth Mouse M336/M337/M535 (0xb016) does
> > > not
> > > work when HID++ is enabled for it,
> >
> > This needs the output of the hidpp-list-features tool mentioned
> > earlier
> > in the thread so we can avoid words like "evidently" and provide
> > concrete proof.
>
> Well, so point me to a binary of this, please.
>
> > But why is it needed in this case?
>
> Because it doesn't work otherwise.
>
> > We purposefully try to avoid blanket
> > blocklists. The lack of HID++ can be probed, so the device should
> > work
> > just as it used to (if the fallback code works).
>
> No, because the hid-generic driver has no way to check that the probe
> function of your driver fails for this particular device. The
> probing
> of hid-generic will fail so long as the device matches the device ID
> list of any specific HID driver. With patch [1/2] from this series
> applied this is unless that specific driver has a ->match() callback
> rejecting the given device.
>
> You'd need a list of drivers that have been tried and failed
> somewhere
> for that and AFAICS no such list is present in the code.
This code will start the generic HID stack if the device doesn't
support HID++:
hidpp->supported_reports = hidpp_validate_device(hdev);
if (!hidpp->supported_reports) {
hid_set_drvdata(hdev, NULL);
devm_kfree(&hdev->dev, hidpp);
return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
}
But in your case the device supports HID++ enough to go past that, but
fails to get the HID++ version, which makes the driver think it's not
connected, and just bails.
> So a minimum fix for 6.1 that actually works for me is to add the
> non-working device to the blocklist. More sophisticated stuff can be
> done later.
>
> > We should only list devices that need special handling, and the
> > ones
> > that don't work once HID++ was probed unsuccessfully.
> >
> > > so add it to the list of devices
> > > that are not handled by logitech-hidpp.
> > >
> > > Fixes: 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all
> > > the
> > > Logitech Bluetooth devices")
> > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > ---
> > > drivers/hid/hid-logitech-hidpp.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > Index: linux-pm/drivers/hid/hid-logitech-hidpp.c
> > > =================================================================
> > > ==
> > > --- linux-pm.orig/drivers/hid/hid-logitech-hidpp.c
> > > +++ linux-pm/drivers/hid/hid-logitech-hidpp.c
> > > @@ -4274,6 +4274,7 @@ static const struct hid_device_id unhand
> > > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> > > USB_DEVICE_ID_LOGITECH_HARMONY_PS3) },
> > > /* Handled in hid-generic */
> > > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> > > USB_DEVICE_ID_LOGITECH_DINOVO_EDGE_KBD) },
> > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb016) },
> > > {}
> > > };
On Wed, 2022-12-07 at 10:48 +0100, Benjamin Tissoires wrote:
> On Wed, Dec 7, 2022 at 10:29 AM Bastien Nocera <[email protected]>
> wrote:
> >
> > On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > Evidently, Logitech Bluetooth Mouse M336/M337/M535 (0xb016) does
> > > not
> > > work when HID++ is enabled for it,
> >
> > This needs the output of the hidpp-list-features tool mentioned
> > earlier
> > in the thread so we can avoid words like "evidently" and provide
> > concrete proof.
> >
> > But why is it needed in this case? We purposefully try to avoid
> > blanket
> > blocklists. The lack of HID++ can be probed, so the device should
> > work
> > just as it used to (if the fallback code works).
>
> If I read the probe function correctly, we should have the HID++
> reports present, so a static analysis will not allow us to detect
> that
> information.
>
> However, this reminds me of the Litra Glow[0]. On this device,
> hidpp_root_get_protocol_version() also reports an error when it is
> obvious it is connected.
On the Litra Glow, the error isn't HIDPP_ERROR_RESOURCE_ERROR, but
HIDPP20_ERROR_UNSUPPORTED (0x09). I have a patch to add those constants
to the driver.
> And AFAICT, a BLE device is supposed to always be connected when it
> is
> presented to the kernel (because disconnect is handled in the BLE
> layer, in bluez).
>
> Apparently Rafael's mouse is Bluetooth classic, so I have a doubt on
> what happens when it goes into low power.
It would probably just disconnect after a timeout. Reconnection isn't
as fast as with BLE, but it's fast enough.
> > We should only list devices that need special handling, and the
> > ones
> > that don't work once HID++ was probed unsuccessfully.
> >
>
> Given the current state of Rafael's mouse it goes into the second
> category. But I suspect we should be smarter in the probe's decision
> to consider if a device is connected or not.
Sure, and that's the data I'm trying to get out of the device.
>
> Cheers,
> Benjamin
>
> [0]
> https://lore.kernel.org/linux-input/CABfF9mO3SQZvkQGOC09H5s7EEd2UGhpE=GYB46g_zF3aEOVn=Q@mail.gmail.com/
>
On Wed, Dec 7, 2022 at 10:47 AM Rafael J. Wysocki <[email protected]> wrote:
>
> On Wed, Dec 7, 2022 at 10:29 AM Bastien Nocera <[email protected]> wrote:
> >
> > On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > Evidently, Logitech Bluetooth Mouse M336/M337/M535 (0xb016) does not
> > > work when HID++ is enabled for it,
> >
> > This needs the output of the hidpp-list-features tool mentioned earlier
> > in the thread so we can avoid words like "evidently" and provide
> > concrete proof.
>
> Well, so point me to a binary of this, please.
>
> > But why is it needed in this case?
>
> Because it doesn't work otherwise.
>
> > We purposefully try to avoid blanket
> > blocklists. The lack of HID++ can be probed, so the device should work
> > just as it used to (if the fallback code works).
>
> No, because the hid-generic driver has no way to check that the probe
> function of your driver fails for this particular device. The probing
> of hid-generic will fail so long as the device matches the device ID
> list of any specific HID driver. With patch [1/2] from this series
> applied this is unless that specific driver has a ->match() callback
> rejecting the given device.
>
> You'd need a list of drivers that have been tried and failed somewhere
> for that and AFAICS no such list is present in the code.
That is the reason why I never wanted to enable HID++ on all Logitech
mice, and this comes back to bite us at the worst time possible, right
before the merge window opens :(
>
> So a minimum fix for 6.1 that actually works for me is to add the
> non-working device to the blocklist. More sophisticated stuff can be
> done later.
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).
Cheers,
Benjamin
>
> > We should only list devices that need special handling, and the ones
> > that don't work once HID++ was probed unsuccessfully.
> >
> > > so add it to the list of devices
> > > that are not handled by logitech-hidpp.
> > >
> > > Fixes: 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the
> > > Logitech Bluetooth devices")
> > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > ---
> > > drivers/hid/hid-logitech-hidpp.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > Index: linux-pm/drivers/hid/hid-logitech-hidpp.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/hid/hid-logitech-hidpp.c
> > > +++ linux-pm/drivers/hid/hid-logitech-hidpp.c
> > > @@ -4274,6 +4274,7 @@ static const struct hid_device_id unhand
> > > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> > > USB_DEVICE_ID_LOGITECH_HARMONY_PS3) },
> > > /* Handled in hid-generic */
> > > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> > > USB_DEVICE_ID_LOGITECH_DINOVO_EDGE_KBD) },
> > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb016) },
> > > {}
> > > };
>