Re: [PATCH v2 3/3] drm/rockchip: dw_dp: Attach "max bpc" connector property

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Unconditional connector state allocation causes a memory leak if an HDMI bridge is present in the chain.

Pre-existing issues:
- [High] `devm_` allocations tied to the component device in `dw_dp_bind` cause resource leaks and permanent driver failure if `bind` fails.
--

commit 47f0f730df97f46871d26a1858e6f2594ea7c383
Author: Igor Paunovic <royalnet026@gmail.com>

drm/rockchip: dw_dp: Attach "max bpc" connector property

This commit attaches the "max bpc" property to the dw-dp bridge connector
with a range of 6 to 10. It also explicitly creates the connector state
before drm_mode_config_reset() runs, allowing the property to be attached
successfully during driver initialization.

> diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> index b23efb153c9e6..8ff27353b1417 100644
> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
[ ... ]
> @@ -112,6 +112,32 @@ static int dw_dp_rockchip_bind(struct device *dev, struct device *master, void *
>  				     "Failed to init bridge connector\n");
>  	}
>  
> +	/*
> +	 * The components are bound before drm_mode_config_reset() runs, so
> +	 * the connector has no state yet, but
> +	 * drm_connector_attach_max_bpc_property() requires one. Create the
> +	 * connector state first, like drmm_connector_hdmi_init() does.
> +	 */
> +	if (connector->funcs->atomic_create_state) {
> +		struct drm_connector_state *state;
> +
> +		state = connector->funcs->atomic_create_state(connector);
> +		if (IS_ERR(state)) {
> +			dw_dp_unbind(dp->base);
> +			return PTR_ERR(state);
> +		}
> +
> +		connector->state = state;

[Severity: Medium]
Does this unconditionally overwrite connector->state?

If the display pipeline contains an HDMI bridge (e.g., a DP-to-HDMI chip),
drm_bridge_connector_init() internally calls drmm_connector_hdmi_init(),
which allocates and assigns connector->state. Overwriting it here seems to
leak the previously allocated drm_connector_state.

> +	} else if (connector->funcs->reset) {
> +		connector->funcs->reset(connector);
> +	}
> +
> +	ret = drm_connector_attach_max_bpc_property(connector, 6, 10);
> +	if (ret) {
> +		dw_dp_unbind(dp->base);
> +		return ret;

[Severity: High]
This isn't a regression introduced by this patch, but if this newly added
error path is taken, does it permanently break the driver on deferred probe?

The component bind callback invokes dw_dp_bind() earlier in this function,
which performs devm_ allocations tied to the component device (dev) rather
than the master device. For example:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bind() {
    ...
    ret = devm_request_threaded_irq(dev, dp->irq, NULL, dw_dp_irq,
                                    IRQF_ONESHOT, dev_name(dev), dp);
    ...
}

Because the component device remains probed even if the bind callback fails,
these devm_ resources are not freed when this error path returns. On a
subsequent bind retry, dw_dp_bind() executes again. Since
devm_request_threaded_irq() does not use IRQF_SHARED, it will return
-EBUSY, permanently failing.

> +	}
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909163206.36409-1-royalnet026@gmail.com?part=3