[v7,2/5] drm/scdc-helper: Add scdc_status debugfs entry

SCDC provides status information on the current display link. At the
very least, it may be useful to expose this info through debugfs.

Add a debugfs entry for it under the connector, which displays a few
more details parsed out of the SCDC registers. A new
drm_scdc_debugfs_init function can be called by the connector
implementation to initialise the debugfs file.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/display/drm_scdc_helper.c | 184 ++++++++++++++++++++++++++++++
 include/drm/display/drm_scdc_helper.h     |  32 ++++++
 2 files changed, 216 insertions(+)
  

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Shared `scdc_debugfs_priv` state causes a data race on concurrent debugfs reads and leaks memory for dynamic connectors.
- [Medium] Connector `status` and `display_info` fields are accessed without holding the required DRM mode config mutex.
--

--- Patch [2]: [PATCH v7 2/5] drm/scdc-helper: Add scdc_status debugfs entry ---
commit 16b712ae50ae43d528857efe5b800f5abdef139f
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

drm/scdc-helper: Add scdc_status debugfs entry

This commit adds a debugfs entry under the DRM connector to expose SCDC link
status information.

> diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
> index 8403f2390ab6a..5871fc101815d 100644
> --- a/drivers/gpu/drm/display/drm_scdc_helper.c
> +++ b/drivers/gpu/drm/display/drm_scdc_helper.c
[ ... ]
> @@ -276,3 +284,179 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector,
[ ... ]
> +static int scdc_status_show(struct seq_file *m, void *data)
> +{
> +	struct scdc_debugfs_priv *priv = m->private;
> +	struct drm_scdc_state *st = &priv->state;
> +	struct drm_connector *connector = priv->connector;
> +	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
> +	int i, ret;
> +
> +	drm_connector_get(connector);
> +
> +	if (connector->status != connector_status_connected) {

[Severity: Medium]
Are we missing a lock here? Reading connector->status and 
connector->display_info (such as scdc->supported below) without holding the 
required drm_mode_config.mutex might race with concurrent modeset or hotplug 
events, potentially leading to torn reads or an inconsistent state.

> +		ret = -ENODEV;
> +		goto err_conn_put;
> +	}
> +
> +	if (scdc->supported) {
> +		ret = drm_scdc_read_state(connector, st);

[Severity: High]
Can this cause a data race? Multiple processes reading this debugfs file
concurrently will simultaneously mutate the shared priv->state buffer inside 
drm_scdc_read_state(), leading to corrupted state data being displayed.

> +		if (ret)
> +			goto err_conn_put;
> +
> +		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");
> +	}
[ ... ]
> +void drm_scdc_debugfs_init(struct drm_connector *connector, struct dentry *root)
> +{
> +	struct scdc_debugfs_priv *priv;
> +
> +	if (!root || !connector)
> +		return;
> +
> +	priv = drmm_kzalloc(connector->dev, sizeof(*priv), GFP_KERNEL);

[Severity: High]
Does this leak memory on hot-unplug? Allocating the priv structure using 
drmm_kzalloc() ties its lifetime to the DRM device rather than the connector. 
For dynamic hotpluggable connectors (like DP MST), this memory might not be 
freed when the connector is destroyed.

> +	if (!priv)
> +		return;
> +
> +	priv->connector = connector;
> +
> +	debugfs_create_file("scdc_status", 0444, root, priv, &scdc_status_fops);
> +}
> +EXPORT_SYMBOL(drm_scdc_debugfs_init);
  

On Tue, Jul 21, 2026 at 10:58:58AM +0200, Nicolas Frattaroli wrote:
> SCDC provides status information on the current display link. At the
> very least, it may be useful to expose this info through debugfs.
>
> Add a debugfs entry for it under the connector, which displays a few
> more details parsed out of the SCDC registers. A new
> drm_scdc_debugfs_init function can be called by the connector
> implementation to initialise the debugfs file.
>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_scdc_helper.c | 184 ++++++++++++++++++++++++++++++
>  include/drm/display/drm_scdc_helper.h     |  32 ++++++
>  2 files changed, 216 insertions(+)
>
> diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
> +/**
> + * 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)
> +{
> +	struct scdc_debugfs_priv *priv;
> +
> +	if (!root || !connector)
> +		return;
> +
> +	priv = drmm_kzalloc(connector->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return;
> +
> +	priv->connector = connector;

You need to take a reference to the connector pointer here with
drm_connector_get()

With this fixed,
Reviewed-by: Maxime Ripard <mripard@kernel.org>

Maxime
  

On Tuesday, 21 July 2026 14:41:42 Central European Summer Time Maxime Ripard wrote:
> On Tue, Jul 21, 2026 at 10:58:58AM +0200, Nicolas Frattaroli wrote:
> > SCDC provides status information on the current display link. At the
> > very least, it may be useful to expose this info through debugfs.
> >
> > Add a debugfs entry for it under the connector, which displays a few
> > more details parsed out of the SCDC registers. A new
> > drm_scdc_debugfs_init function can be called by the connector
> > implementation to initialise the debugfs file.
> >
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  drivers/gpu/drm/display/drm_scdc_helper.c | 184 ++++++++++++++++++++++++++++++
> >  include/drm/display/drm_scdc_helper.h     |  32 ++++++
> >  2 files changed, 216 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
> > +/**
> > + * 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)
> > +{
> > +	struct scdc_debugfs_priv *priv;
> > +
> > +	if (!root || !connector)
> > +		return;
> > +
> > +	priv = drmm_kzalloc(connector->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return;
> > +
> > +	priv->connector = connector;
> 
> You need to take a reference to the connector pointer here with
> drm_connector_get()
> 
> With this fixed,
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> 
> Maxime
> 

I think sashiko is onto something here with the memleak and the
entirety of the priv thing should go, which is fairly easy to do.
We don't reuse the state anymore anyways.

I'll then just pass the connector as the priv, which will be valid
for the lifetime of the debugfs entry. This means there's no cleanup.

Kind regards,
Nicolas Frattaroli
  

On Tue, Jul 21, 2026 at 03:01:12PM +0200, Nicolas Frattaroli wrote:
> On Tuesday, 21 July 2026 14:41:42 Central European Summer Time Maxime Ripard wrote:
> > On Tue, Jul 21, 2026 at 10:58:58AM +0200, Nicolas Frattaroli wrote:
> > > SCDC provides status information on the current display link. At the
> > > very least, it may be useful to expose this info through debugfs.
> > >
> > > Add a debugfs entry for it under the connector, which displays a few
> > > more details parsed out of the SCDC registers. A new
> > > drm_scdc_debugfs_init function can be called by the connector
> > > implementation to initialise the debugfs file.
> > >
> > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > > ---
> > >  drivers/gpu/drm/display/drm_scdc_helper.c | 184 ++++++++++++++++++++++++++++++
> > >  include/drm/display/drm_scdc_helper.h     |  32 ++++++
> > >  2 files changed, 216 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
> > > +/**
> > > + * 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)
> > > +{
> > > +	struct scdc_debugfs_priv *priv;
> > > +
> > > +	if (!root || !connector)
> > > +		return;
> > > +
> > > +	priv = drmm_kzalloc(connector->dev, sizeof(*priv), GFP_KERNEL);
> > > +	if (!priv)
> > > +		return;
> > > +
> > > +	priv->connector = connector;
> > 
> > You need to take a reference to the connector pointer here with
> > drm_connector_get()
> > 
> > With this fixed,
> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> > 
> > Maxime
> > 
> 
> I think sashiko is onto something here with the memleak and the
> entirety of the priv thing should go, which is fairly easy to do.
> We don't reuse the state anymore anyways.
> 
> I'll then just pass the connector as the priv, which will be valid
> for the lifetime of the debugfs entry. This means there's no cleanup.

Connectors can be hotplugged, so they have a shorter (or equal) lifetime
than what drmm_kzalloc provides.

Maxime