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

[Greg KH <gregkh@linuxfoundation.org>]

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