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

["Andy Yan" <andyshrk@163.com>]

Hello Sebastian,

At 2026-08-07 01:02:08, "Sebastian Reichel" <sebastian.reichel@collabora.com> wrote:
>Currently the Synopsys DesignWare DP controller driver's bind function
>requests lots of resources using device managed functions. These are
>free'd on driver removal instead of at unbind time. Fix this discrepancy
>by introducing a new probe helper function and moving over the whole
>bind function. This results in a fully functional DRM bridge once probe
>succeeded. The only thing still happening when the component is bound
>is the bridge attachment, which requires the encoder.
>
>The interrupt is kept disabled while the bridge is detached to ensure no
>spurious interrupts can arrive as the interrupt handler triggers a
>worker, which accesses the DRM device.
>
>Fixes: 86eecc3a9c2e ("drm/bridge: synopsys: Add DW DPTX Controller support library")
>Reported-by: Sashiko <sashiko-bot@kernel.org>
>Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

   Acked-by: Andy Yan <andy.yan@rock-chips.com>

>---
> drivers/gpu/drm/bridge/synopsys/dw-dp.c   | 73 ++++++++++++++++---------------
> drivers/gpu/drm/rockchip/dw_dp-rockchip.c | 53 ++++++++++++----------
> include/drm/bridge/dw_dp.h                |  5 ++-
> 3 files changed, 72 insertions(+), 59 deletions(-)
>
>diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
>index 60feb3d1e14b..d7945f7fe9f0 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);
> }
> 
>@@ -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);
>+
> static void dw_dp_phy_exit(void *data)
> {
> 	struct dw_dp *dp = data;
>@@ -1989,13 +2007,12 @@ static void dw_dp_phy_exit(void *data)
> 	phy_exit(dp->phy);
> }
> 
>-struct dw_dp *dw_dp_bind(struct device *dev, struct drm_encoder *encoder,
>-			 const struct dw_dp_plat_data *plat_data)
>+struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_data *plat_data)
> {
>-	struct platform_device *pdev = to_platform_device(dev);
>-	struct dw_dp *dp;
>+	struct device *dev = &pdev->dev;
> 	struct drm_bridge *bridge;
> 	void __iomem *res;
>+	struct dw_dp *dp;
> 	int ret;
> 
> 	dp = devm_drm_bridge_alloc(dev, struct dw_dp, bridge, &dw_dp_bridge_funcs);
>@@ -2004,9 +2021,8 @@ struct dw_dp *dw_dp_bind(struct device *dev, struct drm_encoder *encoder,
> 
> 	dp->dev = dev;
> 	dp->pixel_mode = plat_data->pixel_mode;
>-
> 	dp->plat_data.max_link_rate = plat_data->max_link_rate;
>-	bridge = &dp->bridge;
>+
> 	mutex_init(&dp->irq_lock);
> 	INIT_WORK(&dp->hpd_work, dw_dp_hpd_work);
> 	init_completion(&dp->complete);
>@@ -2063,18 +2079,14 @@ struct dw_dp *dw_dp_bind(struct device *dev, struct drm_encoder *encoder,
> 		return ERR_CAST(dp->rstc);
> 	}
> 
>-	bridge->of_node = dev->of_node;
>-	bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD;
>-	bridge->type = DRM_MODE_CONNECTOR_DisplayPort;
>-	bridge->ycbcr_420_allowed = true;
>-
>-	ret = devm_drm_bridge_add(dev, bridge);
>-	if (ret)
>-		return ERR_PTR(ret);
>+	dp->irq = platform_get_irq(pdev, 0);
>+	if (dp->irq < 0)
>+		return ERR_PTR(dp->irq);
> 
>-	ret = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>+	ret = devm_request_threaded_irq(dev, dp->irq, NULL, dw_dp_irq,
>+					IRQF_ONESHOT | IRQF_NO_AUTOEN, dev_name(dev), dp);
> 	if (ret) {
>-		dev_err_probe(dev, ret, "Failed to attach bridge\n");
>+		dev_err_probe(dev, ret, "failed to request irq\n");
> 		return ERR_PTR(ret);
> 	}
> 
>@@ -2090,28 +2102,19 @@ struct dw_dp *dw_dp_bind(struct device *dev, struct drm_encoder *encoder,
> 	if (ret)
> 		return ERR_PTR(ret);
> 
>-	dp->irq = platform_get_irq(pdev, 0);
>-	if (dp->irq < 0) {
>-		ret = dp->irq;
>-		return ERR_PTR(ret);
>-	}
>+	bridge = &dp->bridge;
>+	bridge->of_node = dev->of_node;
>+	bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD;
>+	bridge->type = DRM_MODE_CONNECTOR_DisplayPort;
>+	bridge->ycbcr_420_allowed = true;
> 
>-	ret = devm_request_threaded_irq(dev, dp->irq, NULL, dw_dp_irq,
>-					IRQF_ONESHOT, dev_name(dev), dp);
>-	if (ret) {
>-		dev_err_probe(dev, ret, "failed to request irq\n");
>+	ret = devm_drm_bridge_add(dev, bridge);
>+	if (ret)
> 		return ERR_PTR(ret);
>-	}
> 
> 	return dp;
> }
>-EXPORT_SYMBOL_GPL(dw_dp_bind);
>-
>-void dw_dp_unbind(struct dw_dp *dp)
>-{
>-	/* nothing to do */
>-}
>-EXPORT_SYMBOL_GPL(dw_dp_unbind);
>+EXPORT_SYMBOL_GPL(dw_dp_probe);
> 
> MODULE_AUTHOR("Andy Yan <andyshrk@163.com>");
> MODULE_DESCRIPTION("DW DP Core Library");
>diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
>index b23efb153c9e..38e8fe75718e 100644
>--- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
>+++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
>@@ -26,7 +26,7 @@
> struct rockchip_dw_dp {
> 	struct dw_dp *base;
> 	struct device *dev;
>-	struct rockchip_encoder encoder;
>+	struct rockchip_encoder *encoder;
> };
> 
> static int dw_dp_encoder_atomic_check(struct drm_encoder *encoder,
>@@ -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)) {
>@@ -128,12 +119,30 @@ static const struct component_ops dw_dp_rockchip_component_ops = {
> 	.unbind = dw_dp_rockchip_unbind,
> };
> 
>-static int dw_dp_probe(struct platform_device *pdev)
>+static int dw_dp_rockchip_probe(struct platform_device *pdev)
> {
>+	const struct dw_dp_plat_data *plat_data;
>+	struct device *dev = &pdev->dev;
>+	struct rockchip_dw_dp *dp;
>+
>+	plat_data = of_device_get_match_data(dev);
>+	if (!plat_data)
>+		return -ENODEV;
>+
>+	dp = devm_kzalloc(dev, sizeof(*dp), GFP_KERNEL);
>+	if (!dp)
>+		return -ENOMEM;
>+	platform_set_drvdata(pdev, dp);
>+	dp->dev = dev;
>+
>+	dp->base = dw_dp_probe(pdev, plat_data);
>+	if (IS_ERR(dp->base))
>+		return PTR_ERR(dp->base);
>+
> 	return component_add(&pdev->dev, &dw_dp_rockchip_component_ops);
> }
> 
>-static void dw_dp_remove(struct platform_device *pdev)
>+static void dw_dp_rockchip_remove(struct platform_device *pdev)
> {
> 	component_del(&pdev->dev, &dw_dp_rockchip_component_ops);
> }
>@@ -161,8 +170,8 @@ static const struct of_device_id dw_dp_of_match[] = {
> MODULE_DEVICE_TABLE(of, dw_dp_of_match);
> 
> struct platform_driver dw_dp_driver = {
>-	.probe	= dw_dp_probe,
>-	.remove = dw_dp_remove,
>+	.probe	= dw_dp_rockchip_probe,
>+	.remove = dw_dp_rockchip_remove,
> 	.driver = {
> 		.name = "dw-dp",
> 		.of_match_table = dw_dp_of_match,
>diff --git a/include/drm/bridge/dw_dp.h b/include/drm/bridge/dw_dp.h
>index 22105c3e8e4d..a82412a9e769 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__ */
>
>-- 
>2.53.0
>