Re: [PATCH] hid: usbhid: fix possible deadlock in __usbhid_submit_report

Am Samstag, 21. April 2012, 02:37:35 schrieb Alan Stern:

On Fri, 20 Apr 2012, Oliver Neukum wrote:

quoted

As I said, I'd very much appreciate sane semantics for usb_unlink_urb().

Aside from the practicality issue of altering a large number of
existing drivers, changing the semantics the way you want would be
difficult because it would force the HCDs to defer some giveback
operations to a bottom half or timer routine.

Or a work queue, which would have to be dedicated to avoid deadlocks
with storage.

Think about what happens if the URB being unlinked hasn't been
presented to the hardware yet.  Once it has been removed from the HCD's
internal lists, there's no reason not to give it back right away.  And
there's no natural time to give it back later.

Now. The question is not when, but from which context.
The context should be uniform, so that the requirements
for locking should also be uniform.

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

On Sat, Apr 21, 2012 at 6:25 PM, Oliver Neukum [off-list ref] wrote:

Am Samstag, 21. April 2012, 02:37:35 schrieb Alan Stern:

quoted

On Fri, 20 Apr 2012, Oliver Neukum wrote:

quoted

As I said, I'd very much appreciate sane semantics for usb_unlink_urb().

Aside from the practicality issue of altering a large number of
existing drivers, changing the semantics the way you want would be
difficult because it would force the HCDs to defer some giveback
operations to a bottom half or timer routine.

Or a work queue, which would have to be dedicated to avoid deadlocks
with storage.

quoted

Think about what happens if the URB being unlinked hasn't been
presented to the hardware yet.  Once it has been removed from the HCD's
internal lists, there's no reason not to give it back right away.  And
there's no natural time to give it back later.

Now. The question is not when, but from which context.
The context should be uniform, so that the requirements
for locking should also be uniform.

How about always scheduling a tasklet to run what usb_unlink_urb does?
just implement usb_unlink_urb as something like
tasklet_schedule(unlink_tasklet).

Then we can have a uniform lock requirement and no changes are involved
on host controller drivers.


Thanks,
--
Ming Lei
--
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

On Sat, 21 Apr 2012, Ming Lei wrote:

quoted

quoted

Think about what happens if the URB being unlinked hasn't been
presented to the hardware yet. �Once it has been removed from the HCD's
internal lists, there's no reason not to give it back right away. �And
there's no natural time to give it back later.

Now. The question is not when, but from which context.
The context should be uniform, so that the requirements
for locking should also be uniform.

How about always scheduling a tasklet to run what usb_unlink_urb does?
just implement usb_unlink_urb as something like
tasklet_schedule(unlink_tasklet).

Then we can have a uniform lock requirement and no changes are involved
on host controller drivers.

The return values would not be correct.

On the other hand, usbnet could call usb_unlink_urb from within a 
tasklet.

Alan Stern

--
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

On Sun, Apr 22, 2012 at 1:31 AM, Alan Stern [off-list ref] wrote:

On Sat, 21 Apr 2012, Ming Lei wrote:

quoted

How about always scheduling a tasklet to run what usb_unlink_urb does?
just implement usb_unlink_urb as something like
tasklet_schedule(unlink_tasklet).

Then we can have a uniform lock requirement and no changes are involved
on host controller drivers.

The return values would not be correct.

If you run 'git grep -n usb_unlink_urb drivers/usb/', it may show that
most of callers do not check its return value, and the others only check
for dumping warnings. If usb_unlink_urb is converted into tasklet
implementation, we still can dump these warnings inside its tasklet function.

On the other hand, usbnet could call usb_unlink_urb from within a
tasklet.

Sorry, you mean tasklet_schedule can't be called inside a tasklet?

Thanks,
--
Ming Lei

On Sun, 22 Apr 2012, Ming Lei wrote:

On Sun, Apr 22, 2012 at 1:31 AM, Alan Stern [off-list ref] wrote:

quoted

On Sat, 21 Apr 2012, Ming Lei wrote:

quoted

How about always scheduling a tasklet to run what usb_unlink_urb does?
just implement usb_unlink_urb as something like
tasklet_schedule(unlink_tasklet).

Then we can have a uniform lock requirement and no changes are involved
on host controller drivers.

The return values would not be correct.

If you run 'git grep -n usb_unlink_urb drivers/usb/', it may show that
most of callers do not check its return value, and the others only check
for dumping warnings. If usb_unlink_urb is converted into tasklet
implementation, we still can dump these warnings inside its tasklet function.

That sounds rather awkward.  How would the "tasklet-ized" version of 
usb_unlink_urb know what warnings to issue?

quoted

On the other hand, usbnet could call usb_unlink_urb from within a
tasklet.

Sorry, you mean tasklet_schedule can't be called inside a tasklet?

What I meant is: If you're going to run in a tasklet, it doesn't matter
whether the tasklet is started by the usb_unlink_urb function or by its
caller.  The end result should be the same either way.

However Oliver has already objected to using a tasklet for unlinking.

Alan Stern