[PATCH v2] usb: gadget: Add uevent to notify userspace

[PATCH v2] usb: gadget: Add uevent to notify userspace

[Baolin Wang]

From: Badhri Jagan Sridharan <Badhri@google.com>

Some USB managament on userspace (like Android system) rely on the uevents
generated by the composition driver to generate user notifications. Thus this
patch adds uevents to be generated whenever USB changes its state: connected,
disconnected, configured.

The original code was created by Badhri Jagan Sridharan, and I did some
optimization.

Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes since v1:
 - Add Badhri's Signed-off-by.
---
 drivers/usb/gadget/Kconfig    |    8 +++
 drivers/usb/gadget/configfs.c |  107 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+)

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 2ea3fc3..9f5d0c6 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -223,6 +223,14 @@ config USB_CONFIGFS
 	  appropriate symbolic links.
 	  For more information see Documentation/usb/gadget_configfs.txt.
 
+config USB_CONFIGFS_UEVENT
+	boolean "Uevent notification of Gadget state"
+	depends on USB_CONFIGFS
+	help
+	  Enable uevent notifications to userspace when the gadget
+	  state changes. The gadget can be in any of the following
+	  three states: "CONNECTED/DISCONNECTED/CONFIGURED"
+
 config USB_CONFIGFS_SERIAL
 	bool "Generic serial bulk in/out"
 	depends on USB_CONFIGFS
diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 3984787..4c2bc27 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -60,6 +60,11 @@ struct gadget_info {
 	bool use_os_desc;
 	char b_vendor_code;
 	char qw_sign[OS_STRING_QW_SIGN_LEN];
+#ifdef CONFIG_USB_CONFIGFS_UEVENT
+	bool connected;
+	bool sw_connected;
+	struct work_struct work;
+#endif
 };
 
 static inline struct gadget_info *to_gadget_info(struct config_item *item)
@@ -1197,6 +1202,57 @@ int composite_dev_prepare(struct usb_composite_driver *composite,
 int composite_os_desc_req_prepare(struct usb_composite_dev *cdev,
 				  struct usb_ep *ep0);
 
+#ifdef CONFIG_USB_CONFIGFS_UEVENT
+static void configfs_work(struct work_struct *data)
+{
+	struct gadget_info *gi = container_of(data, struct gadget_info, work);
+	struct usb_composite_dev *cdev = &gi->cdev;
+	char *disconnected[2] = { "USB_STATE=DISCONNECTED", NULL };
+	char *connected[2] = { "USB_STATE=CONNECTED", NULL };
+	char *configured[2] = { "USB_STATE=CONFIGURED", NULL };
+	/* 0-connected 1-configured 2-disconnected */
+	bool status[3] = { false, false, false };
+	unsigned long flags;
+	bool uevent_sent = false;
+
+	spin_lock_irqsave(&cdev->lock, flags);
+	if (cdev->config && gi->connected)
+		status[1] = true;
+
+	if (gi->connected != gi->sw_connected) {
+		if (gi->connected)
+			status[0] = true;
+		else
+			status[2] = true;
+		gi->sw_connected = gi->connected;
+	}
+	spin_unlock_irqrestore(&cdev->lock, flags);
+
+	if (status[0]) {
+		kobject_uevent_env(&gi->dev->kobj, KOBJ_CHANGE, connected);
+		pr_info("%s: sent uevent %s\n", __func__, connected[0]);
+		uevent_sent = true;
+	}
+
+	if (status[1]) {
+		kobject_uevent_env(&gi->dev->kobj, KOBJ_CHANGE, configured);
+		pr_info("%s: sent uevent %s\n", __func__, configured[0]);
+		uevent_sent = true;
+	}
+
+	if (status[2]) {
+		kobject_uevent_env(&gi->dev->kobj, KOBJ_CHANGE, disconnected);
+		pr_info("%s: sent uevent %s\n", __func__, disconnected[0]);
+		uevent_sent = true;
+	}
+
+	if (!uevent_sent) {
+		pr_info("%s: did not send uevent (%d %d %p)\n", __func__,
+			gi->connected, gi->sw_connected, cdev->config);
+	}
+}
+#endif
+
 static void purge_configs_funcs(struct gadget_info *gi)
 {
 	struct usb_configuration	*c;
@@ -1386,13 +1442,60 @@ static void configfs_composite_unbind(struct usb_gadget *gadget)
 	set_gadget_data(gadget, NULL);
 }
 
+#ifdef CONFIG_USB_CONFIGFS_UEVENT
+static int configfs_setup(struct usb_gadget *gadget,
+			  const struct usb_ctrlrequest *c)
+{
+	struct usb_composite_dev *cdev = get_gadget_data(gadget);
+	struct gadget_info *gi = container_of(cdev, struct gadget_info, cdev);
+	int value = -EOPNOTSUPP;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cdev->lock, flags);
+	if (!gi->connected) {
+		gi->connected = 1;
+		schedule_work(&gi->work);
+	}
+	spin_unlock_irqrestore(&cdev->lock, flags);
+
+	value = composite_setup(gadget, c);
+
+	spin_lock_irqsave(&cdev->lock, flags);
+	if (c->bRequest == USB_REQ_SET_CONFIGURATION && cdev->config)
+		schedule_work(&gi->work);
+	spin_unlock_irqrestore(&cdev->lock, flags);
+
+	return value;
+}
+
+static void configfs_disconnect(struct usb_gadget *gadget)
+{
+	struct usb_composite_dev *cdev = get_gadget_data(gadget);
+	struct gadget_info *gi = container_of(cdev, struct gadget_info, cdev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&cdev->lock, flags);
+	gi->connected = 0;
+	schedule_work(&gi->work);
+	spin_unlock_irqrestore(&cdev->lock, flags);
+
+	composite_disconnect(gadget);
+}
+#endif
+
 static const struct usb_gadget_driver configfs_driver_template = {
 	.bind           = configfs_composite_bind,
 	.unbind         = configfs_composite_unbind,
 
+#ifdef CONFIG_USB_CONFIGFS_UEVENT
+	.setup          = configfs_setup,
+	.reset          = configfs_disconnect,
+	.disconnect     = configfs_disconnect,
+#else
 	.setup          = composite_setup,
 	.reset          = composite_disconnect,
 	.disconnect     = composite_disconnect,
+#endif
 
 	.suspend	= composite_suspend,
 	.resume		= composite_resume,
@@ -1453,6 +1556,10 @@ static struct config_group *gadgets_make(
 	gi->composite.gadget_driver.function = kstrdup(name, GFP_KERNEL);
 	gi->composite.name = gi->composite.gadget_driver.function;
 
+#ifdef CONFIG_USB_CONFIGFS_UEVENT
+	INIT_WORK(&gi->work, configfs_work);
+#endif
+
 	if (!gi->composite.gadget_driver.function)
 		goto err;
 
-- 
1.7.9.5

Re: [PATCH v2] usb: gadget: Add uevent to notify userspace

[Greg KH]

On Thu, Sep 22, 2016 at 07:43:59PM +0800, Baolin Wang wrote:
> From: Badhri Jagan Sridharan <Badhri@google.com>
> 
> Some USB managament on userspace (like Android system) rely on the uevents
> generated by the composition driver to generate user notifications. Thus this
> patch adds uevents to be generated whenever USB changes its state: connected,
> disconnected, configured.
> 
> The original code was created by Badhri Jagan Sridharan, and I did some
> optimization.
> 
> Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>

You can't just add someone's signed-off-by to a patch, go read what the
legal agreement you just made for someone else at a different company
(hint, you might get a nasty-gram from a google lawyer...)

> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes since v1:
>  - Add Badhri's Signed-off-by.
> ---
>  drivers/usb/gadget/Kconfig    |    8 +++
>  drivers/usb/gadget/configfs.c |  107 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 115 insertions(+)
> 
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 2ea3fc3..9f5d0c6 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -223,6 +223,14 @@ config USB_CONFIGFS
>  	  appropriate symbolic links.
>  	  For more information see Documentation/usb/gadget_configfs.txt.
>  
> +config USB_CONFIGFS_UEVENT
> +	boolean "Uevent notification of Gadget state"
> +	depends on USB_CONFIGFS



> +	help
> +	  Enable uevent notifications to userspace when the gadget
> +	  state changes. The gadget can be in any of the following
> +	  three states: "CONNECTED/DISCONNECTED/CONFIGURED"
> +
>  config USB_CONFIGFS_SERIAL
>  	bool "Generic serial bulk in/out"
>  	depends on USB_CONFIGFS
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 3984787..4c2bc27 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -60,6 +60,11 @@ struct gadget_info {
>  	bool use_os_desc;
>  	char b_vendor_code;
>  	char qw_sign[OS_STRING_QW_SIGN_LEN];
> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
> +	bool connected;
> +	bool sw_connected;
> +	struct work_struct work;
> +#endif
>  };
>  
>  static inline struct gadget_info *to_gadget_info(struct config_item *item)
> @@ -1197,6 +1202,57 @@ int composite_dev_prepare(struct usb_composite_driver *composite,
>  int composite_os_desc_req_prepare(struct usb_composite_dev *cdev,
>  				  struct usb_ep *ep0);
>  
> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
> +static void configfs_work(struct work_struct *data)
> +{
> +	struct gadget_info *gi = container_of(data, struct gadget_info, work);
> +	struct usb_composite_dev *cdev = &gi->cdev;
> +	char *disconnected[2] = { "USB_STATE=DISCONNECTED", NULL };
> +	char *connected[2] = { "USB_STATE=CONNECTED", NULL };
> +	char *configured[2] = { "USB_STATE=CONFIGURED", NULL };
> +	/* 0-connected 1-configured 2-disconnected */
> +	bool status[3] = { false, false, false };
> +	unsigned long flags;
> +	bool uevent_sent = false;
> +
> +	spin_lock_irqsave(&cdev->lock, flags);
> +	if (cdev->config && gi->connected)
> +		status[1] = true;
> +
> +	if (gi->connected != gi->sw_connected) {
> +		if (gi->connected)
> +			status[0] = true;
> +		else
> +			status[2] = true;
> +		gi->sw_connected = gi->connected;
> +	}
> +	spin_unlock_irqrestore(&cdev->lock, flags);
> +
> +	if (status[0]) {
> +		kobject_uevent_env(&gi->dev->kobj, KOBJ_CHANGE, connected);
> +		pr_info("%s: sent uevent %s\n", __func__, connected[0]);

You are kidding, right?

> +		uevent_sent = true;
> +	}
> +
> +	if (status[1]) {
> +		kobject_uevent_env(&gi->dev->kobj, KOBJ_CHANGE, configured);
> +		pr_info("%s: sent uevent %s\n", __func__, configured[0]);

Come on, please...

> +		uevent_sent = true;
> +	}
> +
> +	if (status[2]) {
> +		kobject_uevent_env(&gi->dev->kobj, KOBJ_CHANGE, disconnected);
> +		pr_info("%s: sent uevent %s\n", __func__, disconnected[0]);

This is getting funny...

> +		uevent_sent = true;
> +	}
> +
> +	if (!uevent_sent) {
> +		pr_info("%s: did not send uevent (%d %d %p)\n", __func__,
> +			gi->connected, gi->sw_connected, cdev->config);

Nope, not funny anymore.

> +	}
> +}
> +#endif
> +
>  static void purge_configs_funcs(struct gadget_info *gi)
>  {
>  	struct usb_configuration	*c;
> @@ -1386,13 +1442,60 @@ static void configfs_composite_unbind(struct usb_gadget *gadget)
>  	set_gadget_data(gadget, NULL);
>  }
>  
> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
> +static int configfs_setup(struct usb_gadget *gadget,
> +			  const struct usb_ctrlrequest *c)
> +{
> +	struct usb_composite_dev *cdev = get_gadget_data(gadget);
> +	struct gadget_info *gi = container_of(cdev, struct gadget_info, cdev);
> +	int value = -EOPNOTSUPP;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&cdev->lock, flags);
> +	if (!gi->connected) {
> +		gi->connected = 1;
> +		schedule_work(&gi->work);
> +	}
> +	spin_unlock_irqrestore(&cdev->lock, flags);
> +
> +	value = composite_setup(gadget, c);
> +
> +	spin_lock_irqsave(&cdev->lock, flags);
> +	if (c->bRequest == USB_REQ_SET_CONFIGURATION && cdev->config)
> +		schedule_work(&gi->work);
> +	spin_unlock_irqrestore(&cdev->lock, flags);
> +
> +	return value;
> +}
> +
> +static void configfs_disconnect(struct usb_gadget *gadget)
> +{
> +	struct usb_composite_dev *cdev = get_gadget_data(gadget);
> +	struct gadget_info *gi = container_of(cdev, struct gadget_info, cdev);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&cdev->lock, flags);
> +	gi->connected = 0;
> +	schedule_work(&gi->work);
> +	spin_unlock_irqrestore(&cdev->lock, flags);
> +
> +	composite_disconnect(gadget);
> +}
> +#endif
> +
>  static const struct usb_gadget_driver configfs_driver_template = {
>  	.bind           = configfs_composite_bind,
>  	.unbind         = configfs_composite_unbind,
>  
> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
> +	.setup          = configfs_setup,
> +	.reset          = configfs_disconnect,
> +	.disconnect     = configfs_disconnect,
> +#else
>  	.setup          = composite_setup,
>  	.reset          = composite_disconnect,
>  	.disconnect     = composite_disconnect,
> +#endif
>  
>  	.suspend	= composite_suspend,
>  	.resume		= composite_resume,
> @@ -1453,6 +1556,10 @@ static struct config_group *gadgets_make(
>  	gi->composite.gadget_driver.function = kstrdup(name, GFP_KERNEL);
>  	gi->composite.name = gi->composite.gadget_driver.function;
>  
> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
> +	INIT_WORK(&gi->work, configfs_work);
> +#endif

This is just way too ugly, please make it so there are no #ifdefs in the
.c files.

Or, as others said, why is this a build option at all, why would you not
always want this enabled if you are relying on it all of the time?

thanks,

greg k-h

Re: [PATCH v2] usb: gadget: Add uevent to notify userspace

[Baolin Wang]

On 22 September 2016 at 20:23, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Sep 22, 2016 at 07:43:59PM +0800, Baolin Wang wrote:
>> From: Badhri Jagan Sridharan <Badhri@google.com>
>>
>> Some USB managament on userspace (like Android system) rely on the uevents
>> generated by the composition driver to generate user notifications. Thus this
>> patch adds uevents to be generated whenever USB changes its state: connected,
>> disconnected, configured.
>>
>> The original code was created by Badhri Jagan Sridharan, and I did some
>> optimization.
>>
>> Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
>
> You can't just add someone's signed-off-by to a patch, go read what the
> legal agreement you just made for someone else at a different company
> (hint, you might get a nasty-gram from a google lawyer...)

OK. I will talk with Badhri if I can upstream these.

>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>> Changes since v1:
>>  - Add Badhri's Signed-off-by.
>> ---
>>  drivers/usb/gadget/Kconfig    |    8 +++
>>  drivers/usb/gadget/configfs.c |  107 +++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 115 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>> index 2ea3fc3..9f5d0c6 100644
>> --- a/drivers/usb/gadget/Kconfig
>> +++ b/drivers/usb/gadget/Kconfig
>> @@ -223,6 +223,14 @@ config USB_CONFIGFS
>>         appropriate symbolic links.
>>         For more information see Documentation/usb/gadget_configfs.txt.
>>
>> +config USB_CONFIGFS_UEVENT
>> +     boolean "Uevent notification of Gadget state"
>> +     depends on USB_CONFIGFS
>
>
>
>> +     help
>> +       Enable uevent notifications to userspace when the gadget
>> +       state changes. The gadget can be in any of the following
>> +       three states: "CONNECTED/DISCONNECTED/CONFIGURED"
>> +
>>  config USB_CONFIGFS_SERIAL
>>       bool "Generic serial bulk in/out"
>>       depends on USB_CONFIGFS
>> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>> index 3984787..4c2bc27 100644
>> --- a/drivers/usb/gadget/configfs.c
>> +++ b/drivers/usb/gadget/configfs.c
>> @@ -60,6 +60,11 @@ struct gadget_info {
>>       bool use_os_desc;
>>       char b_vendor_code;
>>       char qw_sign[OS_STRING_QW_SIGN_LEN];
>> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
>> +     bool connected;
>> +     bool sw_connected;
>> +     struct work_struct work;
>> +#endif
>>  };
>>
>>  static inline struct gadget_info *to_gadget_info(struct config_item *item)
>> @@ -1197,6 +1202,57 @@ int composite_dev_prepare(struct usb_composite_driver *composite,
>>  int composite_os_desc_req_prepare(struct usb_composite_dev *cdev,
>>                                 struct usb_ep *ep0);
>>
>> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
>> +static void configfs_work(struct work_struct *data)
>> +{
>> +     struct gadget_info *gi = container_of(data, struct gadget_info, work);
>> +     struct usb_composite_dev *cdev = &gi->cdev;
>> +     char *disconnected[2] = { "USB_STATE=DISCONNECTED", NULL };
>> +     char *connected[2] = { "USB_STATE=CONNECTED", NULL };
>> +     char *configured[2] = { "USB_STATE=CONFIGURED", NULL };
>> +     /* 0-connected 1-configured 2-disconnected */
>> +     bool status[3] = { false, false, false };
>> +     unsigned long flags;
>> +     bool uevent_sent = false;
>> +
>> +     spin_lock_irqsave(&cdev->lock, flags);
>> +     if (cdev->config && gi->connected)
>> +             status[1] = true;
>> +
>> +     if (gi->connected != gi->sw_connected) {
>> +             if (gi->connected)
>> +                     status[0] = true;
>> +             else
>> +                     status[2] = true;
>> +             gi->sw_connected = gi->connected;
>> +     }
>> +     spin_unlock_irqrestore(&cdev->lock, flags);
>> +
>> +     if (status[0]) {
>> +             kobject_uevent_env(&gi->dev->kobj, KOBJ_CHANGE, connected);
>> +             pr_info("%s: sent uevent %s\n", __func__, connected[0]);
>
> You are kidding, right?
>
>> +             uevent_sent = true;
>> +     }
>> +
>> +     if (status[1]) {
>> +             kobject_uevent_env(&gi->dev->kobj, KOBJ_CHANGE, configured);
>> +             pr_info("%s: sent uevent %s\n", __func__, configured[0]);
>
> Come on, please...
>
>> +             uevent_sent = true;
>> +     }
>> +
>> +     if (status[2]) {
>> +             kobject_uevent_env(&gi->dev->kobj, KOBJ_CHANGE, disconnected);
>> +             pr_info("%s: sent uevent %s\n", __func__, disconnected[0]);
>
> This is getting funny...
>
>> +             uevent_sent = true;
>> +     }
>> +
>> +     if (!uevent_sent) {
>> +             pr_info("%s: did not send uevent (%d %d %p)\n", __func__,
>> +                     gi->connected, gi->sw_connected, cdev->config);
>
> Nope, not funny anymore.
>
>> +     }
>> +}
>> +#endif
>> +
>>  static void purge_configs_funcs(struct gadget_info *gi)
>>  {
>>       struct usb_configuration        *c;
>> @@ -1386,13 +1442,60 @@ static void configfs_composite_unbind(struct usb_gadget *gadget)
>>       set_gadget_data(gadget, NULL);
>>  }
>>
>> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
>> +static int configfs_setup(struct usb_gadget *gadget,
>> +                       const struct usb_ctrlrequest *c)
>> +{
>> +     struct usb_composite_dev *cdev = get_gadget_data(gadget);
>> +     struct gadget_info *gi = container_of(cdev, struct gadget_info, cdev);
>> +     int value = -EOPNOTSUPP;
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&cdev->lock, flags);
>> +     if (!gi->connected) {
>> +             gi->connected = 1;
>> +             schedule_work(&gi->work);
>> +     }
>> +     spin_unlock_irqrestore(&cdev->lock, flags);
>> +
>> +     value = composite_setup(gadget, c);
>> +
>> +     spin_lock_irqsave(&cdev->lock, flags);
>> +     if (c->bRequest == USB_REQ_SET_CONFIGURATION && cdev->config)
>> +             schedule_work(&gi->work);
>> +     spin_unlock_irqrestore(&cdev->lock, flags);
>> +
>> +     return value;
>> +}
>> +
>> +static void configfs_disconnect(struct usb_gadget *gadget)
>> +{
>> +     struct usb_composite_dev *cdev = get_gadget_data(gadget);
>> +     struct gadget_info *gi = container_of(cdev, struct gadget_info, cdev);
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&cdev->lock, flags);
>> +     gi->connected = 0;
>> +     schedule_work(&gi->work);
>> +     spin_unlock_irqrestore(&cdev->lock, flags);
>> +
>> +     composite_disconnect(gadget);
>> +}
>> +#endif
>> +
>>  static const struct usb_gadget_driver configfs_driver_template = {
>>       .bind           = configfs_composite_bind,
>>       .unbind         = configfs_composite_unbind,
>>
>> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
>> +     .setup          = configfs_setup,
>> +     .reset          = configfs_disconnect,
>> +     .disconnect     = configfs_disconnect,
>> +#else
>>       .setup          = composite_setup,
>>       .reset          = composite_disconnect,
>>       .disconnect     = composite_disconnect,
>> +#endif
>>
>>       .suspend        = composite_suspend,
>>       .resume         = composite_resume,
>> @@ -1453,6 +1556,10 @@ static struct config_group *gadgets_make(
>>       gi->composite.gadget_driver.function = kstrdup(name, GFP_KERNEL);
>>       gi->composite.name = gi->composite.gadget_driver.function;
>>
>> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
>> +     INIT_WORK(&gi->work, configfs_work);
>> +#endif
>
> This is just way too ugly, please make it so there are no #ifdefs in the
> .c files.
>
> Or, as others said, why is this a build option at all, why would you not
> always want this enabled if you are relying on it all of the time?

Sometimes userspace does not need the notification, it is not all the
time. Anyway I will remove the macro if you still insist on that.

-- 
Baolin.wang
Best Regards

Re: [PATCH v2] usb: gadget: Add uevent to notify userspace

[Felipe Balbi]

Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>>>  static const struct usb_gadget_driver configfs_driver_template = {
>>>       .bind           = configfs_composite_bind,
>>>       .unbind         = configfs_composite_unbind,
>>>
>>> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
>>> +     .setup          = configfs_setup,
>>> +     .reset          = configfs_disconnect,
>>> +     .disconnect     = configfs_disconnect,
>>> +#else
>>>       .setup          = composite_setup,
>>>       .reset          = composite_disconnect,
>>>       .disconnect     = composite_disconnect,
>>> +#endif

nope, this is quite wrong.

>>> @@ -1453,6 +1556,10 @@ static struct config_group *gadgets_make(
>>>       gi->composite.gadget_driver.function = kstrdup(name, GFP_KERNEL);
>>>       gi->composite.name = gi->composite.gadget_driver.function;
>>>
>>> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
>>> +     INIT_WORK(&gi->work, configfs_work);
>>> +#endif
>>
>> This is just way too ugly, please make it so there are no #ifdefs in the
>> .c files.
>>
>> Or, as others said, why is this a build option at all, why would you not
>> always want this enabled if you are relying on it all of the time?
>
> Sometimes userspace does not need the notification, it is not all the
> time. Anyway I will remove the macro if you still insist on that.

what's wrong with the sysfs we already have for this?

-- 
balbi

Re: [PATCH v2] usb: gadget: Add uevent to notify userspace

[Greg KH]

On Thu, Sep 22, 2016 at 08:41:09PM +0800, Baolin Wang wrote:
> On 22 September 2016 at 20:23, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Thu, Sep 22, 2016 at 07:43:59PM +0800, Baolin Wang wrote:
> >> From: Badhri Jagan Sridharan <Badhri@google.com>
> >>
> >> Some USB managament on userspace (like Android system) rely on the uevents
> >> generated by the composition driver to generate user notifications. Thus this
> >> patch adds uevents to be generated whenever USB changes its state: connected,
> >> disconnected, configured.
> >>
> >> The original code was created by Badhri Jagan Sridharan, and I did some
> >> optimization.
> >>
> >> Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
> >
> > You can't just add someone's signed-off-by to a patch, go read what the
> > legal agreement you just made for someone else at a different company
> > (hint, you might get a nasty-gram from a google lawyer...)
> 
> OK. I will talk with Badhri if I can upstream these.

That's not an issue, you can keep the "From:" line on it, if you got it
in a legal way, and then just have your signed off by on it, go read the
DCO for the specifics.  I don't know why someone else told you
otherwise.

If you have _any_ questions about this, go talk to the Linaro lawyers,
they know how to handle this properly.

> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> >> ---
> >> Changes since v1:
> >>  - Add Badhri's Signed-off-by.
> >> ---
> >>  drivers/usb/gadget/Kconfig    |    8 +++
> >>  drivers/usb/gadget/configfs.c |  107 +++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 115 insertions(+)
> >>
> >> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> >> index 2ea3fc3..9f5d0c6 100644
> >> --- a/drivers/usb/gadget/Kconfig
> >> +++ b/drivers/usb/gadget/Kconfig
> >> @@ -223,6 +223,14 @@ config USB_CONFIGFS
> >>         appropriate symbolic links.
> >>         For more information see Documentation/usb/gadget_configfs.txt.
> >>
> >> +config USB_CONFIGFS_UEVENT
> >> +     boolean "Uevent notification of Gadget state"
> >> +     depends on USB_CONFIGFS
> >
> >
> >
> >> +     help
> >> +       Enable uevent notifications to userspace when the gadget
> >> +       state changes. The gadget can be in any of the following
> >> +       three states: "CONNECTED/DISCONNECTED/CONFIGURED"
> >> +
> >>  config USB_CONFIGFS_SERIAL
> >>       bool "Generic serial bulk in/out"
> >>       depends on USB_CONFIGFS
> >> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> >> index 3984787..4c2bc27 100644
> >> --- a/drivers/usb/gadget/configfs.c
> >> +++ b/drivers/usb/gadget/configfs.c
> >> @@ -60,6 +60,11 @@ struct gadget_info {
> >>       bool use_os_desc;
> >>       char b_vendor_code;
> >>       char qw_sign[OS_STRING_QW_SIGN_LEN];
> >> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
> >> +     bool connected;
> >> +     bool sw_connected;
> >> +     struct work_struct work;
> >> +#endif
> >>  };
> >>
> >>  static inline struct gadget_info *to_gadget_info(struct config_item *item)
> >> @@ -1197,6 +1202,57 @@ int composite_dev_prepare(struct usb_composite_driver *composite,
> >>  int composite_os_desc_req_prepare(struct usb_composite_dev *cdev,
> >>                                 struct usb_ep *ep0);
> >>
> >> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
> >> +static void configfs_work(struct work_struct *data)
> >> +{
> >> +     struct gadget_info *gi = container_of(data, struct gadget_info, work);
> >> +     struct usb_composite_dev *cdev = &gi->cdev;
> >> +     char *disconnected[2] = { "USB_STATE=DISCONNECTED", NULL };
> >> +     char *connected[2] = { "USB_STATE=CONNECTED", NULL };
> >> +     char *configured[2] = { "USB_STATE=CONFIGURED", NULL };
> >> +     /* 0-connected 1-configured 2-disconnected */
> >> +     bool status[3] = { false, false, false };
> >> +     unsigned long flags;
> >> +     bool uevent_sent = false;
> >> +
> >> +     spin_lock_irqsave(&cdev->lock, flags);
> >> +     if (cdev->config && gi->connected)
> >> +             status[1] = true;
> >> +
> >> +     if (gi->connected != gi->sw_connected) {
> >> +             if (gi->connected)
> >> +                     status[0] = true;
> >> +             else
> >> +                     status[2] = true;
> >> +             gi->sw_connected = gi->connected;
> >> +     }
> >> +     spin_unlock_irqrestore(&cdev->lock, flags);
> >> +
> >> +     if (status[0]) {
> >> +             kobject_uevent_env(&gi->dev->kobj, KOBJ_CHANGE, connected);
> >> +             pr_info("%s: sent uevent %s\n", __func__, connected[0]);
> >
> > You are kidding, right?
> >
> >> +             uevent_sent = true;
> >> +     }
> >> +
> >> +     if (status[1]) {
> >> +             kobject_uevent_env(&gi->dev->kobj, KOBJ_CHANGE, configured);
> >> +             pr_info("%s: sent uevent %s\n", __func__, configured[0]);
> >
> > Come on, please...
> >
> >> +             uevent_sent = true;
> >> +     }
> >> +
> >> +     if (status[2]) {
> >> +             kobject_uevent_env(&gi->dev->kobj, KOBJ_CHANGE, disconnected);
> >> +             pr_info("%s: sent uevent %s\n", __func__, disconnected[0]);
> >
> > This is getting funny...
> >
> >> +             uevent_sent = true;
> >> +     }
> >> +
> >> +     if (!uevent_sent) {
> >> +             pr_info("%s: did not send uevent (%d %d %p)\n", __func__,
> >> +                     gi->connected, gi->sw_connected, cdev->config);
> >
> > Nope, not funny anymore.
> >
> >> +     }
> >> +}
> >> +#endif
> >> +
> >>  static void purge_configs_funcs(struct gadget_info *gi)
> >>  {
> >>       struct usb_configuration        *c;
> >> @@ -1386,13 +1442,60 @@ static void configfs_composite_unbind(struct usb_gadget *gadget)
> >>       set_gadget_data(gadget, NULL);
> >>  }
> >>
> >> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
> >> +static int configfs_setup(struct usb_gadget *gadget,
> >> +                       const struct usb_ctrlrequest *c)
> >> +{
> >> +     struct usb_composite_dev *cdev = get_gadget_data(gadget);
> >> +     struct gadget_info *gi = container_of(cdev, struct gadget_info, cdev);
> >> +     int value = -EOPNOTSUPP;
> >> +     unsigned long flags;
> >> +
> >> +     spin_lock_irqsave(&cdev->lock, flags);
> >> +     if (!gi->connected) {
> >> +             gi->connected = 1;
> >> +             schedule_work(&gi->work);
> >> +     }
> >> +     spin_unlock_irqrestore(&cdev->lock, flags);
> >> +
> >> +     value = composite_setup(gadget, c);
> >> +
> >> +     spin_lock_irqsave(&cdev->lock, flags);
> >> +     if (c->bRequest == USB_REQ_SET_CONFIGURATION && cdev->config)
> >> +             schedule_work(&gi->work);
> >> +     spin_unlock_irqrestore(&cdev->lock, flags);
> >> +
> >> +     return value;
> >> +}
> >> +
> >> +static void configfs_disconnect(struct usb_gadget *gadget)
> >> +{
> >> +     struct usb_composite_dev *cdev = get_gadget_data(gadget);
> >> +     struct gadget_info *gi = container_of(cdev, struct gadget_info, cdev);
> >> +     unsigned long flags;
> >> +
> >> +     spin_lock_irqsave(&cdev->lock, flags);
> >> +     gi->connected = 0;
> >> +     schedule_work(&gi->work);
> >> +     spin_unlock_irqrestore(&cdev->lock, flags);
> >> +
> >> +     composite_disconnect(gadget);
> >> +}
> >> +#endif
> >> +
> >>  static const struct usb_gadget_driver configfs_driver_template = {
> >>       .bind           = configfs_composite_bind,
> >>       .unbind         = configfs_composite_unbind,
> >>
> >> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
> >> +     .setup          = configfs_setup,
> >> +     .reset          = configfs_disconnect,
> >> +     .disconnect     = configfs_disconnect,
> >> +#else
> >>       .setup          = composite_setup,
> >>       .reset          = composite_disconnect,
> >>       .disconnect     = composite_disconnect,
> >> +#endif
> >>
> >>       .suspend        = composite_suspend,
> >>       .resume         = composite_resume,
> >> @@ -1453,6 +1556,10 @@ static struct config_group *gadgets_make(
> >>       gi->composite.gadget_driver.function = kstrdup(name, GFP_KERNEL);
> >>       gi->composite.name = gi->composite.gadget_driver.function;
> >>
> >> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
> >> +     INIT_WORK(&gi->work, configfs_work);
> >> +#endif
> >
> > This is just way too ugly, please make it so there are no #ifdefs in the
> > .c files.
> >
> > Or, as others said, why is this a build option at all, why would you not
> > always want this enabled if you are relying on it all of the time?
> 
> Sometimes userspace does not need the notification, it is not all the
> time. Anyway I will remove the macro if you still insist on that.

What do you mean "sometimes"?  That "sometime" means you have to build a
new kernel if you do, or do not, want it.

Either userspace relies on this, or it doesn't, it should be pretty
simple.

thanks,

greg k-h

Re: [PATCH v2] usb: gadget: Add uevent to notify userspace

[Mark Brown]

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

On Thu, Sep 22, 2016 at 03:38:30PM +0200, Greg KH wrote:
> On Thu, Sep 22, 2016 at 08:41:09PM +0800, Baolin Wang wrote:

> > OK. I will talk with Badhri if I can upstream these.

> That's not an issue, you can keep the "From:" line on it, if you got it
> in a legal way, and then just have your signed off by on it, go read the
> DCO for the specifics.  I don't know why someone else told you
> otherwise.

Given the weakness people have in understanding the legal part of
signoffs it seems very unwise to just trust that they've actually
got everything clearly lined up and IME when they're missing they've
often just been removed or sometimes there's a very good reason why the
original author didn't provide a signoff.  It seems safer for everyone
to query what's going on and it's been fairly unusual that it's anything
other than a mistake.

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

Re: [PATCH v2] usb: gadget: Add uevent to notify userspace

[Badhri Jagan Sridharan]

Hi all,

I do agree that udc-core's uevents can potentially be used here.
I will check the complete list of uevents sent by the udc-core's
usb_gadget_set_state function and see if the userspace is happy to work with it.
Now that Android is starting to use the configfs driver, it might be
much easier.

Android userspace always relies on uevents to notify the users. This is might
be the right opportunity to dig into the uevents fired by the udc-core code
and shift to it.

I will come back with questions/ findings.

Thanks.

On Thu, Sep 22, 2016 at 9:36 AM, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Sep 22, 2016 at 03:38:30PM +0200, Greg KH wrote:
> > On Thu, Sep 22, 2016 at 08:41:09PM +0800, Baolin Wang wrote:
>
> > > OK. I will talk with Badhri if I can upstream these.
>
> > That's not an issue, you can keep the "From:" line on it, if you got it
> > in a legal way, and then just have your signed off by on it, go read the
> > DCO for the specifics.  I don't know why someone else told you
> > otherwise.
>
> Given the weakness people have in understanding the legal part of
> signoffs it seems very unwise to just trust that they've actually
> got everything clearly lined up and IME when they're missing they've
> often just been removed or sometimes there's a very good reason why the
> original author didn't provide a signoff.  It seems safer for everyone
> to query what's going on and it's been fairly unusual that it's anything
> other than a mistake.

Re: [PATCH v2] usb: gadget: Add uevent to notify userspace

[Baolin Wang]

Hi,

On 22 September 2016 at 20:53, Felipe Balbi <balbi@kernel.org> wrote:
>
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>  static const struct usb_gadget_driver configfs_driver_template = {
>>>>       .bind           = configfs_composite_bind,
>>>>       .unbind         = configfs_composite_unbind,
>>>>
>>>> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
>>>> +     .setup          = configfs_setup,
>>>> +     .reset          = configfs_disconnect,
>>>> +     .disconnect     = configfs_disconnect,
>>>> +#else
>>>>       .setup          = composite_setup,
>>>>       .reset          = composite_disconnect,
>>>>       .disconnect     = composite_disconnect,
>>>> +#endif
>
> nope, this is quite wrong.
>
>>>> @@ -1453,6 +1556,10 @@ static struct config_group *gadgets_make(
>>>>       gi->composite.gadget_driver.function = kstrdup(name, GFP_KERNEL);
>>>>       gi->composite.name = gi->composite.gadget_driver.function;
>>>>
>>>> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
>>>> +     INIT_WORK(&gi->work, configfs_work);
>>>> +#endif
>>>
>>> This is just way too ugly, please make it so there are no #ifdefs in the
>>> .c files.
>>>
>>> Or, as others said, why is this a build option at all, why would you not
>>> always want this enabled if you are relying on it all of the time?
>>
>> Sometimes userspace does not need the notification, it is not all the
>> time. Anyway I will remove the macro if you still insist on that.
>
> what's wrong with the sysfs we already have for this?

If Android system userspace can support udc-core's uevents like Badhri
said, I am fine with that.

-- 
Baolin.wang
Best Regards