Re: [PATCH v2 10/12] drm/amd/display: Refuse DP AUX while GMUX keeps the GPU off

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/amd/display: unsynchronized runtime PM check in
  dm_dp_aux_transfer()
--

commit 7acdd21f6d25e531ba11673cecccfbbda8f8f481
Author: Andre Eikmeyer <dev@deq.rocks>

drm/amd/display: Refuse DP AUX while GMUX keeps the GPU off

This commit adds a runtime PM check around dc_link_aux_transfer_raw() in
dm_dp_aux_transfer() using pm_runtime_get_noresume() and adev->runpm_active.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 7261317d4c0f4..0d11a9443ca01 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
[ ... ]
> @@ -111,6 +112,20 @@ STATIC_IFN_KUNIT ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux,
>  	if (WARN_ON(msg->size > 16))
>  		return -E2BIG;
>  
> +	ddc = TO_DM_AUX(aux)->ddc_service;
> +	adev = ddc->ctx->driver_context;
> +
> +	/*
> +	 * User space reaches this through the drm_dp_aux_dev character device
> +	 * check if the gpu is active or not.
> +	 */
> +	if (adev->dev) {
> +		pm_runtime_get_noresume(adev->dev);
> +		if (!READ_ONCE(adev->runpm_active)) {
> +			pm_runtime_put_noidle(adev->dev);
> +			return -EBUSY;
> +		}
> +	}
>  	payload.address = msg->address;
>  	payload.data = msg->buffer;
>  	payload.length = msg->size;
> @@ -126,13 +141,13 @@ STATIC_IFN_KUNIT ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux,
>  	result = dc_link_aux_transfer_raw(TO_DM_AUX(aux)->ddc_service, &payload,
>  				      &operation_result);
>  
> +	if (adev->dev)
> +		pm_runtime_put_autosuspend(adev->dev);

[Severity: High]
As reported by Alex Deucher on v1 [1], why gate this on GMUX-specific
adev->runpm_active and pm_runtime_get_noresume() instead of waking or
synchronizing with runtime PM across all runtime PM modes?

pm_runtime_get_noresume() only increments dev->power.usage_count without
checking runtime_status or waiting on in-progress suspend or resume
transitions, and adev->runpm_active is only cleared at the very end of
amdgpu_pmops_runtime_suspend() when rpm_mode == AMDGPU_RUNPM_GMUX:

drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c:amdgpu_pmops_runtime_suspend() {
	...
	if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX ||
	    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX) {
		amdgpu_device_cache_pci_state(pdev);
		pci_disable_device(pdev);
		pci_ignore_hotplug(pdev);
		pci_set_power_state(pdev, PCI_D3cold);
		drm_dev->switch_power_state = DRM_SWITCH_POWER_DYNAMIC_OFF;
	...
	if (adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX)
		WRITE_ONCE(adev->runpm_active, false);
}

Once rpm_suspend() sets runtime_status to RPM_SUSPENDING, a concurrent
/dev/drm_dp_aux* transfer still races with PCI disable and GMUX power-off:

CPU0                                  CPU1
rpm_suspend()
  runtime_status = RPM_SUSPENDING
  vga_switcheroo_runtime_suspend()
    amdgpu_pmops_runtime_suspend()
      amdgpu_device_suspend()
      pci_disable_device(pdev)
      pci_set_power_state(..., PCI_D3cold)
                                      aux_dev_read_iter()
                                        ...
                                          dm_dp_aux_transfer()
                                            pm_runtime_get_noresume()
                                            READ_ONCE(adev->runpm_active)
                                              /* still true */
      WRITE_ONCE(adev->runpm_active, false)
    vga_switcheroo_power_switch(..., OFF)
                                            dc_link_aux_transfer_raw()
                                              /* MMIO on powered-off GPU */

The same race occurs during amdgpu_pmops_runtime_resume(), which sets
adev->runpm_active = true at the start of the callback before PCI power
state, BAR state, and DMUB are restored.

On non-GMUX systems (PX, BOCO, BACO, BAMACO), adev->runpm_active is never
cleared on runtime suspend, so pm_runtime_get_noresume() leaves the GPU
suspended while allowing dc_link_aux_transfer_raw() to touch powered-down
registers.

In addition, dm_dp_aux_transfer() is called by internal display paths during
dm_suspend() and dm_resume(), where unconditional pm_runtime_get_noresume()
and pm_runtime_put_autosuspend() manipulate usage_count and last_busy in the
middle of a runtime PM transition.

[1] https://lore.kernel.org/r/CADnq5_PRG_O7jy9P+mf7AQLerpfz6up=BejRSPMgYDh9xigm=A@mail.gmail.com

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