diff mbox series

[RFC] drivers core: cpu: add hotplug callback to update cpu_dev state to resumed

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

Commit Message

Sudeep Holla Jan. 25, 2019, 3:09 p.m. UTC 
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

Comments

Eugeniu Rosca Jan. 27, 2019, 1:57 p.m. UTC | #1 
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>
Rafael J. Wysocki Jan. 30, 2019, 11:48 p.m. UTC | #2 
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

> 

>
Sudeep Holla Jan. 31, 2019, 4:05 p.m. UTC | #3 
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;
Sudeep Holla Feb. 4, 2019, 3:37 p.m. UTC | #4 
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;
Greg Kroah-Hartman Feb. 4, 2019, 3:44 p.m. UTC | #5 
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
Sudeep Holla Feb. 4, 2019, 3:52 p.m. UTC | #6 
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
Rafael J. Wysocki Feb. 6, 2019, 10:31 a.m. UTC | #7 
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
Sudeep Holla Feb. 6, 2019, 1:59 p.m. UTC | #8 
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 mbox series

Patch

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,