Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Returning success (0) from `msm_dp_audio_prepare` when the display is disconnected causes silent audio failures upon hotplug and NoC errors due to unclocked DSP hardware access. Pre-existing issues: - [High] Race condition between `msm_dp_audio_prepare` and `msm_dp_display_disable` causes unclocked MMIO accesses. -- commit bfb65364e66313dc1385a42e09172f44a1e1fbd1 Author: Kumar Anurag <[email protected]> drm/msm/dp: return 0 from audio_prepare when cable is disconnected PipeWire treats a non-zero return from prepare as fatal, marking the DP audio device as a dummy sink when the cable is unplugged. > The active_stream_cnt guard already prevents any unclocked hardware access, > so return success instead of -EINVAL when the link is not active. [Severity: High] Is this commit message accurate for the upstream tree? There doesn't appear to be an active_stream_cnt guard in the mainline driver. Without this guard, will the DSP push data to powered-off, unclocked DP hardware and cause NoC errors? > diff --git a/drivers/gpu/drm/msm/dp/dp_audio.c > b/drivers/gpu/drm/msm/dp/dp_audio.c > index 41018e82efa10..df222943ae57f 100644 > --- a/drivers/gpu/drm/msm/dp/dp_audio.c > +++ b/drivers/gpu/drm/msm/dp/dp_audio.c > @@ -284,10 +284,8 @@ int msm_dp_audio_prepare(struct drm_bridge *bridge, > * such cases check for connection status and bail out if not > * connected. > */ > - if (!msm_dp_display->power_on) { > - rc = -EINVAL; > + if (!msm_dp_display->power_on) > goto end; [Severity: High] Does returning 0 here trick the ALSA/ASoC framework into marking the stream as successfully prepared? If the stream is marked RUNNING, ALSA won't re-invoke prepare() when the display is later connected. Could this regression result in silent playback because the DP hardware is never actually configured? [Severity: High] This is a pre-existing issue, but does this lockless check of power_on race with msm_dp_display_disable()? If msm_dp_display_disable() runs concurrently (e.g., from a hotplug interrupt) immediately after this check but before audio_enabled is set to true, it will skip waiting and disable the DP link and clocks. When msm_dp_audio_prepare() resumes, it will call msm_dp_audio_setup_sdp() and attempt MMIO operations on unclocked DP audio registers. Could this lead to a synchronous external abort? > - } > > audio = msm_dp_audio_get_data(msm_dp_display); > if (IS_ERR(audio)) { -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
