Re: [PATCH 11/13] 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 2 potential issue(s) to consider:
- [High] drm/amd/display: unsynchronized runtime PM check in dm_dp_aux_transfer()
- [Medium] drm/amd/display: NULL adev->dev dereference in dm_dp_aux_transfer() KUnit
--

commit 06da2e04484d301378e2740f320dbda5681bdad8
Author: Andre Eikmeyer <dev@deq.rocks>

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

This commit adds a runtime PM reference and checks adev->gmux_off in
dm_dp_aux_transfer() to refuse DP AUX transfers while GMUX keeps the GPU
powered off.

> 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..b099bf5206734 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
> +	 * without a runtime PM reference. Refuse the transfer while GMUX keeps
> +	 * the GPU powered off, a register access then hangs the PEG link.
> +	 */
> +	pm_runtime_get_noresume(adev->dev);

[Severity: Medium]
Can this dereference a NULL adev->dev pointer when running the
amdgpu_dm_mst_types_test KUnit suite with CONFIG_PM enabled?

In drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c,
the KUnit test cases (such as dm_mst_test_aux_transfer_native_read() and
dm_mst_test_aux_transfer_native_write()) allocate a mock struct
amdgpu_device via kunit_kzalloc() and wire it through
dm_mst_test_setup_dm_aux() without initializing adev->dev:

drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c:
dm_mst_test_aux_transfer_native_read() {
	...
	dm_aux = kunit_kzalloc(test, sizeof(*dm_aux), GFP_KERNEL);
	adev = kunit_kzalloc(test, sizeof(*adev), GFP_KERNEL);
	...
	dm_mst_test_setup_dm_aux(dm_aux, ddc, link, dc, link_srv, ctx, adev);

	ret = drm_dp_dpcd_read(&dm_aux->aux, 4, buffer, sizeof(buffer));
	...
}

When drm_dp_dpcd_read() invokes dm_dp_aux_transfer(), passing adev->dev
(which is NULL) to pm_runtime_get_noresume(), pm_runtime_put_noidle(), or
pm_runtime_put_autosuspend() will dereference a NULL pointer.

> +	if (READ_ONCE(adev->gmux_off)) {
> +		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);
>  
> +	pm_runtime_put_autosuspend(adev->dev);

[Severity: High]
Does pm_runtime_get_noresume() combined with checking adev->gmux_off prevent
dm_dp_aux_transfer() from accessing hardware registers while the GPU is
suspending, resuming, or runtime-suspended under other runtime PM modes?

In dm_dp_aux_transfer(), pm_runtime_get_noresume(adev->dev) only increments
dev->power.usage_count without acquiring dev->power.lock, checking
dev->power.runtime_status, or waiting on an in-flight RPM_SUSPENDING or
RPM_RESUMING transition.

In rpm_suspend(), the PM core checks usage_count before setting
runtime_status to RPM_SUSPENDING and invoking the driver callback. In
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c, amdgpu_pmops_runtime_suspend() only
sets adev->gmux_off at the very end of the function, after
amdgpu_device_suspend(), pci_disable_device(), and
pci_set_power_state(pdev, PCI_D3cold) have already run:

drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c:amdgpu_pmops_runtime_suspend() {
	...
		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;
	} else if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO) {
		/* nothing to do */
	} else if ((adev->pm.rpm_mode == AMDGPU_RUNPM_BACO) ||
			(adev->pm.rpm_mode == AMDGPU_RUNPM_BAMACO)) {
		amdgpu_device_baco_enter(adev);
	}

	if (adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX)
		WRITE_ONCE(adev->gmux_off, true);
	...
}

And in amdgpu_pmops_runtime_resume(), adev->gmux_off is cleared at the very
beginning of the callback before PCI state is restored to PCI_D0,
pci_enable_device() is called, and amdgpu_device_resume() reinitializes the
device:

drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c:amdgpu_pmops_runtime_resume() {
	...
	WRITE_ONCE(adev->gmux_off, false);

	/* Avoids registers access if device is physically gone */
	if (!pci_device_is_present(adev->pdev))
		adev->no_hw_access = true;

	if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX ||
	    adev->pm.rpm_mode == AMDGPU_RUNPM_GMUX) {
		if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX)
			drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;

		/* Only need to handle PCI state in the driver for ATPX and GMUX.
		 * PCI core handles it for _PR3.
		 */
		pci_set_power_state(pdev, PCI_D0);
		amdgpu_device_load_pci_state(pdev);
		ret = pci_enable_device(pdev);
	...
}

If userspace reads /dev/drm_dp_aux* concurrently with
amdgpu_pmops_runtime_suspend() or amdgpu_pmops_runtime_resume(),
dm_dp_aux_transfer() can observe adev->gmux_off == false and call
dc_link_aux_transfer_raw() while the PCI device is disabled or in
PCI_D3cold, or while vga_switcheroo_runtime_suspend() is cutting GMUX power.

In addition, on non-GMUX runtime PM configurations (such as
AMDGPU_RUNPM_PX, AMDGPU_RUNPM_BOCO, AMDGPU_RUNPM_BACO, or
AMDGPU_RUNPM_BAMACO), adev->gmux_off remains false while the GPU is runtime
suspended, and pm_runtime_get_noresume() will not wake the device before
dc_link_aux_transfer_raw() accesses AUX registers.

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