Re: [PATCH v7 5/5] drm/display: hdmi-state-helper: Init SCDC debugfs for HDMI

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Reading the `scdc_status` debugfs file causes a kernel panic if the HDMI connector lacks an I2C DDC adapter (`connector->ddc == NULL`).
- [Medium] Concurrent reads of the `scdc_status` debugfs file cause a data race on the shared per-connector `priv->state` buffer.
- [Low] SCDC debugfs initialization is incorrectly skipped if the creation of the `infoframes` debugfs directory fails.
--

--- Patch [5]: [PATCH v7 5/5] drm/display: hdmi-state-helper: Init SCDC debugfs for HDMI ---
commit 4d2f5fcf6dba954346bc26d1af8d8e70d0cf5de0
Author: Nicolas Frattaroli <[email protected]>

drm/display: hdmi-state-helper: Init SCDC debugfs for HDMI

Initialise the SCDC debugfs file for connectors that call the HDMI state
helper's debugfs_init helper. This makes all connectors that use the
helper expose the scdc_status file in their connector debugfs.

> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 8d5ee10ceb874..709524b40c26f 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
[ ... ]
> @@ -1116,6 +1117,7 @@ void drm_hdmi_connector_debugfs_init(struct drm_connector *connector,
>  		return;

[Severity: Low]
Is it intended that drm_scdc_debugfs_init() is skipped if the infoframes
directory creation fails?

Right above this line, the code checks:

	dir = debugfs_create_dir("infoframes", root);
	if (IS_ERR(dir))
		return;

If debugfs_create_dir() fails, the early return skips the independent
drm_scdc_debugfs_init() call entirely.

>  
>  	create_hdmi_infoframe_files(connector, dir);
> +	drm_scdc_debugfs_init(connector, root);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does exposing this to all HDMI connectors cause a regression with a NULL
pointer dereference?

If an HDMI connector lacks an I2C adapter (connector->ddc is NULL) but
indicates SCDC support, reading the newly exposed scdc_status debugfs
file leads to a panic:

drivers/gpu/drm/display/drm_scdc_helper.c:scdc_status_show() {
	if (scdc->supported) {
		ret = drm_scdc_read_state(connector, st);
...
}

drivers/gpu/drm/display/drm_scdc_helper.c:drm_scdc_read_state() {
	ddc = connector->ddc;
	if (!scdc->supported)
		return -EOPNOTSUPP;
	/* Read in 128-byte chunks, to work around DP<->HDMI converters with issues. */
	ret = drm_scdc_read(ddc, 0, buf, 128);
...
}

drm_scdc_read() unconditionally passes the NULL pointer to i2c_transfer(),
causing a panic.

[Severity: Medium]
Could concurrent reads of the scdc_status debugfs file cause a regression
due to a data race?

drm_scdc_debugfs_init() allocates a single, shared struct scdc_debugfs_priv
per connector:

drivers/gpu/drm/display/drm_scdc_helper.c:drm_scdc_debugfs_init() {
	priv = drmm_kzalloc(connector->dev, sizeof(*priv), GFP_KERNEL);
...
}

When multiple processes concurrently read the debugfs file, they extract this
shared pointer:

drivers/gpu/drm/display/drm_scdc_helper.c:scdc_status_show() {
	struct scdc_debugfs_priv *priv = m->private;
	struct drm_scdc_state *st = &priv->state;
...
}

Since there is no locking across the sequential I2C reads and data parsing
in drm_scdc_read_state(), concurrent threads could overwrite priv->state
simultaneously.

>  }
>  EXPORT_SYMBOL(drm_hdmi_connector_debugfs_init);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5