Re: [PATCH v6 4/4] usb: gadget: udc-core: independent registration of gadgets and gadget drivers

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hello,

On 2015-11-20 10:26, Peter Chen wrote:
> On Fri, Nov 20, 2015 at 09:54:12AM +0100, Marek Szyprowski wrote:
>> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>
>> Change behavior during registration of gadgets and
>> gadget drivers in udc-core. Instead of previous
>> approach when for successful probe of usb gadget driver
>> at least one usb gadget should be already registered
>> use another one where gadget drivers and gadgets
>> can be registered in udc-core independently.
>>
>> Independent registration of gadgets and gadget drivers
>> is useful for built-in into kernel gadget and gadget
>> driver case - because it's possible that gadget is
>> really probed only on late_init stage (due to deferred
>> probe) whereas gadget driver's probe is silently failed
>> on module_init stage due to no any UDC added.
>>
>> Also it is useful for modules case - now there is no
>> difference what module to insert first: gadget module
>> or gadget driver one.
>>
>> Tested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>> [simplified code as requested by Alan Stern and Felipe Balbi]
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/usb/gadget/udc/udc-core.c | 43 +++++++++++++++++++++++++++++++--------
>>   include/linux/usb/gadget.h        |  2 ++
>>   2 files changed, 37 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
>> index f76ebc8..461b311 100644
>> --- a/drivers/usb/gadget/udc/udc-core.c
>> +++ b/drivers/usb/gadget/udc/udc-core.c
>> @@ -51,8 +51,12 @@ struct usb_udc {
>>   
>>   static struct class *udc_class;
>>   static LIST_HEAD(udc_list);
>> +static LIST_HEAD(gadget_driver_pending_list);
>>   static DEFINE_MUTEX(udc_lock);
>>   
>> +static int udc_bind_to_driver(struct usb_udc *udc,
>> +		struct usb_gadget_driver *driver);
>> +
>>   /* ------------------------------------------------------------------------- */
>>   
>>   #ifdef	CONFIG_HAS_DMA
>> @@ -356,6 +360,7 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
>>   		void (*release)(struct device *dev))
>>   {
>>   	struct usb_udc		*udc;
>> +	struct usb_gadget_driver *driver;
>>   	int			ret = -ENOMEM;
>>   
>>   	udc = kzalloc(sizeof(*udc), GFP_KERNEL);
>> @@ -403,6 +408,18 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
>>   	usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
>>   	udc->vbus = true;
>>   
>> +	/* pick up one of pending gadget drivers */
>> +	list_for_each_entry(driver, &gadget_driver_pending_list, pending) {
>> +		if (!driver->udc_name || strcmp(driver->udc_name,
>> +						dev_name(&udc->dev)) == 0) {
>> +			ret = udc_bind_to_driver(udc, driver);
>> +			if (ret)
>> +				goto err4;
>> +			list_del(&driver->pending);
>> +			break;
>> +		}
>> +	}
>> +
>>   	mutex_unlock(&udc_lock);
>>   
>>   	return 0;
>> @@ -475,9 +492,16 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>>   	list_del(&udc->list);
>>   	mutex_unlock(&udc_lock);
>>   
>> -	if (udc->driver)
>> +	if (udc->driver) {
>> +		struct usb_gadget_driver *driver = udc->driver;
>> +
>>   		usb_gadget_remove_driver(udc);
>>   
>> +		mutex_lock(&udc_lock);
>> +		list_add(&driver->pending, &gadget_driver_pending_list);
>> +		mutex_unlock(&udc_lock);
>> +	}
>> +
>>   	kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
>>   	flush_work(&gadget->work);
>>   	device_unregister(&udc->dev);
>> @@ -535,11 +559,7 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
>>   			if (!ret)
>>   				break;
>>   		}
>> -		if (ret)
>> -			ret = -ENODEV;
>> -		else if (udc->driver)
>> -			ret = -EBUSY;
>> -		else
>> +		if (!ret && !udc->driver)
>>   			goto found;
>>   	} else {
>>   		list_for_each_entry(udc, &udc_list, list) {
>> @@ -549,9 +569,12 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
>>   		}
>>   	}
>>   
>> -	pr_debug("couldn't find an available UDC\n");
>> +	list_add_tail(&driver->pending, &gadget_driver_pending_list);
>> +	pr_info("udc-core: couldn't find an available UDC "
>> +			"- added [%s] to list of pending drivers\n",
>> +			driver->function);
>>   	mutex_unlock(&udc_lock);
>> -	return ret;
>> +	return 0;
>>   found:
>>   	ret = udc_bind_to_driver(udc, driver);
>>   	mutex_unlock(&udc_lock);
>> @@ -577,6 +600,10 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
>>   			break;
>>   		}
>>   
>> +	if (ret) {
>> +		list_del(&driver->pending);
>> +		ret = 0;
>> +	}
>>   	mutex_unlock(&udc_lock);
>>   	return ret;
>>   }
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index e11f5a2..a3436bf 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -1014,6 +1014,7 @@ static inline int usb_gadget_activate(struct usb_gadget *gadget)
>>    * @driver: Driver model state for this driver.
>>    * @udc_name: A name of UDC this driver should be bound to. If udc_name is NULL,
>>    * 	this driver will be bound to any available UDC.
>> + * @pending: UDC core private data used for deferred probe of this driver.
>>    *
>>    * Devices are disabled till a gadget driver successfully bind()s, which
>>    * means the driver will handle setup() requests needed to enumerate (and
>> @@ -1076,6 +1077,7 @@ struct usb_gadget_driver {
>>   	struct device_driver	driver;
>>   
>>   	char			*udc_name;
>> +	struct list_head	pending;
>>   };
>>   
>>   
> And it seems can't apply for felipe's testing/next which I just rebased
> on it.

I really have no idea why it fails for You. The patchset applies 
correctly on both
Felipe's 'next' and 'testing/next' branches from today.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland