Hi,
On 10/10/23 11:36, Benjamin Tissoires wrote:
On Oct 09 2023, Hans de Goede wrote:
<snip>
quoted
quoted
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()".
quoted
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.
Note that hid_hw_start() never calls hid_connect() when called with
a connect_mask of 0, so just simply calling hid_connect() later should
be fine without needing to do a hid_disconnect() before:
int hid_hw_start(struct hid_device *hdev, unsigned int connect_mask)
{
int error;
error = hdev->ll_driver->start(hdev);
if (error)
return error;
if (connect_mask) {
error = hid_connect(hdev, connect_mask);
if (error) {
hdev->ll_driver->stop(hdev);
return error;
}
}
return 0;
}
EXPORT_SYMBOL_GPL(hid_hw_start);
Regards,
Hans