[PATCH] usb: gadget: udc-core: call udc_stop() before gadget unbind

[PATCH] usb: gadget: udc-core: call udc_stop() before gadget unbind

[Robert Baldyga]

As usb function drivers assumes that all usb request will be completed
before function unbind call, we should supply such behavior. In some
cases ep_disable() won't kill all request effectively, because some
IN requests can be in running state. In such situation it's possible
to have unbind function called before last request completion, which
can cause problems.

For example unbinding f_ecm function while request on 'notify' endpoint
is not completed, ends up NULL pointer dereference in unbind() function.

usb_gadget_udc_stop() call causes completion of all requests so if it's
called before gadget unbind there is no risk that some of requests will
stay uncompleted.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/udc/udc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index e31d574..6f0d233 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -331,8 +331,8 @@ static void usb_gadget_remove_driver(struct usb_udc *udc)
 
 	usb_gadget_disconnect(udc->gadget);
 	udc->driver->disconnect(udc->gadget);
-	udc->driver->unbind(udc->gadget);
 	usb_gadget_udc_stop(udc);
+	udc->driver->unbind(udc->gadget);
 
 	udc->driver = NULL;
 	udc->dev.driver = NULL;
-- 
1.9.1

RE: [PATCH] usb: gadget: udc-core: call udc_stop() before gadget unbind

[Krzysztof Opasiak]

> -----Original Message-----
> From: Robert Baldyga [mailto:r.baldyga@samsung.com]
> Sent: Friday, December 12, 2014 2:17 PM
> To: balbi@ti.com
> Cc: gregkh@linuxfoundation.org; peter.chen@freescale.com; linux-
> usb@vger.kernel.org; linux-kernel@vger.kernel.org;
> m.szyprowski@samsung.com; k.opasiak@samsung.com; Robert Baldyga
> Subject: [PATCH] usb: gadget: udc-core: call udc_stop() before
> gadget unbind
> 
> As usb function drivers assumes that all usb request will be
> completed
> before function unbind call, we should supply such behavior. In
> some
> cases ep_disable() won't kill all request effectively, because some
> IN requests can be in running state. In such situation it's
> possible
> to have unbind function called before last request completion,
> which
> can cause problems.
> 
> For example unbinding f_ecm function while request on 'notify'
> endpoint
> is not completed, ends up NULL pointer dereference in unbind()
> function.
> 
> usb_gadget_udc_stop() call causes completion of all requests so if
> it's
> called before gadget unbind there is no risk that some of requests
> will
> stay uncompleted.
> 
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>

This finally solves issue described before in [1]

Tested-by: Krzysztof Opasiak <k.opasiak@samsung.com>

Footnotes:
1 - https://lkml.org/lkml/2014/12/9/283

-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
k.opasiak@samsung.com

Re: [PATCH] usb: gadget: udc-core: call udc_stop() before gadget unbind

[Peter Chen]

On Fri, Dec 12, 2014 at 02:17:28PM +0100, Robert Baldyga wrote:
> As usb function drivers assumes that all usb request will be completed
> before function unbind call, we should supply such behavior. In some
> cases ep_disable() won't kill all request effectively, because some
> IN requests can be in running state. In such situation it's possible
> to have unbind function called before last request completion, which
> can cause problems.

Doesn't the function's disable/unbind should call usb_ep_dequeue to make
sure the transfer has ended?

.udc_stop may stop the controller, and .unbind may still visit hardware.

> 
> For example unbinding f_ecm function while request on 'notify' endpoint
> is not completed, ends up NULL pointer dereference in unbind() function.
> 
> usb_gadget_udc_stop() call causes completion of all requests so if it's
> called before gadget unbind there is no risk that some of requests will
> stay uncompleted.
> 
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/usb/gadget/udc/udc-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> index e31d574..6f0d233 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -331,8 +331,8 @@ static void usb_gadget_remove_driver(struct usb_udc *udc)
>  
>  	usb_gadget_disconnect(udc->gadget);
>  	udc->driver->disconnect(udc->gadget);
> -	udc->driver->unbind(udc->gadget);
>  	usb_gadget_udc_stop(udc);
> +	udc->driver->unbind(udc->gadget);
>  
>  	udc->driver = NULL;
>  	udc->dev.driver = NULL;
> -- 
> 1.9.1
> 

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: gadget: udc-core: call udc_stop() before gadget unbind

[Robert Baldyga]

On 12/15/2014 06:13 AM, Peter Chen wrote:
> On Fri, Dec 12, 2014 at 02:17:28PM +0100, Robert Baldyga wrote:
>> As usb function drivers assumes that all usb request will be completed
>> before function unbind call, we should supply such behavior. In some
>> cases ep_disable() won't kill all request effectively, because some
>> IN requests can be in running state. In such situation it's possible
>> to have unbind function called before last request completion, which
>> can cause problems.
> 
> Doesn't the function's disable/unbind should call usb_ep_dequeue to make
> sure the transfer has ended?

USB function drivers like ECM or HID surely doesn't. It looks like
there's assumption that all requests will be completed by UDC driver.

Function ep_disable() should complete requests in hardware driver, but
at least in DWC2 driver not all requests are completed at this stage
(request which are in hardware FIFO are omitted to give them chance to
be transferred). Those requests are forced to complete in udc_stop()
function, and that's why it's needed to be called before unbind.

> 
> .udc_stop may stop the controller, and .unbind may still visit hardware.

Hmm, indeed it can be problem.

Thanks,
Robert Baldyga

> 
>>
>> For example unbinding f_ecm function while request on 'notify' endpoint
>> is not completed, ends up NULL pointer dereference in unbind() function.
>>
>> usb_gadget_udc_stop() call causes completion of all requests so if it's
>> called before gadget unbind there is no risk that some of requests will
>> stay uncompleted.
>>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>  drivers/usb/gadget/udc/udc-core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
>> index e31d574..6f0d233 100644
>> --- a/drivers/usb/gadget/udc/udc-core.c
>> +++ b/drivers/usb/gadget/udc/udc-core.c
>> @@ -331,8 +331,8 @@ static void usb_gadget_remove_driver(struct usb_udc *udc)
>>  
>>  	usb_gadget_disconnect(udc->gadget);
>>  	udc->driver->disconnect(udc->gadget);
>> -	udc->driver->unbind(udc->gadget);
>>  	usb_gadget_udc_stop(udc);
>> +	udc->driver->unbind(udc->gadget);
>>  
>>  	udc->driver = NULL;
>>  	udc->dev.driver = NULL;
>> -- 
>> 1.9.1
>>
> 

Re: [PATCH] usb: gadget: udc-core: call udc_stop() before gadget unbind

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]

On Mon, Dec 15, 2014 at 11:05:22AM +0100, Robert Baldyga wrote:
> On 12/15/2014 06:13 AM, Peter Chen wrote:
> > On Fri, Dec 12, 2014 at 02:17:28PM +0100, Robert Baldyga wrote:
> >> As usb function drivers assumes that all usb request will be completed
> >> before function unbind call, we should supply such behavior. In some
> >> cases ep_disable() won't kill all request effectively, because some
> >> IN requests can be in running state. In such situation it's possible
> >> to have unbind function called before last request completion, which
> >> can cause problems.
> > 
> > Doesn't the function's disable/unbind should call usb_ep_dequeue to make
> > sure the transfer has ended?
> 
> USB function drivers like ECM or HID surely doesn't. It looks like
> there's assumption that all requests will be completed by UDC driver.

that's a bug on those drivers :-)

> Function ep_disable() should complete requests in hardware driver, but
> at least in DWC2 driver not all requests are completed at this stage

and that's a bug on dwc2 :-)

> (request which are in hardware FIFO are omitted to give them chance to
> be transferred). Those requests are forced to complete in udc_stop()

that's wrong, we're disabling the endpoint anyway. Either dwc2 needs to
wait_for_completion() or forcibly cancel the request. The bottom line is
that control should not exit ep_disable() until all requests have been
quiesced.

> function, and that's why it's needed to be called before unbind.
> 
> > 
> > .udc_stop may stop the controller, and .unbind may still visit hardware.
> 
> Hmm, indeed it can be problem.

yes, it will be :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: gadget: udc-core: call udc_stop() before gadget unbind

[Robert Baldyga]

Hi Felipe,

On 12/22/2014 05:34 PM, Felipe Balbi wrote:
> On Mon, Dec 15, 2014 at 11:05:22AM +0100, Robert Baldyga wrote:
>> On 12/15/2014 06:13 AM, Peter Chen wrote:
>>> On Fri, Dec 12, 2014 at 02:17:28PM +0100, Robert Baldyga wrote:
>>>> As usb function drivers assumes that all usb request will be completed
>>>> before function unbind call, we should supply such behavior. In some
>>>> cases ep_disable() won't kill all request effectively, because some
>>>> IN requests can be in running state. In such situation it's possible
>>>> to have unbind function called before last request completion, which
>>>> can cause problems.
>>>
>>> Doesn't the function's disable/unbind should call usb_ep_dequeue to make
>>> sure the transfer has ended?
>>
>> USB function drivers like ECM or HID surely doesn't. It looks like
>> there's assumption that all requests will be completed by UDC driver.
> 
> that's a bug on those drivers :-)

So we can't make assumptions that requests will be completed in
ep_disable()?

> 
>> Function ep_disable() should complete requests in hardware driver, but
>> at least in DWC2 driver not all requests are completed at this stage
> 
> and that's a bug on dwc2 :-)

I have already found that out. Another UDC drivers simply kill all
request without waiting for currently running, so I did the same in
DWC2. Here is my patch:
http://www.spinics.net/lists/linux-usb/msg118698.html

> 
>> (request which are in hardware FIFO are omitted to give them chance to
>> be transferred). Those requests are forced to complete in udc_stop()
> 
> that's wrong, we're disabling the endpoint anyway. Either dwc2 needs to
> wait_for_completion() or forcibly cancel the request. The bottom line is
> that control should not exit ep_disable() until all requests have been
> quiesced.

So that's not bug in function drivers. They make correct assumption,
because they expect that requests will be completed in ep_disable().

> 
>> function, and that's why it's needed to be called before unbind.
>>
>>>
>>> .udc_stop may stop the controller, and .unbind may still visit hardware.
>>
>> Hmm, indeed it can be problem.
> 
> yes, it will be :-)
> 

Thanks,
Robert Baldyga

Re: [PATCH] usb: gadget: udc-core: call udc_stop() before gadget unbind

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2221 bytes --]

On Tue, Dec 23, 2014 at 07:34:15AM +0100, Robert Baldyga wrote:
> Hi Felipe,
> 
> On 12/22/2014 05:34 PM, Felipe Balbi wrote:
> > On Mon, Dec 15, 2014 at 11:05:22AM +0100, Robert Baldyga wrote:
> >> On 12/15/2014 06:13 AM, Peter Chen wrote:
> >>> On Fri, Dec 12, 2014 at 02:17:28PM +0100, Robert Baldyga wrote:
> >>>> As usb function drivers assumes that all usb request will be completed
> >>>> before function unbind call, we should supply such behavior. In some
> >>>> cases ep_disable() won't kill all request effectively, because some
> >>>> IN requests can be in running state. In such situation it's possible
> >>>> to have unbind function called before last request completion, which
> >>>> can cause problems.
> >>>
> >>> Doesn't the function's disable/unbind should call usb_ep_dequeue to make
> >>> sure the transfer has ended?
> >>
> >> USB function drivers like ECM or HID surely doesn't. It looks like
> >> there's assumption that all requests will be completed by UDC driver.
> > 
> > that's a bug on those drivers :-)
> 
> So we can't make assumptions that requests will be completed in
> ep_disable()?

oh, no you can. I misread it.

> >> Function ep_disable() should complete requests in hardware driver, but
> >> at least in DWC2 driver not all requests are completed at this stage
> > 
> > and that's a bug on dwc2 :-)
> 
> I have already found that out. Another UDC drivers simply kill all
> request without waiting for currently running, so I did the same in
> DWC2. Here is my patch:
> http://www.spinics.net/lists/linux-usb/msg118698.html

should be in my pull request already.

> >> (request which are in hardware FIFO are omitted to give them chance to
> >> be transferred). Those requests are forced to complete in udc_stop()
> > 
> > that's wrong, we're disabling the endpoint anyway. Either dwc2 needs to
> > wait_for_completion() or forcibly cancel the request. The bottom line is
> > that control should not exit ep_disable() until all requests have been
> > quiesced.
> 
> So that's not bug in function drivers. They make correct assumption,
> because they expect that requests will be completed in ep_disable().

right :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: gadget: udc-core: call udc_stop() before gadget unbind

[Robert Baldyga]

Hi Felipe,

On 12/23/2014 07:31 PM, Felipe Balbi wrote:
> On Tue, Dec 23, 2014 at 07:34:15AM +0100, Robert Baldyga wrote:
>> On 12/22/2014 05:34 PM, Felipe Balbi wrote:
>>> On Mon, Dec 15, 2014 at 11:05:22AM +0100, Robert Baldyga wrote:
>>>> On 12/15/2014 06:13 AM, Peter Chen wrote:
>>>>> On Fri, Dec 12, 2014 at 02:17:28PM +0100, Robert Baldyga wrote:
>>>>>> As usb function drivers assumes that all usb request will be completed
>>>>>> before function unbind call, we should supply such behavior. In some
>>>>>> cases ep_disable() won't kill all request effectively, because some
>>>>>> IN requests can be in running state. In such situation it's possible
>>>>>> to have unbind function called before last request completion, which
>>>>>> can cause problems.
>>>>>
>>>>> Doesn't the function's disable/unbind should call usb_ep_dequeue to make
>>>>> sure the transfer has ended?
>>>>
>>>> USB function drivers like ECM or HID surely doesn't. It looks like
>>>> there's assumption that all requests will be completed by UDC driver.
>>>
>>> that's a bug on those drivers :-)
>>
>> So we can't make assumptions that requests will be completed in
>> ep_disable()?
> 
> oh, no you can. I misread it.
> 
>>>> Function ep_disable() should complete requests in hardware driver, but
>>>> at least in DWC2 driver not all requests are completed at this stage
>>>
>>> and that's a bug on dwc2 :-)
>>
>> I have already found that out. Another UDC drivers simply kill all
>> request without waiting for currently running, so I did the same in
>> DWC2. Here is my patch:
>> http://www.spinics.net/lists/linux-usb/msg118698.html
> 
> should be in my pull request already.

It looks like you applied wrong patch. I meant patch titled "drivers:
usb: dwc2: remove 'force' parameter from kill_all_requests()" is the
latest and complete fix. The patch you have applied named "usb: dwc2:
gadget: kill requests with 'force' in s3c_hsotg_udc_stop()" do not solve
problem completely without changes in udc-core, which we concluded are
not acceptable.

Sorry for the mess. I understand that titles of both patches are
confusing similar.

Best regards,
Robert Baldyga

Re: [PATCH] usb: gadget: udc-core: call udc_stop() before gadget unbind

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2442 bytes --]

On Mon, Dec 29, 2014 at 08:30:04AM +0100, Robert Baldyga wrote:
> Hi Felipe,
> 
> On 12/23/2014 07:31 PM, Felipe Balbi wrote:
> > On Tue, Dec 23, 2014 at 07:34:15AM +0100, Robert Baldyga wrote:
> >> On 12/22/2014 05:34 PM, Felipe Balbi wrote:
> >>> On Mon, Dec 15, 2014 at 11:05:22AM +0100, Robert Baldyga wrote:
> >>>> On 12/15/2014 06:13 AM, Peter Chen wrote:
> >>>>> On Fri, Dec 12, 2014 at 02:17:28PM +0100, Robert Baldyga wrote:
> >>>>>> As usb function drivers assumes that all usb request will be completed
> >>>>>> before function unbind call, we should supply such behavior. In some
> >>>>>> cases ep_disable() won't kill all request effectively, because some
> >>>>>> IN requests can be in running state. In such situation it's possible
> >>>>>> to have unbind function called before last request completion, which
> >>>>>> can cause problems.
> >>>>>
> >>>>> Doesn't the function's disable/unbind should call usb_ep_dequeue to make
> >>>>> sure the transfer has ended?
> >>>>
> >>>> USB function drivers like ECM or HID surely doesn't. It looks like
> >>>> there's assumption that all requests will be completed by UDC driver.
> >>>
> >>> that's a bug on those drivers :-)
> >>
> >> So we can't make assumptions that requests will be completed in
> >> ep_disable()?
> > 
> > oh, no you can. I misread it.
> > 
> >>>> Function ep_disable() should complete requests in hardware driver, but
> >>>> at least in DWC2 driver not all requests are completed at this stage
> >>>
> >>> and that's a bug on dwc2 :-)
> >>
> >> I have already found that out. Another UDC drivers simply kill all
> >> request without waiting for currently running, so I did the same in
> >> DWC2. Here is my patch:
> >> http://www.spinics.net/lists/linux-usb/msg118698.html
> > 
> > should be in my pull request already.
> 
> It looks like you applied wrong patch. I meant patch titled "drivers:
> usb: dwc2: remove 'force' parameter from kill_all_requests()" is the
> latest and complete fix. The patch you have applied named "usb: dwc2:
> gadget: kill requests with 'force' in s3c_hsotg_udc_stop()" do not solve
> problem completely without changes in udc-core, which we concluded are
> not acceptable.
> 
> Sorry for the mess. I understand that titles of both patches are
> confusing similar.

Can you send a patch on top fixing things up ? Either that or a revert
followed by correct patch.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: gadget: udc-core: call udc_stop() before gadget unbind

[Alan Stern]

On Fri, 12 Dec 2014, Robert Baldyga wrote:

> As usb function drivers assumes that all usb request will be completed
> before function unbind call, we should supply such behavior. In some
> cases ep_disable() won't kill all request effectively, because some
> IN requests can be in running state. In such situation it's possible
> to have unbind function called before last request completion, which
> can cause problems.
> 
> For example unbinding f_ecm function while request on 'notify' endpoint
> is not completed, ends up NULL pointer dereference in unbind() function.
> 
> usb_gadget_udc_stop() call causes completion of all requests so if it's
> called before gadget unbind there is no risk that some of requests will
> stay uncompleted.
> 
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/usb/gadget/udc/udc-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> index e31d574..6f0d233 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -331,8 +331,8 @@ static void usb_gadget_remove_driver(struct usb_udc *udc)
>  
>  	usb_gadget_disconnect(udc->gadget);
>  	udc->driver->disconnect(udc->gadget);
> -	udc->driver->unbind(udc->gadget);
>  	usb_gadget_udc_stop(udc);
> +	udc->driver->unbind(udc->gadget);
>  
>  	udc->driver = NULL;
>  	udc->dev.driver = NULL;

There has been a lot of churn and a lot of bug fixes involving those
lines of code.  Have you checked the git log for this function?  It's
quite possible that interchanging those two statements will recreate a
bug that has already been fixed.

Alan Stern

Re: [PATCH] usb: gadget: udc-core: call udc_stop() before gadget unbind

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1821 bytes --]

On Fri, Dec 12, 2014 at 02:17:28PM +0100, Robert Baldyga wrote:
> As usb function drivers assumes that all usb request will be completed
> before function unbind call, we should supply such behavior. In some
> cases ep_disable() won't kill all request effectively, because some
> IN requests can be in running state. In such situation it's possible
> to have unbind function called before last request completion, which
> can cause problems.
> 
> For example unbinding f_ecm function while request on 'notify' endpoint
> is not completed, ends up NULL pointer dereference in unbind() function.

this is a bug on f_ecm, however.

> usb_gadget_udc_stop() call causes completion of all requests so if it's
> called before gadget unbind there is no risk that some of requests will
> stay uncompleted.

we can't really stop the controller before the function's ->unbind() has
been called. Keep in mind that we can completely kill off the controller
(including gating clocks and, in some rare cases, disabling the power
domain) after ->udc_stop() has been called.

> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/usb/gadget/udc/udc-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> index e31d574..6f0d233 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -331,8 +331,8 @@ static void usb_gadget_remove_driver(struct usb_udc *udc)
>  
>  	usb_gadget_disconnect(udc->gadget);
>  	udc->driver->disconnect(udc->gadget);
> -	udc->driver->unbind(udc->gadget);
>  	usb_gadget_udc_stop(udc);
> +	udc->driver->unbind(udc->gadget);
>  
>  	udc->driver = NULL;
>  	udc->dev.driver = NULL;
> -- 
> 1.9.1
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]