Re: [PATCH v8 10/39] drm/display: hdmi-state-helper: Sync SCDC state on hotplug

[sashiko-bot@kernel.org]

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 <cristian.ciocaltea@collabora.com>

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/20260702-dw-hdmi-qp-scramb-v8-0-d79890d00b6a@collabora.com?part=10