Re: [PATCH v2 00/14] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO

[Hans de Goede <hdegoede@redhat.com>]

Hi Benjamin,

On 10/9/23 10:13, Benjamin Tissoires wrote:
> 
> Hi Hans,
> 
> On Oct 08 2023, Hans de Goede wrote:
>> Hi,
>>
>> On 10/8/23 11:54, Hans de Goede wrote:
>>> Hi Benjamin,
>>>
>>> Here is a v2 of my series to fix issues with hidpp_connect_event() running
>>> while restarting IO, which now also fixes the issues you mentioned with
>>> potentially missing connect events.
> 
> Great, thanks a lot for this hard work.

You're welcome.

>>> This series is best explained by a brief sketch of how probe()
>>> looks at the end of the series (1):
> 
> TBH, I couldn't parse the following yesterday evening, but after looking
> at all patches one by one I can now get it :)

I'm glad you get it now :)

> 
>>>
>>> Prep work:
>>>
>>> 1. All code depending on a device being in connected state is moved to
>>>    the hidpp_connect_event() workqueue item
>>>
>>> 2. hidpp_connect_event() now checks the connected state itself by
>>>    checking that hidpp_root_get_protocol_version() succeeds, instead
>>>    of relying on possibly stale (racy) data in struct hidpp_device
>>>
>>> With this in place the new probe() sequence looks like this:
>>>
>>> 1. enable_connect_event flag starts at 0, this filters out / ignores any
>>>    connect-events in hidpp_raw_hidpp_event() avoiding
>>>    hidpp_connect_event() getting queued before the IO restart
>>>
>>> 2. IO is started with connect-mask = 0
>>>    this avoids hid-input and hidraw connecting while probe() is setting
>>>    hdev->name and hdev->uniq
>>>
>>> 3. Name and serialnr are retrieved and stored in hdev
>>>
>>> 4. IO is fully restarted (including hw_open + io_start, not just hw_start)
>>>    with the actual connect-mask.
>>>
>>> 5. enable_connect_event atomic_t is set to 1 to enable processing of
>>>    connect events.
>>>
>>> 6. hidpp_connect_event() is queued + flushed to query the connected state
>>>    and do the connect work if the device is connected.
>>>
>>> 7. probe() now ends with:
>>>
>>>         /*
>>>          * This relies on logi_dj_ll_close() being a no-op so that
>>>          * DJ connection events will still be received.
>>>          */
>>>         hid_hw_close(hdev);
>>>
>>>    Since on restarting IO it now is fully restarted so the hid_hw_open()
>>>    there needs to be balanced. 
>>>
>>> This series now obviously is no longer 6.6 material, instead I hope we
>>> can get this rework (and IMHO nice cleanup) into 6.7 .
> 
> Yeah, not 6.6 anymore, but should be doable for 6.7.
> 
>>>
>>> Regards,
>>>
>>> Hans
>>
>> I forgot to add info on the list of devices I tested this on:
>>
>> Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0)
>> Logitech M720 Triathlon (bluetooth, 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 Keyboard LX501 (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0)
>> Logitech 27Mhz mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0)
> 
> We should probably add this list to the commit messages.
> I'll need to test also myself on some problematic devices that have a
> special case (WTP, USB wired, BLE).

So you want to add this to all 14 commit messages ?

> Anyway, I'll have to test everything, but this looks like it's in a
> better shape than previously.
> 
> However, the thing I am afraid is that commit 498ba2069035 ("HID:
> logitech-hidpp: Don't restart communication if not necessary") was
> fixing devices that did not like the hid_hw_stop/start. I can't find the
> bug numbers however... So with your series, we might breaking those
> once again.
> 
> How about we do the following (in pseudo code):
> probe():
>   hidpp_connect_and_start(connect_mask = 0)
>   // retrieve name and serial
>   hid_connect(connect_mask) // with connect_mask ensuring we don't
>                             // create inputs if HIDPP_QUIRK_DELAYED_INIT
>                             // is set, instead of stop/start
>   hid_hw_close(hdev); // to balance hidpp_connect_and_start()
> 
> I think the above should even remove the need for the
> enable_connect_event atomic_t given that now we are not restarting the
> devices at all.

Interesting yes that looks good, any idea why this was not done
in commit 91cf9a98ae41 ("HID: logitech-hidpp: make .probe usbhid capable")
right away ?

Let me rework the series to use that tomorrow. This will probably also
allow dropping a bunch of the patches.

Regards,

Hans




> 
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>> 1) For reviewing you may also want to apply the entire series and look
>>> at the end result to help you understand why various intermediate steps
>>> are taken.
>>>
>>>
>>> Hans de Goede (14):
>>>   HID: logitech-hidpp: Revert "Don't restart communication if not
>>>     necessary"
>>>   HID: logitech-hidpp: Move hidpp_overwrite_name() to before connect
>>>     check
>>>   HID: logitech-hidpp: Add hidpp_non_unifying_init() helper
>>>   HID: logitech-hidpp: Remove connected check for non-unifying devices
>>>   HID: logitech-hidpp: Move get_wireless_feature_index() check to
>>>     hidpp_connect_event()
>>>   HID: logitech-hidpp: Remove wtp_get_config() call from probe()
>>>   HID: logitech-hidpp: Remove connected check for g920_get_config() call
>>>   HID: logitech-hidpp: Add a hidpp_connect_and_start() helper
>>>   HID: logitech-hidpp: Move the connected check to after restarting IO
>>>   HID: logitech-hidpp: Move g920_get_config() to just before
>>>     hidpp_ff_init()
>>>   HID: logitech-hidpp: Remove unused connected param from *_connect()
>>>   HID: logitech-hidpp: Fix connect event race
>>>   HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe()
>>>     restarts IO
>>>   HID: logitech-hidpp: Drop delayed_work_cb()
>>>
>>>  drivers/hid/hid-logitech-hidpp.c | 211 +++++++++++++------------------
>>>  1 file changed, 91 insertions(+), 120 deletions(-)
>>>
> 
> I like when the total number of deletions is higher than the additions
> :)
> 
> Cheers,
> Benjamin
> 
>>
>