Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues: - [High] The return value of `pm_runtime_get_sync()` is ignored in `dw_dp_audio_startup()`, violating PM runtime API contracts and risking hardware access while suspended. - [High] Missing error checks on `clk_prepare_enable()` in `dw_dp_audio_prepare()` lead to hardware access failures and clock reference count underflows. - [High] SDP register bank slots are permanently leaked because `dw_dp_audio_unprepare()` does not free them. - [Medium] Prematurely updating `dp->audio_interface` before hardware validation causes an unbalanced clock disable in the teardown path on error. - [Medium] `dw_dp_audio_prepare()` explicitly rejects valid intermediate channel counts, breaking multi-channel playback. Pre-existing issues: - [High] Concurrent accesses to the `dp->sdp_reg_bank` bitmap between DRM and ALSA threads lack locking, leading to SDP register corruption. -- commit 91c310c87d4ddad349da89d89896e7fa9319465d Author: Sebastian Reichel <[email protected]> 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 7f4f36c614844..f1946f2c945da 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c [ ... ] > +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"); > + pm_runtime_get_sync(dp->dev); [Severity: High] Does this code need to check the return value of pm_runtime_get_sync()? If the power domain or clock fails to resume, it returns a negative error code. Ignoring this might lead to accessing suspended hardware registers when ALSA triggers startup. > + > + return 0; > +} > + > +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)); > + > + if (dp->audio_interface == DW_DP_AUDIO_SPDIF) > + clk_disable_unprepare(dp->spdif_clk); > + else if (dp->audio_interface == DW_DP_AUDIO_I2S) > + clk_disable_unprepare(dp->i2s_clk); > + > + dp->audio_interface = DW_DP_AUDIO_UNUSED; > +} [Severity: High] Does dw_dp_audio_unprepare() leak SDP register bank slots? dw_dp_audio_prepare() allocates a new SDP slot by calling dw_dp_send_sdp(), which sets a bit in dp->sdp_reg_bank. However, dw_dp_audio_unprepare() never clears the allocated bit. Could this exhaust the register bank slots if userspace starts and stops audio multiple times? [ ... ] > +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; > + 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] Will this explicit rejection break valid multi-channel playback? The driver sets bridge->hdmi_audio_max_i2s_playback_channels = 8 indicating hardware support, but this switch statement returns -EINVAL for intermediate channel counts like 4 or 6. Is it intended to reject valid multi-channel audio setups? > + > + switch (daifmt->fmt) { > + case HDMI_SPDIF: > + dp->audio_interface = DW_DP_AUDIO_SPDIF; > + break; > + case HDMI_I2S: > + /* > + * It is recommended to use SPDIF instead of I2S, since I2S > mode requires > + * manually inserting PCUV control bits from userspace and this > is done > + * automatically in hardware for SPDIF mode. > + */ > + dp->audio_interface = DW_DP_AUDIO_I2S; > + break; > + default: > + dev_err(dp->dev, "invalid DAI format %d\n", daifmt->fmt); > + return -EINVAL; > + } > + > + regmap_read(dp->regmap, DW_DP_CONFIG_REG1, &cfg1); > + supported_audio_interfaces = FIELD_GET(AUDIO_SELECT, cfg1); > + > + if (supported_audio_interfaces != DW_DP_AUDIO_I2S_AND_SPDIF && > + supported_audio_interfaces != dp->audio_interface) { > + dev_err(dp->dev, "unsupported DAI %d\n", daifmt->fmt); > + return -EINVAL; > + } [Severity: Medium] Could this leave dp->audio_interface in a corrupted state on failure? If an unsupported DAI format is provided, dp->audio_interface is updated prior to the validation check. If this function then returns -EINVAL, the ALSA core later calls dw_dp_audio_shutdown() -> dw_dp_audio_unprepare(), which acts on the corrupted state and attempts to disable a clock that was never enabled. > + > + clk_prepare_enable(dp->spdif_clk); > + clk_prepare_enable(dp->i2s_clk); [Severity: High] Does this code need to check the return values of clk_prepare_enable()? If clk_prepare_enable() fails, the driver continues to program hardware registers, which could lead to hardware lockups. Furthermore, the teardown path unconditionally disables the failed clock, causing reference count underflows. [ ... ] > + ret = dw_dp_audio_infoframe_send(dp); > + if (ret < 0) > + dev_err(dp->dev, "failed to send audio infoframe\n"); [Severity: High] This isn't a bug introduced by this patch, but adding audio support introduces a concurrent access path that makes a pre-existing race possible. dw_dp_audio_infoframe_send() calls dw_dp_send_sdp(), which allocates a slot using non-atomic test and set: 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); Can ALSA audio preparation running concurrently with DRM display updates lead to slot allocation collisions and SDP hardware register corruption? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260612-synopsys-dw-dp-improvements-v3-0-dc61e6352...@collabora.com?part=10
