| Message ID | 20231010102029.111003-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO | expand |
Hi Hans, FWIW, your series looks good, but I came accross a small nitpick here: On Oct 10 2023, Hans de Goede wrote: > Restarting IO causes 2 problems: > > 1. Some devices do not like IO being restarted this was addressed in > commit 498ba2069035 ("HID: logitech-hidpp: Don't restart communication > if not necessary"), but that change has issues of its own and needs to > be reverted. > > 2. Restarting IO and specifically calling hid_device_io_stop() causes > received packets to be missed, which may cause connect-events to > get missed. > > Restarting IO was introduced in commit 91cf9a98ae41 ("HID: logitech-hidpp: > make .probe usbhid capable") to allow to retrieve the device's name and > serial number and store these in hdev->name and hdev->uniq before > connecting any hid subdrivers (hid-input, hidraw) exporting this info > to userspace. > > But this does not require restarting IO, this merely requires deferring > calling hid_connect(). Calling hid_hw_start() with a connect-mask of > 0 makes it skip calling hid_connect(), so hidpp_probe() can simply call > hid_connect() later without needing to restart IO. > > Remove the stop + restart of IO and instead just call hid_connect() later > to avoid the issues caused by restarting IO. > > Now that IO is no longer stopped, hid_hw_close() must be called at the end > of probe() to balance the hid_hw_open() done at the beginning probe(). > > This series has been tested on the following devices: > Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0) > Logitech M720 Triathlon (bluetooth, HID++ 4.5) > Logitech M720 Triathlon (unifying, HID++ 4.5) > Logitech K400 Pro (unifying, HID++ 4.1) > Logitech K270 (eQUAD nano Lite, HID++ 2.0) > Logitech M185 (eQUAD nano Lite, HID++ 4.5) > Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0) > Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0) > > Fixes: 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary") > Suggested-by: Benjamin Tissoires <bentiss@kernel.org> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/hid/hid-logitech-hidpp.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > index a209d51bd247..aa4f232c4518 100644 > --- a/drivers/hid/hid-logitech-hidpp.c > +++ b/drivers/hid/hid-logitech-hidpp.c > @@ -4460,8 +4460,10 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) > hdev->name); > > /* > - * Plain USB connections need to actually call start and open > - * on the transport driver to allow incoming data. > + * First call hid_hw_start(hdev, 0) to allow IO without connecting any > + * hid subdrivers (hid-input, hidraw). This allows retrieving the dev's > + * name and serial number and store these in hdev->name and hdev->uniq, > + * before the hid-input and hidraw drivers expose these to userspace. > */ > ret = hid_hw_start(hdev, will_restart ? 0 : connect_mask); > if (ret) { > @@ -4519,19 +4521,14 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) > flush_work(&hidpp->work); > > if (will_restart) { > - /* Reset the HID node state */ > - hid_device_io_stop(hdev); > - hid_hw_close(hdev); > - hid_hw_stop(hdev); > - > if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT) > connect_mask &= ~HID_CONNECT_HIDINPUT; > > /* Now export the actual inputs and hidraw nodes to the world */ > - ret = hid_hw_start(hdev, connect_mask); > + ret = hid_connect(hdev, connect_mask); On plain USB devices, we get a new warning here "io already started". This is because hid_connect() will call hid_pidff_init() from drivers/hid/usbhid/hid-pidff.c if connect_mask has the `HID_CONNECT_FF` bit set. And hid_pidff_init() blindly calls hid_device_io_start() then hid_device_io_stop(). It's not a big issue, but still it's a new warning we have to tackly on. I see 3 possible solutions: - teach hid_pidff_init() to only start/stop the IOs if it's not already done so - if a device is actually connected through USB, call hid_device_io_stop() before hid_connect() - unconditionally call hid_device_io_stop() before hid_connect() The latter has my current preference as we won't get biten in the future if something else decides to change the io state. However, do you thing it'll be an issue to disable IOs there? And maybe we should re-enable them after? If it's fine to simply disable the IOs, we can add an extra patch on top of this series to fix that warning in the USB case. As mentioned above, I have tested with the T650, T651 that were likely to be a problem, and they are working just fine :) So with that minor fix, we should be able to stage this series. Thanks again for your hard work Cheers, Benjamin > if (ret) { > - hid_err(hdev, "%s:hid_hw_start returned error\n", __func__); > - goto hid_hw_start_fail; > + hid_err(hdev, "%s:hid_connect returned error %d\n", __func__, ret); > + goto hid_hw_init_fail; > } > } > > @@ -4543,6 +4540,11 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) > ret); > } > > + /* > + * This relies on logi_dj_ll_close() being a no-op so that DJ connection > + * events will still be received. > + */ > + hid_hw_close(hdev); > return ret; > > hid_hw_init_fail: > -- > 2.41.0 >
On Oct 18 2023, Benjamin Tissoires wrote: > Hi Hans, > > FWIW, your series looks good, but I came accross a small nitpick here: > > On Oct 10 2023, Hans de Goede wrote: > > Restarting IO causes 2 problems: > > > > 1. Some devices do not like IO being restarted this was addressed in > > commit 498ba2069035 ("HID: logitech-hidpp: Don't restart communication > > if not necessary"), but that change has issues of its own and needs to > > be reverted. > > > > 2. Restarting IO and specifically calling hid_device_io_stop() causes > > received packets to be missed, which may cause connect-events to > > get missed. > > > > Restarting IO was introduced in commit 91cf9a98ae41 ("HID: logitech-hidpp: > > make .probe usbhid capable") to allow to retrieve the device's name and > > serial number and store these in hdev->name and hdev->uniq before > > connecting any hid subdrivers (hid-input, hidraw) exporting this info > > to userspace. > > > > But this does not require restarting IO, this merely requires deferring > > calling hid_connect(). Calling hid_hw_start() with a connect-mask of > > 0 makes it skip calling hid_connect(), so hidpp_probe() can simply call > > hid_connect() later without needing to restart IO. > > > > Remove the stop + restart of IO and instead just call hid_connect() later > > to avoid the issues caused by restarting IO. > > > > Now that IO is no longer stopped, hid_hw_close() must be called at the end > > of probe() to balance the hid_hw_open() done at the beginning probe(). > > > > This series has been tested on the following devices: > > Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0) > > Logitech M720 Triathlon (bluetooth, HID++ 4.5) > > Logitech M720 Triathlon (unifying, HID++ 4.5) > > Logitech K400 Pro (unifying, HID++ 4.1) > > Logitech K270 (eQUAD nano Lite, HID++ 2.0) > > Logitech M185 (eQUAD nano Lite, HID++ 4.5) > > Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0) > > Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0) > > > > Fixes: 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary") > > Suggested-by: Benjamin Tissoires <bentiss@kernel.org> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > drivers/hid/hid-logitech-hidpp.c | 22 ++++++++++++---------- > > 1 file changed, 12 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > > index a209d51bd247..aa4f232c4518 100644 > > --- a/drivers/hid/hid-logitech-hidpp.c > > +++ b/drivers/hid/hid-logitech-hidpp.c > > @@ -4460,8 +4460,10 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) > > hdev->name); > > > > /* > > - * Plain USB connections need to actually call start and open > > - * on the transport driver to allow incoming data. > > + * First call hid_hw_start(hdev, 0) to allow IO without connecting any > > + * hid subdrivers (hid-input, hidraw). This allows retrieving the dev's > > + * name and serial number and store these in hdev->name and hdev->uniq, > > + * before the hid-input and hidraw drivers expose these to userspace. > > */ > > ret = hid_hw_start(hdev, will_restart ? 0 : connect_mask); > > if (ret) { > > @@ -4519,19 +4521,14 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) > > flush_work(&hidpp->work); > > > > if (will_restart) { > > - /* Reset the HID node state */ > > - hid_device_io_stop(hdev); > > - hid_hw_close(hdev); > > - hid_hw_stop(hdev); > > - > > if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT) > > connect_mask &= ~HID_CONNECT_HIDINPUT; > > > > /* Now export the actual inputs and hidraw nodes to the world */ > > - ret = hid_hw_start(hdev, connect_mask); > > + ret = hid_connect(hdev, connect_mask); > > On plain USB devices, we get a new warning here "io already started". > > This is because hid_connect() will call hid_pidff_init() from > drivers/hid/usbhid/hid-pidff.c if connect_mask has the `HID_CONNECT_FF` > bit set. > > And hid_pidff_init() blindly calls hid_device_io_start() then > hid_device_io_stop(). > > It's not a big issue, but still it's a new warning we have to tackly on. > > I see 3 possible solutions: > - teach hid_pidff_init() to only start/stop the IOs if it's not already > done so > - if a device is actually connected through USB, call > hid_device_io_stop() before hid_connect() > - unconditionally call hid_device_io_stop() before hid_connect() > > The latter has my current preference as we won't get biten in the future > if something else decides to change the io state. > > However, do you thing it'll be an issue to disable IOs there? > And maybe we should re-enable them after? > > If it's fine to simply disable the IOs, we can add an extra patch on top > of this series to fix that warning in the USB case. > > As mentioned above, I have tested with the T650, T651 that were likely to > be a problem, and they are working just fine :) > > So with that minor fix, we should be able to stage this series. The merge window is coming very soon. So I'm taking this series as it is (I just added the few devices I made the tests), and we can work on an extra patch to remove that warning. Cheers, Benjamin > > Thanks again for your hard work > > Cheers, > Benjamin > > > if (ret) { > > - hid_err(hdev, "%s:hid_hw_start returned error\n", __func__); > > - goto hid_hw_start_fail; > > + hid_err(hdev, "%s:hid_connect returned error %d\n", __func__, ret); > > + goto hid_hw_init_fail; > > } > > } > > > > @@ -4543,6 +4540,11 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) > > ret); > > } > > > > + /* > > + * This relies on logi_dj_ll_close() being a no-op so that DJ connection > > + * events will still be received. > > + */ > > + hid_hw_close(hdev); > > return ret; > > > > hid_hw_init_fail: > > -- > > 2.41.0 > >
Hi, On 10/25/23 18:43, Benjamin Tissoires wrote: > On Oct 18 2023, Benjamin Tissoires wrote: >> Hi Hans, >> >> FWIW, your series looks good, but I came accross a small nitpick here: >> >> On Oct 10 2023, Hans de Goede wrote: >>> Restarting IO causes 2 problems: >>> >>> 1. Some devices do not like IO being restarted this was addressed in >>> commit 498ba2069035 ("HID: logitech-hidpp: Don't restart communication >>> if not necessary"), but that change has issues of its own and needs to >>> be reverted. >>> >>> 2. Restarting IO and specifically calling hid_device_io_stop() causes >>> received packets to be missed, which may cause connect-events to >>> get missed. >>> >>> Restarting IO was introduced in commit 91cf9a98ae41 ("HID: logitech-hidpp: >>> make .probe usbhid capable") to allow to retrieve the device's name and >>> serial number and store these in hdev->name and hdev->uniq before >>> connecting any hid subdrivers (hid-input, hidraw) exporting this info >>> to userspace. >>> >>> But this does not require restarting IO, this merely requires deferring >>> calling hid_connect(). Calling hid_hw_start() with a connect-mask of >>> 0 makes it skip calling hid_connect(), so hidpp_probe() can simply call >>> hid_connect() later without needing to restart IO. >>> >>> Remove the stop + restart of IO and instead just call hid_connect() later >>> to avoid the issues caused by restarting IO. >>> >>> Now that IO is no longer stopped, hid_hw_close() must be called at the end >>> of probe() to balance the hid_hw_open() done at the beginning probe(). >>> >>> This series has been tested on the following devices: >>> Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0) >>> Logitech M720 Triathlon (bluetooth, HID++ 4.5) >>> Logitech M720 Triathlon (unifying, HID++ 4.5) >>> Logitech K400 Pro (unifying, HID++ 4.1) >>> Logitech K270 (eQUAD nano Lite, HID++ 2.0) >>> Logitech M185 (eQUAD nano Lite, HID++ 4.5) >>> Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0) >>> Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0) >>> >>> Fixes: 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary") >>> Suggested-by: Benjamin Tissoires <bentiss@kernel.org> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> drivers/hid/hid-logitech-hidpp.c | 22 ++++++++++++---------- >>> 1 file changed, 12 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c >>> index a209d51bd247..aa4f232c4518 100644 >>> --- a/drivers/hid/hid-logitech-hidpp.c >>> +++ b/drivers/hid/hid-logitech-hidpp.c >>> @@ -4460,8 +4460,10 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) >>> hdev->name); >>> >>> /* >>> - * Plain USB connections need to actually call start and open >>> - * on the transport driver to allow incoming data. >>> + * First call hid_hw_start(hdev, 0) to allow IO without connecting any >>> + * hid subdrivers (hid-input, hidraw). This allows retrieving the dev's >>> + * name and serial number and store these in hdev->name and hdev->uniq, >>> + * before the hid-input and hidraw drivers expose these to userspace. >>> */ >>> ret = hid_hw_start(hdev, will_restart ? 0 : connect_mask); >>> if (ret) { >>> @@ -4519,19 +4521,14 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) >>> flush_work(&hidpp->work); >>> >>> if (will_restart) { >>> - /* Reset the HID node state */ >>> - hid_device_io_stop(hdev); >>> - hid_hw_close(hdev); >>> - hid_hw_stop(hdev); >>> - >>> if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT) >>> connect_mask &= ~HID_CONNECT_HIDINPUT; >>> >>> /* Now export the actual inputs and hidraw nodes to the world */ >>> - ret = hid_hw_start(hdev, connect_mask); >>> + ret = hid_connect(hdev, connect_mask); >> >> On plain USB devices, we get a new warning here "io already started". >> >> This is because hid_connect() will call hid_pidff_init() from >> drivers/hid/usbhid/hid-pidff.c if connect_mask has the `HID_CONNECT_FF` >> bit set. >> >> And hid_pidff_init() blindly calls hid_device_io_start() then >> hid_device_io_stop(). >> >> It's not a big issue, but still it's a new warning we have to tackly on. >> >> I see 3 possible solutions: >> - teach hid_pidff_init() to only start/stop the IOs if it's not already >> done so >> - if a device is actually connected through USB, call >> hid_device_io_stop() before hid_connect() >> - unconditionally call hid_device_io_stop() before hid_connect() >> >> The latter has my current preference as we won't get biten in the future >> if something else decides to change the io state. >> >> However, do you thing it'll be an issue to disable IOs there? Not really an issue, but if we disable IOs then we may loose incoming packets with a connect event in there. >> And maybe we should re-enable them after? If we disable IOs before hid_connect() (or at any point after enabling them) then connect events may be lost so we must re-enable IOs then and move the hidpp_connect_event() work queuing, which polls for already connected devices to after the re-enabling. Also IOs need to be re-enabled for the g920_get_config() call later during hidpp_probe(). I have just written a patch for this and submitted it upstream :) >> If it's fine to simply disable the IOs, we can add an extra patch on top >> of this series to fix that warning in the USB case. >> >> As mentioned above, I have tested with the T650, T651 that were likely to >> be a problem, and they are working just fine :) >> >> So with that minor fix, we should be able to stage this series. > > The merge window is coming very soon. So I'm taking this series as it is > (I just added the few devices I made the tests), and we can work on an > extra patch to remove that warning. Extra patch submitted :) Regards, Hans
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index a209d51bd247..aa4f232c4518 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -4460,8 +4460,10 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) hdev->name); /* - * Plain USB connections need to actually call start and open - * on the transport driver to allow incoming data. + * First call hid_hw_start(hdev, 0) to allow IO without connecting any + * hid subdrivers (hid-input, hidraw). This allows retrieving the dev's + * name and serial number and store these in hdev->name and hdev->uniq, + * before the hid-input and hidraw drivers expose these to userspace. */ ret = hid_hw_start(hdev, will_restart ? 0 : connect_mask); if (ret) { @@ -4519,19 +4521,14 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) flush_work(&hidpp->work); if (will_restart) { - /* Reset the HID node state */ - hid_device_io_stop(hdev); - hid_hw_close(hdev); - hid_hw_stop(hdev); - if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT) connect_mask &= ~HID_CONNECT_HIDINPUT; /* Now export the actual inputs and hidraw nodes to the world */ - ret = hid_hw_start(hdev, connect_mask); + ret = hid_connect(hdev, connect_mask); if (ret) { - hid_err(hdev, "%s:hid_hw_start returned error\n", __func__); - goto hid_hw_start_fail; + hid_err(hdev, "%s:hid_connect returned error %d\n", __func__, ret); + goto hid_hw_init_fail; } } @@ -4543,6 +4540,11 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) ret); } + /* + * This relies on logi_dj_ll_close() being a no-op so that DJ connection + * events will still be received. + */ + hid_hw_close(hdev); return ret; hid_hw_init_fail:
Restarting IO causes 2 problems: 1. Some devices do not like IO being restarted this was addressed in commit 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary"), but that change has issues of its own and needs to be reverted. 2. Restarting IO and specifically calling hid_device_io_stop() causes received packets to be missed, which may cause connect-events to get missed. Restarting IO was introduced in commit 91cf9a98ae41 ("HID: logitech-hidpp: make .probe usbhid capable") to allow to retrieve the device's name and serial number and store these in hdev->name and hdev->uniq before connecting any hid subdrivers (hid-input, hidraw) exporting this info to userspace. But this does not require restarting IO, this merely requires deferring calling hid_connect(). Calling hid_hw_start() with a connect-mask of 0 makes it skip calling hid_connect(), so hidpp_probe() can simply call hid_connect() later without needing to restart IO. Remove the stop + restart of IO and instead just call hid_connect() later to avoid the issues caused by restarting IO. Now that IO is no longer stopped, hid_hw_close() must be called at the end of probe() to balance the hid_hw_open() done at the beginning probe(). This series has been tested on the following devices: Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0) Logitech M720 Triathlon (bluetooth, HID++ 4.5) Logitech M720 Triathlon (unifying, HID++ 4.5) Logitech K400 Pro (unifying, HID++ 4.1) Logitech K270 (eQUAD nano Lite, HID++ 2.0) Logitech M185 (eQUAD nano Lite, HID++ 4.5) Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0) Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0) Fixes: 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary") Suggested-by: Benjamin Tissoires <bentiss@kernel.org> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/hid/hid-logitech-hidpp.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)