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

[Robert Baldyga <r.baldyga@samsung.com>]

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
>>
>