| Message ID | 20190125150906.27614-1-sudeep.holla@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] drivers core: cpu: add hotplug callback to update cpu_dev state to resumed | expand |
FWIW the "cache: parent cpuN should not be sleeping" warnings no longer
appear during s2ram procedure on R-Car H3-ES20 Salvator-X with this patch.
The kernel version is v5.0-rc3-241-g7c2614bf7a1f.
The test procedure is:
root@salvator-x:~# modprobe i2c-dev
root@salvator-x:~# i2cset -f -y 7 0x30 0x20 0x0F
root@salvator-x:~# echo deep > /sys/power/mem_sleep
root@salvator-x:~# echo mem > /sys/power/state
Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
On Friday, January 25, 2019 4:09:06 PM CET Sudeep Holla wrote: > The sysfs for the cpu caches are managed by adding devices with cpu > as the parent in cpu_device_create() when secondary cpu is brought > onlin. Generally when the secondary CPUs are hotplugged back is as part > of resume from suspend-to-ram, we call cpu_device_create() from the cpu > hotplug state machine while the cpu device associated with that CPU is > not yet ready to be resumed as the device_resume() call happens bit later. > It's not really needed to set the flag is_prepared for cpu devices are > they are mostly pseudo device and hotplug framework deals with state > machine and not managed through the cpu device. > > This often results in annoying warning when resuming: > Enabling non-boot CPUs ... > CPU1: Booted secondary processor > cache: parent cpu1 should not be sleeping > CPU1 is up > CPU2: Booted secondary processor > cache: parent cpu2 should not be sleeping > CPU2 is up > .... and so on. > > Just fix the warning by updating the device state quite early. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Reported-by: Jisheng Zhang <jszhang@marvell.com> > Reported-by: Steve Longerbeam <slongerbeam@gmail.com> > Reported-by: Eugeniu Rosca <erosca@de.adit-jv.com> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/base/cpu.c | 20 +++++++++++++++++++- > include/linux/cpuhotplug.h | 1 + > 2 files changed, 20 insertions(+), 1 deletion(-) > > Hi Rafael, > > This is getting reported for quite some time. Let me know if you have > better solution to fix this harmless yet annoying warnings during system > resume. I'd rather have a flag in struct dev_pm_info that will cause the message to be suppressed if set. It could be used for other purposes too then in princple (like skipping the creation of empty "power" attr groups in sysfs for devices that don't need them etc.). > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c > index eb9443d5bae1..ae0403528bff 100644 > --- a/drivers/base/cpu.c > +++ b/drivers/base/cpu.c > @@ -80,6 +80,8 @@ void unregister_cpu(struct cpu *cpu) > > device_unregister(&cpu->dev); > per_cpu(cpu_sys_devices, logical_cpu) = NULL; > + > + cpuhp_remove_state_nocalls(CPUHP_CPUDEV_PM_PREPARE); > return; > } > > @@ -355,6 +357,20 @@ static int cpu_uevent(struct device *dev, struct kobj_uevent_env *env) > } > #endif > > +static int cpu_dev_pm_unset_is_prepared(unsigned int cpu) > +{ > + struct device *cpu_dev = get_cpu_device(cpu); > + > + /* > + * device_resume sets this for cpu_dev bit later but the child > + * devices are resumes in the cpu hotplug state machine much > + * before device_resume is called. > + */ > + if (cpu_dev) > + cpu_dev->power.is_prepared = false; > + > + return 0; > +} > /* > * register_cpu - Setup a sysfs device for a CPU. > * @cpu - cpu->hotpluggable field set to 1 will generate a control file in > @@ -392,7 +408,9 @@ int register_cpu(struct cpu *cpu, int num) > dev_pm_qos_expose_latency_limit(&cpu->dev, > PM_QOS_RESUME_LATENCY_NO_CONSTRAINT); > > - return 0; > + return cpuhp_setup_state_nocalls(CPUHP_CPUDEV_PM_PREPARE, > + "base/cpu/dev_pm:prepare", > + cpu_dev_pm_unset_is_prepared, NULL); > } > > struct device *get_cpu_device(unsigned cpu) > diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h > index fd586d0301e7..2ecb4c9370ce 100644 > --- a/include/linux/cpuhotplug.h > +++ b/include/linux/cpuhotplug.h > @@ -69,6 +69,7 @@ enum cpuhp_state { > CPUHP_SLAB_PREPARE, > CPUHP_MD_RAID5_PREPARE, > CPUHP_RCUTREE_PREP, > + CPUHP_CPUDEV_PM_PREPARE, > CPUHP_CPUIDLE_COUPLED_PREPARE, > CPUHP_POWERPC_PMAC_PREPARE, > CPUHP_POWERPC_MMU_CTX_PREPARE, > -- > 2.17.1 > >
On Thu, Jan 31, 2019 at 12:48:49AM +0100, Rafael J. Wysocki wrote: > On Friday, January 25, 2019 4:09:06 PM CET Sudeep Holla wrote: > > The sysfs for the cpu caches are managed by adding devices with cpu > > as the parent in cpu_device_create() when secondary cpu is brought > > onlin. Generally when the secondary CPUs are hotplugged back is as part > > of resume from suspend-to-ram, we call cpu_device_create() from the cpu > > hotplug state machine while the cpu device associated with that CPU is > > not yet ready to be resumed as the device_resume() call happens bit later. > > It's not really needed to set the flag is_prepared for cpu devices are > > they are mostly pseudo device and hotplug framework deals with state > > machine and not managed through the cpu device. > > > > This often results in annoying warning when resuming: > > Enabling non-boot CPUs ... > > CPU1: Booted secondary processor > > cache: parent cpu1 should not be sleeping > > CPU1 is up > > CPU2: Booted secondary processor > > cache: parent cpu2 should not be sleeping > > CPU2 is up > > .... and so on. > > > > Just fix the warning by updating the device state quite early. > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > Reported-by: Jisheng Zhang <jszhang@marvell.com> > > Reported-by: Steve Longerbeam <slongerbeam@gmail.com> > > Reported-by: Eugeniu Rosca <erosca@de.adit-jv.com> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > --- > > drivers/base/cpu.c | 20 +++++++++++++++++++- > > include/linux/cpuhotplug.h | 1 + > > 2 files changed, 20 insertions(+), 1 deletion(-) > > > > Hi Rafael, > > > > This is getting reported for quite some time. Let me know if you have > > better solution to fix this harmless yet annoying warnings during system > > resume. > > I'd rather have a flag in struct dev_pm_info that will cause the message to > be suppressed if set. > > It could be used for other purposes too then in princple (like skipping the > creation of empty "power" attr groups in sysfs for devices that don't > need them etc.). > Thanks for the suggestion. I did quick hack and came up with something below. I wanted to run through you once before I materialise it into a formal patch to check if I understood your suggestion correctly. We can move no_pm_required outside dev_pm_info struct and rename with any better names. -->8 diff --git i/drivers/base/cpu.c w/drivers/base/cpu.c index eb9443d5bae1..b61f9772ed33 100644 --- i/drivers/base/cpu.c +++ w/drivers/base/cpu.c @@ -379,6 +379,7 @@ int register_cpu(struct cpu *cpu, int num) cpu->dev.bus->uevent = cpu_uevent; #endif cpu->dev.groups = common_cpu_attr_groups; + cpu->dev.power.no_pm_required = true; if (cpu->hotpluggable) cpu->dev.groups = hotplugable_cpu_attr_groups; error = device_register(&cpu->dev); @@ -427,6 +428,7 @@ __cpu_device_create(struct device *parent, void *drvdata, dev->parent = parent; dev->groups = groups; dev->release = device_create_release; + dev->power.no_pm_required = true; dev_set_drvdata(dev, drvdata); retval = kobject_set_name_vargs(&dev->kobj, fmt, args); diff --git i/drivers/base/power/main.c w/drivers/base/power/main.c index 0992e67e862b..ed1b133f73db 100644 --- i/drivers/base/power/main.c +++ w/drivers/base/power/main.c @@ -124,6 +124,8 @@ void device_pm_unlock(void) */ void device_pm_add(struct device *dev) { + if (dev->power.no_pm_required) + return; pr_debug("PM: Adding info for %s:%s\n", dev->bus ? dev->bus->name : "No Bus", dev_name(dev)); device_pm_check_callbacks(dev); @@ -142,6 +144,8 @@ void device_pm_add(struct device *dev) */ void device_pm_remove(struct device *dev) { + if (dev->power.no_pm_required) + return; pr_debug("PM: Removing info for %s:%s\n", dev->bus ? dev->bus->name : "No Bus", dev_name(dev)); complete_all(&dev->power.completion); diff --git i/drivers/base/power/sysfs.c w/drivers/base/power/sysfs.c index d713738ce796..54c1bfec396e 100644 --- i/drivers/base/power/sysfs.c +++ w/drivers/base/power/sysfs.c @@ -648,6 +648,9 @@ int dpm_sysfs_add(struct device *dev) { int rc; + if (dev->power.no_pm_required) + return 0; + rc = sysfs_create_group(&dev->kobj, &pm_attr_group); if (rc) return rc; diff --git i/include/linux/pm.h w/include/linux/pm.h index 0bd9de116826..300ab9f0b858 100644 --- i/include/linux/pm.h +++ w/include/linux/pm.h @@ -592,6 +592,7 @@ struct dev_pm_info { bool is_suspended:1; /* Ditto */ bool is_noirq_suspended:1; bool is_late_suspended:1; + bool no_pm_required:1; bool early_init:1; /* Owned by the PM core */ bool direct_complete:1; /* Owned by the PM core */ u32 driver_flags;
On Thu, Jan 31, 2019 at 04:05:59PM +0000, Sudeep Holla wrote: > On Thu, Jan 31, 2019 at 12:48:49AM +0100, Rafael J. Wysocki wrote: > > On Friday, January 25, 2019 4:09:06 PM CET Sudeep Holla wrote: > > > The sysfs for the cpu caches are managed by adding devices with cpu > > > as the parent in cpu_device_create() when secondary cpu is brought > > > onlin. Generally when the secondary CPUs are hotplugged back is as part > > > of resume from suspend-to-ram, we call cpu_device_create() from the cpu > > > hotplug state machine while the cpu device associated with that CPU is > > > not yet ready to be resumed as the device_resume() call happens bit later. > > > It's not really needed to set the flag is_prepared for cpu devices are > > > they are mostly pseudo device and hotplug framework deals with state > > > machine and not managed through the cpu device. > > > > > > This often results in annoying warning when resuming: > > > Enabling non-boot CPUs ... > > > CPU1: Booted secondary processor > > > cache: parent cpu1 should not be sleeping > > > CPU1 is up > > > CPU2: Booted secondary processor > > > cache: parent cpu2 should not be sleeping > > > CPU2 is up > > > .... and so on. > > > > > > Just fix the warning by updating the device state quite early. > > > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > > Reported-by: Jisheng Zhang <jszhang@marvell.com> > > > Reported-by: Steve Longerbeam <slongerbeam@gmail.com> > > > Reported-by: Eugeniu Rosca <erosca@de.adit-jv.com> > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > > --- > > > drivers/base/cpu.c | 20 +++++++++++++++++++- > > > include/linux/cpuhotplug.h | 1 + > > > 2 files changed, 20 insertions(+), 1 deletion(-) > > > > > > Hi Rafael, > > > > > > This is getting reported for quite some time. Let me know if you have > > > better solution to fix this harmless yet annoying warnings during system > > > resume. > > > > I'd rather have a flag in struct dev_pm_info that will cause the message to > > be suppressed if set. > > > > It could be used for other purposes too then in princple (like skipping the > > creation of empty "power" attr groups in sysfs for devices that don't > > need them etc.). > > > Thanks for the suggestion. I did quick hack and came up with something > below. I wanted to run through you once before I materialise it into > a formal patch to check if I understood your suggestion correctly. > We can move no_pm_required outside dev_pm_info struct and rename with > any better names. > Sorry for the nag, since the title has RFC, thought there are chances of this getting lost. Let me know if the below idea aligns with your suggestion ? Regards, Sudeep > -->8 > > diff --git i/drivers/base/cpu.c w/drivers/base/cpu.c > index eb9443d5bae1..b61f9772ed33 100644 > --- i/drivers/base/cpu.c > +++ w/drivers/base/cpu.c > @@ -379,6 +379,7 @@ int register_cpu(struct cpu *cpu, int num) > cpu->dev.bus->uevent = cpu_uevent; > #endif > cpu->dev.groups = common_cpu_attr_groups; > + cpu->dev.power.no_pm_required = true; > if (cpu->hotpluggable) > cpu->dev.groups = hotplugable_cpu_attr_groups; > error = device_register(&cpu->dev); > @@ -427,6 +428,7 @@ __cpu_device_create(struct device *parent, void *drvdata, > dev->parent = parent; > dev->groups = groups; > dev->release = device_create_release; > + dev->power.no_pm_required = true; > dev_set_drvdata(dev, drvdata); > > retval = kobject_set_name_vargs(&dev->kobj, fmt, args); > diff --git i/drivers/base/power/main.c w/drivers/base/power/main.c > index 0992e67e862b..ed1b133f73db 100644 > --- i/drivers/base/power/main.c > +++ w/drivers/base/power/main.c > @@ -124,6 +124,8 @@ void device_pm_unlock(void) > */ > void device_pm_add(struct device *dev) > { > + if (dev->power.no_pm_required) > + return; > pr_debug("PM: Adding info for %s:%s\n", > dev->bus ? dev->bus->name : "No Bus", dev_name(dev)); > device_pm_check_callbacks(dev); > @@ -142,6 +144,8 @@ void device_pm_add(struct device *dev) > */ > void device_pm_remove(struct device *dev) > { > + if (dev->power.no_pm_required) > + return; > pr_debug("PM: Removing info for %s:%s\n", > dev->bus ? dev->bus->name : "No Bus", dev_name(dev)); > complete_all(&dev->power.completion); > diff --git i/drivers/base/power/sysfs.c w/drivers/base/power/sysfs.c > index d713738ce796..54c1bfec396e 100644 > --- i/drivers/base/power/sysfs.c > +++ w/drivers/base/power/sysfs.c > @@ -648,6 +648,9 @@ int dpm_sysfs_add(struct device *dev) > { > int rc; > > + if (dev->power.no_pm_required) > + return 0; > + > rc = sysfs_create_group(&dev->kobj, &pm_attr_group); > if (rc) > return rc; > diff --git i/include/linux/pm.h w/include/linux/pm.h > index 0bd9de116826..300ab9f0b858 100644 > --- i/include/linux/pm.h > +++ w/include/linux/pm.h > @@ -592,6 +592,7 @@ struct dev_pm_info { > bool is_suspended:1; /* Ditto */ > bool is_noirq_suspended:1; > bool is_late_suspended:1; > + bool no_pm_required:1; > bool early_init:1; /* Owned by the PM core */ > bool direct_complete:1; /* Owned by the PM core */ > u32 driver_flags;
On Mon, Feb 04, 2019 at 03:37:20PM +0000, Sudeep Holla wrote: > On Thu, Jan 31, 2019 at 04:05:59PM +0000, Sudeep Holla wrote: > > On Thu, Jan 31, 2019 at 12:48:49AM +0100, Rafael J. Wysocki wrote: > > > On Friday, January 25, 2019 4:09:06 PM CET Sudeep Holla wrote: > > > > The sysfs for the cpu caches are managed by adding devices with cpu > > > > as the parent in cpu_device_create() when secondary cpu is brought > > > > onlin. Generally when the secondary CPUs are hotplugged back is as part > > > > of resume from suspend-to-ram, we call cpu_device_create() from the cpu > > > > hotplug state machine while the cpu device associated with that CPU is > > > > not yet ready to be resumed as the device_resume() call happens bit later. > > > > It's not really needed to set the flag is_prepared for cpu devices are > > > > they are mostly pseudo device and hotplug framework deals with state > > > > machine and not managed through the cpu device. > > > > > > > > This often results in annoying warning when resuming: > > > > Enabling non-boot CPUs ... > > > > CPU1: Booted secondary processor > > > > cache: parent cpu1 should not be sleeping > > > > CPU1 is up > > > > CPU2: Booted secondary processor > > > > cache: parent cpu2 should not be sleeping > > > > CPU2 is up > > > > .... and so on. > > > > > > > > Just fix the warning by updating the device state quite early. > > > > > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > > > Reported-by: Jisheng Zhang <jszhang@marvell.com> > > > > Reported-by: Steve Longerbeam <slongerbeam@gmail.com> > > > > Reported-by: Eugeniu Rosca <erosca@de.adit-jv.com> > > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > > > --- > > > > drivers/base/cpu.c | 20 +++++++++++++++++++- > > > > include/linux/cpuhotplug.h | 1 + > > > > 2 files changed, 20 insertions(+), 1 deletion(-) > > > > > > > > Hi Rafael, > > > > > > > > This is getting reported for quite some time. Let me know if you have > > > > better solution to fix this harmless yet annoying warnings during system > > > > resume. > > > > > > I'd rather have a flag in struct dev_pm_info that will cause the message to > > > be suppressed if set. > > > > > > It could be used for other purposes too then in princple (like skipping the > > > creation of empty "power" attr groups in sysfs for devices that don't > > > need them etc.). > > > > > Thanks for the suggestion. I did quick hack and came up with something > > below. I wanted to run through you once before I materialise it into > > a formal patch to check if I understood your suggestion correctly. > > We can move no_pm_required outside dev_pm_info struct and rename with > > any better names. > > > > Sorry for the nag, since the title has RFC, thought there are chances of > this getting lost. Let me know if the below idea aligns with your suggestion ? Personally, I ignore RFC patches unless I'm accidentally interested in them, as it shows that the author doesn't feel good enough to propose them as a real solution :) But that's just me... thanks, greg k-h
On Mon, Feb 04, 2019 at 04:44:21PM +0100, Greg Kroah-Hartman wrote: > On Mon, Feb 04, 2019 at 03:37:20PM +0000, Sudeep Holla wrote: > > On Thu, Jan 31, 2019 at 04:05:59PM +0000, Sudeep Holla wrote: > > > On Thu, Jan 31, 2019 at 12:48:49AM +0100, Rafael J. Wysocki wrote: > > > > On Friday, January 25, 2019 4:09:06 PM CET Sudeep Holla wrote: > > > > > The sysfs for the cpu caches are managed by adding devices with cpu > > > > > as the parent in cpu_device_create() when secondary cpu is brought > > > > > onlin. Generally when the secondary CPUs are hotplugged back is as part > > > > > of resume from suspend-to-ram, we call cpu_device_create() from the cpu > > > > > hotplug state machine while the cpu device associated with that CPU is > > > > > not yet ready to be resumed as the device_resume() call happens bit later. > > > > > It's not really needed to set the flag is_prepared for cpu devices are > > > > > they are mostly pseudo device and hotplug framework deals with state > > > > > machine and not managed through the cpu device. > > > > > > > > > > This often results in annoying warning when resuming: > > > > > Enabling non-boot CPUs ... > > > > > CPU1: Booted secondary processor > > > > > cache: parent cpu1 should not be sleeping > > > > > CPU1 is up > > > > > CPU2: Booted secondary processor > > > > > cache: parent cpu2 should not be sleeping > > > > > CPU2 is up > > > > > .... and so on. > > > > > > > > > > Just fix the warning by updating the device state quite early. > > > > > > > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > > > > Reported-by: Jisheng Zhang <jszhang@marvell.com> > > > > > Reported-by: Steve Longerbeam <slongerbeam@gmail.com> > > > > > Reported-by: Eugeniu Rosca <erosca@de.adit-jv.com> > > > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > > > > --- > > > > > drivers/base/cpu.c | 20 +++++++++++++++++++- > > > > > include/linux/cpuhotplug.h | 1 + > > > > > 2 files changed, 20 insertions(+), 1 deletion(-) > > > > > > > > > > Hi Rafael, > > > > > > > > > > This is getting reported for quite some time. Let me know if you have > > > > > better solution to fix this harmless yet annoying warnings during system > > > > > resume. > > > > > > > > I'd rather have a flag in struct dev_pm_info that will cause the message to > > > > be suppressed if set. > > > > > > > > It could be used for other purposes too then in princple (like skipping the > > > > creation of empty "power" attr groups in sysfs for devices that don't > > > > need them etc.). > > > > > > > Thanks for the suggestion. I did quick hack and came up with something > > > below. I wanted to run through you once before I materialise it into > > > a formal patch to check if I understood your suggestion correctly. > > > We can move no_pm_required outside dev_pm_info struct and rename with > > > any better names. > > > > > > > Sorry for the nag, since the title has RFC, thought there are chances of > > this getting lost. Let me know if the below idea aligns with your suggestion ? > > Personally, I ignore RFC patches unless I'm accidentally interested in > them, as it shows that the author doesn't feel good enough to propose > them as a real solution :) > I understand. Since Rafael did suggest alternate approach, just thought of pinging him to check if I understood his idea. > But that's just me... > :) Regards, Sudeep
On Monday, February 4, 2019 4:37:20 PM CET Sudeep Holla wrote: > On Thu, Jan 31, 2019 at 04:05:59PM +0000, Sudeep Holla wrote: > > On Thu, Jan 31, 2019 at 12:48:49AM +0100, Rafael J. Wysocki wrote: > > > On Friday, January 25, 2019 4:09:06 PM CET Sudeep Holla wrote: > > > > The sysfs for the cpu caches are managed by adding devices with cpu > > > > as the parent in cpu_device_create() when secondary cpu is brought > > > > onlin. Generally when the secondary CPUs are hotplugged back is as part > > > > of resume from suspend-to-ram, we call cpu_device_create() from the cpu > > > > hotplug state machine while the cpu device associated with that CPU is > > > > not yet ready to be resumed as the device_resume() call happens bit later. > > > > It's not really needed to set the flag is_prepared for cpu devices are > > > > they are mostly pseudo device and hotplug framework deals with state > > > > machine and not managed through the cpu device. > > > > > > > > This often results in annoying warning when resuming: > > > > Enabling non-boot CPUs ... > > > > CPU1: Booted secondary processor > > > > cache: parent cpu1 should not be sleeping > > > > CPU1 is up > > > > CPU2: Booted secondary processor > > > > cache: parent cpu2 should not be sleeping > > > > CPU2 is up > > > > .... and so on. > > > > > > > > Just fix the warning by updating the device state quite early. > > > > > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > > > Reported-by: Jisheng Zhang <jszhang@marvell.com> > > > > Reported-by: Steve Longerbeam <slongerbeam@gmail.com> > > > > Reported-by: Eugeniu Rosca <erosca@de.adit-jv.com> > > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > > > --- > > > > drivers/base/cpu.c | 20 +++++++++++++++++++- > > > > include/linux/cpuhotplug.h | 1 + > > > > 2 files changed, 20 insertions(+), 1 deletion(-) > > > > > > > > Hi Rafael, > > > > > > > > This is getting reported for quite some time. Let me know if you have > > > > better solution to fix this harmless yet annoying warnings during system > > > > resume. > > > > > > I'd rather have a flag in struct dev_pm_info that will cause the message to > > > be suppressed if set. > > > > > > It could be used for other purposes too then in princple (like skipping the > > > creation of empty "power" attr groups in sysfs for devices that don't > > > need them etc.). > > > > > Thanks for the suggestion. I did quick hack and came up with something > > below. I wanted to run through you once before I materialise it into > > a formal patch to check if I understood your suggestion correctly. > > We can move no_pm_required outside dev_pm_info struct and rename with > > any better names. > > > > Sorry for the nag, since the title has RFC, thought there are chances of > this getting lost. Let me know if the below idea aligns with your suggestion ? RFC would be fine, but Patchwork doesn't pick up patches posted as replies in the middle of a thread. :-) Yes, this is basically what I suggested, please post. Cheers, Rafael
On Wed, Feb 06, 2019 at 11:31:04AM +0100, Rafael J. Wysocki wrote: > On Monday, February 4, 2019 4:37:20 PM CET Sudeep Holla wrote: > > > > Sorry for the nag, since the title has RFC, thought there are chances of > > this getting lost. Let me know if the below idea aligns with your suggestion ? > > RFC would be fine, but Patchwork doesn't pick up patches posted as replies > in the middle of a thread. :-) > I completely understand and I had intentions to post it as a patch so that patchwork sees it. Just wanted to check if I align well with what you suggested. > Yes, this is basically what I suggested, please post. > Thanks for confirming. I will post the patch shortly. -- Regards, Sudeep
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c index eb9443d5bae1..ae0403528bff 100644 --- a/drivers/base/cpu.c +++ b/drivers/base/cpu.c @@ -80,6 +80,8 @@ void unregister_cpu(struct cpu *cpu) device_unregister(&cpu->dev); per_cpu(cpu_sys_devices, logical_cpu) = NULL; + + cpuhp_remove_state_nocalls(CPUHP_CPUDEV_PM_PREPARE); return; } @@ -355,6 +357,20 @@ static int cpu_uevent(struct device *dev, struct kobj_uevent_env *env) } #endif +static int cpu_dev_pm_unset_is_prepared(unsigned int cpu) +{ + struct device *cpu_dev = get_cpu_device(cpu); + + /* + * device_resume sets this for cpu_dev bit later but the child + * devices are resumes in the cpu hotplug state machine much + * before device_resume is called. + */ + if (cpu_dev) + cpu_dev->power.is_prepared = false; + + return 0; +} /* * register_cpu - Setup a sysfs device for a CPU. * @cpu - cpu->hotpluggable field set to 1 will generate a control file in @@ -392,7 +408,9 @@ int register_cpu(struct cpu *cpu, int num) dev_pm_qos_expose_latency_limit(&cpu->dev, PM_QOS_RESUME_LATENCY_NO_CONSTRAINT); - return 0; + return cpuhp_setup_state_nocalls(CPUHP_CPUDEV_PM_PREPARE, + "base/cpu/dev_pm:prepare", + cpu_dev_pm_unset_is_prepared, NULL); } struct device *get_cpu_device(unsigned cpu) diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h index fd586d0301e7..2ecb4c9370ce 100644 --- a/include/linux/cpuhotplug.h +++ b/include/linux/cpuhotplug.h @@ -69,6 +69,7 @@ enum cpuhp_state { CPUHP_SLAB_PREPARE, CPUHP_MD_RAID5_PREPARE, CPUHP_RCUTREE_PREP, + CPUHP_CPUDEV_PM_PREPARE, CPUHP_CPUIDLE_COUPLED_PREPARE, CPUHP_POWERPC_PMAC_PREPARE, CPUHP_POWERPC_MMU_CTX_PREPARE,
The sysfs for the cpu caches are managed by adding devices with cpu as the parent in cpu_device_create() when secondary cpu is brought onlin. Generally when the secondary CPUs are hotplugged back is as part of resume from suspend-to-ram, we call cpu_device_create() from the cpu hotplug state machine while the cpu device associated with that CPU is not yet ready to be resumed as the device_resume() call happens bit later. It's not really needed to set the flag is_prepared for cpu devices are they are mostly pseudo device and hotplug framework deals with state machine and not managed through the cpu device. This often results in annoying warning when resuming: Enabling non-boot CPUs ... CPU1: Booted secondary processor cache: parent cpu1 should not be sleeping CPU1 is up CPU2: Booted secondary processor cache: parent cpu2 should not be sleeping CPU2 is up .... and so on. Just fix the warning by updating the device state quite early. Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Reported-by: Jisheng Zhang <jszhang@marvell.com> Reported-by: Steve Longerbeam <slongerbeam@gmail.com> Reported-by: Eugeniu Rosca <erosca@de.adit-jv.com> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/base/cpu.c | 20 +++++++++++++++++++- include/linux/cpuhotplug.h | 1 + 2 files changed, 20 insertions(+), 1 deletion(-) Hi Rafael, This is getting reported for quite some time. Let me know if you have better solution to fix this harmless yet annoying warnings during system resume. Regards, Sudeep -- 2.17.1