Re: [PATCH v2 09/12] drm/amdgpu: Catch register access while GMUX keeps the GPU off

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/amdgpu: adev->runpm_active initialized after
  amdgpu_virt_init()
- [Medium] drm/amdgpu: premature runpm_active set and missing unwind on
  resume
--

commit 720e36a43938e97cb3f99b288c561e7448e2fbc3
Author: Andre Eikmeyer <dev@deq.rocks>

drm/amdgpu: Catch register access while GMUX keeps the GPU off

This commit adds a runpm_active flag to struct amdgpu_device and checks it
in amdgpu_device_skip_hw_access() and the indirect register accessors to
skip and warn on register accesses while the GPU is runtime suspended via
GMUX.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index ac336594bf055..ed205352f0f0a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -947,6 +947,10 @@ bool amdgpu_device_skip_hw_access(struct amdgpu_device *adev)
>  	if (adev->no_hw_access)
>  		return true;
>  
> +	if (unlikely(!READ_ONCE(adev->runpm_active))) {
> +		WARN_RATELIMIT(1, "%s: register access while powered off\n", dev_name(adev->dev));
> +		return true;
> +	}
>  #ifdef CONFIG_LOCKDEP
>  	/*
>  	 * This is a bit complicated to understand, so worth a comment. What we assert
> @@ -2029,6 +2033,7 @@ static int amdgpu_device_ip_early_init(struct amdgpu_device *adev)
>  	int i, r;
>  
>  	amdgpu_device_enable_virtual_display(adev);
> +	adev->runpm_active = true;
>  
>  	if (amdgpu_sriov_vf(adev)) {
>  		/*

[Severity: High]
Is adev->runpm_active initialized too late during device probe?

In amdgpu_device_init(), amdgpu_virt_init() runs before
amdgpu_device_ip_early_init():

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:amdgpu_device_init() {
	...
	/* detect hw virtualization here */
	amdgpu_virt_init(adev);
	...
	/* early init functions */
	r = amdgpu_device_ip_early_init(adev);
}

Inside amdgpu_virt_init() -> amdgpu_virt_init_detect_asic(), the driver
calls RREG32(mmRCC_IOV_FUNC_IDENTIFIER) (or mmBIF_IOV_FUNC_IDENTIFIER on
Tonga/Fiji).  Because adev is zero-allocated and adev->runpm_active is
still false at that point, amdgpu_device_skip_hw_access() fires a
WARN_RATELIMIT splat on every probe of those GPUs and makes RREG32()
return 0, which also prevents SR-IOV VF detection from setting
AMDGPU_SRIOV_CAPS_IS_VF.

Setting adev->runpm_active = true in amdgpu_device_init() before
amdgpu_virt_init() would avoid this regression.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 3978ed284048f..d1b0c6c7301ea 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2935,6 +2935,9 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
>  		amdgpu_device_baco_enter(adev);
>  	}
>  
> +	if (adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX)
> +		WRITE_ONCE(adev->runpm_active, false);
> +
>  	dev_dbg(&pdev->dev, "asic/device is runtime suspended\n");
>  
>  	return 0;
> @@ -2963,6 +2966,8 @@ static int amdgpu_pmops_runtime_resume(struct device *dev)
>  	if (adev->pm.rpm_mode == AMDGPU_RUNPM_NONE)
>  		return -EINVAL;
>  
> +	WRITE_ONCE(adev->runpm_active, true);
> +
>  	/* Avoids registers access if device is physically gone */
>  	if (!pci_device_is_present(adev->pdev))
>  		adev->no_hw_access = true;

[Severity: Medium]
Should amdgpu_pmops_runtime_resume() set adev->runpm_active only after
restoring PCI power and config state, and clear it again if resume fails?

In the AMDGPU_RUNPM_GMUX branch right below, the device is still in
PCI_D3cold with PCI memory decoding disabled until pci_set_power_state(),
amdgpu_device_load_pci_state(), and pci_enable_device() complete.  If
pci_enable_device() fails, or if amdgpu_device_resume() fails and calls
pci_disable_device(pdev), amdgpu_pmops_runtime_resume() returns an error
with adev->runpm_active left set to true while the PCI device is disabled,
allowing subsequent register accesses to bypass
amdgpu_device_skip_hw_access().

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