[1/2] usb: gadget: udc: core: update usb_ep_queue() documentation

[Felipe Balbi <felipe.balbi@linux.intel.com>]

Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
>> Alan Stern <stern@rowland.harvard.edu> 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 <felipe.balbi@linux.intel.com>
>> >> ---
>> >>  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.