On Thu, Jul 14, 2022 at 10:41:01AM -0700, Wesley Cheng wrote:
> Hi Thinh,
>
> On 7/14/2022 10:38 AM, Thinh Nguyen wrote:
> > On 7/12/2022, Wesley Cheng wrote:
> > > Local interrupts are currently being disabled as part of aquiring the
> > > spin lock before issuing the endxfer command. Leave interrupts enabled, so
> > > that EP0 events can continue to be processed. Also, ensure that there are
> > > no pending interrupts before attempting to handle any soft
> > > connect/disconnect.
> > >
> > > Fixes: 861c010a2ee1 ("usb: dwc3: gadget: Refactor pullup()")
> > > Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
> > > ---
> > > drivers/usb/dwc3/gadget.c | 21 ++++++++++++---------
> > > 1 file changed, 12 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > > index a455f8d4631d..ee85b773e3fe 100644
> > > --- a/drivers/usb/dwc3/gadget.c
> > > +++ b/drivers/usb/dwc3/gadget.c
> > > @@ -1674,6 +1674,7 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
> > > static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt)
> > > {
> > > struct dwc3_gadget_ep_cmd_params params;
> > > + struct dwc3 *dwc = dep->dwc;
> > > u32 cmd;
> > > int ret;
> > > @@ -1682,7 +1683,9 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
> > > cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
> > > cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
> > > memset(¶ms, 0, sizeof(params));
> > > + spin_unlock(&dwc->lock);
> > > ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
> > > + spin_lock(&dwc->lock);
> > > WARN_ON_ONCE(ret);
> > > dep->resource_index = 0;
> > > @@ -2029,12 +2032,11 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
> > > struct dwc3_ep *dep = to_dwc3_ep(ep);
> > > struct dwc3 *dwc = dep->dwc;
> > > - unsigned long flags;
> > > int ret = 0;
> > > trace_dwc3_ep_dequeue(req);
> > > - spin_lock_irqsave(&dwc->lock, flags);
> > > + spin_lock(&dwc->lock);
> > > list_for_each_entry(r, &dep->cancelled_list, list) {
> > > if (r == req)
> > > @@ -2073,7 +2075,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
> > > request, ep->name);
> > > ret = -EINVAL;
> > > out:
> > > - spin_unlock_irqrestore(&dwc->lock, flags);
> > > + spin_unlock(&dwc->lock);
> > > return ret;
> > > }
> > > @@ -2489,9 +2491,7 @@ static int __dwc3_gadget_start(struct dwc3 *dwc);
> > > static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
> > > {
> > > - unsigned long flags;
> > > -
> > > - spin_lock_irqsave(&dwc->lock, flags);
> > > + spin_lock(&dwc->lock);
> > > dwc->connected = false;
> > > /*
> > > @@ -2506,10 +2506,10 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
> > > reinit_completion(&dwc->ep0_in_setup);
> > > - spin_unlock_irqrestore(&dwc->lock, flags);
> > > + spin_unlock(&dwc->lock);
> > > ret = wait_for_completion_timeout(&dwc->ep0_in_setup,
> > > msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT));
> > > - spin_lock_irqsave(&dwc->lock, flags);
> > > + spin_lock(&dwc->lock);
> > > if (ret == 0)
> > > dev_warn(dwc->dev, "timed out waiting for SETUP phase\n");
> > > }
> > > @@ -2523,7 +2523,7 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
> > > */
> > > dwc3_stop_active_transfers(dwc);
> > > __dwc3_gadget_stop(dwc);
> > > - spin_unlock_irqrestore(&dwc->lock, flags);
> > > + spin_unlock(&dwc->lock);
> > > /*
> > > * Note: if the GEVNTCOUNT indicates events in the event buffer, the
> > > @@ -2569,6 +2569,8 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> > > return 0;
> > > }
> > > + synchronize_irq(dwc->irq_gadget);
> > > +
> > > if (!is_on) {
> > > ret = dwc3_gadget_soft_disconnect(dwc);
> > > } else {
> > > @@ -3729,6 +3731,7 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
> > > */
> > > __dwc3_stop_active_transfer(dep, force, interrupt);
> > > +
> > > }
> > > static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
> >
> > Hi Greg,
> >
> > Please don't pick up this patch yet. We're still in discussion with
> > this. I have some concern with unlocking/locking when sending End
> > Transfer command. For example, this patch may cause issues with
> > DWC3_EP_END_TRANSFER_PENDING checks.
> >
>
> Agreed.
>
> > Hi Wesley,
> >
> > Did you try out my suggestion yet?
> >
>
> In process of testing. Will update you in a few days, since it might take a
> day or so to reproduce.
Ok, I'll drop this whole series from my tree for now. Please resend
when you have it working.
greg k-h
