Re: "PM / wakeup: Show wakeup sources stats in sysfs" causes boot warnings
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 wrote: > > > > > > On Wed, Aug 14, 2019 at 1:40 AM Tony Lindgren wrote: > > > > > > > > * Stephen Boyd [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.
Re: "PM / wakeup: Show wakeup sources stats in sysfs" causes boot warnings
Quoting Rafael J. Wysocki (2019-08-16 05:17:23) > On Wed, Aug 14, 2019 at 8:37 PM Tri Vo wrote: > > > > On Wed, Aug 14, 2019 at 1:40 AM Tony Lindgren wrote: > > > > > > * Stephen Boyd [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.
Re: "PM / wakeup: Show wakeup sources stats in sysfs" causes boot warnings
On Wed, Aug 14, 2019 at 8:37 PM Tri Vo wrote: > > On Wed, Aug 14, 2019 at 1:40 AM Tony Lindgren wrote: > > > > * Stephen Boyd [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?
Re: "PM / wakeup: Show wakeup sources stats in sysfs" causes boot warnings
On Wed, Aug 14, 2019 at 1:40 AM Tony Lindgren wrote: > > * Stephen Boyd [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.
Re: "PM / wakeup: Show wakeup sources stats in sysfs" causes boot warnings
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/20190807014846.143949-4-tr...@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()
Re: "PM / wakeup: Show wakeup sources stats in sysfs" causes boot warnings
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/20190807014846.143949-4-tr...@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
>
> 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(_ws.lock, flags);
> }
>
> +static void wakeup_source_free(struct wakeup_source *ws)
> +{
> + ida_free(_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(_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);
Re: "PM / wakeup: Show wakeup sources stats in sysfs" causes boot warnings
* Stephen Boyd [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
Re: "PM / wakeup: Show wakeup sources stats in sysfs" causes boot warnings
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-tr...@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
Re: "PM / wakeup: Show wakeup sources stats in sysfs" causes boot warnings
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-tr...@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: 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.
Re: "PM / wakeup: Show wakeup sources stats in sysfs" causes boot warnings
On Wed, Aug 14, 2019 at 1:04 AM Tri Vo wrote: > > On Tue, Aug 13, 2019 at 3:35 PM 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/20190807014846.143949-4-tr...@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 > > > > 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.
Re: "PM / wakeup: Show wakeup sources stats in sysfs" causes boot warnings
On Tue, Aug 13, 2019 at 3:35 PM 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/20190807014846.143949-4-tr...@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 > > 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.
Re: "PM / wakeup: Show wakeup sources stats in sysfs" causes boot warnings
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-tr...@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
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(_ws.lock, flags);
}
+static void wakeup_source_free(struct wakeup_source *ws)
+{
+ ida_free(_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(_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);
