Re: [PATCH 1/2] usb: gadget: add usb_gadget_activate/deactivate functions

[Robert Baldyga]

On 04/29/2015 05:30 PM, Felipe Balbi wrote:
> On Wed, Apr 29, 2015 at 11:08:06AM +0200, Robert Baldyga wrote:
>> Hi Felipe,
>>
>> On 04/28/2015 06:40 PM, Felipe Balbi wrote:
>>> On Tue, Apr 07, 2015 at 10:31:52AM +0200, Robert Baldyga wrote:
>>>> These functions allows to deactivate gadget to make it not visible to
>>>> host and make it active again when gadget driver is finally ready.
>>>>
>>>> They are needed to fix usb_function_activate() and usb_function_deactivate()
>>>> functions which currently are not working as usb_gadget_connect() is
>>>> called immediately after function bind regardless to previous calls of
>>>> usb_gadget_disconnect() function.
>>>
>>> and that's what needs to be fixed, a long time ago I wrote the patch
>>> below which I never got to finishing:
>>>
>>> commit a23800e2463ae1f4eafa7c0a15bb44afee75994f
>>> Author: Felipe Balbi <balbi@xxxxxx>
>>> Date:   Thu Jul 26 14:23:44 2012 +0300
>>>
>>>     usb: gadget: let gadgets control pullup on their own
>>>     
>>>     This is useful on gadgets that depend on userland
>>>     daemons to function properly. We can delay connection
>>>     to the host until userland is ready.
>>>     
>>>     Signed-off-by: Felipe Balbi <balbi@xxxxxx>
>>>
>>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>>> index 7c821de8ce3d..790ccf29f2ee 100644
>>> --- a/drivers/usb/gadget/composite.c
>>> +++ b/drivers/usb/gadget/composite.c
>>> @@ -1784,8 +1784,9 @@ int usb_composite_probe(struct usb_composite_driver *driver)
>>>  		driver->name = "composite";
>>>  
>>>  	driver->gadget_driver = composite_driver_template;
>>> -	gadget_driver = &driver->gadget_driver;
>>>  
>>> +	gadget_driver = &driver->gadget_driver;
>>> +	gadget_driver->controls_pullups = driver->controls_pullups;
>>>  	gadget_driver->function =  (char *) driver->name;
>>>  	gadget_driver->driver.name = driver->name;
>>>  	gadget_driver->max_speed = driver->max_speed;
>>> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
>>> index 8a1eeb24ae6a..c0f4fca9384b 100644
>>> --- a/drivers/usb/gadget/udc-core.c
>>> +++ b/drivers/usb/gadget/udc-core.c
>>> @@ -235,7 +235,18 @@ static void usb_gadget_remove_driver(struct usb_udc *udc)
>>>  
>>>  	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>>>  
>>> -	usb_gadget_disconnect(udc->gadget);
>>> +	/*
>>> +	 * NOTICE: if gadget driver wants to control
>>> +	 * pullup, it needs to make sure that when
>>> +	 * user tries to rmmod the gadget driver, it
>>> +	 * will disconnect the pullups before returning
>>> +	 * from its ->unbind() method.
>>> +	 *
>>> +	 * We are truly trusting the gadget driver here.
>>> +	 */
>>> +	if (!udc->driver->controls_pullups)
>>> +		usb_gadget_disconnect(udc->gadget);
>>> +
>>>  	udc->driver->disconnect(udc->gadget);
>>>  	udc->driver->unbind(udc->gadget);
>>>  	usb_gadget_udc_stop(udc->gadget, udc->driver);
>>> @@ -300,7 +311,18 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri
>>>  		driver->unbind(udc->gadget);
>>>  		goto err1;
>>>  	}
>>> -	usb_gadget_connect(udc->gadget);
>>> +
>>> +	/*
>>> +	 * NOTICE: if gadget driver wants to control
>>> +	 * pullups, it needs to make sure its calls
>>> +	 * to usb_function_activate() and
>>> +	 * usb_function_deactivate() are balanced,
>>> +	 * otherwise gadget_driver will never enumerate.
>>> +	 *
>>> +	 * We are truly trusting the gadget driver here.
>>> +	 */
>>> +	if (!driver->controls_pullups)
>>> +		usb_gadget_connect(udc->gadget);
>>>  
>>>  	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>>>  	return 0;
>>> diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
>>> index 3c671c1b37f6..7ae797c85cb9 100644
>>> --- a/include/linux/usb/composite.h
>>> +++ b/include/linux/usb/composite.h
>>> @@ -157,6 +157,7 @@ struct usb_function {
>>>  	int			(*get_status)(struct usb_function *);
>>>  	int			(*func_suspend)(struct usb_function *,
>>>  						u8 suspend_opt);
>>> +
>>>  	/* private: */
>>>  	/* internals */
>>>  	struct list_head		list;
>>> @@ -279,6 +280,8 @@ enum {
>>>   * @max_speed: Highest speed the driver supports.
>>>   * @needs_serial: set to 1 if the gadget needs userspace to provide
>>>   * 	a serial number.  If one is not provided, warning will be printed.
>>> + * @controls_pullups: this driver will control pullup and udc-core shouldn't
>>> + *	enable it by default
>>>   * @bind: (REQUIRED) Used to allocate resources that are shared across the
>>>   *	whole device, such as string IDs, and add its configurations using
>>>   *	@usb_add_config(). This may fail by returning a negative errno
>>> @@ -308,6 +311,7 @@ struct usb_composite_driver {
>>>  	struct usb_gadget_strings		**strings;
>>>  	enum usb_device_speed			max_speed;
>>>  	unsigned		needs_serial:1;
>>> +	unsigned		controls_pullups:1;
>>>  
>>>  	int			(*bind)(struct usb_composite_dev *cdev);
>>>  	int			(*unbind)(struct usb_composite_dev *);
>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>>> index 32b734d88d6b..87971fa38f08 100644
>>> --- a/include/linux/usb/gadget.h
>>> +++ b/include/linux/usb/gadget.h
>>> @@ -774,6 +774,7 @@ static inline int usb_gadget_disconnect(struct usb_gadget *gadget)
>>>   * @suspend: Invoked on USB suspend.  May be called in_interrupt.
>>>   * @resume: Invoked on USB resume.  May be called in_interrupt.
>>>   * @driver: Driver model state for this driver.
>>> + * @controls_pullups: tells udc-core to not enable pullups by default
>>>   *
>>>   * Devices are disabled till a gadget driver successfully bind()s, which
>>>   * means the driver will handle setup() requests needed to enumerate (and
>>> @@ -833,6 +834,8 @@ struct usb_gadget_driver {
>>>  
>>>  	/* FIXME support safe rmmod */
>>>  	struct device_driver	driver;
>>> +
>>> +	unsigned		controls_pullups:1;
>>>  };
>>>  
>>>  
>>>
>>> Together with that, I wrote:
>>>
>>> commit 0b733885e276a38cc9fa415c0977f063f9ce4d9d
>>> Author: Felipe Balbi <balbi@xxxxxx>
>>> Date:   Wed Feb 6 12:34:33 2013 +0200
>>>
>>>     usb: gadget: webcam: let it control pullups
>>>     
>>>     this gadget driver needs to make sure that
>>>     we will only enumerate after its userland
>>>     counterpart is up and running.
>>>     
>>>     Signed-off-by: Felipe Balbi <balbi@xxxxxx>
>>>
>>> diff --git a/drivers/usb/gadget/webcam.c b/drivers/usb/gadget/webcam.c
>>> index 8cef1e658c29..41a4d03715bc 100644
>>> --- a/drivers/usb/gadget/webcam.c
>>> +++ b/drivers/usb/gadget/webcam.c
>>> @@ -388,6 +388,7 @@ static __refdata struct usb_composite_driver webcam_driver = {
>>>  	.max_speed	= USB_SPEED_SUPER,
>>>  	.bind		= webcam_bind,
>>>  	.unbind		= webcam_unbind,
>>> +	.controls_pullups = true,
>>>  };
>>>  
>>>  static int __init
>>>
>>> This makes it really simple, either a gadget driver wants to control
>>> pullups, or it doesn't. For those who wish to control pullups (for
>>> whatever reason) we rely completely on the gadget telling us it's ok to
>>> connect pullups by means of usb_function_activate()/deactivate(), for
>>> all others, we just connect straight away.
>>>
>>
>> It looks like your patch fixes problem at gadget level, but in case of
>> gadgets composed using ConfigFS we don't know if any function will want
>> to deactivate gadget. We would need to supply mechanism which functions
>> could use to tell composite that they want to control pullups by
>> themselves. That's why I made it a little more complicated, but in
>> result we have no need to modify function drivers.
> 
> I really prefer functions to tell us that they can control pullups; much
> like Host stack requires a "supports_autosuspend" flag if your driver
> supports runtime PM.
> 
> I see that your patch is much more granular, but then, we need a
> per-function flag then we could:
> 
> for_each_function(config)
> 	if (fuction->controls_pullup)
> 		usb_function_deactivate(function);
> 
> (well, just illustrating)
> 
> Then, usb_function_deactivate() is initially called by the framework so
> that deactivate counter already has the correct initial value and gadget
> will only connect after all functions in a particular configuration have
> called usb_function_activate(). How do you feel about that ?

Looks good to me, makes things more clear. I will update my patches.

Thanks,
Robert Baldyga
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/