On Mon, Sep 10, 2018 at 03:12:22PM -0700, Jon Flatley wrote:
> On Mon, Sep 10, 2018 at 11:14 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Fri, Aug 31, 2018 at 10:14:19AM -0700, Jon Flatley wrote:
> > > After commit 1cbd53c8cd85 ("usb: core: introduce per-port over-current
> > > counters") usb ports expose a sysfs value 'over_current_count'
> > > to user space. This value on its own is not very useful as it requires
> > > manual polling.
> > >
> > > As a solution, fire a udev event from the usb hub device that specifies
> > > the values 'OVER_CURRENT_PORT' and 'OVER_CURRENT_COUNT' that indicate
> > > the path of the usb port where the over-current event occurred and the
> > > value of 'over_current_count' in sysfs. Additionally, call
> > > sysfs_notify() so the sysfs value supports poll().
> > >
> > > Signed-off-by: Jon Flatley <jflat@xxxxxxxxxxxx>
> > > ---
> > > drivers/usb/core/hub.c | 36 ++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 36 insertions(+)
> > >
> > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > > index 65feedd69323..c986b0fc2daa 100644
> > > --- a/drivers/usb/core/hub.c
> > > +++ b/drivers/usb/core/hub.c
> > > @@ -27,6 +27,7 @@
> > > #include <linux/random.h>
> > > #include <linux/pm_qos.h>
> > > #include <linux/pm_dark_resume.h>
> > > +#include <linux/kobject.h>
> > >
> > > #include <asm/uaccess.h>
> > > #include <asm/byteorder.h>
> > > @@ -5096,6 +5097,40 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
> > > usb_lock_port(port_dev);
> > > }
> > >
> > > +/* Handle notifying userspace about hub over-current events */
> > > +static void port_over_current_notify(struct usb_port *port_dev)
> > > +{
> > > + static char *envp[] = { NULL, NULL, NULL };
> > > + struct device *hub_dev;
> > > + char *port_dev_path;
> > > +
> > > + sysfs_notify(&port_dev->dev.kobj, NULL, "over_current_count");
> > > +
> > > + hub_dev = port_dev->dev.parent;
> > > +
> > > + if (!hub_dev)
> > > + return;
> > > +
> > > + port_dev_path = kobject_get_path(&port_dev->dev.kobj, GFP_KERNEL);
> > > + if (!port_dev_path)
> > > + return;
> > > +
> > > + envp[0] = kasprintf(GFP_KERNEL, "OVER_CURRENT_PORT=%s", port_dev_path);
> > > + if (!envp[0])
> > > + return;
> > > +
> > > + envp[1] = kasprintf(GFP_KERNEL, "OVER_CURRENT_COUNT=%u",
> > > + port_dev->over_current_count);
> > > + if (!envp[1])
> > > + goto exit;
> > > +
> > > + kobject_uevent_env(&hub_dev->kobj, KOBJ_CHANGE, envp);
> > > +
> > > + kfree(envp[1]);
> > > +exit:
> > > + kfree(envp[0]);
> > > +}
> > > +
> > > static void port_event(struct usb_hub *hub, int port1)
> > > __must_hold(&port_dev->status_lock)
> > > {
> > > @@ -5138,6 +5173,7 @@ static void port_event(struct usb_hub *hub, int port1)
> > > if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
> > > u16 status = 0, unused;
> > > port_dev->over_current_count++;
> > > + port_over_current_notify(port_dev);
> >
> > When an overcurrent "event" happens, does it just stay overloaded for
> > long periods of time? Would this flood the system with lots of kobject
> > messages, or is this rate-limited somehow?
> >
> > Also, as this is a new user/kernel abi you are creating, it needs to be
> > documented somewhere. Perhaps in the sysfs file information for this
> > attribute?
> >
> > thanks,
> >
> > greg k-h
>
> The kobject message is only sent when the USB_PORT_STAT_C_OVERCURRENT bit
> is set. This happens when the port enters or leaves in over-current
> condition or when the port is powered off due to an over-current condition
> on another port. This bit is cleared after being processed so typically
> only two messages per over-current event should be expected.
So these do not "flap" really quickly? Just want to make sure we aren't
creating a way for the system to DoS itself :)
> Are you referring to documenting this udev event alongside the sysfs
> documentation for this attribute? Is this typically where this type of
> documentation goes?
Yes, there are other sysfs attributes documented this way, so that is
the best place that I can think of at the time.
Can you respin this patch with that change added?
thanks,
greg k-h
