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.
> >
> > 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 :)
> >
> > 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).
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
>
>
>
> > 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
>
