Re: [PATCH v7 17/18] drm/bridge: synopsys: dw-dp: Clear only enabled SDPs on atomic disable

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unbalanced and unconditional clearing of SDP slot 0 defeats tracking and risks concurrent stream corruption.
- [High] Sleeping functions are called from an atomic commit callback path in violation of DRM rules.
--

--- Patch [17]: [PATCH v7 17/18] drm/bridge: synopsys: dw-dp: Clear only enabled SDPs on atomic disable ---
commit 7e0bb4945c91158df2f4360bed9dbdb18041404b
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Clear only enabled SDPs on atomic disable

This commit updates dw_dp_bridge_atomic_disable() to selectively clear only
the previously configured SDPs (like VSC SDP) instead of bulk clearing the
entire SDP allocation bitmap. This aims to prevent handing out in-use SDPs
to other systems like the upcoming audio infrastructure.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index fcee47c6716a7..c0026bd4c7b53 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c

[ ... ]

> +static void dw_dp_clear_sdp(struct dw_dp *dp, int nr)
> +{
> +	regmap_clear_bits(dp->regmap, DW_DP_SDP_VERTICAL_CTRL,
> +			  EN_VERTICAL_SDP << nr);
> +
> +	regmap_clear_bits(dp->regmap, DW_DP_SDP_HORIZONTAL_CTRL,
> +			  EN_HORIZONTAL_SDP << nr);
> +
> +	scoped_guard(mutex, &dp->sdp_lock)
> +		clear_bit(nr, dp->sdp_reg_bank);
> +}

[ ... ]

> @@ -1738,8 +1751,14 @@ static void dw_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>  
>  	dw_dp_video_disable(dp);
>  	dw_dp_link_disable(dp);
> +
> +	if (dp->vsc_sdp_nr >= 0) {
> +		dw_dp_clear_sdp(dp, dp->vsc_sdp_nr);

[Severity: High]
This isn't a bug introduced by this patch, but does this code call sleeping
functions in an atomic commit path?

The DRM subsystem calls dw_dp_bridge_atomic_disable() during non-blocking
commits via drm_atomic_helper_commit_tail(), which runs in atomic context.
This patch adds a call to dw_dp_clear_sdp(), which acquires a mutex using
scoped_guard(mutex, &dp->sdp_lock). 

While the function already acquired this mutex and called sleeping functions
on this path previously, adding new mutex acquisitions here can lead to
system instability or deadlocks when the display mode is disabled.

> +		dp->vsc_sdp_nr = -1;
> +	}
>  	scoped_guard(mutex, &dp->sdp_lock)
> -		bitmap_zero(dp->sdp_reg_bank, SDP_REG_BANK_SIZE);
> +		clear_bit(0, dp->sdp_reg_bank);

[Severity: High]
This is a pre-existing issue, but does unconditionally clearing bit 0 here
defeat the tracking mechanism?

If userspace opens the ALSA PCM device while the DRM bridge is disabled,
ASoC might allocate slot 0 for audio. When the display is later enabled
and disabled, dw_dp_bridge_atomic_disable() unconditionally executes
clear_bit(0, dp->sdp_reg_bank). 

This silently revokes the active audio allocation, allowing another
allocation to claim it while the audio subsystem still believes it owns
the slot.

While the previous code wiped the entire bank via bitmap_zero(),
explicitly clearing bit 0 seems to leave the audio tracking vulnerable.
Should this unconditional clear_bit() be removed entirely since
vsc_sdp_nr is now being handled explicitly?

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