Re: [PATCH 22/24] drm/msm/dp: stop storing the connector in struct msm_dp

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] msm_dp_display_get_connector() returns a connector pointer without holding a reference, violating the DRM iterator API.

Pre-existing issues:
- [High] Missing assignment of `debug->connector` leads to a guaranteed NULL pointer dereference when reading debugfs files.
--

--- Patch [22]: [PATCH 22/24] drm/msm/dp: stop storing the connector in struct msm_dp ---
commit 6517fb125f3c91df434e357450ad362fc4f2fec3
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm/dp: stop storing the connector in struct msm_dp

This commit removes the cached drm_connector from struct msm_dp. Instead of
storing the connector, consumers are updated to obtain it from their calling
context, the atomic state, or by looking it up from the encoder.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -1329,6 +1341,25 @@ bool msm_dp_wide_bus_available(const struct msm_dp *msm_dp_display)
>  	return dp->wide_bus_supported;
>  }
>  
> +static struct drm_connector *
> +msm_dp_display_get_connector(struct msm_dp *msm_dp_display)
> +{
> +	struct drm_encoder *encoder = msm_dp_display->bridge->encoder;
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_connector *connector, *found = NULL;
> +
> +	drm_connector_list_iter_begin(msm_dp_display->drm_dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		if (drm_connector_has_possible_encoder(connector, encoder)) {
> +			found = connector;
> +			break;
> +		}
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +
> +	return found;
> +}

[Severity: Medium]
When the loop breaks, drm_connector_list_iter_end() automatically drops the
reference on the current iterator connector. Since drm_connector_get() is not
called on found before returning, does this return a raw pointer without an
elevated reference count?

> +
>  void msm_dp_display_debugfs_init(struct msm_dp *msm_dp_display, struct dentry *root, bool is_edp)
>  {
>  	struct msm_dp_display_private *dp;
> @@ -1338,7 +1369,9 @@ void msm_dp_display_debugfs_init(struct msm_dp *msm_dp_display, struct dentry *r
>  	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
>  	dev = &dp->msm_dp_display.pdev->dev;
>  
> -	rc = msm_dp_debug_init(dev, dp->panel, dp->link, dp->msm_dp_display.connector, root, is_edp);
> +	rc = msm_dp_debug_init(dev, dp->panel, dp->link,
> +			       msm_dp_display_get_connector(msm_dp_display),
> +			       root, is_edp);

[Severity: High]
This is a pre-existing issue, but I noticed that msm_dp_debug_init() in
dp_debug.c accepts the connector argument but never assigns it to the
allocated debug structure (missing debug->connector = connector).

Later, when a user reads from debugfs nodes like dp_test_active, the show
handlers dereference debug->connector. Since it remains NULL from the
devm_kzalloc() allocation, will reading these debugfs files cause a NULL
pointer dereference?

>  	if (rc)
>  		DRM_ERROR("failed to initialize debug, rc = %d\n", rc);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-drm-msm-display-interface-v1-0-368c10fe62fd@oss.qualcomm.com?part=22