Hi Felipe,
Sorry for late reply due to my holiday.
On 9 September 2016 at 18:47, Felipe Balbi <ba...@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.w...@linaro.org> writes:
>> When system has stpped the gadget, we should avoid queuing any requests
>> which will cause tranfer failed. Thus adding some disconnect checking to
>> avoid this situation.
>>
>> Signed-off-by: Baolin Wang <baolin.w...@linaro.org>
>
> do you mind if we discuss this for a little longer?
Sure.
>
>> ---
>> Changes since v1:
>> - Split into 2 separate ptaches.
>> - Choose complete mechanism instead of polling.
>> ---
>> drivers/usb/dwc3/ep0.c | 8 ++++++++
>> drivers/usb/dwc3/gadget.c | 12 +++++++++---
>> 2 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index fe79d77..632e5a4 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -228,6 +228,14 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct
>> usb_request *request,
>> int ret;
>>
>> spin_lock_irqsave(&dwc->lock, flags);
>> + if (!dwc->pullups_connected) {
>> + dwc3_trace(trace_dwc3_ep0,
>> + "queuing request %p to %s when gadget is disconnected",
>> + request, dep->name);
>> + ret = -ESHUTDOWN;
>> + goto out;
>> + }
>
> I have been thinking about this branch here. It's not a problem to queue
> a request with pullups disconnected. It's only a problem to issue
> START_TRANSFER without RUN_STOP bit set.
>
> So maybe this check should be done in dwc3_send_gadget_ep_cmd(). By
> doing that we also make sure to do the check in one place and one place
> only because all endpoints rely dwc3_send_gadget_ep_cmd().
OK, that makes sense to me.
>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 1783406..1a33308 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1040,6 +1040,13 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep
>> *dep, struct dwc3_request *req)
>> struct dwc3 *dwc = dep->dwc;
>> int ret;
>>
>> + if (!dwc->pullups_connected) {
>> + dwc3_trace(trace_dwc3_gadget,
>> + "queuing request %p to %s when gadget is disconnected",
>> + &req->request, dep->endpoint.name);
>> + return -ESHUTDOWN;
>> + }
>> +
>> if (!dep->endpoint.desc) {
>> dwc3_trace(trace_dwc3_gadget,
>> "trying to queue request %p to disabled %s",
>> @@ -1984,13 +1991,12 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc,
>> struct dwc3_ep *dep,
>> if (ret)
>> break;
>> }
>> -
>
> trailing change.
Ah, sorry. Will remove it.
>
>> /*
>> * Our endpoint might get disabled by another thread during
>> * dwc3_gadget_giveback(). If that happens, we're just gonna return 1
>> * early on so DWC3_EP_BUSY flag gets cleared
>> */
>> - if (!dep->endpoint.desc)
>> + if (!dep->endpoint.desc || !dwc->pullups_connected)
>
> I'm still considering this as well. Sure, we kill pullups before the
> descriptor is set to NULL, but that shouldn't be a problem. What will
> happen is:
>
> usb_gadget_disconnect();
> udc->driver->disconnect();
> for_each_ep() {
> for_each_request() {
> usb_ep_dequeue();
> }
> usb_ep_disable();
> dep->endpoint.desc = NULL;
> }
> udc->driver->unbind();
> usb_gadget_udc_stop();
>
> I don't see a problem here. Did you manage to trigger any failure when
> you didn't have this check? Care to show some logs? We might have a bug
> elsewhere which we don't want to mask by adding this check here.
If we did not add this 'pullups_connected' checking here, it maybe
will kick one transfer to cause a problem of TART_TRANSFER, but it
also can be removed if we check in dwc3_send_gadget_ep_cmd() as you
suggested. Thanks for your comments and I will send out the new patch.
--
Baolin.wang
Best Regards
