| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20160922133830.GA20931@kroah.com>
Date: Thu, 22 Sep 2016 15:38:30 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Baolin Wang <baolin.wang@...aro.org>
Cc: Felipe Balbi <balbi@...nel.org>,
Badhri Jagan Sridharan <Badhri@...gle.com>,
Mark Brown <broonie@...nel.org>,
USB <linux-usb@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] usb: gadget: Add uevent to notify userspace
On Thu, Sep 22, 2016 at 08:41:09PM +0800, Baolin Wang wrote:
> On 22 September 2016 at 20:23, Greg KH <gregkh@...uxfoundation.org> wrote:
> > On Thu, Sep 22, 2016 at 07:43:59PM +0800, Baolin Wang wrote:
> >> From: Badhri Jagan Sridharan <Badhri@...gle.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@...gle.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@...aro.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
Powered by blists - more mailing lists