Hi Maxime,
On Wednesday, 22 July 2026 11:27:25 Central European Summer Time sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The function `scdc_status_show` leaks `connector->dev->mode_config.mutex` on the disconnected error path.
> --
>
> --- Patch [2]: [PATCH v8 2/5] drm/scdc-helper: Add scdc_status debugfs entry ---
> commit 524b3064a7221dcef8708b2269e2ccaa3ede6629
> Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
>
> drm/scdc-helper: Add scdc_status debugfs entry
>
> This commit adds a debugfs entry under the connector to expose SCDC status
> information. It introduces drm_scdc_read_state to read and parse the SCDC
> registers and a debugfs init function for connectors to use.
>
> > diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
> > index 8403f2390ab6a..97256b272329a 100644
> > --- a/drivers/gpu/drm/display/drm_scdc_helper.c
> > +++ b/drivers/gpu/drm/display/drm_scdc_helper.c
> [ ... ]
> > @@ -276,3 +278,187 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector,
> [ ... ]
> > +static int scdc_status_show(struct seq_file *m, void *data)
> > +{
> > + struct drm_connector *connector = m->private;
> > + struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
> > + struct drm_scdc_state *st;
> > + int i, ret;
> > +
> > + drm_connector_get(connector);
> > +
> > + ret = mutex_lock_interruptible(&connector->dev->mode_config.mutex);
> > + if (ret)
> > + goto err_conn_put;
> > +
> > + if (connector->status != connector_status_connected) {
> > + ret = -ENODEV;
> > + goto err_conn_put;
>
> [Severity: High]
> Does this error path leak connector->dev->mode_config.mutex?
>
> The mutex is successfully acquired just above this check. If the connector
> is not connected, jumping directly to err_conn_put bypasses the err_unlock
> label where the mutex is supposed to be released.
This one is real, so I'll need to send a v9 to go to the right label here.
Feel free to provide any feedback you've got on top of that problem, so
that I don't needlessly spam the list with a 1-line-oopsie every day.
Kind regards,
Nicolas Frattaroli
> > + }
> > +
> > + if (scdc->supported) {
> [ ... ]
> > +
> > +err_free_state:
> > + kfree(st);
> > +err_unlock:
> > + mutex_unlock(&connector->dev->mode_config.mutex);
> > +err_conn_put:
> > + drm_connector_put(connector);
> > +
> > + return ret;
> > +}
>
>
@@ -24,7 +24,9 @@
#include <linux/export.h>
#include <linux/i2c.h>
#include <linux/slab.h>
+#include <linux/debugfs.h>
#include <linux/delay.h>
+#include <linux/overflow.h>
#include <drm/display/drm_scdc_helper.h>
#include <drm/drm_connector.h>
@@ -276,3 +278,187 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector,
return true;
}
EXPORT_SYMBOL(drm_scdc_set_high_tmds_clock_ratio);
+
+static void
+drm_scdc_parse_status0_flags(u8 val, struct drm_scdc_status_flags *flags)
+{
+ flags->clock_detected = val & SCDC_CLOCK_DETECT;
+ flags->ch0_locked = val & SCDC_CH0_LOCK;
+ flags->ch1_locked = val & SCDC_CH1_LOCK;
+ flags->ch2_locked = val & SCDC_CH2_LOCK;
+}
+
+static int drm_scdc_parse_error_counters(const u8 scdc[256], u16 counter[3])
+{
+ u8 sum = 0;
+ int i;
+
+ for (i = SCDC_ERR_DET_0_L; i <= SCDC_ERR_DET_CHECKSUM ; i++)
+ sum = wrapping_add(u8, sum, scdc[i]);
+
+ if (sum)
+ return -EPROTO;
+
+ for (i = 0; i < 3; i++) {
+ if (scdc[SCDC_ERR_DET_0_H + i * 2] & SCDC_CHANNEL_VALID)
+ counter[i] = (scdc[SCDC_ERR_DET_0_H + i * 2] &
+ ~SCDC_CHANNEL_VALID) << 8 |
+ scdc[SCDC_ERR_DET_0_L + i * 2];
+ else
+ counter[i] = 0;
+ }
+
+ return 0;
+}
+
+/**
+ * drm_scdc_read_state - Update state from SCDC
+ * @connector: pointer to a &struct drm_connector on which to operate on
+ * @state: pointer to a &struct drm_scdc_state to fill
+ *
+ * Reads the entire 256 byte SCDC state and parses it.
+ *
+ * Returns: %0 on success, negative errno on failure.
+ */
+int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *state)
+{
+ struct i2c_adapter *ddc;
+ struct drm_scdc *scdc;
+ u8 *buf = state->scdc;
+ int ret;
+
+ if (!state || !connector)
+ return -ENODEV;
+
+ scdc = &connector->display_info.hdmi.scdc;
+ ddc = connector->ddc;
+
+ if (!scdc->supported || !ddc)
+ return -EOPNOTSUPP;
+
+ /* Read in 128-byte chunks, to work around DP<->HDMI converters with issues. */
+ ret = drm_scdc_read(ddc, 0, buf, 128);
+ if (ret)
+ return ret;
+
+ ret = drm_scdc_read(ddc, 128, &buf[128], 128);
+ if (ret)
+ return ret;
+
+ state->scrambling_enabled = buf[SCDC_TMDS_CONFIG] & SCDC_SCRAMBLING_ENABLE;
+ state->tmds_bclk_x40 = buf[SCDC_TMDS_CONFIG] & SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
+
+ state->scrambling_detected = buf[SCDC_SCRAMBLER_STATUS] & SCDC_SCRAMBLING_STATUS;
+
+ drm_scdc_parse_status0_flags(buf[SCDC_STATUS_FLAGS_0], &state->stf);
+ ret = drm_scdc_parse_error_counters(buf, state->error_count);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_scdc_read_state);
+
+#define scdc_print_str(_f, key, s) \
+ (seq_printf((_f), "%-30s: %s\n", (key), (s)))
+#define scdc_print_flag(_f, key, val) \
+ (scdc_print_str((_f), (key), str_yes_no((val))))
+#define scdc_print_dec(_f, key, val) \
+ (seq_printf((_f), "%-30s: %d\n", (key), (val)))
+
+static int scdc_status_show(struct seq_file *m, void *data)
+{
+ struct drm_connector *connector = m->private;
+ struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
+ struct drm_scdc_state *st;
+ int i, ret;
+
+ drm_connector_get(connector);
+
+ ret = mutex_lock_interruptible(&connector->dev->mode_config.mutex);
+ if (ret)
+ goto err_conn_put;
+
+ if (connector->status != connector_status_connected) {
+ ret = -ENODEV;
+ goto err_conn_put;
+ }
+
+ if (scdc->supported) {
+ st = kzalloc_obj(*st);
+ if (!st) {
+ ret = -ENOMEM;
+ goto err_unlock;
+ }
+ ret = drm_scdc_read_state(connector, st);
+ if (ret)
+ goto err_free_state;
+
+ for (i = 0; i < ARRAY_SIZE(st->scdc); i += 16)
+ seq_printf(m, "%*ph\n", 16, &st->scdc[i]);
+
+ seq_puts(m, "\n----------------\n\n");
+ }
+
+ scdc_print_flag(m, "SCDC Supported", scdc->supported);
+ if (!scdc->supported) {
+ ret = 0;
+ goto err_unlock;
+ }
+
+ scdc_print_flag(m, "Sink Read Request Capable", scdc->read_request);
+ scdc_print_flag(m, "Scrambling Supported", scdc->scrambling.supported);
+ scdc_print_flag(m, "Low Rate Scrambling Supported", scdc->scrambling.low_rates);
+
+ mutex_unlock(&connector->dev->mode_config.mutex);
+
+ drm_connector_put(connector);
+
+ scdc_print_flag(m, "Scrambling Enabled", st->scrambling_enabled);
+ scdc_print_flag(m, "Scrambling Detected", st->scrambling_detected);
+
+ if (st->tmds_bclk_x40)
+ scdc_print_str(m, "TMDS Bit Clock Ratio", "1/40");
+ else
+ scdc_print_str(m, "TMDS Bit Clock Ratio", "1/10");
+
+ scdc_print_flag(m, "Clock Detected", st->stf.clock_detected);
+ scdc_print_flag(m, "Channel 0 Locked", st->stf.ch0_locked);
+ scdc_print_flag(m, "Channel 1 Locked", st->stf.ch1_locked);
+ scdc_print_flag(m, "Channel 2 Locked", st->stf.ch2_locked);
+
+ scdc_print_dec(m, "Channel 0 Errors", st->error_count[0]);
+ scdc_print_dec(m, "Channel 1 Errors", st->error_count[1]);
+ scdc_print_dec(m, "Channel 2 Errors", st->error_count[2]);
+
+ kfree(st);
+
+ return 0;
+
+err_free_state:
+ kfree(st);
+err_unlock:
+ mutex_unlock(&connector->dev->mode_config.mutex);
+err_conn_put:
+ drm_connector_put(connector);
+
+ return ret;
+}
+DEFINE_SHOW_ATTRIBUTE(scdc_status);
+
+/**
+ * drm_scdc_debugfs_init - Initialize scdc files in connector debugfs
+ * @connector: pointer to &struct drm_connector to operate on
+ * @root: debugfs &struct dentry for the debugfs root of @connector
+ *
+ * Creates SCDC-related debugfs files for @connector. Must be called after
+ * @root is already created.
+ */
+void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root)
+{
+ if (!root || !connector)
+ return;
+
+ debugfs_create_file("scdc_status", 0444, root, connector, &scdc_status_fops);
+}
+EXPORT_SYMBOL(drm_scdc_debugfs_init);
@@ -30,6 +30,34 @@
struct drm_connector;
struct i2c_adapter;
+struct dentry;
+
+struct drm_scdc_status_flags {
+ /* Status Register 0 */
+ bool clock_detected;
+ bool ch0_locked;
+ bool ch1_locked;
+ bool ch2_locked;
+};
+
+struct drm_scdc_state {
+ /** @stf: contents of the status flag registers */
+ struct drm_scdc_status_flags stf;
+ /** @scramling_enabled: true if TMDS scrambling is on */
+ bool scrambling_enabled;
+ /** @scrambling_detected: true if the sink actually detected scrambling */
+ bool scrambling_detected;
+ /**
+ * @tmds_bclk_x40: true if TMDS bit period is 1/40th of the TMDS
+ * clock period, false if it's 1/10th of the clock period.
+ */
+ bool tmds_bclk_x40;
+ /** @error_count: character error counts for each channel */
+ u16 error_count[3];
+
+ /** @scdc: raw SCDC data buffer */
+ u8 scdc[256];
+};
int drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
size_t size);
@@ -77,4 +105,8 @@ bool drm_scdc_get_scrambling_status(struct drm_connector *connector);
bool drm_scdc_set_scrambling(struct drm_connector *connector, bool enable);
bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector, bool set);
+int drm_scdc_read_state(struct drm_connector *connector,
+ struct drm_scdc_state *state);
+void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root);
+
#endif