Re: [PATCH v7 14/18] drm/bridge: synopsys: dw-dp: Protect sdp_reg_bank from concurrent access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The atomic_disable path indiscriminately wipes the entire shared SDP bitmap, breaking the concurrent access model this patch aims to introduce.
--

--- Patch [14]: [PATCH v7 14/18] drm/bridge: synopsys: dw-dp: Protect sdp_reg_bank from concurrent access ---
commit 20478fb4c1990e713fd92f6e28d07f388653d106
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Protect sdp_reg_bank from concurrent access

This patch introduces a mutex to protect sdp_reg_bank from concurrent access,
preparing the driver for additional SDP users like audio support.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
[ ... ]
> @@ -1691,7 +1695,8 @@ static void dw_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  		return;
>  	}
>  
> -	set_bit(0, dp->sdp_reg_bank);
> +	scoped_guard(mutex, &dp->sdp_lock)
> +		set_bit(0, dp->sdp_reg_bank);

[Severity: High]
This isn't a bug introduced by this patch, but since the goal is to support
concurrent users, is it safe to unconditionally claim bit 0 here?

If video is disabled and a concurrent user like audio allocates an SDP, it
might be assigned bit 0. When video re-enables, unconditionally setting bit 0
could overwrite the other subsystem's reservation.

[ ... ]
> @@ -1735,7 +1740,8 @@ static void dw_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>  
>  	dw_dp_video_disable(dp);
>  	dw_dp_link_disable(dp);
> -	bitmap_zero(dp->sdp_reg_bank, SDP_REG_BANK_SIZE);
> +	scoped_guard(mutex, &dp->sdp_lock)
> +		bitmap_zero(dp->sdp_reg_bank, SDP_REG_BANK_SIZE);

[Severity: High]
This is also a pre-existing issue, but will clearing the entire bitmap here
break the concurrent access model this patch aims to protect?

If audio or another subsystem holds an SDP reservation when video is disabled,
bitmap_zero() will incorrectly mark their bits as free. If a new user then
requests an SDP, it could be allocated the same bit, causing multiple
subsystems to write to the same hardware register bank concurrently.

Should dw_dp_bridge_atomic_disable() only clear the specific bits reserved by
the video path rather than zeroing out the entire shared state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-synopsys-dw-dp-improvements-v7-0-b7640fa8cf48@collabora.com?part=14