Hi, Alan Stern <[email protected]> writes: >> Alan Stern <[email protected]> writes: >> > On Mon, 26 Mar 2018, Felipe Balbi wrote: >> > >> >> Mention that ->complete() should never be called from within >> >> usb_ep_queue(). >> >> >> >> Signed-off-by: Felipe Balbi <[email protected]> >> >> --- >> >> drivers/usb/gadget/udc/core.c | 3 +++ >> >> 1 file changed, 3 insertions(+) >> >> >> >> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c >> >> index 50988b21a21b..842814bc0e4f 100644 >> >> --- a/drivers/usb/gadget/udc/core.c >> >> +++ b/drivers/usb/gadget/udc/core.c >> >> @@ -238,6 +238,9 @@ EXPORT_SYMBOL_GPL(usb_ep_free_request); >> >> * arranges to poll once per interval, and the gadget driver usually will >> >> * have queued some data to transfer at that time. >> >> * >> >> + * Note that @req's ->complete() callback must never be called from >> >> + * within usb_ep_queue() as that can create deadlock situations. >> >> + * >> > >> > I think this is highly questionable. Certainly it was not David >> > Brownell's original intention; his dummy-hcd driver will sometimes >> > give back a request from within usb_ep_queue() -- and I believe he >> > wrote it that way in order to emulate a feature of his net2280 driver. >> > >> > In this particular case, the problem is that a driver acquires a >> > spinlock in its complete() routine, but then it holds that same >> > spinlock while submitting a request. This is a bug; it should be fixed >> > in the driver. The spinlock should be dropped while the request is >> > submitted. I'm sure there are examples whether other drivers do this. >> >> usb_ep_queue() can be called from atomic, there's no explicit >> requirement that locks should be released. Either one case or the other >> should be made explicit. > > Agreed. The requirement should be that a routine calling > usb_ep_queue() should not hold any locks which can be acquired by the > request's completion handler. This is independent of whether the call > is made in process context or interrupt/atomic context.
fair enough. In that case, f_hid.c still needs to release its own lock
before calling usb_ep_queue(). Something along the lines of:
modified drivers/usb/gadget/function/f_hid.c
@@ -391,15 +391,16 @@ static ssize_t f_hidg_write(struct file *file, const char
__user *buffer,
req->complete = f_hidg_req_complete;
req->context = hidg;
+ spin_unlock_irqrestore(&hidg->write_spinlock, flags);
+
status = usb_ep_queue(hidg->in_ep, req, GFP_ATOMIC);
if (status < 0) {
ERROR(hidg->func.config->cdev,
"usb_ep_queue error on int endpoint %zd\n", status);
- goto release_write_pending_unlocked;
+ goto release_write_pending;
} else {
status = count;
}
- spin_unlock_irqrestore(&hidg->write_spinlock, flags);
return status;
release_write_pending:
ps: locking in that driver is horrible :-( I should try to spend some
time cleaning that up.
--
balbi
signature.asc
Description: PGP signature
