[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-input
Subject:    Re: [PATCH v3 01/12] HID: logitech-hidpp: Don't restart IO, instead defer hid_connect() only
From:       Hans de Goede <hdegoede () redhat ! com>
Date:       2023-10-25 19:02:38
Message-ID: eddd433b-6243-1479-7881-f33a6694783c () redhat ! com
[Download RAW message or body]

Hi,

On 10/25/23 18:43, Benjamin Tissoires wrote:
> On Oct 18 2023, Benjamin Tissoires wrote:
> > Hi Hans,
> > 
> > FWIW, your series looks good, but I came accross a small nitpick here:
> > 
> > On Oct 10 2023, Hans de Goede wrote:
> > > Restarting IO causes 2 problems:
> > > 
> > > 1. Some devices do not like IO being restarted this was addressed in
> > > commit 498ba2069035 ("HID: logitech-hidpp: Don't restart communication
> > > if not necessary"), but that change has issues of its own and needs to
> > > be reverted.
> > > 
> > > 2. Restarting IO and specifically calling hid_device_io_stop() causes
> > > received packets to be missed, which may cause connect-events to
> > > get missed.
> > > 
> > > Restarting IO was introduced in commit 91cf9a98ae41 ("HID: logitech-hidpp:
> > > make .probe usbhid capable") to allow to retrieve the device's name and
> > > serial number and store these in hdev->name and hdev->uniq before
> > > connecting any hid subdrivers (hid-input, hidraw) exporting this info
> > > to userspace.
> > > 
> > > But this does not require restarting IO, this merely requires deferring
> > > calling hid_connect(). Calling hid_hw_start() with a connect-mask of
> > > 0 makes it skip calling hid_connect(), so hidpp_probe() can simply call
> > > hid_connect() later without needing to restart IO.
> > > 
> > > Remove the stop + restart of IO and instead just call hid_connect() later
> > > to avoid the issues caused by restarting IO.
> > > 
> > > Now that IO is no longer stopped, hid_hw_close() must be called at the end
> > > of probe() to balance the hid_hw_open() done at the beginning probe().
> > > 
> > > This series has been tested on the following devices:
> > > Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0)
> > > Logitech M720 Triathlon (bluetooth, HID++ 4.5)
> > > Logitech M720 Triathlon (unifying, 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 LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0)
> > > Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0)
> > > 
> > > Fixes: 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not \
> > >                 necessary")
> > > Suggested-by: Benjamin Tissoires <bentiss@kernel.org>
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > > drivers/hid/hid-logitech-hidpp.c | 22 ++++++++++++----------
> > > 1 file changed, 12 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/hid/hid-logitech-hidpp.c \
> > > b/drivers/hid/hid-logitech-hidpp.c index a209d51bd247..aa4f232c4518 100644
> > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > @@ -4460,8 +4460,10 @@ static int hidpp_probe(struct hid_device *hdev, const \
> > > struct hid_device_id *id)  hdev->name);
> > > 
> > > 	/*
> > > -	 * Plain USB connections need to actually call start and open
> > > -	 * on the transport driver to allow incoming data.
> > > +	 * First call hid_hw_start(hdev, 0) to allow IO without connecting any
> > > +	 * hid subdrivers (hid-input, hidraw). This allows retrieving the dev's
> > > +	 * name and serial number and store these in hdev->name and hdev->uniq,
> > > +	 * before the hid-input and hidraw drivers expose these to userspace.
> > > 	 */
> > > 	ret = hid_hw_start(hdev, will_restart ? 0 : connect_mask);
> > > 	if (ret) {
> > > @@ -4519,19 +4521,14 @@ static int hidpp_probe(struct hid_device *hdev, const \
> > > struct hid_device_id *id)  flush_work(&hidpp->work);
> > > 
> > > 	if (will_restart) {
> > > -		/* Reset the HID node state */
> > > -		hid_device_io_stop(hdev);
> > > -		hid_hw_close(hdev);
> > > -		hid_hw_stop(hdev);
> > > -
> > > 		if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
> > > 			connect_mask &= ~HID_CONNECT_HIDINPUT;
> > > 
> > > 		/* Now export the actual inputs and hidraw nodes to the world */
> > > -		ret = hid_hw_start(hdev, connect_mask);
> > > +		ret = hid_connect(hdev, connect_mask);
> > 
> > On plain USB devices, we get a new warning here "io already started".
> > 
> > This is because hid_connect() will call hid_pidff_init() from
> > drivers/hid/usbhid/hid-pidff.c if connect_mask has the `HID_CONNECT_FF`
> > bit set.
> > 
> > And hid_pidff_init() blindly calls hid_device_io_start() then
> > hid_device_io_stop().
> > 
> > It's not a big issue, but still it's a new warning we have to tackly on.
> > 
> > I see 3 possible solutions:
> > - teach hid_pidff_init() to only start/stop the IOs if it's not already
> > done so
> > - if a device is actually connected through USB, call
> > hid_device_io_stop() before hid_connect()
> > - unconditionally call hid_device_io_stop() before hid_connect()
> > 
> > The latter has my current preference as we won't get biten in the future
> > if something else decides to change the io state.
> > 
> > However, do you thing it'll be an issue to disable IOs there?

Not really an issue, but if we disable IOs then we may loose
incoming packets with a connect event in there.

> > And maybe we should re-enable them after?

If we disable IOs before hid_connect() (or at any point
after enabling them) then connect events may be lost
so we must re-enable IOs then and move the hidpp_connect_event()
work queuing, which polls for already connected devices to
after the re-enabling.

Also IOs need to be re-enabled for the g920_get_config()
call later during hidpp_probe().

I have just written a patch for this and submitted it upstream :)

> > If it's fine to simply disable the IOs, we can add an extra patch on top
> > of this series to fix that warning in the USB case.
> > 
> > As mentioned above, I have tested with the T650, T651 that were likely to
> > be a problem, and they are working just fine :)
> > 
> > So with that minor fix, we should be able to stage this series.
> 
> The merge window is coming very soon. So I'm taking this series as it is
> (I just added the few devices I made the tests), and we can work on an
> extra patch to remove that warning.

Extra patch submitted :)

Regards,

Hans


[prev in list] [next in list] [prev in thread] [next in thread]