[PATCH v3] usb/hcd: Send a uevent signaling that the host controller had died

[PATCH v3] usb/hcd: Send a uevent signaling that the host controller had died

[Raul E Rangel]

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 <rrangel@chromium.org>
---
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:	linux-usb@vger.kernel.org
+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:		chromium-os-dev@chromium.org
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

Re: [PATCH v3] usb/hcd: Send a uevent signaling that the host controller had died

[Alan Stern]

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

Re: [PATCH v3] usb/hcd: Send a uevent signaling that the host controller had died

[Raul Rangel]

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
> 

Re: [PATCH v3] usb/hcd: Send a uevent signaling that the host controller had died

[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

Re: [PATCH v3] usb/hcd: Send a uevent signaling that the host controller had died

[Raul Rangel]

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
>

Re: [PATCH v3] usb/hcd: Send a uevent signaling that the host controller had died

[Guenter Roeck]

On Wed, Apr 17, 2019 at 3:11 PM Raul Rangel <rrangel@chromium.org> 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
> >

Re: [PATCH v3] usb/hcd: Send a uevent signaling that the host controller had died

[Raul Rangel]

On Wed, Apr 17, 2019 at 03:23:52PM -0700, Guenter Roeck wrote:
> On Wed, Apr 17, 2019 at 3:11 PM Raul Rangel <rrangel@chromium.org> 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
> > >

Re: [PATCH v3] usb/hcd: Send a uevent signaling that the host controller had died

[Guenter Roeck]

On Wed, Apr 17, 2019 at 3:41 PM Raul Rangel <rrangel@chromium.org> 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 <rrangel@chromium.org> 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
> > > >

Re: [PATCH v3] usb/hcd: Send a uevent signaling that the host controller had died

[Greg Kroah-Hartman]

On Wed, Apr 17, 2019 at 04:20:09PM -0700, Guenter Roeck wrote:
> On Wed, Apr 17, 2019 at 3:41 PM Raul Rangel <rrangel@chromium.org> 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 <rrangel@chromium.org> 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 :(

Re: [PATCH v3] usb/hcd: Send a uevent signaling that the host controller had died

[Alan Stern]

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 <rrangel@chromium.org> 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 <rrangel@chromium.org> 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

Re: [PATCH v3] usb/hcd: Send a uevent signaling that the host controller had died

[Greg Kroah-Hartman]

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 <rrangel@chromium.org> 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 <rrangel@chromium.org> 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

Re: [PATCH v3] usb/hcd: Send a uevent signaling that the host controller had died

[Raul Rangel]

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 <rrangel@chromium.org> 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 <rrangel@chromium.org> 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

Re: [PATCH v3] usb/hcd: Send a uevent signaling that the host controller had died

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2851 bytes --]

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 <lkp@intel.com>


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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29985 bytes --]

Re: [PATCH v3] usb/hcd: Send a uevent signaling that the host controller had died

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2824 bytes --]

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 <lkp@intel.com>


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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30325 bytes --]

Re: [PATCH v3] usb/hcd: Send a uevent signaling that the host controller had died

[Greg Kroah-Hartman]

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 <rrangel@chromium.org>

I'll wait for a v4 to fix up the bugs 0-day found in this before
reviewing :)

thanks,

greg k-h