drivers: base: add support to skip power management in device/driver model
diff mbox series

Message ID 20190206150935.12140-1-sudeep.holla@arm.com
State Deferred, archived
Headers show
Series
  • drivers: base: add support to skip power management in device/driver model
Related show

Commit Message

Sudeep Holla Feb. 6, 2019, 3:09 p.m. UTC
All device objects in the driver model contain fields that control the
handling of various power management activities. However, it's not
always useful. There are few instances where pseudo devices are added
to the model just to take advantage of many other features like
kobjects, udev events, and so on. One such example is cpu devices and
their caches.

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
online. Generally when the secondary CPUs are hotplugged back in 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 as 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.

So in order to fix these kind of errors, we could just completely avoid
doing any power management related initialisations and operations if
they are not used by these devices.

Lets add no_pm_required flags to indicate that the device doesn't
require any sort of pm activities and all of them can be completely
skipped. We can use the same flag to also avoid adding not used *power*
sysfs entries for these devices.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/base/cpu.c         |  2 ++
 drivers/base/power/main.c  |  7 +++++++
 drivers/base/power/sysfs.c |  4 ++++
 include/linux/device.h     | 10 ++++++++++
 include/linux/pm.h         |  1 +
 5 files changed, 24 insertions(+)

Comments

Sudeep Holla Feb. 6, 2019, 3:41 p.m. UTC | #1
(completely forgot to add people who reported original issue, cc-ing now)

On Wed, Feb 06, 2019 at 03:09:35PM +0000, Sudeep Holla wrote:
> All device objects in the driver model contain fields that control the
> handling of various power management activities. However, it's not
> always useful. There are few instances where pseudo devices are added
> to the model just to take advantage of many other features like
> kobjects, udev events, and so on. One such example is cpu devices and
> their caches.
> 
> 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
> online. Generally when the secondary CPUs are hotplugged back in 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 as 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.
> 
> So in order to fix these kind of errors, we could just completely avoid
> doing any power management related initialisations and operations if
> they are not used by these devices.
> 
> Lets add no_pm_required flags to indicate that the device doesn't
> require any sort of pm activities and all of them can be completely
> skipped. We can use the same flag to also avoid adding not used *power*
> sysfs entries for these devices.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/base/cpu.c         |  2 ++
>  drivers/base/power/main.c  |  7 +++++++
>  drivers/base/power/sysfs.c |  4 ++++
>  include/linux/device.h     | 10 ++++++++++
>  include/linux/pm.h         |  1 +
>  5 files changed, 24 insertions(+)
> 
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index eb9443d5bae1..ccec353aa24c 100644
> --- a/drivers/base/cpu.c
> +++ b/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;
> +	device_set_pm_not_required(&cpu->dev);
>  	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;
> +	device_set_pm_not_required(dev);
>  	dev_set_drvdata(dev, drvdata);
>  
>  	retval = kobject_set_name_vargs(&dev->kobj, fmt, args);
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 0992e67e862b..2a29c3d4e240 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -124,6 +124,10 @@ void device_pm_unlock(void)
>   */
>  void device_pm_add(struct device *dev)
>  {
> +	/* No need to create pm sysfs if explicitly specified as not required */
> +	if (device_pm_not_required(dev))
> +		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 +146,9 @@ void device_pm_add(struct device *dev)
>   */
>  void device_pm_remove(struct device *dev)
>  {
> +	if (device_pm_not_required(dev))
> +		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 a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index d713738ce796..6cd159b51114 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -648,6 +648,10 @@ int dpm_sysfs_add(struct device *dev)
>  {
>  	int rc;
>  
> +	/* No need to create pm sysfs if explicitly disabled */
> +	if (device_pm_not_required(dev))
> +		return 0;
> +
>  	rc = sysfs_create_group(&dev->kobj, &pm_attr_group);
>  	if (rc)
>  		return rc;
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 6cb4640b6160..739d0b62e4d4 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1165,6 +1165,16 @@ static inline bool device_async_suspend_enabled(struct device *dev)
>  	return !!dev->power.async_suspend;
>  }
>  
> +static inline bool device_pm_not_required(struct device *dev)
> +{
> +	return dev->power.no_pm_required;
> +}
> +
> +static inline void device_set_pm_not_required(struct device *dev)
> +{
> +	dev->power.no_pm_required = true;
> +}
> +
>  static inline void dev_pm_syscore_device(struct device *dev, bool val)
>  {
>  #ifdef CONFIG_PM_SLEEP
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 0bd9de116826..300ab9f0b858 100644
> --- a/include/linux/pm.h
> +++ b/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;
> -- 
> 2.17.1
>
Ulf Hansson Feb. 7, 2019, 9:53 a.m. UTC | #2
On Wed, 6 Feb 2019 at 16:09, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> All device objects in the driver model contain fields that control the
> handling of various power management activities. However, it's not
> always useful. There are few instances where pseudo devices are added
> to the model just to take advantage of many other features like
> kobjects, udev events, and so on. One such example is cpu devices and
> their caches.
>
> 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
> online. Generally when the secondary CPUs are hotplugged back in 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 as they are mostly pseudo device and hotplug framework deals
> with state machine and not managed through the cpu device.

What's the point of removing and then re-adding the sysfs attributes
for the cpu caches, at each hotplug off/on sequence? To me that sounds
inefficient and unnecessary, no?

If you avoid this, would that solve this problem?

Kind regards
Uffe

>
> 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.
>
> So in order to fix these kind of errors, we could just completely avoid
> doing any power management related initialisations and operations if
> they are not used by these devices.
>
> Lets add no_pm_required flags to indicate that the device doesn't
> require any sort of pm activities and all of them can be completely
> skipped. We can use the same flag to also avoid adding not used *power*
> sysfs entries for these devices.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/base/cpu.c         |  2 ++
>  drivers/base/power/main.c  |  7 +++++++
>  drivers/base/power/sysfs.c |  4 ++++
>  include/linux/device.h     | 10 ++++++++++
>  include/linux/pm.h         |  1 +
>  5 files changed, 24 insertions(+)
>
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index eb9443d5bae1..ccec353aa24c 100644
> --- a/drivers/base/cpu.c
> +++ b/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;
> +       device_set_pm_not_required(&cpu->dev);
>         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;
> +       device_set_pm_not_required(dev);
>         dev_set_drvdata(dev, drvdata);
>
>         retval = kobject_set_name_vargs(&dev->kobj, fmt, args);
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 0992e67e862b..2a29c3d4e240 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -124,6 +124,10 @@ void device_pm_unlock(void)
>   */
>  void device_pm_add(struct device *dev)
>  {
> +       /* No need to create pm sysfs if explicitly specified as not required */
> +       if (device_pm_not_required(dev))
> +               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 +146,9 @@ void device_pm_add(struct device *dev)
>   */
>  void device_pm_remove(struct device *dev)
>  {
> +       if (device_pm_not_required(dev))
> +               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 a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index d713738ce796..6cd159b51114 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -648,6 +648,10 @@ int dpm_sysfs_add(struct device *dev)
>  {
>         int rc;
>
> +       /* No need to create pm sysfs if explicitly disabled */
> +       if (device_pm_not_required(dev))
> +               return 0;
> +
>         rc = sysfs_create_group(&dev->kobj, &pm_attr_group);
>         if (rc)
>                 return rc;
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 6cb4640b6160..739d0b62e4d4 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1165,6 +1165,16 @@ static inline bool device_async_suspend_enabled(struct device *dev)
>         return !!dev->power.async_suspend;
>  }
>
> +static inline bool device_pm_not_required(struct device *dev)
> +{
> +       return dev->power.no_pm_required;
> +}
> +
> +static inline void device_set_pm_not_required(struct device *dev)
> +{
> +       dev->power.no_pm_required = true;
> +}
> +
>  static inline void dev_pm_syscore_device(struct device *dev, bool val)
>  {
>  #ifdef CONFIG_PM_SLEEP
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 0bd9de116826..300ab9f0b858 100644
> --- a/include/linux/pm.h
> +++ b/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;
> --
> 2.17.1
>
Sudeep Holla Feb. 7, 2019, 10:36 a.m. UTC | #3
On Thu, Feb 07, 2019 at 10:53:56AM +0100, Ulf Hansson wrote:
> On Wed, 6 Feb 2019 at 16:09, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > All device objects in the driver model contain fields that control the
> > handling of various power management activities. However, it's not
> > always useful. There are few instances where pseudo devices are added
> > to the model just to take advantage of many other features like
> > kobjects, udev events, and so on. One such example is cpu devices and
> > their caches.
> >
> > 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
> > online. Generally when the secondary CPUs are hotplugged back in 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 as they are mostly pseudo device and hotplug framework deals
> > with state machine and not managed through the cpu device.
>
> What's the point of removing and then re-adding the sysfs attributes
> for the cpu caches, at each hotplug off/on sequence? To me that sounds
> inefficient and unnecessary, no?
>

Oh well, you must look at PowerPC pHyp which can change cache nodes for
suspend/resume operation too, let alone hotplug operations. So, we can't
assume the same CPUs with exact same caches are always hotplugged back
in. That's may happen in embedded and other static platforms but not
everywhere. Anyway that's my understand as why it's done in hotplug
patch. So it's not so simple to call it inefficient and unnecessary IMO.

> If you avoid this, would that solve this problem?
>

May be, but as mentioned above we can't really. Also this change will
help to avoid creating unnecessary power sysfs which is mainly runtime
pm related for some of the devices created. CPU/caches was just one
example which triggered this, but this can be more useful. We can avoid
adding them to dpm list.

--
Regards,
Sudeep
Ulf Hansson Feb. 7, 2019, 2:29 p.m. UTC | #4
On Thu, 7 Feb 2019 at 11:36, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Feb 07, 2019 at 10:53:56AM +0100, Ulf Hansson wrote:
> > On Wed, 6 Feb 2019 at 16:09, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > All device objects in the driver model contain fields that control the
> > > handling of various power management activities. However, it's not
> > > always useful. There are few instances where pseudo devices are added
> > > to the model just to take advantage of many other features like
> > > kobjects, udev events, and so on. One such example is cpu devices and
> > > their caches.
> > >
> > > 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
> > > online. Generally when the secondary CPUs are hotplugged back in 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 as they are mostly pseudo device and hotplug framework deals
> > > with state machine and not managed through the cpu device.
> >
> > What's the point of removing and then re-adding the sysfs attributes
> > for the cpu caches, at each hotplug off/on sequence? To me that sounds
> > inefficient and unnecessary, no?
> >
>
> Oh well, you must look at PowerPC pHyp which can change cache nodes for
> suspend/resume operation too, let alone hotplug operations. So, we can't
> assume the same CPUs with exact same caches are always hotplugged back
> in. That's may happen in embedded and other static platforms but not
> everywhere. Anyway that's my understand as why it's done in hotplug
> patch. So it's not so simple to call it inefficient and unnecessary IMO.

Okay, I see.

>
> > If you avoid this, would that solve this problem?
> >
>
> May be, but as mentioned above we can't really. Also this change will
> help to avoid creating unnecessary power sysfs which is mainly runtime
> pm related for some of the devices created. CPU/caches was just one
> example which triggered this, but this can be more useful. We can avoid
> adding them to dpm list.

Well, to me the approach you suggest sounds prone to errors and I am
afraid people may abuse it. Moreover, I don't know if there is other
problems with it, let's see what Rafael thinks about it.

Instead I think we should make the PM core to deal with this scenario,
as all it boils down to, is to allow a device to be unregistered and
registered during system suspend/resume, with a parent device that is
"persistent" during the sequence.

Perhaps we could even just drop the corresponding printed warning,
"cache: parent cpu1 should not be sleeping", in device_pm_add() as I
wonder if it's really a necessary print.

Kind regards
Uffe
Sudeep Holla Feb. 7, 2019, 3:06 p.m. UTC | #5
On Thu, Feb 07, 2019 at 03:29:07PM +0100, Ulf Hansson wrote:
> On Thu, 7 Feb 2019 at 11:36, Sudeep Holla <sudeep.holla@arm.com> wrote:

[..]

> >
> > May be, but as mentioned above we can't really. Also this change will
> > help to avoid creating unnecessary power sysfs which is mainly runtime
> > pm related for some of the devices created. CPU/caches was just one
> > example which triggered this, but this can be more useful. We can avoid
> > adding them to dpm list.
>
> Well, to me the approach you suggest sounds prone to errors and I am
> afraid people may abuse it. Moreover, I don't know if there is other
> problems with it, let's see what Rafael thinks about it.
>

Sorry, I should have put reference to earlier discussion that led to this
patch. For your reference: [1]

> Instead I think we should make the PM core to deal with this scenario,
> as all it boils down to, is to allow a device to be unregistered and
> registered during system suspend/resume, with a parent device that is
> "persistent" during the sequence.
>

OK

> Perhaps we could even just drop the corresponding printed warning,
> "cache: parent cpu1 should not be sleeping", in device_pm_add() as I
> wonder if it's really a necessary print.
>
Indeed, I was ignoring knowing that it's harmless. But more people
started to complain, and Rafael suggested this which I agree as we
have several pseudo devices created in the kernel that we can bypass
some of these pm handling knowing we won't need it.

--
Regards,
Sudeep

[1] https://lkml.org/lkml/2019/1/30/1078
Ulf Hansson Feb. 7, 2019, 3:18 p.m. UTC | #6
On Thu, 7 Feb 2019 at 16:06, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Feb 07, 2019 at 03:29:07PM +0100, Ulf Hansson wrote:
> > On Thu, 7 Feb 2019 at 11:36, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> [..]
>
> > >
> > > May be, but as mentioned above we can't really. Also this change will
> > > help to avoid creating unnecessary power sysfs which is mainly runtime
> > > pm related for some of the devices created. CPU/caches was just one
> > > example which triggered this, but this can be more useful. We can avoid
> > > adding them to dpm list.
> >
> > Well, to me the approach you suggest sounds prone to errors and I am
> > afraid people may abuse it. Moreover, I don't know if there is other
> > problems with it, let's see what Rafael thinks about it.
> >
>
> Sorry, I should have put reference to earlier discussion that led to this
> patch. For your reference: [1]

Yeah, that would have been nice. :-)

>
> > Instead I think we should make the PM core to deal with this scenario,
> > as all it boils down to, is to allow a device to be unregistered and
> > registered during system suspend/resume, with a parent device that is
> > "persistent" during the sequence.
> >
>
> OK
>
> > Perhaps we could even just drop the corresponding printed warning,
> > "cache: parent cpu1 should not be sleeping", in device_pm_add() as I
> > wonder if it's really a necessary print.
> >
> Indeed, I was ignoring knowing that it's harmless. But more people
> started to complain, and Rafael suggested this which I agree as we
> have several pseudo devices created in the kernel that we can bypass
> some of these pm handling knowing we won't need it.

Okay, I see.

Anyway, I will likely need to restore part of this change, via my
cluster idling series then. As from that point, the cpu device that
you call device_set_pm_not_required() for, starts to be used from both
PM core and runtime PM point of view. But I guess that's okay then.

Kind regards
Uffe
Sudeep Holla Feb. 7, 2019, 3:29 p.m. UTC | #7
On Thu, Feb 07, 2019 at 04:18:57PM +0100, Ulf Hansson wrote:
> On Thu, 7 Feb 2019 at 16:06, Sudeep Holla <sudeep.holla@arm.com> wrote:

[...]

> > Indeed, I was ignoring knowing that it's harmless. But more people
> > started to complain, and Rafael suggested this which I agree as we
> > have several pseudo devices created in the kernel that we can bypass
> > some of these pm handling knowing we won't need it.
>
> Okay, I see.
>
> Anyway, I will likely need to restore part of this change, via my
> cluster idling series then. As from that point, the cpu device that
> you call device_set_pm_not_required() for, starts to be used from both
> PM core and runtime PM point of view. But I guess that's okay then.
>

Ah I see. I can drop for CPU devices then. Since I didn't see any use for
them, I set the flag, but I can drop it now or you can do that as part
of that series. There are quite a few devices(especially the ones
registered under system subsys can set this but I would take it separate
once we settle on this). Also Rafael may have seen use for few more
devices when he suggested this.

--
Regards,
Sudeep
Ulf Hansson Feb. 11, 2019, 4:20 p.m. UTC | #8
On Thu, 7 Feb 2019 at 16:29, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Feb 07, 2019 at 04:18:57PM +0100, Ulf Hansson wrote:
> > On Thu, 7 Feb 2019 at 16:06, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> [...]
>
> > > Indeed, I was ignoring knowing that it's harmless. But more people
> > > started to complain, and Rafael suggested this which I agree as we
> > > have several pseudo devices created in the kernel that we can bypass
> > > some of these pm handling knowing we won't need it.
> >
> > Okay, I see.
> >
> > Anyway, I will likely need to restore part of this change, via my
> > cluster idling series then. As from that point, the cpu device that
> > you call device_set_pm_not_required() for, starts to be used from both
> > PM core and runtime PM point of view. But I guess that's okay then.
> >
>
> Ah I see. I can drop for CPU devices then. Since I didn't see any use for
> them, I set the flag, but I can drop it now or you can do that as part
> of that series.

Well, I prefer if you drop it for CPU devices, as least for now.

> There are quite a few devices(especially the ones
> registered under system subsys can set this but I would take it separate
> once we settle on this). Also Rafael may have seen use for few more
> devices when he suggested this.

Yep, let's find another first user of this.

Additionally, it seems like we should drop the print in device_pm_add().

Kind regards
Uffe
Sudeep Holla Feb. 12, 2019, 5:49 p.m. UTC | #9
On Mon, Feb 11, 2019 at 05:20:20PM +0100, Ulf Hansson wrote:
> On Thu, 7 Feb 2019 at 16:29, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Feb 07, 2019 at 04:18:57PM +0100, Ulf Hansson wrote:
> > > On Thu, 7 Feb 2019 at 16:06, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > [...]
> >
> > > > Indeed, I was ignoring knowing that it's harmless. But more people
> > > > started to complain, and Rafael suggested this which I agree as we
> > > > have several pseudo devices created in the kernel that we can bypass
> > > > some of these pm handling knowing we won't need it.
> > >
> > > Okay, I see.
> > >
> > > Anyway, I will likely need to restore part of this change, via my
> > > cluster idling series then. As from that point, the cpu device that
> > > you call device_set_pm_not_required() for, starts to be used from both
> > > PM core and runtime PM point of view. But I guess that's okay then.
> > >
> >
> > Ah I see. I can drop for CPU devices then. Since I didn't see any use for
> > them, I set the flag, but I can drop it now or you can do that as part
> > of that series.
>
> Well, I prefer if you drop it for CPU devices, as least for now.
>
> > There are quite a few devices(especially the ones
> > registered under system subsys can set this but I would take it separate
> > once we settle on this). Also Rafael may have seen use for few more
> > devices when he suggested this.
>
> Yep, let's find another first user of this.
>
> Additionally, it seems like we should drop the print in device_pm_add().
>

Hi Rafael,

Do you prefer to drop the error message or retain it as is ? With this
patch, we don't have to. I will repost v2 dropping this flags for cpus
and just retaining the cache nodes for now.

Regards,
Sudeep
Rafael J. Wysocki Feb. 12, 2019, 6:07 p.m. UTC | #10
On Tue, Feb 12, 2019 at 6:49 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Mon, Feb 11, 2019 at 05:20:20PM +0100, Ulf Hansson wrote:
> > On Thu, 7 Feb 2019 at 16:29, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Feb 07, 2019 at 04:18:57PM +0100, Ulf Hansson wrote:
> > > > On Thu, 7 Feb 2019 at 16:06, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > [...]
> > >
> > > > > Indeed, I was ignoring knowing that it's harmless. But more people
> > > > > started to complain, and Rafael suggested this which I agree as we
> > > > > have several pseudo devices created in the kernel that we can bypass
> > > > > some of these pm handling knowing we won't need it.
> > > >
> > > > Okay, I see.
> > > >
> > > > Anyway, I will likely need to restore part of this change, via my
> > > > cluster idling series then. As from that point, the cpu device that
> > > > you call device_set_pm_not_required() for, starts to be used from both
> > > > PM core and runtime PM point of view. But I guess that's okay then.
> > > >
> > >
> > > Ah I see. I can drop for CPU devices then. Since I didn't see any use for
> > > them, I set the flag, but I can drop it now or you can do that as part
> > > of that series.
> >
> > Well, I prefer if you drop it for CPU devices, as least for now.
> >
> > > There are quite a few devices(especially the ones
> > > registered under system subsys can set this but I would take it separate
> > > once we settle on this). Also Rafael may have seen use for few more
> > > devices when he suggested this.
> >
> > Yep, let's find another first user of this.
> >
> > Additionally, it seems like we should drop the print in device_pm_add().
> >
>
> Hi Rafael,
>
> Do you prefer to drop the error message or retain it as is ?

Do you mean the one in device_pm_add()?

> With this patch, we don't have to. I will repost v2 dropping this flags for cpus
> and just retaining the cache nodes for now.

OK
Sudeep Holla Feb. 12, 2019, 6:20 p.m. UTC | #11
On Tue, Feb 12, 2019 at 07:07:31PM +0100, Rafael J. Wysocki wrote:
> On Tue, Feb 12, 2019 at 6:49 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Mon, Feb 11, 2019 at 05:20:20PM +0100, Ulf Hansson wrote:
> > > On Thu, 7 Feb 2019 at 16:29, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > On Thu, Feb 07, 2019 at 04:18:57PM +0100, Ulf Hansson wrote:
> > > > > On Thu, 7 Feb 2019 at 16:06, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > [...]
> > > >
> > > > > > Indeed, I was ignoring knowing that it's harmless. But more people
> > > > > > started to complain, and Rafael suggested this which I agree as we
> > > > > > have several pseudo devices created in the kernel that we can bypass
> > > > > > some of these pm handling knowing we won't need it.
> > > > >
> > > > > Okay, I see.
> > > > >
> > > > > Anyway, I will likely need to restore part of this change, via my
> > > > > cluster idling series then. As from that point, the cpu device that
> > > > > you call device_set_pm_not_required() for, starts to be used from both
> > > > > PM core and runtime PM point of view. But I guess that's okay then.
> > > > >
> > > >
> > > > Ah I see. I can drop for CPU devices then. Since I didn't see any use for
> > > > them, I set the flag, but I can drop it now or you can do that as part
> > > > of that series.
> > >
> > > Well, I prefer if you drop it for CPU devices, as least for now.
> > >
> > > > There are quite a few devices(especially the ones
> > > > registered under system subsys can set this but I would take it separate
> > > > once we settle on this). Also Rafael may have seen use for few more
> > > > devices when he suggested this.
> > >
> > > Yep, let's find another first user of this.
> > >
> > > Additionally, it seems like we should drop the print in device_pm_add().
> > >
> >
> > Hi Rafael,
> >
> > Do you prefer to drop the error message or retain it as is ?
>
> Do you mean the one in device_pm_add()?
>

Yes, the warning "parent should not be sleeping" in device_pm_add(). I
would prefer to keep it as is for now. Just asking since Ulf was suggesting
to drop them.

Regards,
Sudeep
Rafael J. Wysocki Feb. 12, 2019, 6:59 p.m. UTC | #12
On Tue, Feb 12, 2019 at 7:20 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Feb 12, 2019 at 07:07:31PM +0100, Rafael J. Wysocki wrote:
> > On Tue, Feb 12, 2019 at 6:49 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Mon, Feb 11, 2019 at 05:20:20PM +0100, Ulf Hansson wrote:
> > > > On Thu, 7 Feb 2019 at 16:29, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > >
> > > > > On Thu, Feb 07, 2019 at 04:18:57PM +0100, Ulf Hansson wrote:
> > > > > > On Thu, 7 Feb 2019 at 16:06, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > >
> > > > > [...]
> > > > >
> > > > > > > Indeed, I was ignoring knowing that it's harmless. But more people
> > > > > > > started to complain, and Rafael suggested this which I agree as we
> > > > > > > have several pseudo devices created in the kernel that we can bypass
> > > > > > > some of these pm handling knowing we won't need it.
> > > > > >
> > > > > > Okay, I see.
> > > > > >
> > > > > > Anyway, I will likely need to restore part of this change, via my
> > > > > > cluster idling series then. As from that point, the cpu device that
> > > > > > you call device_set_pm_not_required() for, starts to be used from both
> > > > > > PM core and runtime PM point of view. But I guess that's okay then.
> > > > > >
> > > > >
> > > > > Ah I see. I can drop for CPU devices then. Since I didn't see any use for
> > > > > them, I set the flag, but I can drop it now or you can do that as part
> > > > > of that series.
> > > >
> > > > Well, I prefer if you drop it for CPU devices, as least for now.
> > > >
> > > > > There are quite a few devices(especially the ones
> > > > > registered under system subsys can set this but I would take it separate
> > > > > once we settle on this). Also Rafael may have seen use for few more
> > > > > devices when he suggested this.
> > > >
> > > > Yep, let's find another first user of this.
> > > >
> > > > Additionally, it seems like we should drop the print in device_pm_add().
> > > >
> > >
> > > Hi Rafael,
> > >
> > > Do you prefer to drop the error message or retain it as is ?
> >
> > Do you mean the one in device_pm_add()?
> >
>
> Yes, the warning "parent should not be sleeping" in device_pm_add(). I
> would prefer to keep it as is for now.

OK, do that, then. :-)

Patch
diff mbox series

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index eb9443d5bae1..ccec353aa24c 100644
--- a/drivers/base/cpu.c
+++ b/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;
+	device_set_pm_not_required(&cpu->dev);
 	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;
+	device_set_pm_not_required(dev);
 	dev_set_drvdata(dev, drvdata);
 
 	retval = kobject_set_name_vargs(&dev->kobj, fmt, args);
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 0992e67e862b..2a29c3d4e240 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -124,6 +124,10 @@  void device_pm_unlock(void)
  */
 void device_pm_add(struct device *dev)
 {
+	/* No need to create pm sysfs if explicitly specified as not required */
+	if (device_pm_not_required(dev))
+		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 +146,9 @@  void device_pm_add(struct device *dev)
  */
 void device_pm_remove(struct device *dev)
 {
+	if (device_pm_not_required(dev))
+		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 a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index d713738ce796..6cd159b51114 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -648,6 +648,10 @@  int dpm_sysfs_add(struct device *dev)
 {
 	int rc;
 
+	/* No need to create pm sysfs if explicitly disabled */
+	if (device_pm_not_required(dev))
+		return 0;
+
 	rc = sysfs_create_group(&dev->kobj, &pm_attr_group);
 	if (rc)
 		return rc;
diff --git a/include/linux/device.h b/include/linux/device.h
index 6cb4640b6160..739d0b62e4d4 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1165,6 +1165,16 @@  static inline bool device_async_suspend_enabled(struct device *dev)
 	return !!dev->power.async_suspend;
 }
 
+static inline bool device_pm_not_required(struct device *dev)
+{
+	return dev->power.no_pm_required;
+}
+
+static inline void device_set_pm_not_required(struct device *dev)
+{
+	dev->power.no_pm_required = true;
+}
+
 static inline void dev_pm_syscore_device(struct device *dev, bool val)
 {
 #ifdef CONFIG_PM_SLEEP
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 0bd9de116826..300ab9f0b858 100644
--- a/include/linux/pm.h
+++ b/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;