Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Felipe Balbi <balbi@kernel.org>]

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


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> On Mon, 3 Apr 2017, Roger Quadros wrote:
>
>> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
>> repeatedly on the same gadget->dev structure.
>> 
>> We need to clear the gadget->dev structure so that kobject_init()
>> doesn't complain about already initialized object.
>> 
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/usb/gadget/udc/core.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>> index d685d82..efce68e 100644
>> --- a/drivers/usb/gadget/udc/core.c
>> +++ b/drivers/usb/gadget/udc/core.c
>> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>>  	flush_work(&gadget->work);
>>  	device_unregister(&udc->dev);
>>  	device_unregister(&gadget->dev);
>> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
>>  }
>>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>
> Isn't this dangerous?  It's quite possible that the device_unregister() 

not on the gadget API, no.

> call on the previous line invokes the gadget->dev.release callback, 
> which might deallocate gadget.  If that happens, your new memset will 
> oops.

that won't happen. struct usb_gadget is a member of the UDC's private
structure, like this:

struct dwc3 {
	[...]
	struct usb_gadget	gadget;
	struct usb_gadget_driver *gadget_driver;
	[...]
};

I'm actually thinking that struct usb_gadget shouldn't have a struct
device at all. Just a pointer to a device, that would solve all these
issues.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]