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/[email protected]/
[ 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
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/[email protected]/
>
> [ 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
We shouldn't call wakeup_source_destroy() from the error path in
wakeup_source_register() because that calls __pm_relax() and that takes
a lock that isn't initialized until wakeup_source_add() is called. Can
you try this patch?
----8<----
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 3a7f5803aa81..f7925820b5ca 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -137,6 +137,13 @@ static void wakeup_source_record(struct wakeup_source *ws)
spin_unlock_irqrestore(&deleted_ws.lock, flags);
}
+static void wakeup_source_free(struct wakeup_source *ws)
+{
+ ida_free(&wakeup_ida, ws->id);
+ kfree_const(ws->name);
+ kfree(ws);
+}
+
/**
* wakeup_source_destroy - Destroy a struct wakeup_source object.
* @ws: Wakeup source to destroy.
@@ -150,9 +157,7 @@ void wakeup_source_destroy(struct wakeup_source *ws)
__pm_relax(ws);
wakeup_source_record(ws);
- ida_free(&wakeup_ida, ws->id);
- kfree_const(ws->name);
- kfree(ws);
+ wakeup_source_free(ws);
}
EXPORT_SYMBOL_GPL(wakeup_source_destroy);
@@ -217,7 +222,7 @@ struct wakeup_source *wakeup_source_register(struct device *dev,
if (ws) {
ret = wakeup_source_sysfs_add(dev, ws);
if (ret) {
- wakeup_source_destroy(ws);
+ wakeup_source_free(ws);
return NULL;
}
wakeup_source_add(ws);
On Tue, Aug 13, 2019 at 3:35 PM Stephen Boyd <[email protected]> wrote:
>
> 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/[email protected]/
> >
> > [ 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
>
> We shouldn't call wakeup_source_destroy() from the error path in
> wakeup_source_register() because that calls __pm_relax() and that takes
> a lock that isn't initialized until wakeup_source_add() is called. Can
> you try this patch?
Right, that makes sense. Thanks for sending a fix, Stephen!
What's the preferred procedure for merging this fix? Should we apply
this commit on top of pm tree? Or should I resend a new version of the
offending patch? Sorry, I'm still new to this.
On Wed, Aug 14, 2019 at 1:04 AM Tri Vo <[email protected]> wrote:
>
> On Tue, Aug 13, 2019 at 3:35 PM Stephen Boyd <[email protected]> wrote:
> >
> > 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/[email protected]/
> > >
> > > [ 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
> >
> > We shouldn't call wakeup_source_destroy() from the error path in
> > wakeup_source_register() because that calls __pm_relax() and that takes
> > a lock that isn't initialized until wakeup_source_add() is called. Can
> > you try this patch?
>
> Right, that makes sense. Thanks for sending a fix, Stephen!
>
> What's the preferred procedure for merging this fix? Should we apply
> this commit on top of pm tree? Or should I resend a new version of the
> offending patch? Sorry, I'm still new to this.
I can apply this patch as a fix if it is resent with a proper changelog.
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/[email protected]/
>
> [ 39.195053][ T1] serio: i8042 KBD port at 0x60,0x64 irq 1
> [ 39.197347][ T1] kobject_add_internal failed for wakeup (error: -2 parent:
Also, this is weird. Why is the kobject name just 'wakeup' and not
something like wakeup0 or wakeup1? This is why we're seeing the lockdep
warning happen after, but I'm not sure why the name doesn't have a
number associated with it.
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/[email protected]/
>
> [ 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;
* Stephen Boyd <[email protected]> [691231 23:00]:
> 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_*().
Hmm just guessing.. It's maybe because drivers can enable and disable
the wakeup capability at any point for example like driver/net drivers
do based on WOL etc?
> 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?
I think we should at least initially handle case #1 above as multiple
places otherwise seem to break. Then maybe we could add a warning to
help fix all the #2 cases if needed?
Regards,
Tony
On Tue, 2019-08-13 at 15:35 -0700, Stephen Boyd wrote:
> 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/[email protected]/
> >
> > [ 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
>
> We shouldn't call wakeup_source_destroy() from the error path in
> wakeup_source_register() because that calls __pm_relax() and that takes
> a lock that isn't initialized until wakeup_source_add() is called. Can
> you try this patch?
It works fine which takes care of the lockdep issue.
>
> ----8<----
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 3a7f5803aa81..f7925820b5ca 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -137,6 +137,13 @@ static void wakeup_source_record(struct wakeup_source
> *ws)
> spin_unlock_irqrestore(&deleted_ws.lock, flags);
> }
>
> +static void wakeup_source_free(struct wakeup_source *ws)
> +{
> + ida_free(&wakeup_ida, ws->id);
> + kfree_const(ws->name);
> + kfree(ws);
> +}
> +
> /**
> * wakeup_source_destroy - Destroy a struct wakeup_source object.
> * @ws: Wakeup source to destroy.
> @@ -150,9 +157,7 @@ void wakeup_source_destroy(struct wakeup_source *ws)
>
> __pm_relax(ws);
> wakeup_source_record(ws);
> - ida_free(&wakeup_ida, ws->id);
> - kfree_const(ws->name);
> - kfree(ws);
> + wakeup_source_free(ws);
> }
> EXPORT_SYMBOL_GPL(wakeup_source_destroy);
>
> @@ -217,7 +222,7 @@ struct wakeup_source *wakeup_source_register(struct device
> *dev,
> if (ws) {
> ret = wakeup_source_sysfs_add(dev, ws);
> if (ret) {
> - wakeup_source_destroy(ws);
> + wakeup_source_free(ws);
> return NULL;
> }
> wakeup_source_add(ws);
On Wed, Aug 14, 2019 at 12:03:19AM -0700, Stephen Boyd wrote:
> 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/[email protected]/
> >
> > [???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.
So from i8042 POV what it is doing now is completely wrong, as it is
operating on structure in an unknown state, and what you propose below
makes total sense, except I'd use bus notifier and not a dedicated
callback, unless we see more serio drivers needing it.
Thanks.
--
Dmitry
On Wed, Aug 14, 2019 at 1:40 AM Tony Lindgren <[email protected]> wrote:
>
> * Stephen Boyd <[email protected]> [691231 23:00]:
> > 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_*().
>
> Hmm just guessing.. It's maybe because drivers can enable and disable
> the wakeup capability at any point for example like driver/net drivers
> do based on WOL etc?
>
> > 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?
>
> I think we should at least initially handle case #1 above as multiple
> places otherwise seem to break. Then maybe we could add a warning to
> help fix all the #2 cases if needed?
Makes sense. For case#1, we could also just register the wakeup source
without specifying the parent device if the latter hasn't been
registered yet. Userspace won't be able to associate a wakeup source
to the parent device. But I think it's a reasonable fix, assuming we
want to fix devices not being added before calling wakeup APIs #2.
On Wed, Aug 14, 2019 at 8:37 PM Tri Vo <[email protected]> wrote:
>
> On Wed, Aug 14, 2019 at 1:40 AM Tony Lindgren <[email protected]> wrote:
> >
> > * Stephen Boyd <[email protected]> [691231 23:00]:
> > > 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_*().
> >
> > Hmm just guessing.. It's maybe because drivers can enable and disable
> > the wakeup capability at any point for example like driver/net drivers
> > do based on WOL etc?
> >
> > > 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?
> >
> > I think we should at least initially handle case #1 above as multiple
> > places otherwise seem to break. Then maybe we could add a warning to
> > help fix all the #2 cases if needed?
>
> Makes sense. For case#1, we could also just register the wakeup source
> without specifying the parent device if the latter hasn't been
> registered yet. Userspace won't be able to associate a wakeup source
> to the parent device. But I think it's a reasonable fix, assuming we
> want to fix devices not being added before calling wakeup APIs #2.
Well, OK
I'm going to drop the entire series from linux-next at this point and
let's start over.
Also note that all of this is not an issue until we start to add
children under the device passed to device_set_wakeup_enable() and
friends so maybe that is not a good idea after all?
Quoting Rafael J. Wysocki (2019-08-16 05:17:23)
> On Wed, Aug 14, 2019 at 8:37 PM Tri Vo <[email protected]> wrote:
> >
> > On Wed, Aug 14, 2019 at 1:40 AM Tony Lindgren <[email protected]> wrote:
> > >
> > > * Stephen Boyd <[email protected]> [691231 23:00]:
> > > > 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_*().
> > >
> > > Hmm just guessing.. It's maybe because drivers can enable and disable
> > > the wakeup capability at any point for example like driver/net drivers
> > > do based on WOL etc?
> > >
> > > > 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?
> > >
> > > I think we should at least initially handle case #1 above as multiple
> > > places otherwise seem to break. Then maybe we could add a warning to
> > > help fix all the #2 cases if needed?
> >
> > Makes sense. For case#1, we could also just register the wakeup source
> > without specifying the parent device if the latter hasn't been
> > registered yet. Userspace won't be able to associate a wakeup source
> > to the parent device. But I think it's a reasonable fix, assuming we
> > want to fix devices not being added before calling wakeup APIs #2.
>
> Well, OK
>
> I'm going to drop the entire series from linux-next at this point and
> let's start over.
I was going to send the first patch I floated as a more formal patch to
be applied to the PM tree. I was waiting to see if the semantics of
device_set_wakeup_*() could be clarified because I don't understand if
they're allowed to be called before device_add().
>
> Also note that all of this is not an issue until we start to add
> children under the device passed to device_set_wakeup_enable() and
> friends so maybe that is not a good idea after all?
My primary goal is to know what wakeup is associated with a device. If
we delay creation of the sysfs node to the time that device_add() is
called then it will allow device_set_wakeup_enable() to be called before
the device is published to userspace. Is anything wrong with that? This
seems to be the intention of the API based on the way
device_set_wakeup_capable() is written. Furthermore, if we make this
change then we don't need to fix various drivers to reorder calls to
device_set_wakeup_enable() and device_add(), so it looks like the right
approach.
I'll send the patch over the list now and let you decide. I'll also send
a patch for serio to have it operate on the device in a less racy way,
but not necessarily after the device_add() is called.
On Friday, August 16, 2019 4:19:35 PM CEST Stephen Boyd wrote:
> Quoting Rafael J. Wysocki (2019-08-16 05:17:23)
> > On Wed, Aug 14, 2019 at 8:37 PM Tri Vo <[email protected]> wrote:
> > >
> > > On Wed, Aug 14, 2019 at 1:40 AM Tony Lindgren <[email protected]> wrote:
> > > >
> > > > * Stephen Boyd <[email protected]> [691231 23:00]:
> > > > > 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_*().
> > > >
> > > > Hmm just guessing.. It's maybe because drivers can enable and disable
> > > > the wakeup capability at any point for example like driver/net drivers
> > > > do based on WOL etc?
> > > >
> > > > > 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?
> > > >
> > > > I think we should at least initially handle case #1 above as multiple
> > > > places otherwise seem to break. Then maybe we could add a warning to
> > > > help fix all the #2 cases if needed?
> > >
> > > Makes sense. For case#1, we could also just register the wakeup source
> > > without specifying the parent device if the latter hasn't been
> > > registered yet. Userspace won't be able to associate a wakeup source
> > > to the parent device. But I think it's a reasonable fix, assuming we
> > > want to fix devices not being added before calling wakeup APIs #2.
> >
> > Well, OK
> >
> > I'm going to drop the entire series from linux-next at this point and
> > let's start over.
>
> I was going to send the first patch I floated as a more formal patch to
> be applied to the PM tree. I was waiting to see if the semantics of
> device_set_wakeup_*() could be clarified because I don't understand if
> they're allowed to be called before device_add().
>
> >
> > Also note that all of this is not an issue until we start to add
> > children under the device passed to device_set_wakeup_enable() and
> > friends so maybe that is not a good idea after all?
>
> My primary goal is to know what wakeup is associated with a device. If
> we delay creation of the sysfs node to the time that device_add() is
> called then it will allow device_set_wakeup_enable() to be called before
> the device is published to userspace. Is anything wrong with that? This
> seems to be the intention of the API based on the way
> device_set_wakeup_capable() is written. Furthermore, if we make this
> change then we don't need to fix various drivers to reorder calls to
> device_set_wakeup_enable() and device_add(), so it looks like the right
> approach.
Sounds reasonable.