Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/dp_mst: drm_dp_remove_payload_part1() returns true when no payload removed --
commit bd40c8f829b5d0efffbe970022942f5fcad0e46a Author: Xiao Lu <[email protected]> drm/dp_mst: Return whether drm_dp_remove_payload_part1() removed the payload This commit changes drm_dp_remove_payload_part1() to return a boolean indicating whether the payload removal was sent to the branch devices, allowing callers to skip generating and waiting for an ACT when nothing was sent. > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c > b/drivers/gpu/drm/display/drm_dp_mst_topology.c > index b7fce0bf8acc5..82c9c53b3fa96 100644 > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c > @@ -3375,8 +3375,14 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1); > * Removes a payload along the virtual channel if it was successfully > allocated. > * After calling this, the driver should set HW to generate ACT and then > switch to new > * payload allocation state. > + * > + * Returns: > + * %true if the port of the payload is in the topology, so the removal was > + * sent to the branch devices and the driver should generate the ACT and wait > + * for it. %false if the port is not in the topology anymore, so nothing was > + * sent to the branch devices and there is no ACT to generate or wait for. > */ > -void drm_dp_remove_payload_part1(struct drm_dp_mst_topology_mgr *mgr, > +bool drm_dp_remove_payload_part1(struct drm_dp_mst_topology_mgr *mgr, > struct drm_dp_mst_topology_state *mst_state, > struct drm_dp_mst_atomic_payload *payload) > { > @@ -3394,6 +3400,8 @@ void drm_dp_remove_payload_part1(struct > drm_dp_mst_topology_mgr *mgr, > payload->vcpi); > > payload->payload_allocation_status = > DRM_DP_MST_PAYLOAD_ALLOCATION_LOCAL; > + > + return send_remove; > } > EXPORT_SYMBOL(drm_dp_remove_payload_part1); [Severity: Medium] Can drm_dp_remove_payload_part1() return true here even when no payload removal was sent to the branch devices or written to the DFP DPCD payload table? In drm_dp_remove_payload_part1(), send_remove is determined solely by drm_dp_mst_port_downstream_of_branch(payload->port, mgr->mst_primary). However, drm_dp_destroy_payload_at_remote_and_dfp() only sends sideband removal and updates the DFP DPCD payload table when payload->payload_allocation_status is DRM_DP_MST_PAYLOAD_ALLOCATION_REMOTE or DRM_DP_MST_PAYLOAD_ALLOCATION_DFP: drivers/gpu/drm/display/drm_dp_mst_topology.c:drm_dp_destroy_payload_at_remote_and_dfp() { /* it's okay for these to fail */ if (payload->payload_allocation_status == DRM_DP_MST_PAYLOAD_ALLOCATION_REMOTE) { drm_dp_payload_send_msg(mgr, payload->port, payload->vcpi, 0); payload->payload_allocation_status = DRM_DP_MST_PAYLOAD_ALLOCATION_DFP; } if (payload->payload_allocation_status == DRM_DP_MST_PAYLOAD_ALLOCATION_DFP) drm_dp_dpcd_write_payload(mgr->aux, payload->vcpi, payload->vc_start_slot, 0); } During stream enable in drm_dp_add_payload_part1(), payload->payload_allocation_status is initialized to DRM_DP_MST_PAYLOAD_ALLOCATION_LOCAL. If drm_dp_mst_topology_get_port_validated(mgr, payload->port) returns NULL (for instance, when a downstream port or sub-branch was unlinked while mgr->mst_primary remained connected) or if drm_dp_create_payload_at_dfp() fails, drm_dp_add_payload_part1() returns early without programming the DFP DPCD payload table, leaving payload->payload_allocation_status as DRM_DP_MST_PAYLOAD_ALLOCATION_LOCAL. When the stream is later disabled, drm_dp_mst_port_downstream_of_branch() walks the malloc-referenced port->parent and mstb->port_parent pointers and returns true as long as mgr->mst_primary is still the same root branch object: drivers/gpu/drm/display/drm_dp_mst_topology.c:drm_dp_mst_port_downstream_of_branch() { while (port->parent) { if (port->parent == branch) return true; if (port->parent->port_parent) port = port->parent->port_parent; else break; } return false; } Because payload->payload_allocation_status is still DRM_DP_MST_PAYLOAD_ALLOCATION_LOCAL, drm_dp_destroy_payload_at_remote_and_dfp() does nothing, yet drm_dp_remove_payload_part1() returns true and causes callers to generate an ACT and poll DPCD 0x2c0 in drm_dp_check_act_status() until the 3 second timeout expires. Should drm_dp_remove_payload_part1() check payload->payload_allocation_status (or whether the DFP payload table was actually updated) and update its kernel-doc accordingly? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
