Re: [PATCH v1] PM: sleep: core: Restrict power.set_active propagation

[Johan Hovold <johan@kernel.org>]

On Sat, Feb 08, 2025 at 06:54:28PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Commit 3775fc538f53 ("PM: sleep: core: Synchronize runtime PM status of
> parents and children") exposed an issue related to simple_pm_bus_pm_ops
> that uses pm_runtime_force_suspend() and pm_runtime_force_resume() as
> bus type PM callbacks for the noirq phases of system-wide suspend and

Despite the name of the driver these are plain device driver PM
callbacks (not bus PM ops).

> resume.
> 
> The problem is that pm_runtime_force_suspend() does not distinguish
> runtime-suspended devices from devices for which runtime PM has never
> been enabled, so if it sees a device with runtime PM status set to
> RPM_ACTIVE, it will assume that runtime PM is enabled for that device
> and so it will attempt to suspend it with the help of its runtime PM
> callbacks which may not be ready for that.  As it turns out, this 
> causes simple_pm_bus_runtime_suspend() to crash due to a NULL pointer
> dereference.
> 
> Another problem related to the above commit and simple_pm_bus_pm_ops is
> that setting runtime PM status of a device handled by the latter to
> RPM_ACTIVE will actually prevent it from being resumed because
> pm_runtime_force_resume() only resumes devices with runtime PM status
> set to RPM_SUSPENDED.
> 
> To mitigate these issues, do not allow power.set_active to propagate
> beyond the parent of the device with DPM_FLAG_SMART_SUSPEND set that
> will need to be resumed, which should be a sufficient stop-gap for the
> time being, but they will need to be properly addressed in the future
> because in general during system-wide resume it is necessary to resume
> all devices in a dependency chain in which at least one device is going
> to be resumed.

So this works as long as no parent of a device with
DPM_FLAG_SMART_SUSPEND set is using pm_runtime_force_resume().

This is the case in the systems I work on, but have you
verified that this is currently generally true? Not many drivers use
this flag, but it all depends on what their devices' parents' drivers
do:

	drivers/acpi/acpi_tad.c
	drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
	drivers/i2c/busses/i2c-designware-platdrv.c
	drivers/mfd/intel-lpss.c
	drivers/pci/pcie/portdrv.c
	drivers/pwm/pwm-lpss-platform.c
	drivers/soundwire/intel_auxdevice.c

Most of these look like ACPI drivers so nothing that would sit directly
on a simple-pm-bus at least.

> Fixes: 3775fc538f53 ("PM: sleep: core: Synchronize runtime PM status of parents and children")
> Closes: https://lore.kernel.org/linux-pm/1c2433d4-7e0f-4395-b841-b8eac7c25651@nvidia.com/
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Tested-by: Johan Hovold <johan+linaro@kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Johan