Re: [PATCH v2 00/14] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO

[Hans de Goede <hdegoede@redhat.com>]

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