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;
From:   Minas Harutyunyan <minas.harutyunyan@synopsys.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>,
        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"
Thread-Topic: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on
 disconnect"
Thread-Index: AQHUgbE8ouTBXi+nzkarEEkfP9zP5Q==
Date:   Fri, 7 Dec 2018 14:13:18 +0000
Message-ID: <410670D7E743164D87FA6160E7907A56013A7B4E2E@am04wembxa.internal.synopsys.com>
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>
 <20181207101532.GR3073@unbuntlaptop>
 <410670D7E743164D87FA6160E7907A56013A7B4AAF@am04wembxa.internal.synopsys.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Dan,=0A=
=0A=
On 12/7/2018 3:20 PM, Minas Harutyunyan wrote:=0A=
> Hi Dan,=0A=
> =0A=
> On 12/7/2018 2:16 PM, Dan Carpenter wrote:=0A=
>> On Wed, Dec 05, 2018 at 12:52:22PM +0000, Minas Harutyunyan wrote:=0A=
>>> Hi,=0A=
>>>=0A=
>>> On 12/4/2018 5:29 PM, Dan Carpenter wrote:=0A=
>>>> On Tue, Dec 04, 2018 at 12:34:08PM +0000, Minas Harutyunyan wrote:=0A=
>>>>> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg =
*hsotg)=0A=
>>>>>             hsotg->connected =3D 0;=0A=
>>>>>             hsotg->test_mode =3D 0;=0A=
>>>>>=0A=
>>>>> -       /* all endpoints should be shutdown */=0A=
>>>>>             for (ep =3D 0; ep < hsotg->num_of_eps; ep++) {=0A=
>>>>>                     if (hsotg->eps_in[ep])=0A=
>>>>> -                       dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep)=
;=0A=
>>>>> +                       kill_all_requests(hsotg, hsotg->eps_in[ep],=
=0A=
>>>>> +                                                         -ESHUTDOWN)=
;=0A=
>>>>>                     if (hsotg->eps_out[ep])=0A=
>>>>> -                       dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep=
);=0A=
>>>>> +                       kill_all_requests(hsotg, hsotg->eps_out[ep],=
=0A=
>>>>> +                                                         -ESHUTDOWN)=
;=0A=
>>>>=0A=
>>>>=0A=
>>>> Should this part be in a separate patch?=0A=
>>>>=0A=
>>>> I'm not trying to be rhetorical at all.  I literally don't know the=0A=
>>>> code very well.  Hopefully the full commit message will explain it.=0A=
>>>>=0A=
>>>=0A=
>>> Actually, this fragment of patch revert changes from V2 and keep=0A=
>>> untouched dwc2_hsotg_disconnect() function.=0A=
>>>=0A=
>>=0A=
>> To me it feels like there are two issues.  The first is this change, and=
=0A=
>> the second is fixing the lockdep warning.=0A=
>>=0A=
>>=0A=
>>>>>             }=0A=
>>>>>=0A=
>>>>>             call_gadget(hsotg, disconnect);=0A=
>>>>> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct=0A=
>>>>> dwc2_hsotg *hsotg, bool periodic)=0A=
>>>>>                             GINTSTS_PTXFEMP |  \=0A=
>>>>>                             GINTSTS_RXFLVL)=0A=
>>>>>=0A=
>>>>> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);=0A=
>>>>> +=0A=
>>>>>      /**=0A=
>>>>>       * dwc2_hsotg_core_init - issue softreset to the core=0A=
>>>>>       * @hsotg: The device state=0A=
>>>>> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct=
=0A=
>>>>> dwc2_hsotg *hsotg,=0A=
>>>>>                             return;=0A=
>>>>>             } else {=0A=
>>>>>                     /* all endpoints should be shutdown */=0A=
>>>>> +               spin_unlock(&hsotg->lock);=0A=
>>>>>                     for (ep =3D 1; ep < hsotg->num_of_eps; ep++) {=0A=
>>>>>                             if (hsotg->eps_in[ep])=0A=
>>>>>     =0A=
>>>>> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);=0A=
>>>>>                             if (hsotg->eps_out[ep])=0A=
>>>>>     =0A=
>>>>> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);=0A=
>>>>>                     }=0A=
>>>>> +               spin_lock(&hsotg->lock);=0A=
>>>>>             }=0A=
>>>>>=0A=
>>>>>             /*=0A=
>>>>=0A=
>>>> The idea here is that this is the only caller which is holding the=0A=
>>>> lock and we drop it here and take it again inside dwc2_hsotg_ep_disabl=
e().=0A=
>>>> I don't know the code very well and can't totally swear that this=0A=
>>>> doesn't introduce a small race condition...=0A=
>>>>=0A=
>>> Above fragment of patch allow to keep untouched dwc2_hsotg_ep_disble()=
=0A=
>>> function also, without changing spin_lock/_unlock stuff inside function=
.=0A=
>>>=0A=
>>> My approach here minimally update code to add any races. Just in=0A=
>>> dwc2_hsotg_core_init_disconnected() function on USB reset interrupt=0A=
>>> perform disabling all EP's. Because on USB reset interrupt, called from=
 interrupt=0A=
>>> handler with acquired lock and dwc2_hsotg_ep_disble() function (without=
=0A=
>>> changes) acquire lock, just need to unlock lock to avoid any troubles.=
=0A=
>>>=0A=
>>=0A=
>> Yes.  I understand that.  I just don't like it.=0A=
>>=0A=
>> Although your patch is more "minimal" in that it touches fewer lines of=
=0A=
>> code it's actually more complicated because we have to verify that it's=
=0A=
>> safe to drop the lock.=0A=
>>=0A=
>>=0A=
>>>> Another option would be to introduce a new function which takes the lo=
ck=0A=
>>>> and change all the other callers instead.  To me that would be easier =
to=0A=
>>>> review...  See below for how it might look:=0A=
>>>>=0A=
>>>> regards,=0A=
>>>> dan carpenter=0A=
>>>>=0A=
>>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c=0A=
>>>> index 94f3ba995580..b17a5dbefd5f 100644=0A=
>>>> --- a/drivers/usb/dwc2/gadget.c=0A=
>>>> +++ b/drivers/usb/dwc2/gadget.c=0A=
>>>> @@ -3166,6 +3166,7 @@ static void kill_all_requests(struct dwc2_hsotg =
*hsotg,=0A=
>>>>     }=0A=
>>>>     =0A=
>>>>     static int dwc2_hsotg_ep_disable(struct usb_ep *ep);=0A=
>>>> +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep);=0A=
>>>>     =0A=
>>>>     /**=0A=
>>>>      * dwc2_hsotg_disconnect - disconnect service=0A=
>>>> @@ -3188,9 +3189,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hs=
otg)=0A=
>>>>     	/* all endpoints should be shutdown */=0A=
>>>>     	for (ep =3D 0; ep < hsotg->num_of_eps; ep++) {=0A=
>>>>     		if (hsotg->eps_in[ep])=0A=
>>>> -			dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);=0A=
>>>> +			dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);=0A=
>>>>     		if (hsotg->eps_out[ep])=0A=
>>>> -			dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);=0A=
>>>> +			dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);=0A=
>>>>     	}=0A=
>>>>     =0A=
>>>>     	call_gadget(hsotg, disconnect);=0A=
>>>> @@ -4069,10 +4070,8 @@ static int dwc2_hsotg_ep_disable(struct usb_ep =
*ep)=0A=
>>>>     	struct dwc2_hsotg *hsotg =3D hs_ep->parent;=0A=
>>>>     	int dir_in =3D hs_ep->dir_in;=0A=
>>>>     	int index =3D hs_ep->index;=0A=
>>>> -	unsigned long flags;=0A=
>>>>     	u32 epctrl_reg;=0A=
>>>>     	u32 ctrl;=0A=
>>>> -	int locked;=0A=
>>>>     =0A=
>>>>     	dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);=0A=
>>>>     =0A=
>>>> @@ -4088,10 +4087,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep =
*ep)=0A=
>>>>     =0A=
>>>>     	epctrl_reg =3D dir_in ? DIEPCTL(index) : DOEPCTL(index);=0A=
>>>>     =0A=
>>>> -	locked =3D spin_is_locked(&hsotg->lock);=0A=
>>>> -	if (!locked)=0A=
>>>> -		spin_lock_irqsave(&hsotg->lock, flags);=0A=
>>>> -=0A=
>>>>     	ctrl =3D dwc2_readl(hsotg, epctrl_reg);=0A=
>>>>     =0A=
>>>>     	if (ctrl & DXEPCTL_EPENA)=0A=
>>>> @@ -4114,12 +4109,23 @@ static int dwc2_hsotg_ep_disable(struct usb_ep=
 *ep)=0A=
>>>>     	hs_ep->fifo_index =3D 0;=0A=
>>>>     	hs_ep->fifo_size =3D 0;=0A=
>>>>     =0A=
>>>> -	if (!locked)=0A=
>>>> -		spin_unlock_irqrestore(&hsotg->lock, flags);=0A=
>>>> -=0A=
>>>>     	return 0;=0A=
>>>>     }=0A=
>>>>     =0A=
>>>> +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep)=0A=
>>>> +{=0A=
>>>> +	struct dwc2_hsotg_ep *hs_ep =3D our_ep(ep);=0A=
>>>> +	struct dwc2_hsotg *hsotg =3D hs_ep->parent;=0A=
>>>> +	unsigned long flags;=0A=
>>>> +	int ret;=0A=
>>>> +=0A=
>>>> +	spin_lock_irqsave(&hsotg->lock, flags);=0A=
>>>> +	ret =3D dwc2_hsotg_ep_disable(ep);=0A=
>>>> +	spin_unlock_irqrestore(&hsotg->lock, flags);=0A=
>>>> +=0A=
>>>> +	return ret;=0A=
>>>> +}=0A=
>>>> +=0A=
>>>>     /**=0A=
>>>>      * on_list - check request is on the given endpoint=0A=
>>>>      * @ep: The endpoint to check.=0A=
>>>> @@ -4267,7 +4273,7 @@ static int dwc2_hsotg_ep_sethalt_lock(struct usb=
_ep *ep, int value)=0A=
>>>>     =0A=
>>>>     static const struct usb_ep_ops dwc2_hsotg_ep_ops =3D {=0A=
>>>>     	.enable		=3D dwc2_hsotg_ep_enable,=0A=
>>>> -	.disable	=3D dwc2_hsotg_ep_disable,=0A=
>>>> +	.disable	=3D dwc2_hsotg_ep_disable_lock,=0A=
>>>>     	.alloc_request	=3D dwc2_hsotg_ep_alloc_request,=0A=
>>>>     	.free_request	=3D dwc2_hsotg_ep_free_request,=0A=
>>>>     	.queue		=3D dwc2_hsotg_ep_queue_lock,=0A=
>>>> @@ -4407,9 +4413,9 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget=
 *gadget)=0A=
>>>>     	/* all endpoints should be shutdown */=0A=
>>>>     	for (ep =3D 1; ep < hsotg->num_of_eps; ep++) {=0A=
>>>>     		if (hsotg->eps_in[ep])=0A=
>>>> -			dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);=0A=
>>>> +			dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);=0A=
>>>>     		if (hsotg->eps_out[ep])=0A=
>>>> -			dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);=0A=
>>>> +			dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);=0A=
>>>>     	}=0A=
>>>>     =0A=
>>>>     	spin_lock_irqsave(&hsotg->lock, flags);=0A=
>>>> @@ -4857,9 +4863,9 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)=
=0A=
>>>>     =0A=
>>>>     		for (ep =3D 0; ep < hsotg->num_of_eps; ep++) {=0A=
>>>>     			if (hsotg->eps_in[ep])=0A=
>>>> -				dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);=0A=
>>>> +				dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);=0A=
>>>>     			if (hsotg->eps_out[ep])=0A=
>>>> -				dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);=0A=
>>>> +				dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);=0A=
>>>>     		}=0A=
>>>>     	}=0A=
>>>>     =0A=
>>>>=0A=
>>>=0A=
>>> Your code doesn't take care about fifo_map warnings from=0A=
>>> dwc2_hsotg_init_fifo() function. Before calling dwc2_hsotg_init_fifo()=
=0A=
>>> from dwc2_hsotg_core_init_disconnected() function all Ep's should=0A=
>>> disabled and fifo bitmap should be cleared.=0A=
>>>=0A=
>>=0A=
>> Correct.  I am only trying to fix the locking.  I hope you can fix the=
=0A=
>> rest in a separate patch.=0A=
>>=0A=
> Yeah. I'll try deeper investigate driver locking flow and fix it later.=
=0A=
> Actually, I like your idea with introducing dwc2_hsotg_ep_disable_lock()=
=0A=
> function. Maybe you yourself will submit new patch for safe locking=0A=
> fixes? But please just after my patch will applied :-)=0A=
> Currently there are 2-3 high priority issues reported by community and I=
=0A=
> should find solutions/fixes.=0A=
> Thank you very much for your time and useful feedback.=0A=
> =0A=
> Thanks,=0A=
> Minas=0A=
> =0A=
> =0A=
>> regards,=0A=
>> dan carpenter=0A=
>>=0A=
>>=0A=
> =0A=
> =0A=
=0A=
My patch doesn't pass sparse checking: "warning: context imbalance in =0A=
'dwc2_hsotg_core_init_disconnected' - unexpected unlock". Sparse persist!=
=0A=
So, I need to re-work patch. Can I use your idea with =0A=
dwc2_hsotg_ep_disable_lock() function to prepare new one?=0A=
=0A=
Thanks,=0A=
Minas=0A=