Re: [PATCH] usb: core: added uevent for over-current
On Mon, Sep 10, 2018 at 03:12:22PM -0700, Jon Flatley wrote:
> On Mon, Sep 10, 2018 at 11:14 AM Greg KH 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
> > > ---
> > > 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
> > > #include
> > > #include
> > > +#include
> > >
> > > #include
> > > #include
> > > @@ -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(_dev->dev.kobj, NULL, "over_current_count");
> > > +
> > > + hub_dev = port_dev->dev.parent;
> > > +
> > > + if (!hub_dev)
> > > + return;
> > > +
> > > + port_dev_path = kobject_get_path(_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(_dev->kobj, KOBJ_CHANGE, envp);
> > > +
> > > + kfree(envp[1]);
> > > +exit:
> > > + kfree(envp[0]);
> > > +}
> > > +
> > > static void port_event(struct usb_hub *hub, int port1)
> > > __must_hold(_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
Re: [PATCH] usb: core: added uevent for over-current
On Mon, Sep 10, 2018 at 11:14 AM Greg KH 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
> > ---
> > 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
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -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(_dev->dev.kobj, NULL, "over_current_count");
> > +
> > + hub_dev = port_dev->dev.parent;
> > +
> > + if (!hub_dev)
> > + return;
> > +
> > + port_dev_path = kobject_get_path(_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(_dev->kobj, KOBJ_CHANGE, envp);
> > +
> > + kfree(envp[1]);
> > +exit:
> > + kfree(envp[0]);
> > +}
> > +
> > static void port_event(struct usb_hub *hub, int port1)
> > __must_hold(_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.
Are you referring to documenting this udev event alongside the sysfs
documentation for this attribute? Is this typically where this type of
documentation goes?
Thanks,
Jon
Re: [PATCH] usb: core: added uevent for over-current
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
> ---
> 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
> #include
> #include
> +#include
>
> #include
> #include
> @@ -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(_dev->dev.kobj, NULL, "over_current_count");
> +
> + hub_dev = port_dev->dev.parent;
> +
> + if (!hub_dev)
> + return;
> +
> + port_dev_path = kobject_get_path(_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(_dev->kobj, KOBJ_CHANGE, envp);
> +
> + kfree(envp[1]);
> +exit:
> + kfree(envp[0]);
> +}
> +
> static void port_event(struct usb_hub *hub, int port1)
> __must_hold(_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
[PATCH] usb: core: added uevent for over-current
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
---
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
#include
#include
+#include
#include
#include
@@ -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(_dev->dev.kobj, NULL, "over_current_count");
+
+ hub_dev = port_dev->dev.parent;
+
+ if (!hub_dev)
+ return;
+
+ port_dev_path = kobject_get_path(_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(_dev->kobj, KOBJ_CHANGE, envp);
+
+ kfree(envp[1]);
+exit:
+ kfree(envp[0]);
+}
+
static void port_event(struct usb_hub *hub, int port1)
__must_hold(_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);
dev_dbg(_dev->dev, "over-current change #%u\n",
port_dev->over_current_count);
--
2.19.0.rc1.350.ge57e33dbd1-goog
