HID, driver interaction with device during probe defeated by 4ea5454203d991ec85264f64f89ca8855fce69b0

HID, driver interaction with device during probe defeated by 4ea5454203d991ec85264f64f89ca8855fce69b0

[Bruno Prémont]

Hi,

picolcd driver stopped working (probe fails) recently, probably in
relation with commit 4ea5454203d991ec85264f64f89ca8855fce69b0
(HID: Fix race condition between driver core and ll-driver).

The probe code does send a report to the HID device to query version
information but times-out getting the response.

What would be the proper approach to fix this chicken-egg problem?
Semaphore introduced by above patch is only being released after
driver probe function has returned but prevents and incoming reports
from being delivered.

Touching the semaphore inside driver looks like a bad idea to me...

Thanks,
Bruno

Re: HID, driver interaction with device during probe defeated by 4ea5454203d991ec85264f64f89ca8855fce69b0

[David Herrmann]

Hi

On Mon, Apr 9, 2012 at 12:03 AM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> Hi,
>
> picolcd driver stopped working (probe fails) recently, probably in
> relation with commit 4ea5454203d991ec85264f64f89ca8855fce69b0
> (HID: Fix race condition between driver core and ll-driver).
>
> The probe code does send a report to the HID device to query version
> information but times-out getting the response.

If there is no driver loaded we discard all messages. During probe we
consider a driver unloaded and picolcd seems to send a lot of requests
during probe. So it does make sense that this patch causes the
troubles.

> What would be the proper approach to fix this chicken-egg problem?
> Semaphore introduced by above patch is only being released after
> driver probe function has returned but prevents and incoming reports
> from being delivered.
>
> Touching the semaphore inside driver looks like a bad idea to me...

The initial idea of the semaphore was to avoid that during probe() and
remove() other HID callbacks are called. Otherwise, other drivers will
fail (the commit message explains this in more detail). Therefore, the
design of picolcd seems broken with respect to the semaphore.
The only idea I currently have is adding a flag that releases the
semaphore during driver->probe() and driver->remove() callbacks. I
think the driver-core uses its own locks so we will not get races
here. However, other HID drivers probably require proper
synchronization during probe() and remove() (which is totally
reasonable) as their internal structure might not be initialized, yet.
picolcd has several races here, too. hid_set_drvdata() is not
thread-safe or atomic so the raw-callback might get invalid "data"
pointers if we remove the semaphore again.

The nicest solution probably is releasing the semaphore during
hid_hw_start(). That way every driver is prepared to receive data.
However, it's an ugly solution because we release the lock not in the
same function as we acquire it. We also need to catch cases where
probe() actually did never call hid_hw_start().

Any other ideas?

> Thanks,
> Bruno

Regards
David
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: HID, driver interaction with device during probe defeated by 4ea5454203d991ec85264f64f89ca8855fce69b0

[Bruno Prémont]

Hi David,

On Mon, 09 April 2012 David Herrmann wrote:
> > What would be the proper approach to fix this chicken-egg problem?
> > Semaphore introduced by above patch is only being released after
> > driver probe function has returned but prevents and incoming reports
> > from being delivered.
> >
> > Touching the semaphore inside driver looks like a bad idea to me...
> 
> The initial idea of the semaphore was to avoid that during probe() and
> remove() other HID callbacks are called. Otherwise, other drivers will
> fail (the commit message explains this in more detail). Therefore, the
> design of picolcd seems broken with respect to the semaphore.
> The only idea I currently have is adding a flag that releases the
> semaphore during driver->probe() and driver->remove() callbacks. I
> think the driver-core uses its own locks so we will not get races
> here. However, other HID drivers probably require proper
> synchronization during probe() and remove() (which is totally
> reasonable) as their internal structure might not be initialized, yet.
> picolcd has several races here, too. hid_set_drvdata() is not
> thread-safe or atomic so the raw-callback might get invalid "data"
> pointers if we remove the semaphore again.

Yes, there are a few races around in picolcd driver, I'm looking at how
I can improve one those, the toughest part being proper handling of
"unplug" events where the device is gone such that multi-report
transmissions get stopped right ahead...

> The nicest solution probably is releasing the semaphore during
> hid_hw_start(). That way every driver is prepared to receive data.
> However, it's an ugly solution because we release the lock not in the
> same function as we acquire it. We also need to catch cases where
> probe() actually did never call hid_hw_start().

On probe side:

Maybe instead of doing it within hid_hw_start() it could also be done
in hid_connect() in combination of a flag on hdev->claimed.


On remove side:

Let the semaphore be taken in hid_disconnect() (instead of hid_hw_stop(),
yet again in combination with flag on hdev->claimed.


This way probe and remove could WARN_ON() case where hdev->claim does
not show the flag as expected (that is, assuming probe() and remove()
can't be interlaced for the same device)

Otherwise it would require one more lock or doing some of the probe
initialization in a work queue (but that second option would make race
conditions much more of a problem)...

Thanks,
Bruno

Re: HID, driver interaction with device during probe defeated by 4ea5454203d991ec85264f64f89ca8855fce69b0

[David Herrmann]

Hi Bruno

On Sat, May 5, 2012 at 12:58 PM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
[snip]
>> The nicest solution probably is releasing the semaphore during
>> hid_hw_start(). That way every driver is prepared to receive data.
>> However, it's an ugly solution because we release the lock not in the
>> same function as we acquire it. We also need to catch cases where
>> probe() actually did never call hid_hw_start().
>
> On probe side:
>
> Maybe instead of doing it within hid_hw_start() it could also be done
> in hid_connect() in combination of a flag on hdev->claimed.

Could you elaborate this? I don't understand what you mean.

> On remove side:
>
> Let the semaphore be taken in hid_disconnect() (instead of hid_hw_stop(),
> yet again in combination with flag on hdev->claimed.
>
>
> This way probe and remove could WARN_ON() case where hdev->claim does
> not show the flag as expected (that is, assuming probe() and remove()
> can't be interlaced for the same device)
>
> Otherwise it would require one more lock or doing some of the probe
> initialization in a work queue (but that second option would make race
> conditions much more of a problem)...

My suggestion is to call up(&hdev->driver_lock) in hid_hw_start() if
some quirk is set. hid_device_probe() can then check whether the
semaphore is locked before calling up(). This way we would release the
lock before initializing a device. However, this seems ugly.

Your idea with the work queue seems much cleaner. However, there is
currently no way to make sure the work is executed _after_
hid_device_probe() releases the mutex. We could, however, introduce
some "hid_wait_until_initialized()" function that just tries to take
the semaphore and make the work wait until the hid_device_probe
finishes.

Maybe Jiri can comment here. But I think picolcd is broken and not the
semaphore. If I have some time I will try to send a fix for that. I do
not have the hardware, though, so I cannot test it.

> Thanks,
> Bruno

Cheers
David
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: HID, driver interaction with device during probe defeated by 4ea5454203d991ec85264f64f89ca8855fce69b0

[Bruno Prémont]

Hi David,

On Sat, 05 May 2012 David Herrmann wrote:
> On Sat, May 5, 2012 at 12:58 PM, Bruno Prémont wrote:
> [snip]
> >> The nicest solution probably is releasing the semaphore during
> >> hid_hw_start(). That way every driver is prepared to receive data.
> >> However, it's an ugly solution because we release the lock not in the
> >> same function as we acquire it. We also need to catch cases where
> >> probe() actually did never call hid_hw_start().
> >
> > On probe side:
> >
> > Maybe instead of doing it within hid_hw_start() it could also be done
> > in hid_connect() in combination of a flag on hdev->claimed.
> 
> Could you elaborate this? I don't understand what you mean.

I'm not sure how much of hid_connect() or hid_disconnect() should be
covered by the semaphore.
If it's all of it then there is no big preference between putting the
semaphore operations in hid_hw_start/stop or hid_(dis)connect, otherwise
moving those up/down calls down into hid_(dis)connect is better.

Keeping the up/down in hid_start/stop though causes them to be repeated
everywhere as those are defined in header file.

> > On remove side:
> >
> > Let the semaphore be taken in hid_disconnect() (instead of hid_hw_stop(),
> > yet again in combination with flag on hdev->claimed.
> >
> >
> > This way probe and remove could WARN_ON() case where hdev->claim does
> > not show the flag as expected (that is, assuming probe() and remove()
> > can't be interlaced for the same device)
> >
> > Otherwise it would require one more lock or doing some of the probe
> > initialization in a work queue (but that second option would make race
> > conditions much more of a problem)...
> 
> My suggestion is to call up(&hdev->driver_lock) in hid_hw_start() if
> some quirk is set. hid_device_probe() can then check whether the
> semaphore is locked before calling up(). This way we would release the
> lock before initializing a device. However, this seems ugly.

Choosing where it happens with a quirk definitely seems ugly and checking
for the semaphore would race with potential reports coming from device.

> Your idea with the work queue seems much cleaner. However, there is
> currently no way to make sure the work is executed _after_
> hid_device_probe() releases the mutex. We could, however, introduce
> some "hid_wait_until_initialized()" function that just tries to take
> the semaphore and make the work wait until the hid_device_probe
> finishes.

But then again the queued work may race with remove() call and if the
driver decides it doesn't want to handle the device it would have to
trigger unbind sequence from the queued work which looks rather more
complicated than failing inside probe!

A probe_late() driver callback would be better in that case, this callback
would be called after up()-ing the semaphore but before returning from
probe().
On failure of probe_late() the semaphore would have to be taken again
to clear ->driver

> Maybe Jiri can comment here. But I think picolcd is broken and not the
> semaphore. If I have some time I will try to send a fix for that. I do
> not have the hardware, though, so I cannot test it.

They can't live together in peace with current code :)

Cheers,
Bruno
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html