Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/bridge: dw-hdmi-qp: sleeping in atomic context via mutex in atomic callbacks --
commit 1e5aaa063a714d3e9e667b95f0e3087ce7b8924d Author: Frank Zhang <[email protected]> drm/bridge: dw-hdmi-qp: serialize HDMI audio callbacks with PHY enable/disable This commit introduces a mutex lock and PHY enabled state to serialize PHY state transitions and HDMI audio callbacks, aiming to fix a kernel panic observed during system reboot when ALSA races with the DRM atomic framework. 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 c9568aea98dc8..1ffe159e5a099 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c [ ... ] > @@ -547,6 +571,7 @@ static void dw_hdmi_qp_bridge_atomic_enable(struct > drm_bridge *bridge, > struct drm_connector_state *conn_state; > struct drm_connector *connector; > unsigned int op_mode; > + int ret; > > connector = drm_atomic_get_new_connector_for_encoder(state, > bridge->encoder); > if (WARN_ON(!connector)) > @@ -556,6 +581,7 @@ static void dw_hdmi_qp_bridge_atomic_enable(struct > drm_bridge *bridge, > if (WARN_ON(!conn_state)) > return; > > + mutex_lock(&hdmi->phy.lock); [Severity: High] Does this code acquire a sleeping lock in an atomic context? According to the DRM subsystem rules, bridge atomic enable/disable callbacks execute in atomic context during non-blocking commits where sleeping is strictly forbidden. Acquiring a mutex here will cause a might_sleep() warning, scheduler errors, and possible system deadlocks when a userspace application or compositor issues a non-blocking atomic modeset. > if (connector->display_info.is_hdmi) { > dev_dbg(hdmi->dev, "%s mode=HDMI %s rate=%llu bpc=%u\n", > __func__, > > drm_hdmi_connector_get_output_format_name(conn_state->hdmi.output_format), [ ... ] > @@ -580,9 +615,10 @@ static void dw_hdmi_qp_bridge_atomic_disable(struct > drm_bridge *bridge, > { > struct dw_hdmi_qp *hdmi = bridge->driver_private; > > + guard(mutex)(&hdmi->phy.lock); [Severity: High] Can this sequence cause a panic during atomic commits? Similar to the enable path, dw_hdmi_qp_bridge_atomic_disable() executes in atomic context. Using guard(mutex) acquires a sleeping lock while atomic, which violates the DRM atomic context requirements. > hdmi->tmds_char_rate = 0; > - > hdmi->phy.ops->disable(hdmi, hdmi->phy.data); > + hdmi->phy.enabled = false; > } [ ... ] > @@ -780,7 +825,11 @@ static int > dw_hdmi_qp_bridge_write_audio_infoframe(struct drm_bridge *bridge, > { > struct dw_hdmi_qp *hdmi = bridge->driver_private; > > - dw_hdmi_qp_bridge_clear_audio_infoframe(bridge); > + guard(mutex)(&hdmi->phy.lock); [Severity: High] Is this lock safe to acquire here? dw_hdmi_qp_bridge_write_audio_infoframe() is called downstream from the atomic enable path via drm_atomic_helper_connector_hdmi_update_infoframes(). Since it is reachable from the non-blocking atomic commit chain, acquiring a sleeping mutex here can also lead to system deadlocks. > + if (!hdmi->phy.enabled) > + return -EOPNOTSUPP; > + > + dw_hdmi_qp_clear_audio_infoframe_regs(hdmi); > > /* > * AUDI_CONTENTS0: { RSV, HB2, HB1, RSV } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
