On Oct 09 2023, Hans de Goede wrote:
> 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 ?
I'd like to have this list recorded in the git tree. In case we have to
do forensic once again. Whether it's added in all messages is up to you
I think. But honestly, maybe add those only when you actually tested the
patch. No?
>
> > 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 ?
I tried to do a little bit of digging. Initially I thought that's
because I was always doing that stop/start dance, and so I continued
with that.
But looking slightly more carefully, I think I understand why:
- calling twice hid_connect should fail, because it'll create twice the
sysfs nodes for the HID device itself
- so we need to call hid_disconnect() first
- this was done through hid_hw_stop() and given that the dj driver has
empty stubs for start/stop, this was semantically the same.
So using hid_hw_stop/start was a shortcut because DJ had empty stubs.
But when I enabled direct use of hidpp from other transport layers, I
did not realized that there was this glitch.
And then I completely forgot to clean up that because "at the end of a
HID driver we are supposed to call hid_hw_start()".
>
> Let me rework the series to use that tomorrow. This will probably also
> allow dropping a bunch of the patches.
Yeah, I think we should be able to remove the sync mechanism.
And be careful to call hid_disconnect() before
hid_connect(connect_mask), or you'll probably have errors if not when
plugging the device, but when unplugging it for sure.
Cheers,
Benjamin
>
> 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
> >
> >>
> >
>
