[Public]
-----Original Message----- From: Limonciello, Mario Mario.Limonciello@amd.com Sent: Friday, May 10, 2024 3:18 AM To: Linux regressions mailing list regressions@lists.linux.dev; Wentland, Harry Harry.Wentland@amd.com; Lin, Wayne Wayne.Lin@amd.com Cc: lyude@redhat.com; imre.deak@intel.com; Leon Weiß <leon.weiss@ruhr-uni- bochum.de>; stable@vger.kernel.org; dri-devel@lists.freedesktop.org; amd- gfx@lists.freedesktop.org; intel-gfx@lists.freedesktop.org Subject: Re: [PATCH] drm/mst: Fix NULL pointer dereference at drm_dp_add_payload_part2
On 5/9/2024 07:43, Linux regression tracking (Thorsten Leemhuis) wrote:
On 18.04.24 21:43, Harry Wentland wrote:
On 2024-03-07 01:29, Wayne Lin wrote:
[Why] Commit:
- commit 5aa1dfcdf0a4 ("drm/mst: Refactor the flow for payload
allocation/removement") accidently overwrite the commit
- commit 54d217406afe ("drm: use mgr->dev in drm_dbg_kms in
drm_dp_add_payload_part2") which cause regression.
[How] Recover the original NULL fix and remove the unnecessary input parameter 'state' for drm_dp_add_payload_part2().
Fixes: 5aa1dfcdf0a4 ("drm/mst: Refactor the flow for payload allocation/removement") Reported-by: Leon Weiß leon.weiss@ruhr-uni-bochum.de Link: https://lore.kernel.org/r/38c253ea42072cc825dc969ac4e6b9b600371cc8.c amel@ruhr-uni-bochum.de/ Cc: lyude@redhat.com Cc: imre.deak@intel.com Cc: stable@vger.kernel.org Cc: regressions@lists.linux.dev Signed-off-by: Wayne Lin Wayne.Lin@amd.com
I haven't been deep in MST code in a while but this all looks pretty straightforward and good.
Reviewed-by: Harry Wentland harry.wentland@amd.com
Hmmm, that was three weeks ago, but it seems since then nothing happened to fix the linked regression through this or some other patch. Is there a reason? The build failure report from the CI maybe?
It touches files outside of amd but only has an ack from AMD. I think we /probably/ want an ack from i915 and nouveau to take it through.
Thanks, Mario!
Hi Thorsten, Yeah, like what Mario said. Would also like to have ack from i915 and nouveau.
Wayne Lin, do you know what's up?
Ciao, Thorsten
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 +- drivers/gpu/drm/display/drm_dp_mst_topology.c | 4 +--- drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 +- drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +- include/drm/display/drm_dp_mst_helper.h | 1 - 5 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c index c27063305a13..2c36f3d00ca2 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c @@ -363,7 +363,7 @@ void dm_helpers_dp_mst_send_payload_allocation( mst_state = to_drm_dp_mst_topology_state(mst_mgr->base.state); new_payload = drm_atomic_get_mst_payload_state(mst_state, aconnector->mst_output_port);
- ret = drm_dp_add_payload_part2(mst_mgr, mst_state->base.state,
new_payload);
ret = drm_dp_add_payload_part2(mst_mgr, new_payload);
if (ret) { amdgpu_dm_set_mst_status(&aconnector->mst_status,diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c index 03d528209426..95fd18f24e94 100644 --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c @@ -3421,7 +3421,6 @@
EXPORT_SYMBOL(drm_dp_remove_payload_part2);
/**
- drm_dp_add_payload_part2() - Execute payload update part 2
- @mgr: Manager to use.
- @state: The global atomic state
- @payload: The payload to update
- If @payload was successfully assigned a starting time slot by
drm_dp_add_payload_part1(), this @@ -3430,14 +3429,13 @@
EXPORT_SYMBOL(drm_dp_remove_payload_part2);
- Returns: 0 on success, negative error code on failure.
*/ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
struct drm_atomic_state *state, struct drm_dp_mst_atomic_payload *payload){ int ret = 0;
/* Skip failed payloads */ if (payload->payload_allocation_status !=DRM_DP_MST_PAYLOAD_ALLOCATION_DFP) {
drm_dbg_kms(state->dev, "Part 1 of payload creation for %sfailed, skipping part 2\n",
drm_dbg_kms(mgr->dev, "Part 1 of payload creation for %sfailed,
+skipping part 2\n", payload->port->connector->name); return -EIO; } diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index 53aec023ce92..2fba66aec038 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -1160,7 +1160,7 @@ static void intel_mst_enable_dp(struct
intel_atomic_state *state,
if (first_mst_stream) intel_ddi_wait_for_fec_status(encoder, pipe_config, true);
- drm_dp_add_payload_part2(&intel_dp->mst_mgr, &state->base,
- drm_dp_add_payload_part2(&intel_dp->mst_mgr,
drm_atomic_get_mst_payload_state(mst_state,
connector->port));
if (DISPLAY_VER(dev_priv) >= 12)diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 0c3d88ad0b0e..88728a0b2c25 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -915,7 +915,7 @@ nv50_msto_cleanup(struct drm_atomic_state *state, msto->disabled = false; drm_dp_remove_payload_part2(mgr, new_mst_state,
old_payload, new_payload);
} else if (msto->enabled) {
drm_dp_add_payload_part2(mgr, state, new_payload);
drm_dp_add_payload_part2(mgr, new_payload); msto->enabled = false; }diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h index 9b19d8bd520a..6c9145abc7e2 100644 --- a/include/drm/display/drm_dp_mst_helper.h +++ b/include/drm/display/drm_dp_mst_helper.h @@ -851,7 +851,6 @@ int drm_dp_add_payload_part1(struct
drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_topology_state *mst_state, struct drm_dp_mst_atomic_payload *payload);int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
struct drm_atomic_state *state, struct drm_dp_mst_atomic_payload *payload);*mgr,
struct drm_dp_mst_topology_state *mst_state,
--- Regards, Wayne Lin