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

[Baolin Wang]

On 22 September 2016 at 20:23, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Sep 22, 2016 at 07:43:59PM +0800, Baolin Wang wrote:
>> From: Badhri Jagan Sridharan <Badhri@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
>> ---
>> 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