Re: [PATCH] usb: dwc3: gadget: Add disconnect checking when changing function dynamically

[Baolin Wang]

Hi Felipe,

On 8 September 2016 at 20:00, Felipe Balbi <balbi@xxxxxxxxxx> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@xxxxxxxxxx> writes:
>> When we change the USB function with configfs dynamically, we possibly met this
>> situation: one core is doing the control transfer, another core is trying to
>> unregister the USB gadget from userspace, we must wait for completing this
>> control tranfer, or it will hang the controller to set the DEVCTRLHLT flag.
>>
>> Also adding some disconnect checking to avoid queuing any requests when the
>> gadget is stopped.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
>> ---
>>  drivers/usb/dwc3/ep0.c    |    8 ++++++++
>>  drivers/usb/dwc3/gadget.c |   32 +++++++++++++++++++++++++++-----
>>  2 files changed, 35 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index fe79d77..11519d7 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 == false) {
>> +             dwc3_trace(trace_dwc3_ep0,
>> +                     "queuing request %p to %s when gadget is disconnected",
>> +                     request, dep->name);
>> +             ret = -ESHUTDOWN;
>> +             goto out;
>> +     }
>
> this could go on its own patch, however we can use if
> (!dwc->pullups_connected) instead.

Indeed. I will split it into one separate patch.

>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 1783406..bbac8f5 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 == false) {
>> +             dwc3_trace(trace_dwc3_gadget,
>> +                     "queuing request %p to %s when gadget is disconnected",
>> +                     &req->request, dep->endpoint.name);
>> +             return -ESHUTDOWN;
>> +     }
>
> ditto

OK.

>
>> @@ -1434,6 +1441,13 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>>       if (pm_runtime_suspended(dwc->dev))
>>               return 0;
>>
>> +     /*
>> +      * Per databook, when we want to stop the gadget, if a control transfer
>> +      * is still in process, complete it and get the core into setup phase.
>> +      */
>
> Do you have the section reference for this? Which databook version are
> you reading?

The databook version is 2.80a and you can find this description in
section 8.1.8 "Device-Initiated Disconnect".

>
>> +     if (!is_on && dwc->ep0state != EP0_SETUP_PHASE)
>> +             return -EBUSY;
>> +
>>       reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>       if (is_on) {
>>               if (dwc->revision <= DWC3_REVISION_187A) {
>> @@ -1481,12 +1495,20 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>       struct dwc3             *dwc = gadget_to_dwc(g);
>>       unsigned long           flags;
>>       int                     ret;
>> +     int                     timeout = 500;
>>
>>       is_on = !!is_on;
>>
>> -     spin_lock_irqsave(&dwc->lock, flags);
>> -     ret = dwc3_gadget_run_stop(dwc, is_on, false);
>> -     spin_unlock_irqrestore(&dwc->lock, flags);
>> +     do {
>> +             spin_lock_irqsave(&dwc->lock, flags);
>> +             ret = dwc3_gadget_run_stop(dwc, is_on, false);
>> +             spin_unlock_irqrestore(&dwc->lock, flags);
>> +
>> +             if (ret != -EBUSY)
>> +                     break;
>> +
>> +             udelay(10);
>> +     } while (--timeout);
>
> no, this is not a good idea at all. I'd rather see:
>
> wait_event_timeout(dwc->wq, dwc->ep0state == EP0_SETUP_PHASE,
>         jiffies + msecs_to_jiffies(500));
>
> or, perhaps, a wait_for_completion_timeout(). Then, ep0.c needs to call
> complete() everytime Status Phase completes. That's probably a better
> idea ;-)

Yes, you are right. I will fix that with your suggestion. Thanks.

>
>> @@ -1990,7 +2012,7 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
>>        * 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 == false)
>
> is this really necessary? Can we really get pullups_connect set to false
> with dep->endpoint.desc still valid?

Yes, when we start to unregister gadget by
usb_gadget_unregister_driver(), the first thing is disconnect the
gadget by usb_gadget_disconnect() to set pullups_connect as false,
then will disable the endpoints by composite_disconnect() to set
dep->endpoint.desc as NULL. When 'dwc->pullups_connected' is set
false, we also need to avoiding transfer.

>
>> @@ -2064,7 +2086,7 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc,
>>        * 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 == false)
>
> ditto
>
> --
> balbi



-- 
Baolin.wang
Best Regards