Re: [PATCH RESEND 1/2] drm/dp_mst: Return whether drm_dp_remove_payload_part1() removed the payload

[sashiko-bot@kernel.org]

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 <xiaolu.xie@intel.com>

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/20261031154456.465469-1-xiaolu.xie@intel.com?part=1