Re: "PM / wakeup: Show wakeup sources stats in sysfs" causes boot warnings

[Stephen Boyd <swboyd@chromium.org>]

Quoting Qian Cai (2019-08-13 14:32:56)
> The linux-next commit "PM / wakeup: Show wakeup sources stats in sysfs" [1]
> introduced some baddies during boot on several x86 servers. Reverted the commit
> fixed the issue.
> 
> [1] https://lore.kernel.org/lkml/20190807014846.143949-4-trong@android.com/
> 
> [   39.195053][    T1] serio: i8042 KBD port at 0x60,0x64 irq 1
> [   39.197347][    T1] kobject_add_internal failed for wakeup (error: -2 parent: serio0)
> [   39.199845][    T1] INFO: trying to register non-static key.
> [   39.201582][    T1] the code is fine but needs lockdep annotation.
> [   39.203477][    T1] turning off the locking correctness validator.
> [   39.205399][    T1] CPU: 12 PID: 1 Comm: swapper/0 Not tainted 5.3.0-rc4-
> next-20190813 #3
> [   39.207938][    T1] Hardware name: HP ProLiant XL420 Gen9/ProLiant XL420
> Gen9, BIOS U19 12/27/2015
> [   39.210606][    T1] Call Trace:
> [   39.210606][    T1]  dump_stack+0x62/0x9a
> [   39.210606][    T1]  register_lock_class+0x95a/0x960
> [   39.210606][    T1]  ? __platform_driver_probe+0xcd/0x230
> [   39.210606][    T1]  ? __platform_create_bundle+0xc0/0xe0
> [   39.210606][    T1]  ? i8042_init+0x4ec/0x578
> [   39.210606][    T1]  ? do_one_initcall+0xfe/0x45a
> [   39.219571][    T1]  ? kernel_init_freeable+0x614/0x6a7
> [   39.219571][    T1]  ? kernel_init+0x11/0x138
> [   39.219571][    T1]  ? ret_from_fork+0x35/0x40
> [   39.219571][    T1]  ? is_dynamic_key+0xf0/0xf0
> [   39.219571][    T1]  ? rwlock_bug.part.0+0x60/0x60
> [   39.219571][    T1]  ? __debug_check_no_obj_freed+0x8e/0x250
> [   39.219571][    T1]  __lock_acquire.isra.13+0x5f/0x830
> [   39.229491][    T1]  ? __debug_check_no_obj_freed+0x152/0x250
> [   39.229491][    T1]  lock_acquire+0x107/0x220
> [   39.229491][    T1]  ? __pm_relax.part.2+0x21/0xa0
> [   39.229491][    T1]  _raw_spin_lock_irqsave+0x35/0x50
> [   39.229491][    T1]  ? __pm_relax.part.2+0x21/0xa0
> [   39.229491][    T1]  __pm_relax.part.2+0x21/0xa0
> [   39.239588][    T1]  wakeup_source_destroy.part.3+0x18/0x190
> [   39.239588][    T1]  wakeup_source_register+0x43/0x50
> [   39.239588][    T1]  device_wakeup_enable+0x76/0x170
> [   39.239588][    T1]  device_set_wakeup_enable+0x13/0x20
> [   39.239588][    T1]  i80probe+0x921/0xa45
> [   39.339546][    T1]  ? i8042_toggle_aux+0xeb/0xeb
> [   39.349486][    T1]  ? kernfs_create_link+0xce/0x100
> [   39.349486][    T1]  ? sysfs_do_create_link_sd+0x7b/0xe0
> [   39.349486][    T1]  ? acpi_dev_pm_attach+0x31/0xf0
> [   39.349486][    T1]  platform_drv_probe+0x51/0xe0
> [   39.349486][    T1]  really_probe+0x1a2/0x630
> [   39.349486][    T1]  ? device_driver_attach+0xa0/0xa0
> [   39.349486][    T1]  driver_probe_device+0xcd/0x1f0
> [   39.359562][    T1]  ? device_driver_attach+0xa0/0xa0
> [   39.359562][    T1]  device_driver_attach+0x8f/0xa0
> [   39.359562][    T1]  __driver_attach+0xc7/0x1a0
> [   39.359562][    T1]  bus_for_each_dev+0xfe/0x160
> [   39.359562][    T1]  ? subsys_dev_iter_init+0x80/0x80
> [   39.359562][    T1]  ? __kasan_check_read+0x11/0x20
> [   39.359562][    T1]  ? _raw_spin_unlock+0x27/0x40
> [   39.369488][    T1]  driver_attach+0x2b/0x30
> [   39.369488][    T1]  bus_add_driver+0x298/0x350
> [   39.369488][    T1]  driver_register+0xdc/0x1d0
> [   39.369488][    T1]  ? i8042_toggle_aux+0xeb/0xeb
> [   39.369488][    T1]  __platform_driver_probe+0xcd/0x230
> [   39.3  __platform_create_bundle+0xc0/0xe0
> [   39.769489][    T1]  ? i8042_toggle_aux+0xeb/0xeb
> [   39.779556][    T1]  ? i8042_probe+0xa45/0xa45
> [   39.779556][    T1]  i8042_init+0x4ec/0x578
> [   39.779556][    T1]  ? i8042_probe+0xa45/0xa45
> [   39.779556][    T1]  ? netdev_run_todo+0x2f/0x4a0
> [   39.779556][    T1]  ? qdisc_create_dflt+0xf0/0xf0
> [   39.779556][    T1]  ? net_olddevs_init+0x67/0x67
> [   39.779556][    T1]  ? i8042_probe+0xa45/0xa45
> [   39.789486][    T1]  do_one_initcall+0xfe/0x45a
> [   39.789486][    T1]  ? initcall_blacklisted+0x150/0x150
> [   39.789486][    T1]  ? __kasan_check_write+0x14/0x20
> [   39.789486][    T1]  ? up_write+0xee/0x2a0
> [   39.789486][    T1]  kernel_init_freeable+0x614/0x6a7
> [   39.789486][    T1]  ? rest_init+0x188/0x188
> [   39.789486][    T1]  kernel_init+0x11/0x138
> [   39.799563][    T1]  ? rest_init+0x188/0x188
> [   39.799563][    T1]  ret_from_fork+0x35/0x40
> [   39.803412][    T1] serio: i8042 AUX port at 0x60,0x64 irq 12

Besides the bad error path causing the big stack trace, I think there's
a race between when the serio device is added with device_add() in
serio_add_port() and when i8042_register_ports() calls
device_set_wakeup_enable(). The serio_add_port() function is called from
a workqueue that is schedule to run by i8042_register_ports() calling
serio_register_port(), but otherwise there isn't any guarantee that the
workqueue has actually run by the time the function returns and
i8042_register_ports() calls device_set_wakeup_enable().

This means that the device may not have actually been registered yet,
and thus doing other device like operations on the serio device before
the workqueue runs will lead to weird behavior because the parent device
isn't fully registered with the driver core. That causes the error
message above:

> [   39.197347][    T1] kobject_add_internal failed for wakeup (error: -2 parent: serio0)

So maybe we need to add another hook after the device is added
successfully so we can do the wakeup things.

I also notice that device_set_wakeup_capable() has a check to see if the
device is registered yet and it skips creating sysfs entries for the
device if it isn't created in sysfs yet. Why? Just so it can be called
before the device is created? I guess the same logic is handled by
dpm_sysfs_add() if the device is registered after calling
device_set_wakeup_*().

There's two approaches I see:

	1) Do a similar check for device_set_wakeup_enable() and skip
	adding the wakeup class until dpm_sysfs_add().

	2) Find each case where this happens and only call wakeup APIs
	on the device after the device is added.

I guess it's better to let devices have wakeup modified on them before
they're registered with the device core?

Here's approach #1
----8<-----
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index 1b9c281cbe41..27ee00f50bd7 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -5,6 +5,7 @@
 #include <linux/export.h>
 #include <linux/pm_qos.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_wakeup.h>
 #include <linux/atomic.h>
 #include <linux/jiffies.h>
 #include "power.h"
@@ -661,14 +662,21 @@ int dpm_sysfs_add(struct device *dev)
 		if (rc)
 			goto err_runtime;
 	}
+	if (dev->power.wakeup) {
+		rc = wakeup_source_sysfs_add(dev, dev->power.wakeup);
+		if (rc)
+			goto err_wakeup;
+	}
 	if (dev->power.set_latency_tolerance) {
 		rc = sysfs_merge_group(&dev->kobj,
 				       &pm_qos_latency_tolerance_attr_group);
 		if (rc)
-			goto err_wakeup;
+			goto err_wakeup_source;
 	}
 	return 0;
 
+ err_wakeup_source:
+	wakeup_source_sysfs_remove(dev->power.wakeup);
  err_wakeup:
 	sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
  err_runtime:
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index f7925820b5ca..5817b51d2b15 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -220,10 +220,12 @@ struct wakeup_source *wakeup_source_register(struct device *dev,
 
 	ws = wakeup_source_create(name);
 	if (ws) {
-		ret = wakeup_source_sysfs_add(dev, ws);
-		if (ret) {
-			wakeup_source_free(ws);
-			return NULL;
+		if (!dev || device_is_registered(dev)) {
+			ret = wakeup_source_sysfs_add(dev, ws);
+			if (ret) {
+				wakeup_source_free(ws);
+				return NULL;
+			}
 		}
 		wakeup_source_add(ws);
 	}


And here's the second approach for serio.

---8<----
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index b695094290ab..f12bed00d6d0 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -439,6 +439,24 @@ static int i8042_start(struct serio *serio)
 	return 0;
 }
 
+static int i8042_added(struct serio *serio)
+{
+	device_set_wakeup_capable(&serio->dev, true);
+
+	/*
+	 * On platforms using suspend-to-idle, allow the keyboard to
+	 * wake up the system from sleep by enabling keyboard wakeups
+	 * by default.  This is consistent with keyboard wakeup
+	 * behavior on many platforms using suspend-to-RAM (ACPI S3)
+	 * by default.
+	 */
+	if (pm_suspend_default_s2idle() &&
+	    serio == i8042_ports[I8042_KBD_PORT_NO].serio)
+		device_set_wakeup_enable(&serio->dev, true);
+
+	return 0;
+}
+
 /*
  * i8042_stop() marks serio port as non-existing so i8042_interrupt
  * will not try to send data to the port that is about to go away.
@@ -1312,6 +1330,7 @@ static int __init i8042_create_kbd_port(void)
 	serio->id.type		= i8042_direct ? SERIO_8042 : SERIO_8042_XL;
 	serio->write		= i8042_dumbkbd ? NULL : i8042_kbd_write;
 	serio->start		= i8042_start;
+	serio->added		= i8042_added;
 	serio->stop		= i8042_stop;
 	serio->close		= i8042_port_close;
 	serio->ps2_cmd_mutex	= &i8042_mutex;
@@ -1397,17 +1416,6 @@ static void __init i8042_register_ports(void)
 			(unsigned long) I8042_COMMAND_REG,
 			i8042_ports[i].irq);
 		serio_register_port(serio);
-		device_set_wakeup_capable(&serio->dev, true);
-
-		/*
-		 * On platforms using suspend-to-idle, allow the keyboard to
-		 * wake up the system from sleep by enabling keyboard wakeups
-		 * by default.  This is consistent with keyboard wakeup
-		 * behavior on many platforms using suspend-to-RAM (ACPI S3)
-		 * by default.
-		 */
-		if (pm_suspend_default_s2idle() && i == I8042_KBD_PORT_NO)
-			device_set_wakeup_enable(&serio->dev, true);
 	}
 }
 
diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
index 29f491082926..590639467ea3 100644
--- a/drivers/input/serio/serio.c
+++ b/drivers/input/serio/serio.c
@@ -544,6 +544,8 @@ static void serio_add_port(struct serio *serio)
 		dev_err(&serio->dev,
 			"device_add() failed for %s (%s), error: %d\n",
 			serio->phys, serio->name, error);
+	else if (serio->added)
+		serio->added(serio);
 }
 
 /*
diff --git a/include/linux/serio.h b/include/linux/serio.h
index 6c27d413da92..2e216ba881a9 100644
--- a/include/linux/serio.h
+++ b/include/linux/serio.h
@@ -35,6 +35,7 @@ struct serio {
 	int (*open)(struct serio *);
 	void (*close)(struct serio *);
 	int (*start)(struct serio *);
+	int (*added)(struct serio *);
 	void (*stop)(struct serio *);
 
 	struct serio *parent;