Re: [PATCH v3 2/2] drm/dp_mst: Track allocated_pbn from ALLOCATE_PAYLOAD down-reply

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/dp_mst: data race on allocated_pbn can bypass bandwidth limits
- [Medium] drm/dp_mst: bandwidth check fails on shrinking payloads due to stale allocated_pbn
--

commit d52065aed1fa389dfd394c9d3cae48a882973e04
Author: Xiao Lu <xiaolu.xie@intel.com>

drm/dp_mst: Track allocated_pbn from ALLOCATE_PAYLOAD down-reply

This commit stores the reported PBN from an ALLOCATE_PAYLOAD reply. It uses
the larger of the requested and allocated PBN for bandwidth checks to account
for constraints applied by intermediate branches along the path.

> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 7ce9e212770ad..86a42c5e2debb 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c

[ ... ]

> @@ -3166,10 +3167,14 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
>  	 */
>  	ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
>  	if (ret > 0) {
> -		if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK)
> +		if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK) {
>  			ret = -EINVAL;
> -		else
> +		} else {
> +			if (payload)
> +				payload->allocated_pbn =
> +					txmsg->reply.u.allocate_payload.allocated_pbn;

[Severity: High]
Can this lockless write to payload->allocated_pbn race with an atomic check?

If a non-blocking atomic commit is executing asynchronously, it receives the
ALLOCATE_PAYLOAD reply and drm_dp_payload_send_msg() updates allocated_pbn
without locks. Concurrently, if userspace initiates a new atomic check,
drm_dp_mst_duplicate_state() will copy the payload struct via kmemdup().

Could this result in the atomic check reading a torn or stale value when
evaluating max(payload->pbn, payload->allocated_pbn) in
drm_dp_mst_atomic_check_port_bw_limit(), potentially bypassing bandwidth
constraints?

>  			ret = 0;
> +		}
>  	}
>  	kfree(txmsg);
>  fail_put:

[ ... ]

> @@ -4575,6 +4580,7 @@ int drm_dp_atomic_release_time_slots(struct drm_atomic_commit *state,
>  	drm_dbg_atomic(mgr->dev, "[MST PORT:%p] TU %d -> 0\n", port, payload->time_slots);
>  	if (!payload->delete) {
>  		payload->pbn = 0;
> +		payload->allocated_pbn = 0;

[Severity: Medium]
Does this properly handle payloads that shrink in size?

Since payload->allocated_pbn is only cleared here when releasing the payload
entirely (during a full deletion), what happens if a commit simply shrinks an
existing payload (for example, switching to a smaller display mode on the
same CRTC)?

In that scenario, drm_dp_atomic_release_time_slots() skips active CRTCs,
meaning allocated_pbn remains at its old, larger value. When the new
bandwidth is checked, max(payload->pbn, payload->allocated_pbn) in
drm_dp_mst_atomic_check_port_bw_limit() would yield the old larger value,
which could falsely prevent the freed bandwidth from being reallocated to
other displays.

>  		payload->delete = true;
>  		if (payload->vcpi > 0)
>  			topology_state->payload_mask &= ~BIT(payload->vcpi - 1);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924094316.822211-1-xiaolu.xie@intel.com?part=2