Re: [PATCH v12 02/23] drm/bridge: synopsys: dw-dp: Fix incorrect resource lifetimes in bind callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] drm/rockchip: dw_dp: bridge not detached on unbind in
  dw_dp_rockchip_unbind()
- [Medium] drm/bridge: dw-dp: disable_irq() in dw_dp_bridge_detach()
  starves AUX transfers
- [Low] drm/bridge: dw-dp: missing struct platform_device forward
  declaration
--

commit a9596a762ddd1d48c8d46fe4311e13805a449537
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Fix incorrect resource lifetimes in bind callback

This commit moves the DesignWare DP controller's devm-managed resource
allocation and bridge registration from dw_dp_bind() into a new
dw_dp_probe() helper, leaving only drm_bridge_attach() in dw_dp_bind() and
enabling the interrupt only while the bridge is attached.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 0a34d6a6ace46..ac16d290d49be 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -1827,16 +1827,22 @@ static int dw_dp_bridge_attach(struct drm_bridge *bridge,
>  	dp->aux.transfer = dw_dp_aux_transfer;
>  
>  	ret = drm_dp_aux_register(&dp->aux);
> -	if (ret)
> +	if (ret) {
>  		dev_err(dev, "Aux register failed: %d\n", ret);
> +		return ret;
> +	}
>  
> -	return ret;
> +	enable_irq(dp->irq);
> +
> +	return 0;
>  }
>  
>  static void dw_dp_bridge_detach(struct drm_bridge *bridge)
>  {
>  	struct dw_dp *dp = bridge_to_dp(bridge);
>  
> +	disable_irq(dp->irq);
> +	cancel_work_sync(&dp->hpd_work);
>  	drm_dp_aux_unregister(&dp->aux);
>  }

[Severity: Medium]
Does calling disable_irq(dp->irq) before cancel_work_sync(&dp->hpd_work)
and drm_dp_aux_unregister(&dp->aux) starve in-flight AUX transfers?

Both HPD_EVENT and AUX_REPLY_EVENT share dp->irq, and dw_dp_aux_transfer()
waits on dp->complete, which is signaled from dw_dp_irq().  If
dw_dp_hpd_work() or a /dev/drm_dp_aux* transaction is running when
dw_dp_bridge_detach() disables dp->irq, AUX_REPLY_EVENT is never delivered
and every DPCD retry in drm_dp_dpcd_access() stalls until
wait_for_completion_timeout() expires.

[ ... ]

> @@ -1982,6 +1988,18 @@ static const struct regmap_config dw_dp_regmap_config = {
>  	.rd_table = &dw_dp_readable_table,
>  };
>  
> +int dw_dp_bind(struct dw_dp *dp, struct drm_encoder *encoder)
> +{
> +	return drm_bridge_attach(encoder, &dp->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +}
> +EXPORT_SYMBOL_GPL(dw_dp_bind);
> +
> +void dw_dp_unbind(struct dw_dp *dp)
> +{
> +	/* nothing to do as bridge is detached automatically */
> +}
> +EXPORT_SYMBOL_GPL(dw_dp_unbind);

[ ... ]

> diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> index b23efb153c9e6..38e8fe75718e4 100644
> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c

[ ... ]

> @@ -73,37 +73,28 @@ static const struct drm_encoder_helper_funcs dw_dp_encoder_helper_funcs = {
>  
>  static int dw_dp_rockchip_bind(struct device *dev, struct device *master, void *data)
>  {
> -	struct platform_device *pdev = to_platform_device(dev);
> -	const struct dw_dp_plat_data *plat_data;
> +	struct rockchip_dw_dp *dp = dev_get_drvdata(dev);
>  	struct drm_device *drm_dev = data;
> -	struct rockchip_dw_dp *dp;
>  	struct drm_encoder *encoder;
>  	struct drm_connector *connector;
>  	int ret;
>  
> -	dp = drmm_kzalloc(drm_dev, sizeof(*dp), GFP_KERNEL);
> -	if (!dp)
> +	dp->encoder = drmm_kzalloc(drm_dev, sizeof(*dp->encoder), GFP_KERNEL);
> +	if (!dp->encoder)
>  		return -ENOMEM;
>  
> -	dp->dev = dev;
> -	platform_set_drvdata(pdev, dp);
> -
> -	plat_data = of_device_get_match_data(dev);
> -	if (!plat_data)
> -		return -ENODEV;
> -
> -	encoder = &dp->encoder.encoder;
> +	encoder = &dp->encoder->encoder;
>  	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm_dev, dev->of_node);
> -	rockchip_drm_encoder_set_crtc_endpoint_id(&dp->encoder, dev->of_node, 0, 0);
> +	rockchip_drm_encoder_set_crtc_endpoint_id(dp->encoder, dev->of_node, 0, 0);
>  
>  	ret = drmm_encoder_init(drm_dev, encoder, NULL, DRM_MODE_ENCODER_TMDS, NULL);
>  	if (ret)
>  		return ret;
>  	drm_encoder_helper_add(encoder, &dw_dp_encoder_helper_funcs);
>  
> -	dp->base = dw_dp_bind(dev, encoder, plat_data);
> -	if (IS_ERR(dp->base))
> -		return PTR_ERR(dp->base);
> +	ret = dw_dp_bind(dp->base, encoder);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to bind DW-DP bridge\n");
>  
>  	connector = drm_bridge_connector_init(drm_dev, encoder);
>  	if (IS_ERR(connector)) {

[Severity: High]
Does keeping drmm_encoder_init() while making dw_dp_unbind() a no-op leave
dp->bridge attached across component unbind?

drmm_encoder_init() defers drm_encoder_cleanup() -> drm_bridge_detach() ->
dw_dp_bridge_detach() until the final drm_device reference drops in
drm_managed_release().  If the component is unbound while userspace holds
an open file descriptor to /dev/dri/card*, drm_dev_put() in
rockchip_drm_unbind() does not release drm_dev:

- If the dw-dp platform device is then removed, devres_release_all() frees
  dp->irq, dp->regmap, MMIO, clocks and dp->phy while dw_dp_bridge_detach()
  has not run, leaving dp->hpd_work and dp->aux active against freed
  resources (use-after-free) and later calling disable_irq() on a freed IRQ
  when the DRM fd closes.
- If the component is rebound while the old DRM fd is still open,
  dp->bridge.dev is still non-NULL and drm_bridge_attach() in dw_dp_bind()
  fails with -EBUSY.
- If drm_bridge_connector_init() or a later component's bind fails, the
  error path calls the no-op dw_dp_unbind(), leaving dp->irq enabled and
  dp->aux registered while earlier components are unbound.

Initializing the encoder with drm_encoder_init() and calling
drm_encoder_cleanup() in dw_dp_rockchip_unbind() and on the
drm_bridge_connector_init() error path (as analogix_dp-rockchip.c and
cdn-dp-core.c do) would detach the bridge synchronously at unbind time.

[ ... ]

> diff --git a/include/drm/bridge/dw_dp.h b/include/drm/bridge/dw_dp.h
> index 22105c3e8e4d6..a82412a9e7695 100644
> --- a/include/drm/bridge/dw_dp.h
> +++ b/include/drm/bridge/dw_dp.h
> @@ -22,7 +22,8 @@ struct dw_dp_plat_data {
>  	u8 pixel_mode;
>  };
>  
> -struct dw_dp *dw_dp_bind(struct device *dev, struct drm_encoder *encoder,
> -			 const struct dw_dp_plat_data *plat_data);
> +int dw_dp_bind(struct dw_dp *dp, struct drm_encoder *encoder);
>  void dw_dp_unbind(struct dw_dp *dp);
> +
> +struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_data *plat_data);
>  #endif /* __DW_DP__ */

[Severity: Low]
Does include/drm/bridge/dw_dp.h need a forward declaration for struct
platform_device?

This header only includes <linux/device.h> and forward-declares struct
drm_encoder and struct dw_dp, so compiling it standalone (e.g. with
CONFIG_DRM_HEADER_TEST=y) or before <linux/platform_device.h> triggers a
-Wvisibility warning on dw_dp_probe().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-synopsys-dw-dp-improvements-v12-0-d42d3dc5bdd4@collabora.com?part=2