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 <[email protected]> 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/[email protected]?part=11
