| Message ID | 2283816.ElGaqSPkdT@kreacher (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8) | expand |
On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > 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 <rafael.j.wysocki@intel.com> > --- > 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 <hadess@hadess.net> wrote: > > On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > 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 <rafael.j.wysocki@intel.com> > > --- > > 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 <hadess@hadess.net> wrote: > > On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > 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:48 +0100, Benjamin Tissoires wrote: > On Wed, Dec 7, 2022 at 10:29 AM Bastien Nocera <hadess@hadess.net> > wrote: > > > > On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > 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, 2022-12-07 at 10:47 +0100, Rafael J. Wysocki wrote: > On Wed, Dec 7, 2022 at 10:29 AM Bastien Nocera <hadess@hadess.net> > wrote: > > > > On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > 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 <rafael.j.wysocki@intel.com> > > > --- > > > 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:47 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Dec 7, 2022 at 10:29 AM Bastien Nocera <hadess@hadess.net> wrote: > > > > On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > 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 <rafael.j.wysocki@intel.com> > > > --- > > > 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 11:05 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Wed, Dec 7, 2022 at 10:47 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Wed, Dec 7, 2022 at 10:29 AM Bastien Nocera <hadess@hadess.net> wrote: > > > > > > On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > 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). Obviously that would work for me too, so it's your call. Thanks!
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+. Thanks,
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.
On Wed, Dec 7, 2022 at 1:43 PM Bastien Nocera <hadess@hadess.net> 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. Well, that's why I sent patches to address this particular case without possibly breaking anything else. Improvements can be made on top of them and the blocklist entry added by patch [2/2] need not stay there forever, FWIW.
On Wed, Dec 7, 2022 at 2:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Dec 7, 2022 at 1:43 PM Bastien Nocera <hadess@hadess.net> 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? It's not so much about how many reports, but *what* the end result is. If the device were working-ish, that would have been OK. But here the device is completely ignored by the kernel which basically enters the "no regression rule". > > > > 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. I know, and I feel your pain as I was about to have the same last week for HID-BPF. But as much as I hate dropping patches from the queue, not being able to have at least a week to fix it properly ends up with "fixes" that are broken and that might break other devices. Talking from experience as my first fix from last week was exactly in that category. > > Well, that's why I sent patches to address this particular case > without possibly breaking anything else. My concern is more that we now have a data point were the series broke a device (pretty badly) and if (when) this happens shortly after 6.1 is getting released, we would have to say, oh yes, we know, so we need to patch the kernel because our driver is buggy, and we knew it. This is not acceptable, and I am sure that if Linus reads that thread he would revert the 2 patches or maybe more. > > Improvements can be made on top of them and the blocklist entry added > by patch [2/2] need not stay there forever, FWIW. > I need to check with Jiri, but there is a chance we can re-introduce this in 6.2. This way we will have slightly more time to fix it in a proper way. Cheers, Benjamin
On Wed, Dec 7, 2022 at 2:25 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Wed, Dec 7, 2022 at 2:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Wed, Dec 7, 2022 at 1:43 PM Bastien Nocera <hadess@hadess.net> 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? > > It's not so much about how many reports, but *what* the end result is. > If the device were working-ish, that would have been OK. But here the > device is completely ignored by the kernel which basically enters the > "no regression rule". > > > > > > > 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. > > I know, and I feel your pain as I was about to have the same last week > for HID-BPF. But as much as I hate dropping patches from the queue, > not being able to have at least a week to fix it properly ends up with > "fixes" that are broken and that might break other devices. Talking > from experience as my first fix from last week was exactly in that > category. > > > > > Well, that's why I sent patches to address this particular case > > without possibly breaking anything else. > > My concern is more that we now have a data point were the series broke > a device (pretty badly) and if (when) this happens shortly after 6.1 > is getting released, we would have to say, oh yes, we know, so we need > to patch the kernel because our driver is buggy, and we knew it. This > is not acceptable, and I am sure that if Linus reads that thread he > would revert the 2 patches or maybe more. Well, I agree. > > > > Improvements can be made on top of them and the blocklist entry added > > by patch [2/2] need not stay there forever, FWIW. > > > > I need to check with Jiri, but there is a chance we can re-introduce > this in 6.2. This way we will have slightly more time to fix it in a > proper way. Sounds good to me.
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. Dec 07 15:17:49 classic kernel: logitech-hidpp-device 0005:046D:B342.000C: unknown main item tag 0x0 Dec 07 15:17:49 classic kernel: logitech-hidpp-device 0005:046D:B342.000C: HID++ 4.5 device connected. Dec 07 15:17:50 classic kernel: input: Logitech Bluetooth Multi-Device Keyboard K380 as /devices/pci0000:00/0000:00:14.0/usb1/1-9/1-9:1.0/bluetooth/hci0/hci0:256/0005:046D:B342.000C/input/input36 Dec 07 15:17:50 classic kernel: logitech-hidpp-device 0005:046D:B342.000C: input,hidraw5: BLUETOOTH HID v42.01 Keyboard [Logitech Bluetooth Multi-Device Keyboard K380] on 8c:c6:81:15:0c:6f $ sudo ./_build/src/tools/hidpp-list-features /dev/hidraw5 Logitech Bluetooth Multi-Device Keyboard K380 (046d:b342) is a HID++ 4.5 device Feature 0x01: [0x0001] Feature set Feature 0x02: [0x0003] Device FW version Feature 0x03: [0x0005] Device name Feature 0x04: [0x0007] Device Friendly Name Feature 0x05: [0x0020] Reset Feature 0x06: [0x1000] Battery status Feature 0x07: [0x1814] Change host Feature 0x08: [0x1815] Hosts info Feature 0x09: [0x1b04] Reprog controls v4 Feature 0x0a: [0x1e00] Enable hidden features (hidden) Feature 0x0b: [0x40a2] New fn inversion Feature 0x0c: [0x4220] Lock key state Feature 0x0d: [0x4521] Keyboard disable Feature 0x0e: [0x4531] Multiplatform
On Wed, Dec 7, 2022 at 3:24 PM Bastien Nocera <hadess@hadess.net> 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
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) }, {} };