This change will send an OFFLINE event to udev with the ERROR=DEAD
environment variable set when the HC dies.
By notifying user space the appropriate policies can be applied.
i.e.,
* Collect error logs.
* Notify the user that USB is no longer functional.
* Perform a graceful reboot.
Signed-off-by: Raul E Rangel <[email protected]>
---
I wasn't able to find any good examples of other drivers sending a dead
notification.
Use an EVENT= format
https://github.com/torvalds/linux/blob/master/drivers/acpi/dock.c#L302
https://github.com/torvalds/linux/blob/master/drivers/net/wireless/ath/wil6210/interrupt.c#L497
Uses SDEV_MEDIA_CHANGE=
https://github.com/torvalds/linux/blob/master/drivers/scsi/scsi_lib.c#L2318
Uses ERROR=1.
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/7f6d8aec5803aac44192f03dce5637b66cda7abf/drivers/input/touchscreen/atmel_mxt_ts.c#1581
I'm not a fan because it doesn't signal what the error was.
Changes in v3:
- Added documentation
- Removed use of lock and null check
- Changed event to OFFLINE + ERROR=DEAD
Changes in v2:
- Check that the root hub still exists before sending the uevent.
- Ensure died_work has completed before deallocating.
Documentation/ABI/testing/usb-uevent | 27 +++++++++++++++++++++++++++
drivers/usb/core/hcd.c | 25 +++++++++++++++++++++++++
include/linux/usb/hcd.h | 1 +
3 files changed, 53 insertions(+)
create mode 100644 Documentation/ABI/testing/usb-uevent
diff --git a/Documentation/ABI/testing/usb-uevent b/Documentation/ABI/testing/usb-uevent
new file mode 100644
index 000000000000..d35c3cad892c
--- /dev/null
+++ b/Documentation/ABI/testing/usb-uevent
@@ -0,0 +1,27 @@
+What: Raise a uevent when a USB Host Controller has died
+Date: 2019-04-17
+KernelVersion: 5.2
+Contact: [email protected]
+Description: When the USB Host Controller has entered a state where it is no
+ longer functional a uevent will be raised. The uevent will
+ contain ACTION=offline and ERROR=DEAD.
+
+ Here is an example taken using udevadm monitor -p:
+
+ KERNEL[130.428945] offline /devices/pci0000:00/0000:00:10.0/usb2 (usb)
+ ACTION=offline
+ BUSNUM=002
+ DEVNAME=/dev/bus/usb/002/001
+ DEVNUM=001
+ DEVPATH=/devices/pci0000:00/0000:00:10.0/usb2
+ DEVTYPE=usb_device
+ DRIVER=usb
+ ERROR=DEAD
+ MAJOR=189
+ MINOR=128
+ PRODUCT=1d6b/2/414
+ SEQNUM=2168
+ SUBSYSTEM=usb
+ TYPE=9/0/1
+
+Users: [email protected]
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 975d7c1288e3..145b058705fe 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2343,6 +2343,20 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
return status;
}
+
+/* Workqueue routine for when the root-hub has died. */
+static void hcd_died_work(struct work_struct *work)
+{
+ struct usb_hcd *hcd = container_of(work, struct usb_hcd, died_work);
+ char *env[] = {
+ "ERROR=DEAD",
+ NULL
+ };
+
+ /* Notify user space that the host controller has died */
+ kobject_uevent_env(&hcd->self.root_hub->dev.kobj, KOBJ_OFFLINE, env);
+}
+
/* Workqueue routine for root-hub remote wakeup */
static void hcd_resume_work(struct work_struct *work)
{
@@ -2488,6 +2502,13 @@ void usb_hc_died (struct usb_hcd *hcd)
usb_kick_hub_wq(hcd->self.root_hub);
}
}
+
+ /* Handle the case where this function gets called with a shared HCD */
+ if (usb_hcd_is_primary_hcd(hcd))
+ schedule_work(&hcd->died_work);
+ else
+ schedule_work(&hcd->primary_hcd->died_work);
+
spin_unlock_irqrestore (&hcd_root_hub_lock, flags);
/* Make sure that the other roothub is also deallocated. */
}
@@ -2555,6 +2576,8 @@ struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
INIT_WORK(&hcd->wakeup_work, hcd_resume_work);
#endif
+ INIT_WORK(&hcd->died_work, hcd_died_work);
+
hcd->driver = driver;
hcd->speed = driver->flags & HCD_MASK;
hcd->product_desc = (driver->product_desc) ? driver->product_desc :
@@ -2908,6 +2931,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
#ifdef CONFIG_PM
cancel_work_sync(&hcd->wakeup_work);
#endif
+ cancel_work_sync(&hcd->died_work);
mutex_lock(&usb_bus_idr_lock);
usb_disconnect(&rhdev); /* Sets rhdev to NULL */
mutex_unlock(&usb_bus_idr_lock);
@@ -2968,6 +2992,7 @@ void usb_remove_hcd(struct usb_hcd *hcd)
#ifdef CONFIG_PM
cancel_work_sync(&hcd->wakeup_work);
#endif
+ cancel_work_sync(&hcd->died_work);
mutex_lock(&usb_bus_idr_lock);
usb_disconnect(&rhdev); /* Sets rhdev to NULL */
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 695931b03684..66a24b13e2ab 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -98,6 +98,7 @@ struct usb_hcd {
#ifdef CONFIG_PM
struct work_struct wakeup_work; /* for remote wakeup */
#endif
+ struct work_struct died_work; /* for when the device dies */
/*
* hardware info/state
--
2.21.0.392.gf8f6787159e-goog
On Wed, 17 Apr 2019, Raul E Rangel wrote:
> +/* Workqueue routine for when the root-hub has died. */
> +static void hcd_died_work(struct work_struct *work)
> +{
> + struct usb_hcd *hcd = container_of(work, struct usb_hcd, died_work);
> + char *env[] = {
> + "ERROR=DEAD",
> + NULL
> + };
This can now be
static const char *env[] = ...
right? There's no need for the array to be reinitialized every time
the routine runs.
Alan Stern
On Wed, Apr 17, 2019 at 03:14:14PM -0400, Alan Stern wrote:
> On Wed, 17 Apr 2019, Raul E Rangel wrote:
>
> > +/* Workqueue routine for when the root-hub has died. */
> > +static void hcd_died_work(struct work_struct *work)
> > +{
> > + struct usb_hcd *hcd = container_of(work, struct usb_hcd, died_work);
> > + char *env[] = {
> > + "ERROR=DEAD",
> > + NULL
> > + };
>
> This can now be
>
> static const char *env[] = ...
>
> right? There's no need for the array to be reinitialized every time
> the routine runs.
I originally tried to make it const, but kobject_uevent_env doesn't
declare the parameter as const, so the compiler yelled at me. I could
make it static, but a static without a const makes me wary. I can add it
if you think it's fine.
>
> Alan Stern
>
On Wed, 17 Apr 2019, Raul Rangel wrote:
> On Wed, Apr 17, 2019 at 03:14:14PM -0400, Alan Stern wrote:
> > On Wed, 17 Apr 2019, Raul E Rangel wrote:
> >
> > > +/* Workqueue routine for when the root-hub has died. */
> > > +static void hcd_died_work(struct work_struct *work)
> > > +{
> > > + struct usb_hcd *hcd = container_of(work, struct usb_hcd, died_work);
> > > + char *env[] = {
> > > + "ERROR=DEAD",
> > > + NULL
> > > + };
> >
> > This can now be
> >
> > static const char *env[] = ...
> >
> > right? There's no need for the array to be reinitialized every time
> > the routine runs.
> I originally tried to make it const, but kobject_uevent_env doesn't
> declare the parameter as const, so the compiler yelled at me. I could
> make it static, but a static without a const makes me wary. I can add it
> if you think it's fine.
This sounds like a golden opportunity! Submit a separate patch making
the parameter to kobject_uevent_env be const (actually const char *
const []), then submit this patch on top of that one.
Alan Stern
On Wed, Apr 17, 2019 at 04:39:23PM -0400, Alan Stern wrote:
>
> This sounds like a golden opportunity! Submit a separate patch making
> the parameter to kobject_uevent_env be const (actually const char *
> const []), then submit this patch on top of that one.
So there are other parts of the code base that dynamically create their
array values. So by making the function take const, it breaks :(
>
> Alan Stern
>
On Wed, Apr 17, 2019 at 3:11 PM Raul Rangel <[email protected]> wrote:
>
> On Wed, Apr 17, 2019 at 04:39:23PM -0400, Alan Stern wrote:
> >
> > This sounds like a golden opportunity! Submit a separate patch making
> > the parameter to kobject_uevent_env be const (actually const char *
> > const []), then submit this patch on top of that one.
> So there are other parts of the code base that dynamically create their
> array values. So by making the function take const, it breaks :(
Confused. The calling code can still be non-const. I don't see the
parameter modified in kobject_uevent_env(), so declaring it const
should be possible. Can you give an example of code that no longer
works ?
Thanks,
Guenter
> >
> > Alan Stern
> >
On Wed, Apr 17, 2019 at 03:23:52PM -0700, Guenter Roeck wrote:
> On Wed, Apr 17, 2019 at 3:11 PM Raul Rangel <[email protected]> wrote:
> >
> > On Wed, Apr 17, 2019 at 04:39:23PM -0400, Alan Stern wrote:
> > >
> > > This sounds like a golden opportunity! Submit a separate patch making
> > > the parameter to kobject_uevent_env be const (actually const char *
> > > const []), then submit this patch on top of that one.
> > So there are other parts of the code base that dynamically create their
> > array values. So by making the function take const, it breaks :(
>
> Confused. The calling code can still be non-const. I don't see the
> parameter modified in kobject_uevent_env(), so declaring it const
> should be possible. Can you give an example of code that no longer
> works ?
static int notify_user_space(struct thermal_zone_device *tz, int trip)
{
char *thermal_prop[5];
int i;
mutex_lock(&tz->lock);
thermal_prop[0] = kasprintf(GFP_KERNEL, "NAME=%s", tz->type);
thermal_prop[1] = kasprintf(GFP_KERNEL, "TEMP=%d", tz->temperature);
thermal_prop[2] = kasprintf(GFP_KERNEL, "TRIP=%d", trip);
thermal_prop[3] = kasprintf(GFP_KERNEL, "EVENT=%d", tz->notify_event);
thermal_prop[4] = NULL;
kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
for (i = 0; i < 4; ++i)
kfree(thermal_prop[i]);
mutex_unlock(&tz->lock);
return 0;
}
drivers/thermal/user_space.c:48:52: error: passing 'char *[5]' to parameter of type 'const char *const *' discards qualifiers in nested pointer types [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
^~~~~~~~~~~~
include/linux/kobject.h:238:22: note: passing argument to parameter 'envp' here
const char *const envp[]);
^
http://c-faq.com/ansi/constmismatch.html explains why it fails.
Raul
>
> Thanks,
> Guenter
>
> > >
> > > Alan Stern
> > >
On Wed, Apr 17, 2019 at 3:41 PM Raul Rangel <[email protected]> wrote:
>
> On Wed, Apr 17, 2019 at 03:23:52PM -0700, Guenter Roeck wrote:
> > On Wed, Apr 17, 2019 at 3:11 PM Raul Rangel <[email protected]> wrote:
> > >
> > > On Wed, Apr 17, 2019 at 04:39:23PM -0400, Alan Stern wrote:
> > > >
> > > > This sounds like a golden opportunity! Submit a separate patch making
> > > > the parameter to kobject_uevent_env be const (actually const char *
> > > > const []), then submit this patch on top of that one.
> > > So there are other parts of the code base that dynamically create their
> > > array values. So by making the function take const, it breaks :(
> >
> > Confused. The calling code can still be non-const. I don't see the
> > parameter modified in kobject_uevent_env(), so declaring it const
> > should be possible. Can you give an example of code that no longer
> > works ?
> static int notify_user_space(struct thermal_zone_device *tz, int trip)
> {
> char *thermal_prop[5];
> int i;
>
> mutex_lock(&tz->lock);
> thermal_prop[0] = kasprintf(GFP_KERNEL, "NAME=%s", tz->type);
> thermal_prop[1] = kasprintf(GFP_KERNEL, "TEMP=%d", tz->temperature);
> thermal_prop[2] = kasprintf(GFP_KERNEL, "TRIP=%d", trip);
> thermal_prop[3] = kasprintf(GFP_KERNEL, "EVENT=%d", tz->notify_event);
> thermal_prop[4] = NULL;
> kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
> for (i = 0; i < 4; ++i)
> kfree(thermal_prop[i]);
> mutex_unlock(&tz->lock);
> return 0;
> }
>
> drivers/thermal/user_space.c:48:52: error: passing 'char *[5]' to parameter of type 'const char *const *' discards qualifiers in nested pointer types [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
> kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
> ^~~~~~~~~~~~
> include/linux/kobject.h:238:22: note: passing argument to parameter 'envp' here
> const char *const envp[]);
> ^
>
> http://c-faq.com/ansi/constmismatch.html explains why it fails.
>
Interesting. One never stops learning. So the best you could do would
be char * const envp[], but I guess that doesn't help much.
Guenter
> Raul
>
> >
> > Thanks,
> > Guenter
> >
> > > >
> > > > Alan Stern
> > > >
On Wed, Apr 17, 2019 at 04:20:09PM -0700, Guenter Roeck wrote:
> On Wed, Apr 17, 2019 at 3:41 PM Raul Rangel <[email protected]> wrote:
> >
> > On Wed, Apr 17, 2019 at 03:23:52PM -0700, Guenter Roeck wrote:
> > > On Wed, Apr 17, 2019 at 3:11 PM Raul Rangel <[email protected]> wrote:
> > > >
> > > > On Wed, Apr 17, 2019 at 04:39:23PM -0400, Alan Stern wrote:
> > > > >
> > > > > This sounds like a golden opportunity! Submit a separate patch making
> > > > > the parameter to kobject_uevent_env be const (actually const char *
> > > > > const []), then submit this patch on top of that one.
> > > > So there are other parts of the code base that dynamically create their
> > > > array values. So by making the function take const, it breaks :(
> > >
> > > Confused. The calling code can still be non-const. I don't see the
> > > parameter modified in kobject_uevent_env(), so declaring it const
> > > should be possible. Can you give an example of code that no longer
> > > works ?
> > static int notify_user_space(struct thermal_zone_device *tz, int trip)
> > {
> > char *thermal_prop[5];
> > int i;
> >
> > mutex_lock(&tz->lock);
> > thermal_prop[0] = kasprintf(GFP_KERNEL, "NAME=%s", tz->type);
> > thermal_prop[1] = kasprintf(GFP_KERNEL, "TEMP=%d", tz->temperature);
> > thermal_prop[2] = kasprintf(GFP_KERNEL, "TRIP=%d", trip);
> > thermal_prop[3] = kasprintf(GFP_KERNEL, "EVENT=%d", tz->notify_event);
> > thermal_prop[4] = NULL;
> > kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
> > for (i = 0; i < 4; ++i)
> > kfree(thermal_prop[i]);
> > mutex_unlock(&tz->lock);
> > return 0;
> > }
> >
> > drivers/thermal/user_space.c:48:52: error: passing 'char *[5]' to parameter of type 'const char *const *' discards qualifiers in nested pointer types [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
> > kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
> > ^~~~~~~~~~~~
> > include/linux/kobject.h:238:22: note: passing argument to parameter 'envp' here
> > const char *const envp[]);
> > ^
> >
> > http://c-faq.com/ansi/constmismatch.html explains why it fails.
> >
> Interesting. One never stops learning. So the best you could do would
> be char * const envp[], but I guess that doesn't help much.
Yeah, I went down this path a year or so ago and had to give it up as
well :(
On Thu, 18 Apr 2019, Greg Kroah-Hartman wrote:
> On Wed, Apr 17, 2019 at 04:20:09PM -0700, Guenter Roeck wrote:
> > On Wed, Apr 17, 2019 at 3:41 PM Raul Rangel <[email protected]> wrote:
> > >
> > > On Wed, Apr 17, 2019 at 03:23:52PM -0700, Guenter Roeck wrote:
> > > > On Wed, Apr 17, 2019 at 3:11 PM Raul Rangel <[email protected]> wrote:
> > > > >
> > > > > On Wed, Apr 17, 2019 at 04:39:23PM -0400, Alan Stern wrote:
> > > > > >
> > > > > > This sounds like a golden opportunity! Submit a separate patch making
> > > > > > the parameter to kobject_uevent_env be const (actually const char *
> > > > > > const []), then submit this patch on top of that one.
> > > > > So there are other parts of the code base that dynamically create their
> > > > > array values. So by making the function take const, it breaks :(
> > > >
> > > > Confused. The calling code can still be non-const. I don't see the
> > > > parameter modified in kobject_uevent_env(), so declaring it const
> > > > should be possible. Can you give an example of code that no longer
> > > > works ?
> > > static int notify_user_space(struct thermal_zone_device *tz, int trip)
> > > {
> > > char *thermal_prop[5];
> > > int i;
> > >
> > > mutex_lock(&tz->lock);
> > > thermal_prop[0] = kasprintf(GFP_KERNEL, "NAME=%s", tz->type);
> > > thermal_prop[1] = kasprintf(GFP_KERNEL, "TEMP=%d", tz->temperature);
> > > thermal_prop[2] = kasprintf(GFP_KERNEL, "TRIP=%d", trip);
> > > thermal_prop[3] = kasprintf(GFP_KERNEL, "EVENT=%d", tz->notify_event);
> > > thermal_prop[4] = NULL;
> > > kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
> > > for (i = 0; i < 4; ++i)
> > > kfree(thermal_prop[i]);
> > > mutex_unlock(&tz->lock);
> > > return 0;
> > > }
> > >
> > > drivers/thermal/user_space.c:48:52: error: passing 'char *[5]' to parameter of type 'const char *const *' discards qualifiers in nested pointer types [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
> > > kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
> > > ^~~~~~~~~~~~
> > > include/linux/kobject.h:238:22: note: passing argument to parameter 'envp' here
> > > const char *const envp[]);
> > > ^
> > >
> > > http://c-faq.com/ansi/constmismatch.html explains why it fails.
> > >
> > Interesting. One never stops learning. So the best you could do would
> > be char * const envp[], but I guess that doesn't help much.
>
> Yeah, I went down this path a year or so ago and had to give it up as
> well :(
Well, the signature could still be changed as Guenter suggests.
And the array being added in the new code could still be static.
After all, there isn't really any danger that the contents of those
strings will be modified, right? It's just that the const modifiers
weren't put in until it was too late and there were too many existing
callers. Perhaps a comment about this could be included in the
kerneldoc for kobject_uevent_env.
Alan Stern
On Thu, Apr 18, 2019 at 10:21:32AM -0400, Alan Stern wrote:
> On Thu, 18 Apr 2019, Greg Kroah-Hartman wrote:
>
> > On Wed, Apr 17, 2019 at 04:20:09PM -0700, Guenter Roeck wrote:
> > > On Wed, Apr 17, 2019 at 3:41 PM Raul Rangel <[email protected]> wrote:
> > > >
> > > > On Wed, Apr 17, 2019 at 03:23:52PM -0700, Guenter Roeck wrote:
> > > > > On Wed, Apr 17, 2019 at 3:11 PM Raul Rangel <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, Apr 17, 2019 at 04:39:23PM -0400, Alan Stern wrote:
> > > > > > >
> > > > > > > This sounds like a golden opportunity! Submit a separate patch making
> > > > > > > the parameter to kobject_uevent_env be const (actually const char *
> > > > > > > const []), then submit this patch on top of that one.
> > > > > > So there are other parts of the code base that dynamically create their
> > > > > > array values. So by making the function take const, it breaks :(
> > > > >
> > > > > Confused. The calling code can still be non-const. I don't see the
> > > > > parameter modified in kobject_uevent_env(), so declaring it const
> > > > > should be possible. Can you give an example of code that no longer
> > > > > works ?
> > > > static int notify_user_space(struct thermal_zone_device *tz, int trip)
> > > > {
> > > > char *thermal_prop[5];
> > > > int i;
> > > >
> > > > mutex_lock(&tz->lock);
> > > > thermal_prop[0] = kasprintf(GFP_KERNEL, "NAME=%s", tz->type);
> > > > thermal_prop[1] = kasprintf(GFP_KERNEL, "TEMP=%d", tz->temperature);
> > > > thermal_prop[2] = kasprintf(GFP_KERNEL, "TRIP=%d", trip);
> > > > thermal_prop[3] = kasprintf(GFP_KERNEL, "EVENT=%d", tz->notify_event);
> > > > thermal_prop[4] = NULL;
> > > > kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
> > > > for (i = 0; i < 4; ++i)
> > > > kfree(thermal_prop[i]);
> > > > mutex_unlock(&tz->lock);
> > > > return 0;
> > > > }
> > > >
> > > > drivers/thermal/user_space.c:48:52: error: passing 'char *[5]' to parameter of type 'const char *const *' discards qualifiers in nested pointer types [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
> > > > kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
> > > > ^~~~~~~~~~~~
> > > > include/linux/kobject.h:238:22: note: passing argument to parameter 'envp' here
> > > > const char *const envp[]);
> > > > ^
> > > >
> > > > http://c-faq.com/ansi/constmismatch.html explains why it fails.
> > > >
> > > Interesting. One never stops learning. So the best you could do would
> > > be char * const envp[], but I guess that doesn't help much.
> >
> > Yeah, I went down this path a year or so ago and had to give it up as
> > well :(
>
> Well, the signature could still be changed as Guenter suggests.
>
> And the array being added in the new code could still be static.
> After all, there isn't really any danger that the contents of those
> strings will be modified, right? It's just that the const modifiers
> weren't put in until it was too late and there were too many existing
> callers. Perhaps a comment about this could be included in the
> kerneldoc for kobject_uevent_env.
I am all for changing this, but I remember I tried to, and somehow
failed, but I don't remember the full details sorry, it was a while ago.
If someone figures out how to make this all const, I will gladly take
that patch.
thanks,
greg k-h
On Thu, Apr 18, 2019 at 04:30:48PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Apr 18, 2019 at 10:21:32AM -0400, Alan Stern wrote:
> > On Thu, 18 Apr 2019, Greg Kroah-Hartman wrote:
> >
> > > On Wed, Apr 17, 2019 at 04:20:09PM -0700, Guenter Roeck wrote:
> > > > On Wed, Apr 17, 2019 at 3:41 PM Raul Rangel <[email protected]> wrote:
> > > > >
> > > > > On Wed, Apr 17, 2019 at 03:23:52PM -0700, Guenter Roeck wrote:
> > > > > > On Wed, Apr 17, 2019 at 3:11 PM Raul Rangel <[email protected]> wrote:
> > > > > > >
> > > > > > > On Wed, Apr 17, 2019 at 04:39:23PM -0400, Alan Stern wrote:
> > > > > > > >
> > > > > > > > This sounds like a golden opportunity! Submit a separate patch making
> > > > > > > > the parameter to kobject_uevent_env be const (actually const char *
> > > > > > > > const []), then submit this patch on top of that one.
> > > > > > > So there are other parts of the code base that dynamically create their
> > > > > > > array values. So by making the function take const, it breaks :(
> > > > > >
> > > > > > Confused. The calling code can still be non-const. I don't see the
> > > > > > parameter modified in kobject_uevent_env(), so declaring it const
> > > > > > should be possible. Can you give an example of code that no longer
> > > > > > works ?
> > > > > static int notify_user_space(struct thermal_zone_device *tz, int trip)
> > > > > {
> > > > > char *thermal_prop[5];
> > > > > int i;
> > > > >
> > > > > mutex_lock(&tz->lock);
> > > > > thermal_prop[0] = kasprintf(GFP_KERNEL, "NAME=%s", tz->type);
> > > > > thermal_prop[1] = kasprintf(GFP_KERNEL, "TEMP=%d", tz->temperature);
> > > > > thermal_prop[2] = kasprintf(GFP_KERNEL, "TRIP=%d", trip);
> > > > > thermal_prop[3] = kasprintf(GFP_KERNEL, "EVENT=%d", tz->notify_event);
> > > > > thermal_prop[4] = NULL;
> > > > > kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
> > > > > for (i = 0; i < 4; ++i)
> > > > > kfree(thermal_prop[i]);
> > > > > mutex_unlock(&tz->lock);
> > > > > return 0;
> > > > > }
> > > > >
> > > > > drivers/thermal/user_space.c:48:52: error: passing 'char *[5]' to parameter of type 'const char *const *' discards qualifiers in nested pointer types [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
> > > > > kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
> > > > > ^~~~~~~~~~~~
> > > > > include/linux/kobject.h:238:22: note: passing argument to parameter 'envp' here
> > > > > const char *const envp[]);
> > > > > ^
> > > > >
> > > > > http://c-faq.com/ansi/constmismatch.html explains why it fails.
> > > > >
> > > > Interesting. One never stops learning. So the best you could do would
> > > > be char * const envp[], but I guess that doesn't help much.
> > >
> > > Yeah, I went down this path a year or so ago and had to give it up as
> > > well :(
> >
> > Well, the signature could still be changed as Guenter suggests.
> >
> > And the array being added in the new code could still be static.
> > After all, there isn't really any danger that the contents of those
> > strings will be modified, right? It's just that the const modifiers
> > weren't put in until it was too late and there were too many existing
> > callers. Perhaps a comment about this could be included in the
> > kerneldoc for kobject_uevent_env.
>
> I am all for changing this, but I remember I tried to, and somehow
> failed, but I don't remember the full details sorry, it was a while ago.
> If someone figures out how to make this all const, I will gladly take
> that patch.
>
Well we could use varargs...
int kobject_uevent_env(struct kobject *kobj,
enum kobject_action action,
...);
This will accept both const char* and char *.
Example https://repl.it/@RaulRangel/Const-char-var-args
It seems like most callers have a fixed number of env params, so you
wouldn't need a function that takes a list.
> thanks,
>
> greg k-h
Hi Raul,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v5.1-rc5 next-20190418]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Raul-E-Rangel/usb-hcd-Send-a-uevent-signaling-that-the-host-controller-had-died/20190419-033556
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: i386-randconfig-x014-201915 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All error/warnings (new ones prefixed by >>):
In file included from include/linux/srcu.h:21:0,
from include/linux/notifier.h:16,
from include/linux/memory_hotplug.h:7,
from include/linux/mmzone.h:749,
from include/linux/gfp.h:6,
from include/linux/umh.h:4,
from include/linux/kmod.h:22,
from include/linux/module.h:13,
from drivers/usb/core/hcd.c:13:
drivers/usb/core/hcd.c: In function '__usb_create_hcd':
>> drivers/usb/core/hcd.c:2566:29: error: 'hcd_died_work' undeclared (first use in this function); did you mean 'schedule_work'?
INIT_WORK(&hcd->died_work, hcd_died_work);
^
include/linux/workqueue.h:245:20: note: in definition of macro '__INIT_WORK'
(_work)->func = (_func); \
^~~~~
>> drivers/usb/core/hcd.c:2566:2: note: in expansion of macro 'INIT_WORK'
INIT_WORK(&hcd->died_work, hcd_died_work);
^~~~~~~~~
drivers/usb/core/hcd.c:2566:29: note: each undeclared identifier is reported only once for each function it appears in
INIT_WORK(&hcd->died_work, hcd_died_work);
^
include/linux/workqueue.h:245:20: note: in definition of macro '__INIT_WORK'
(_work)->func = (_func); \
^~~~~
>> drivers/usb/core/hcd.c:2566:2: note: in expansion of macro 'INIT_WORK'
INIT_WORK(&hcd->died_work, hcd_died_work);
^~~~~~~~~
vim +2566 drivers/usb/core/hcd.c
2565
> 2566 INIT_WORK(&hcd->died_work, hcd_died_work);
2567
2568 hcd->driver = driver;
2569 hcd->speed = driver->flags & HCD_MASK;
2570 hcd->product_desc = (driver->product_desc) ? driver->product_desc :
2571 "USB Host Controller";
2572 return hcd;
2573 }
2574 EXPORT_SYMBOL_GPL(__usb_create_hcd);
2575
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Raul,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v5.1-rc5 next-20190418]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Raul-E-Rangel/usb-hcd-Send-a-uevent-signaling-that-the-host-controller-had-died/20190419-033556
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-randconfig-l3-04180125 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
In file included from include/linux/srcu.h:21:0,
from include/linux/notifier.h:16,
from include/linux/memory_hotplug.h:7,
from include/linux/mmzone.h:749,
from include/linux/gfp.h:6,
from include/linux/umh.h:4,
from include/linux/kmod.h:22,
from include/linux/module.h:13,
from drivers/usb//core/hcd.c:13:
drivers/usb//core/hcd.c: In function '__usb_create_hcd':
>> drivers/usb//core/hcd.c:2566:29: error: 'hcd_died_work' undeclared (first use in this function)
INIT_WORK(&hcd->died_work, hcd_died_work);
^
include/linux/workqueue.h:237:20: note: in definition of macro '__INIT_WORK'
(_work)->func = (_func); \
^
drivers/usb//core/hcd.c:2566:2: note: in expansion of macro 'INIT_WORK'
INIT_WORK(&hcd->died_work, hcd_died_work);
^
drivers/usb//core/hcd.c:2566:29: note: each undeclared identifier is reported only once for each function it appears in
INIT_WORK(&hcd->died_work, hcd_died_work);
^
include/linux/workqueue.h:237:20: note: in definition of macro '__INIT_WORK'
(_work)->func = (_func); \
^
drivers/usb//core/hcd.c:2566:2: note: in expansion of macro 'INIT_WORK'
INIT_WORK(&hcd->died_work, hcd_died_work);
^
vim +/hcd_died_work +2566 drivers/usb//core/hcd.c
2565
> 2566 INIT_WORK(&hcd->died_work, hcd_died_work);
2567
2568 hcd->driver = driver;
2569 hcd->speed = driver->flags & HCD_MASK;
2570 hcd->product_desc = (driver->product_desc) ? driver->product_desc :
2571 "USB Host Controller";
2572 return hcd;
2573 }
2574 EXPORT_SYMBOL_GPL(__usb_create_hcd);
2575
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed, Apr 17, 2019 at 01:03:16PM -0600, Raul E Rangel wrote:
> This change will send an OFFLINE event to udev with the ERROR=DEAD
> environment variable set when the HC dies.
>
> By notifying user space the appropriate policies can be applied.
> i.e.,
> * Collect error logs.
> * Notify the user that USB is no longer functional.
> * Perform a graceful reboot.
>
> Signed-off-by: Raul E Rangel <[email protected]>
I'll wait for a v4 to fix up the bugs 0-day found in this before
reviewing :)
thanks,
greg k-h