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
