Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Medium] State mutations (audio and CEC notifications) are applied even when SCDC sync fails with a WW mutex backoff (-EDEADLK). Pre-existing issues: - [High] The disconnect path replaces a TODO with a no-op function call, leaving the scrambler state potentially inconsistent on hotplug. -- commit 079e249ec8df03c559fb868b6a5035e1328d0d31 Author: Cristian Ciocaltea <[email protected]> drm/display: hdmi-state-helper: Sync SCDC state on hotplug This commit hooks drm_connector_hdmi_sync_scdc() into both the connect and disconnect paths of drm_atomic_helper_connector_hdmi_update() to synchronize SCDC status on hotplug events. It also adds a context argument to propagate lock contention errors like -EDEADLK. > diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c > b/drivers/gpu/drm/display/drm_hdmi_state_helper.c > --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c > +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c > @@ -1198,18 +1198,20 @@ > drm_atomic_helper_connector_hdmi_clear_audio_infoframe(struct drm_connector > *con [ ... ] > -static void > +static int > drm_atomic_helper_connector_hdmi_update(struct drm_connector *connector, > - enum drm_connector_status status) > + enum drm_connector_status status, > + struct drm_modeset_acquire_ctx *ctx) > { > const struct drm_edid *drm_edid; > + int ret = 0; > > if (status == connector_status_disconnected) { > - // TODO: also handle scramber, HDMI sink disconnected. > + ret = drm_connector_hdmi_sync_scdc(connector, false, ctx); [Severity: High] This is a pre-existing issue, but the patch actively replaces the TODO with a function call that appears to be a no-op on disconnect. Looking at drm_connector_hdmi_sync_scdc() in drm_hdmi_helper.c, it immediately returns 0 when the plugged parameter is false: int drm_connector_hdmi_sync_scdc(...) { if (!connector || !plugged || !ctx) return 0; ... } Will this leave the scrambler state potentially inconsistent if a user unplugs an SCDC-enabled sink and plugs in a non-SCDC sink without an active modeset? Should the scrambler be explicitly disabled here instead of silently returning? > drm_connector_hdmi_audio_plugged_notify(connector, false); > drm_edid_connector_update(connector, NULL); > drm_connector_cec_phys_addr_invalidate(connector); > - return; > + return ret; > } > [ ... ] > @@ -1222,24 +1224,32 @@ drm_atomic_helper_connector_hdmi_update(struct > drm_connector *connector, > drm_edid_free(drm_edid); > > if (status == connector_status_connected) { > - // TODO: also handle scramber, HDMI sink is now connected. > + ret = drm_connector_hdmi_sync_scdc(connector, true, ctx); > drm_connector_hdmi_audio_plugged_notify(connector, true); > drm_connector_cec_phys_addr_set(connector); > } > + > + return ret; [Severity: Medium] If drm_connector_hdmi_sync_scdc() encounters lock contention and returns -EDEADLK, should we proceed with applying state mutations? Because ret is captured but not checked before calling drm_connector_hdmi_audio_plugged_notify() and drm_connector_cec_phys_addr_set(), this leaks the plugged state to the audio and CEC subsystems before the WW mutex backoff is completed. Could we add an error check here to return early on -EDEADLK before mutating the audio and CEC states? > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
