Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Permanent memory leak of the initial `drm_dp_mst_branch` device on DPCD write failure. - [Medium] Inconsistent MST state leading to a kernel warning in `drm_dp_mst_topology_queue_probe()` and silent functional breakage. --
commit 169a81a07028ce3444674cdec29dcdd6268008f7 Author: WenTao Liang <[email protected]> drm/display: fix MST branch device refcount leak on DPCD write failure drm_dp_add_mst_branch_device initializes mstb with refcount 1, and drm_dp_mst_topology_get_mstb increments it to 2. When drm_dp_dpcd_write_byte fails, out_unlock performs only one drm_dp_mst_topology_put_mstb, leaving the other reference stored in mgr->mst_primary. Since MST was not successfully enabled, no disable path will clean it up. Suggested-by: Greg KH <[email protected]> Fixes: 7a3cbf590e63 ("drm/mst: Some style improvements in drm_dp_mst_topology_mgr_set_mst()") Cc: [email protected] Signed-off-by: WenTao Liang <[email protected]> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c > b/drivers/gpu/drm/display/drm_dp_mst_topology.c > index 4de36fda05440..6a8c9dc217e53 100644 > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c > @@ -3681,8 +3681,10 @@ int drm_dp_mst_topology_mgr_set_mst(struct > drm_dp_mst_topology_mgr *mgr, bool ms > DP_MST_EN | > DP_UP_REQ_EN | > DP_UPSTREAM_IS_SRC); > - if (ret < 0) > + if (ret < 0) { > + mgr->mst_primary = NULL; [Severity: Medium] Does clearing mgr->mst_primary here cause a permanent memory leak? Since drm_dp_mst_topology_get_mstb() previously incremented the refcount to 2, clearing this pointer without first calling drm_dp_mst_topology_put_mstb() leaves one reference orphaned. When the code jumps to out_unlock, it will drop the remaining reference (down to 1), but the memory will never be freed. Furthermore, any subsequent cleanup calls to drm_dp_mst_topology_mgr_set_mst(..., false) will also fail to clean it up because they expect to find the pointer in mgr->mst_primary. [Severity: Medium] Does this also leave the manager in an inconsistent state? Earlier in drm_dp_mst_topology_mgr_set_mst(), mgr->mst_state is set to true. If the DPCD write fails and we clear mgr->mst_primary without reverting mgr->mst_state to false, we end up with mst_state == true but mst_primary == NULL. If a driver later calls drm_dp_mst_topology_queue_probe(), would it trigger this warning due to the mismatched state? if (drm_WARN_ON(mgr->dev, !mgr->mst_state || !mgr->mst_primary)) Additionally, it seems future attempts to enable MST would silently return success because the early check (mst_state == mgr->mst_state) would evaluate to true without configuring the hardware. > goto out_unlock; > + } > > /* Write reset payload */ > drm_dp_dpcd_clear_payload(mgr->aux); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
