Re: [PATCH v2 11/12] drm/amdgpu: Skip AC/DC notification while the GPU is runtime suspended

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] drm/amdgpu: missed AC/DC SMU update in
  amdgpu_pm_acpi_event_handler() race
- [Medium] drm/amd/pm: negative pm_runtime_get_if_active() treated as
  suspended
- [Medium] drm/amdgpu: missing SMU7 AC notification on resume in
  hwmgr_resume()
--

commit 2d835db57775c3299d3a931908d4d082aae8da4d
Author: Andre Eikmeyer <dev@deq.rocks>

drm/amdgpu: Skip AC/DC notification while the GPU is runtime suspended

This commit acquires a runtime PM reference with pm_runtime_get_if_active() in
amdgpu_pm_acpi_event_handler() and skips notifying the SMU of AC/DC changes
when the device is not runtime active.

> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> index 808be6c425bff..3c6dde3868006 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c

[ ... ]

> @@ -498,19 +499,27 @@ int amdgpu_dpm_smu_i2c_bus_access(struct amdgpu_device *adev,
>  
>  void amdgpu_pm_acpi_event_handler(struct amdgpu_device *adev)
>  {
> +	int active = 0;
> +
>  	if (adev->pm.dpm_enabled) {
>  		mutex_lock(&adev->pm.mutex);
> +		active = pm_runtime_get_if_active(adev->dev);
> +
>  		if (power_supply_is_system_supplied() > 0)
>  			adev->pm.ac_power = true;
>  		else
>  			adev->pm.ac_power = false;
>  
> -		if (adev->powerplay.pp_funcs &&
> -		    adev->powerplay.pp_funcs->notify_ac_dc)
> -			amdgpu_dpm_notify_ac_dc(adev);
> +		if (active > 0) {

[Severity: Medium]
Can an AC/DC event racing with runtime resume or an aborted runtime suspend
drop the SMU power-source update?

During rpm_resume(), dev->power.runtime_status stays RPM_RESUMING until
amdgpu_pmops_runtime_resume() returns, so pm_runtime_get_if_active() returns
0 (or -EINVAL while hotplug probing increments disable_depth):

CPU0                                  CPU1
rpm_resume()
  dev->power.runtime_status = RPM_RESUMING
  amdgpu_pmops_runtime_resume()
    amdgpu_device_resume()
      amdgpu_device_ip_late_init()
        smu_late_init()
          smu_set_ac_dc()
                                      amdgpu_pm_acpi_event_handler()
                                        pm_runtime_get_if_active() -> 0
                                        adev->pm.ac_power = ...
                                        /* skips smu_set_ac_dc() */
      flush_delayed_work()
  dev->power.runtime_status = RPM_ACTIVE
                                        <-- GPU runs with stale AC/DC state

The same drop occurs if the event arrives during rpm_suspend() while
runtime_status is RPM_SUSPENDING and amdgpu_pmops_runtime_suspend() later
aborts with -EBUSY, which reverts runtime_status to RPM_ACTIVE without
running resume.  In addition, hwmgr_resume() and hwmgr_suspend() invoke
smu7_apply_state_adjust_rules() and vega10_apply_state_adjust_rules()
without holding adev->pm.mutex, reading adev->pm.ac_power twice while
amdgpu_pm_acpi_event_handler() can update it concurrently.

[Severity: Medium]
Does checking active > 0 also skip AC/DC notifications when runtime PM or
CONFIG_PM is disabled?

pm_runtime_get_if_active() returns -EINVAL when CONFIG_PM is disabled or
when runtime PM is disabled on adev->dev (disable_depth > 0).  With
active > 0, amdgpu_pm_acpi_event_handler() treats -EINVAL as suspended and
never notifies the SMU or DPM even though the GPU is active and no runtime
resume will follow.  Other callers in this subsystem such as
amdgpu_pm_get_access_if_active() check active != 0 to allow hardware access
and only call pm_runtime_put() when active > 0.

> +			if (adev->powerplay.pp_funcs &&
> +			    adev->powerplay.pp_funcs->notify_ac_dc)
> +				amdgpu_dpm_notify_ac_dc(adev);

[Severity: Medium]
If AC power is connected while an SMU7 GPU is runtime suspended, will the
resume path ever send PPSMC_MSG_RunningOnAC to the SMU?

On SMU7 GPUs with PHM_PlatformCaps_AutomaticDCTransition, smu7_notify_ac_dc()
is the only place that sends PPSMC_MSG_RunningOnAC when adev->pm.ac_power is
true:

drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_hwmgr.c:smu7_notify_ac_dc() {
	...
	if (adev->pm.ac_power &&
	    phm_cap_enabled(hwmgr->platform_descriptor.platformCaps,
			    PHM_PlatformCaps_AutomaticDCTransition))
		smum_send_msg_to_smc(hwmgr, PPSMC_MSG_RunningOnAC, NULL);
	...
}

Unlike swSMU, which calls smu_set_ac_dc() in smu_late_init(), neither
hwmgr_resume() nor pp_late_init() invokes notify_ac_dc() or sends
PPSMC_MSG_RunningOnAC.  When SMU firmware stays running across BACO runtime
suspend, skipping amdgpu_dpm_notify_ac_dc() here leaves the SMU in DC mode
after resume.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114160731.3756-1-atharvatiwarilinuxdev@gmail.com?part=11