* [1/2] usb: gadget: udc: core: update usb_ep_queue() documentation
@ 2018-03-28 23:19 Yavuz, Tuba
0 siblings, 0 replies; 7+ messages in thread
From: Yavuz, Tuba @ 2018-03-28 23:19 UTC (permalink / raw)
To: Felipe Balbi, Alan Stern; +Cc: Greg Kroah-Hartman, Linux USB
Sounds good to me. Thanks Felipe.
Best,
Tuba Yavuz
^ permalink raw reply [flat|nested] 7+ messages in thread
* [1/2] usb: gadget: udc: core: update usb_ep_queue() documentation
@ 2018-03-28 7:43 Felipe Balbi
0 siblings, 0 replies; 7+ messages in thread
From: Felipe Balbi @ 2018-03-28 7:43 UTC (permalink / raw)
To: Alan Stern; +Cc: Greg Kroah-Hartman, Linux USB, Tuba Yavuz
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [1/2] usb: gadget: udc: core: update usb_ep_queue() documentation
@ 2018-03-27 14:13 Alan Stern
0 siblings, 0 replies; 7+ messages in thread
From: Alan Stern @ 2018-03-27 14:13 UTC (permalink / raw)
To: Felipe Balbi; +Cc: Greg Kroah-Hartman, Linux USB, Tuba Yavuz
On Tue, 27 Mar 2018, Felipe Balbi wrote:
>
> Hi,
>
> 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.
Alan Stern
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* [1/2] usb: gadget: udc: core: update usb_ep_queue() documentation
@ 2018-03-27 6:59 Felipe Balbi
0 siblings, 0 replies; 7+ messages in thread
From: Felipe Balbi @ 2018-03-27 6:59 UTC (permalink / raw)
To: Alan Stern; +Cc: Greg Kroah-Hartman, Linux USB, Tuba Yavuz
Hi,
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [1/2] usb: gadget: udc: core: update usb_ep_queue() documentation
@ 2018-03-26 17:44 Yavuz, Tuba
0 siblings, 0 replies; 7+ messages in thread
From: Yavuz, Tuba @ 2018-03-26 17:44 UTC (permalink / raw)
To: Alan Stern, Felipe Balbi; +Cc: Greg Kroah-Hartman, Linux USB
I agree with Alan that the spinlock must be dropped before calling usb_ep_queue. An example can be found in the ep0_queue function of the f_mass_storage driver.
Best,
Tuba Yavuz, Ph.D.
Assistant Professor
Electrical and Computer Engineering Department
University of Florida
Gainesville, FL 32611
Webpage: http://www.tuba.ece.ufl.edu/
Email: tuba@ece.ufl.edu
Phone: (352) 846 0202
^ permalink raw reply [flat|nested] 7+ messages in thread
* [1/2] usb: gadget: udc: core: update usb_ep_queue() documentation
@ 2018-03-26 17:32 Alan Stern
0 siblings, 0 replies; 7+ messages in thread
From: Alan Stern @ 2018-03-26 17:32 UTC (permalink / raw)
To: Felipe Balbi; +Cc: Greg Kroah-Hartman, Linux USB, Tuba Yavuz
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.
Alan Stern
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* [1/2] usb: gadget: udc: core: update usb_ep_queue() documentation
@ 2018-03-26 10:14 Felipe Balbi
0 siblings, 0 replies; 7+ messages in thread
From: Felipe Balbi @ 2018-03-26 10:14 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Linux USB, Tuba Yavuz, Felipe Balbi
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.
+ *
* Returns zero, or a negative error code. Endpoints that are not enabled
* report errors; errors will also be
* reported when the usb peripheral is disconnected.
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-03-28 23:19 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-28 23:19 [1/2] usb: gadget: udc: core: update usb_ep_queue() documentation Yavuz, Tuba
-- strict thread matches above, loose matches on Subject: below --
2018-03-28 7:43 Felipe Balbi
2018-03-27 14:13 Alan Stern
2018-03-27 6:59 Felipe Balbi
2018-03-26 17:44 Yavuz, Tuba
2018-03-26 17:32 Alan Stern
2018-03-26 10:14 Felipe Balbi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.