Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] The lockless `if (hdmi->tmds_char_rate > 0)` check introduces a Time-Of-Check to Time-Of-Use (TOCTOU) data race with the DRM atomic commit thread, failing to fully prevent the SError panic. Pre-existing issues: - [High] `tmds_char_rate` is assigned before the PHY is actually initialized in `dw_hdmi_qp_bridge_atomic_enable`, allowing concurrent audio operations to crash the kernel by accessing unpowered hardware. -- commit 9d8465ca2470e46cc48135ada91995d64bb0dec1 Author: Frank Zhang <[email protected]> drm/bridge: dw-hdmi-qp: Guard clear_audio_infoframe when PHY is down This patch attempts to prevent an Asynchronous SError Interrupt kernel panic by checking hdmi->tmds_char_rate before accessing PHY registers during ALSA audio shutdown. It introduces a lockless check to decouple the audio infoframe operations from powered-down hardware states. Link: https://lore.kernel.org/all/[email protected]/ Link: https://lore.kernel.org/all/[email protected]/ Link: https://lore.kernel.org/all/[email protected]/ Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c > index cfd1b18c06e62..f0e9324249d66 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c [ ... ] > @@ -893,6 +893,16 @@ static int > dw_hdmi_qp_bridge_clear_audio_infoframe(struct drm_bridge *bridge) > return 0; > } > > +static int dw_hdmi_qp_bridge_clear_audio_infoframe(struct drm_bridge *bridge) > +{ > + struct dw_hdmi_qp *hdmi = bridge->driver_private; > + > + if (hdmi->tmds_char_rate > 0) > + return __dw_hdmi_qp_bridge_clear_audio_infoframe(bridge); > + > + return 0; > +} > + [Severity: High] Does this lockless check introduce a Time-Of-Check to Time-Of-Use race? If a userspace process closes the ALSA audio stream concurrently with a DRM atomic disable, could the DRM commit thread clear the tmds_char_rate state variable and disable the PHY hardware right after this check passes, but before __dw_hdmi_qp_bridge_clear_audio_infoframe() executes? For example: ALSA thread in dw_hdmi_qp_bridge_clear_audio_infoframe(): if (hdmi->tmds_char_rate > 0) // passes DRM thread in dw_hdmi_qp_bridge_atomic_disable(): hdmi->tmds_char_rate = 0; hdmi->phy.ops->disable(hdmi, hdmi->phy.data); ALSA thread continues: return __dw_hdmi_qp_bridge_clear_audio_infoframe(bridge); This would result in writing to the disabled PHY and causing the same SError panic this patch intends to fix. Would it be safer to use a mutex to serialize audio operations with PHY state changes? [Severity: High] This is a pre-existing issue, but looking at how tmds_char_rate is used as a proxy for hardware readiness, could concurrent audio operations crash the kernel during a DRM atomic enable? In dw_hdmi_qp_bridge_atomic_enable(), tmds_char_rate is assigned a non-zero value before the hardware is actually powered on: dw_hdmi_qp_bridge_atomic_enable(): ... hdmi->tmds_char_rate = conn_state->hdmi.tmds_char_rate; hdmi->phy.ops->init(hdmi, hdmi->phy.data); If an ALSA thread concurrently executes dw_hdmi_qp_audio_prepare() or dw_hdmi_qp_bridge_write_audio_infoframe(), couldn't it observe tmds_char_rate > 0 and proceed to access PHY registers before they are powered on? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
