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

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 <[email protected]>

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);

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