Hi,
On 10/10/23 11:36, Benjamin Tissoires wrote:
> On Oct 09 2023, Hans de Goede wrote:
<snip>
>>> 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.
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
