Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Critical] Unhandled error from `dw_dp_audio_infoframe_send()` results in a negative SDP index, leading to out-of-bounds memory write and undefined bitwise shifts. - [High] Typo in sample width validation allows 32-bit audio to bypass checks and silently overflow the hardware register mask. - [High] `dw_dp_audio_unprepare()` unconditionally clears `audio_sdp_nr` (which defaults to 0), potentially corrupting the active video stream's VSC SDP configuration. - [Medium] `dw_dp_audio_unprepare()` fails to clear the audio stream and timestamp SDP enablement bits, leaving the hardware in an inconsistent state. --
--- Patch [15]: [PATCH v5 15/15] drm/bridge: synopsys: dw-dp: Add audio support --- commit 8d3e8e3c7ab668f3eaf614e21b09c813acc45d46 Author: Sebastian Reichel <[email protected]> drm/bridge: synopsys: dw-dp: Add audio support This commit adds audio support to the Synopsys DesignWare DisplayPort controller. It implements the necessary DRM bridge audio callbacks and configures the hardware to transmit I2S or SPDIF audio streams. > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c > b/drivers/gpu/drm/bridge/synopsys/dw-dp.c > index 2bd37e5c36da8..a9e04cee002e8 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c [ ... ] > @@ -1890,6 +1928,205 @@ static void dw_dp_bridge_oob_notify(struct drm_bridge > *bridge, [ ... ] > +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: Medium] Does this leave the hardware attempting to send stale audio SDPs? The setup logic in dw_dp_audio_prepare() enables EN_AUDIO_STREAM_SDP and EN_AUDIO_TIMESTAMP_SDP in the DW_DP_SDP_VERTICAL_CTRL and DW_DP_SDP_HORIZONTAL_CTRL registers. However, this cleanup path disables AUDIO_DATA_IN_EN and clears the infoframe SDP slot, but does not appear to clear the stream and timestamp SDP bits. > + 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; > + > + dw_dp_clear_sdp(dp, dp->audio_sdp_nr); [Severity: High] Can this unintentionally clear the video stream configuration? When the dw_dp structure is allocated, dp->audio_sdp_nr is initialized to 0. If a userspace application opens the audio device but closes it before hardware parameters are configured, dw_dp_audio_shutdown() will still be called, leading to an unconditional call to dw_dp_clear_sdp(dp, 0). Since slot 0 is typically already in use by the video stream (allocated by dw_dp_send_vsc_sdp), this could cause the display hardware to lose its VSC SDP configuration and break the display. > +} > + > +static int dw_dp_audio_prepare(struct drm_bridge *bridge, > + struct drm_connector *connector, > + struct hdmi_codec_daifmt *daifmt, > + struct hdmi_codec_params *params) > +{ [ ... ] > + if (params->sample_width < 16 || params->sample_width >> 24) { > + dev_err(dp->dev, "invalid data sample width %d\n", > params->sample_width); > + return -EINVAL; > + } [Severity: High] Is this a typo where >> 24 was intended to be > 24? If an ALSA client configures a stream with a 32-bit sample width, params->sample_width >> 24 evaluates to 0, which bypasses this check. Later in this function, the 32 value is passed to FIELD_PREP(AUDIO_DATA_WIDTH, 32). Since AUDIO_DATA_WIDTH is GENMASK(9, 5), this will silently overflow the 5-bit mask and truncate to 0, misconfiguring the hardware. [ ... ] > + ret = dw_dp_audio_infoframe_send(dp); > + if (ret < 0) > + dev_err(dp->dev, "failed to send audio infoframe\n"); > + > + dp->audio_sdp_nr = ret; [Severity: Critical] Could this lead to an out-of-bounds memory write if sending the infoframe fails? If dw_dp_audio_infoframe_send() returns a negative error code (for example, if dw_dp_send_sdp() returns -EBUSY because the SDP bank is full), that negative value is assigned to dp->audio_sdp_nr while the function proceeds without returning the error. During teardown, dw_dp_audio_unprepare() will then call: dw_dp_clear_sdp(dp, dp->audio_sdp_nr); In dw_dp_clear_sdp(), this results in negative bit shifts and an out-of-bounds write: EN_VERTICAL_SDP << nr clear_bit(nr, dp->sdp_reg_bank); Would it be safer to return the error code here instead of continuing? > + > + dev_dbg(dp->dev, "audio prepare with %d channels using DAI=%d\n", > + dp->audio_channels, dp->audio_interface); > + > + return 0; > +} [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260724-synopsys-dw-dp-improvements-v5-0-9445c2e87...@collabora.com?part=15
