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. > > This series is best explained by a brief sketch of how probe() > looks at the end of the series (1): > > 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 . > > 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) 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(-) >
