diff mbox series

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

Message ID 20260721-scdc-link-health-v7-5-92df1b6ad5fc@collabora.com (mailing list archive)
State New
Headers
Series Add SCDC information to connector debugfs |

Commit Message

Nicolas Frattaroli July 21, 2026, 8:59 a.m. UTC 
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.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

sashiko-bot@kernel.org July 21, 2026, 9:11 a.m. UTC | #1 
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 <nicolas.frattaroli@collabora.com>

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);
Maxime Ripard July 21, 2026, 1:49 p.m. UTC | #2 
On Tue, 21 Jul 2026 10:59:01 +0200, Nicolas Frattaroli wrote:
> 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.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index 8d5ee10ceb87..709524b40c26 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -13,6 +13,7 @@ 
 #include <drm/display/drm_hdmi_cec_helper.h>
 #include <drm/display/drm_hdmi_helper.h>
 #include <drm/display/drm_hdmi_state_helper.h>
+#include <drm/display/drm_scdc_helper.h>
 
 /**
  * DOC: hdmi helpers
@@ -1116,6 +1117,7 @@  void drm_hdmi_connector_debugfs_init(struct drm_connector *connector,
 		return;
 
 	create_hdmi_infoframe_files(connector, dir);
+	drm_scdc_debugfs_init(connector, root);
 }
 EXPORT_SYMBOL(drm_hdmi_connector_debugfs_init);