Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 7 Dec 2018 13:15:32 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Minas Harutyunyan <minas.harutyunyan@synopsys.com>
Cc:     Marek Szyprowski <m.szyprowski@samsung.com>,
        Maynard CABIENTE <maynard.cabiente@raritan.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Felipe Balbi <felipe.balbi@linux.intel.com>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Subject: Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on
 disconnect"
Message-ID: <20181207101532.GR3073@unbuntlaptop>
References: <CGME20181121154518eucas1p2cc7ffd4d071ff420c747e1d0c387ee5d@eucas1p2.samsung.com>
 <20181121154504.13052-1-m.szyprowski@samsung.com>
 <410670D7E743164D87FA6160E7907A56013A7AA1EE@am04wembxa.internal.synopsys.com>
 <20181123144307.GC2970@unbuntlaptop>
 <410670D7E743164D87FA6160E7907A56013A7B19C2@am04wembxa.internal.synopsys.com>
 <20181204132913.GH3073@unbuntlaptop>
 <410670D7E743164D87FA6160E7907A56013A7B2847@am04wembxa.internal.synopsys.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <410670D7E743164D87FA6160E7907A56013A7B2847@am04wembxa.internal.synopsys.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Dec 05, 2018 at 12:52:22PM +0000, Minas Harutyunyan wrote:
> Hi,
> 
> On 12/4/2018 5:29 PM, Dan Carpenter wrote:
> > On Tue, Dec 04, 2018 at 12:34:08PM +0000, Minas Harutyunyan wrote:
> >> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
> >>           hsotg->connected = 0;
> >>           hsotg->test_mode = 0;
> >>
> >> -       /* all endpoints should be shutdown */
> >>           for (ep = 0; ep < hsotg->num_of_eps; ep++) {
> >>                   if (hsotg->eps_in[ep])
> >> -                       dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> >> +                       kill_all_requests(hsotg, hsotg->eps_in[ep],
> >> +                                                         -ESHUTDOWN);
> >>                   if (hsotg->eps_out[ep])
> >> -                       dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> >> +                       kill_all_requests(hsotg, hsotg->eps_out[ep],
> >> +                                                         -ESHUTDOWN);
> > 
> > 
> > Should this part be in a separate patch?
> > 
> > I'm not trying to be rhetorical at all.  I literally don't know the
> > code very well.  Hopefully the full commit message will explain it.
> > 
> 
> Actually, this fragment of patch revert changes from V2 and keep 
> untouched dwc2_hsotg_disconnect() function.
> 

To me it feels like there are two issues.  The first is this change, and
the second is fixing the lockdep warning.


> >>           }
> >>
> >>           call_gadget(hsotg, disconnect);
> >> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
> >> dwc2_hsotg *hsotg, bool periodic)
> >>                           GINTSTS_PTXFEMP |  \
> >>                           GINTSTS_RXFLVL)
> >>
> >> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
> >> +
> >>    /**
> >>     * dwc2_hsotg_core_init - issue softreset to the core
> >>     * @hsotg: The device state
> >> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
> >> dwc2_hsotg *hsotg,
> >>                           return;
> >>           } else {
> >>                   /* all endpoints should be shutdown */
> >> +               spin_unlock(&hsotg->lock);
> >>                   for (ep = 1; ep < hsotg->num_of_eps; ep++) {
> >>                           if (hsotg->eps_in[ep])
> >>   
> >> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> >>                           if (hsotg->eps_out[ep])
> >>   
> >> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> >>                   }
> >> +               spin_lock(&hsotg->lock);
> >>           }
> >>
> >>           /*
> > 
> > The idea here is that this is the only caller which is holding the
> > lock and we drop it here and take it again inside dwc2_hsotg_ep_disable().
> > I don't know the code very well and can't totally swear that this
> > doesn't introduce a small race condition...
> > 
> Above fragment of patch allow to keep untouched dwc2_hsotg_ep_disble() 
> function also, without changing spin_lock/_unlock stuff inside function.
> 
> My approach here minimally update code to add any races. Just in 
> dwc2_hsotg_core_init_disconnected() function on USB reset interrupt 
> perform disabling all EP's. Because on USB reset interrupt, called from interrupt 
> handler with acquired lock and dwc2_hsotg_ep_disble() function (without 
> changes) acquire lock, just need to unlock lock to avoid any troubles.
> 

Yes.  I understand that.  I just don't like it.

Although your patch is more "minimal" in that it touches fewer lines of
code it's actually more complicated because we have to verify that it's
safe to drop the lock.


> > Another option would be to introduce a new function which takes the lock
> > and change all the other callers instead.  To me that would be easier to
> > review...  See below for how it might look:
> > 
> > regards,
> > dan carpenter
> > 
> > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > index 94f3ba995580..b17a5dbefd5f 100644
> > --- a/drivers/usb/dwc2/gadget.c
> > +++ b/drivers/usb/dwc2/gadget.c
> > @@ -3166,6 +3166,7 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg,
> >   }
> >   
> >   static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
> > +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep);
> >   
> >   /**
> >    * dwc2_hsotg_disconnect - disconnect service
> > @@ -3188,9 +3189,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
> >   	/* all endpoints should be shutdown */
> >   	for (ep = 0; ep < hsotg->num_of_eps; ep++) {
> >   		if (hsotg->eps_in[ep])
> > -			dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> > +			dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
> >   		if (hsotg->eps_out[ep])
> > -			dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> > +			dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
> >   	}
> >   
> >   	call_gadget(hsotg, disconnect);
> > @@ -4069,10 +4070,8 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
> >   	struct dwc2_hsotg *hsotg = hs_ep->parent;
> >   	int dir_in = hs_ep->dir_in;
> >   	int index = hs_ep->index;
> > -	unsigned long flags;
> >   	u32 epctrl_reg;
> >   	u32 ctrl;
> > -	int locked;
> >   
> >   	dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
> >   
> > @@ -4088,10 +4087,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
> >   
> >   	epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
> >   
> > -	locked = spin_is_locked(&hsotg->lock);
> > -	if (!locked)
> > -		spin_lock_irqsave(&hsotg->lock, flags);
> > -
> >   	ctrl = dwc2_readl(hsotg, epctrl_reg);
> >   
> >   	if (ctrl & DXEPCTL_EPENA)
> > @@ -4114,12 +4109,23 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
> >   	hs_ep->fifo_index = 0;
> >   	hs_ep->fifo_size = 0;
> >   
> > -	if (!locked)
> > -		spin_unlock_irqrestore(&hsotg->lock, flags);
> > -
> >   	return 0;
> >   }
> >   
> > +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep)
> > +{
> > +	struct dwc2_hsotg_ep *hs_ep = our_ep(ep);
> > +	struct dwc2_hsotg *hsotg = hs_ep->parent;
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	spin_lock_irqsave(&hsotg->lock, flags);
> > +	ret = dwc2_hsotg_ep_disable(ep);
> > +	spin_unlock_irqrestore(&hsotg->lock, flags);
> > +
> > +	return ret;
> > +}
> > +
> >   /**
> >    * on_list - check request is on the given endpoint
> >    * @ep: The endpoint to check.
> > @@ -4267,7 +4273,7 @@ static int dwc2_hsotg_ep_sethalt_lock(struct usb_ep *ep, int value)
> >   
> >   static const struct usb_ep_ops dwc2_hsotg_ep_ops = {
> >   	.enable		= dwc2_hsotg_ep_enable,
> > -	.disable	= dwc2_hsotg_ep_disable,
> > +	.disable	= dwc2_hsotg_ep_disable_lock,
> >   	.alloc_request	= dwc2_hsotg_ep_alloc_request,
> >   	.free_request	= dwc2_hsotg_ep_free_request,
> >   	.queue		= dwc2_hsotg_ep_queue_lock,
> > @@ -4407,9 +4413,9 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget *gadget)
> >   	/* all endpoints should be shutdown */
> >   	for (ep = 1; ep < hsotg->num_of_eps; ep++) {
> >   		if (hsotg->eps_in[ep])
> > -			dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> > +			dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
> >   		if (hsotg->eps_out[ep])
> > -			dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> > +			dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
> >   	}
> >   
> >   	spin_lock_irqsave(&hsotg->lock, flags);
> > @@ -4857,9 +4863,9 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)
> >   
> >   		for (ep = 0; ep < hsotg->num_of_eps; ep++) {
> >   			if (hsotg->eps_in[ep])
> > -				dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> > +				dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
> >   			if (hsotg->eps_out[ep])
> > -				dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> > +				dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
> >   		}
> >   	}
> >   
> > 
> 
> Your code doesn't take care about fifo_map warnings from 
> dwc2_hsotg_init_fifo() function. Before calling dwc2_hsotg_init_fifo() 
> from dwc2_hsotg_core_init_disconnected() function all Ep's should 
> disabled and fifo bitmap should be cleared.
> 

Correct.  I am only trying to fix the locking.  I hope you can fix the
rest in a separate patch.

regards,
dan carpenter