Re: [PATCH v4 10/10] drm/bridge: synopsys: dw-dp: Add audio support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Audio infoframe preparation permanently leaks an SDP register bank slot on every call, leading to exhaustion of slots and duplicated Audio InfoFrames.
- [Medium] The return value of `clk_prepare_enable(dp->i2s_clk)` is not assigned to `ret`, rendering the subsequent error check dead code and ignoring failures.
- [Medium] TOCTOU race condition in `dw_dp_send_sdp` for SDP register bank allocation when called concurrently from audio and display contexts.
- [Medium] A 32-bit audio sample width will be silently truncated to 0 when configured in the hardware register via FIELD_PREP.
- [Medium] Incomplete channel count support blocks valid ALSA streams from playing.
--

--- Patch [10]: [PATCH v4 10/10] drm/bridge: synopsys: dw-dp: Add audio support ---
commit 9935aa2c50aed1038e27452a987227adb8e93b00
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Add audio support

Implement audio support for the Synopsys DesignWare DisplayPort
controller.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 636b9e1052978..8d6a52135e54d 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c

[ ... ]

> @@ -1846,6 +1871,195 @@ static void dw_dp_bridge_oob_notify(struct drm_bridge *bridge,
>  		dev_err_once(dp->dev, "Missing platform handler for OOB HPD handling\n");
>  }
>  
> +static int dw_dp_audio_infoframe_send(struct dw_dp *dp)
> +{
> +	struct hdmi_audio_infoframe frame;
> +	struct dw_dp_sdp sdp;
> +	int ret;
> +
> +	ret = hdmi_audio_infoframe_init(&frame);
> +	if (ret < 0)
> +		return ret;
> +
> +	frame.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM;
> +	frame.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
> +	frame.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
> +	frame.channels = dp->audio_channels;
> +
> +	ret = hdmi_audio_infoframe_pack_for_dp(&frame, &sdp.base, DW_DP_SDP_VERSION);
> +	if (ret < 0)
> +		return ret;
> +
> +	sdp.flags = DW_DP_SDP_VERTICAL_INTERVAL;
> +
> +	return dw_dp_send_sdp(dp, &sdp);

[Severity: Medium]
Is there a race condition when allocating SDP register bank slots? 

When dw_dp_audio_infoframe_send() is called, it routes to dw_dp_send_sdp() 
which performs a non-atomic bitmap allocation on the shared dp->sdp_reg_bank 
state:

    nr = find_first_zero_bit(dp->sdp_reg_bank, SDP_REG_BANK_SIZE);
    if (nr < SDP_REG_BANK_SIZE)
        set_bit(nr, dp->sdp_reg_bank);

If a DRM atomic commit and an ALSA stream start execute concurrently on 
different CPUs, could they allocate the same SDP slot and corrupt the 
hardware configuration?

> +}
> +
> +static int dw_dp_audio_startup(struct drm_bridge *bridge,
> +			       struct drm_connector *connector)
> +{
> +	struct dw_dp *dp = bridge_to_dp(bridge);
> +
> +	dev_dbg(dp->dev, "audio startup\n");
> +
> +	return pm_runtime_resume_and_get(dp->dev);
> +}
> +
> +static void dw_dp_audio_unprepare(struct drm_bridge *bridge,
> +				  struct drm_connector *connector)
> +{
> +	struct dw_dp *dp = bridge_to_dp(bridge);
> +
> +	/* Disable all audio streams */
> +	regmap_update_bits(dp->regmap, DW_DP_AUD_CONFIG1, AUDIO_DATA_IN_EN,
> +			   FIELD_PREP(AUDIO_DATA_IN_EN, 0));

[Severity: High]
Does dw_dp_audio_unprepare() miss freeing the SDP register bank slot 
allocated during audio prepare? 

When dw_dp_audio_infoframe_send() calls dw_dp_send_sdp(), a slot in 
dp->sdp_reg_bank is allocated and the corresponding EN_VERTICAL_SDP 
hardware bits are set. 

Since the cleanup path here doesn't clear that bit or disable the hardware 
bits, won't repeatedly starting and stopping an ALSA stream exhaust all 16 
hardware slots and permanently break audio infoframe updates with -EBUSY?

> +
> +	if (dp->audio_interface == DW_DP_AUDIO_SPDIF)
> +		clk_disable_unprepare(dp->spdif_clk);

[ ... ]

> +static int dw_dp_audio_prepare(struct drm_bridge *bridge,
> +			       struct drm_connector *connector,
> +			       struct hdmi_codec_daifmt *daifmt,
> +			       struct hdmi_codec_params *params)
> +{
> +	struct dw_dp *dp = bridge_to_dp(bridge);
> +	u8 audio_data_in_en, supported_audio_interfaces;
> +	enum dw_dp_audio_interface audio_interface;
> +	u32 cfg1;
> +	int ret;
> +
> +	/*
> +	 * prepare might be called multiple times, so release the clocks
> +	 * from previous calls to keep the calls in balance.
> +	 */
> +	if (dp->audio_interface != DW_DP_AUDIO_UNUSED)
> +		dw_dp_audio_unprepare(bridge, connector);
> +
> +	dp->audio_channels = params->cea.channels;
> +	switch (params->cea.channels) {
> +	case 1:
> +	case 2:
> +		audio_data_in_en = AUDIO_DATA_IN_EN_CHANNEL12;
> +		break;
> +	case 8:
> +		audio_data_in_en = AUDIO_DATA_IN_EN_CHANNEL12 |
> +				   AUDIO_DATA_IN_EN_CHANNEL34 |
> +				   AUDIO_DATA_IN_EN_CHANNEL56 |
> +				   AUDIO_DATA_IN_EN_CHANNEL78;
> +		break;
> +	default:
> +		dev_err(dp->dev, "invalid audio channels %d\n", dp->audio_channels);
> +		return -EINVAL;
> +	}

[Severity: Medium]
Does this switch statement need to handle 4 and 6 channel audio streams? 

Since the bridge advertises support for up to 8 channels via 
hdmi_audio_max_i2s_playback_channels = 8, the ALSA framework might 
negotiate 4.0 or 5.1 surround sound configurations. 

It seems these valid channel counts would be rejected here with -EINVAL, 
blocking their playback.

> +
> +	switch (daifmt->fmt) {

[ ... ]

> +	ret = clk_prepare_enable(dp->spdif_clk);
> +	if (ret)
> +		return ret;
> +
> +	clk_prepare_enable(dp->i2s_clk);
> +	if (ret) {

[Severity: Medium]
Does this ignore the return value of clk_prepare_enable() for the I2S clock? 

Since the result isn't assigned to ret, the subsequent error check appears 
to be evaluating the return value from the previous spdif_clk enable call 
(which is already known to be 0). 

If the I2S clock fails to enable, it looks like this will silently proceed 
with an unclocked interface and cause an unbalanced clock state when 
dw_dp_audio_unprepare() is called.

> +		clk_disable_unprepare(dp->spdif_clk);
> +		return ret;
> +	}
> +
> +	regmap_update_bits(dp->regmap, DW_DP_AUD_CONFIG1,
> +			   AUDIO_DATA_IN_EN | NUM_CHANNELS | AUDIO_DATA_WIDTH |
> +			   AUDIO_INF_SELECT | HBR_MODE_ENABLE,
> +			   FIELD_PREP(AUDIO_DATA_IN_EN, audio_data_in_en) |
> +			   FIELD_PREP(NUM_CHANNELS, dp->audio_channels - 1) |
> +			   FIELD_PREP(AUDIO_DATA_WIDTH, params->sample_width) |

[Severity: Medium]
Can a 32-bit audio sample width get silently truncated here? 

The AUDIO_DATA_WIDTH mask is defined as GENMASK(9, 5) which is 5 bits wide. 
If userspace negotiates a 32-bit audio stream (which is allowed by default), 
the value 32 (binary 100000, 6 bits) will be truncated to 0 by FIELD_PREP, 
potentially misconfiguring the hardware.

> +			   FIELD_PREP(AUDIO_INF_SELECT, audio_interface) |
> +			   FIELD_PREP(HBR_MODE_ENABLE, 0));
> +

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-synopsys-dw-dp-improvements-v4-0-f0f4a4ede712@collabora.com?part=10