Hi Hans,
On Oct 08 2023, Hans de Goede wrote:
Hi,
On 10/8/23 11:54, Hans de Goede wrote:
quoted
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.
quoted
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 :)
quoted
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.
quoted
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).
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.
Regards,
Hans
quoted
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