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@xxxxxxxxxx>
>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>>> ---
>>> 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
