diff mbox series

[v9,26/61] drm/rockchip: dw_hdmi_qp: Use local dev variable consistently in bind()

Message ID 20260723-dw-hdmi-qp-scramb-v9-26-4fb12ea22ac9@collabora.com (mailing list archive)
State New
Headers
Series Add HDMI 2.0 support to DW HDMI QP TX |

Commit Message

Cristian Ciocaltea July 23, 2026, 1:35 a.m. UTC 
Replace indirect struct device accesses via hdmi->dev and pdev->dev with
the local dev parameter already available in dw_hdmi_qp_rockchip_bind(),
for consistency and readability.

Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Maud Spierings <maud_spierings@hotmail.com>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 33 +++++++++++++-------------
 1 file changed, 16 insertions(+), 17 deletions(-)

Comments

sashiko-bot@kernel.org July 23, 2026, 1:49 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Improper use of devm_ helpers in a component .bind callback causes resource leaks and permanent bind failures.
- [High] Missing drvdata cleanup on .bind error paths leads to Use-After-Free during system suspend.
--

--- Patch [26]: [PATCH v9 26/61] drm/rockchip: dw_hdmi_qp: Use local dev variable consistently in bind() ---
commit 791ba45ce63b6456a9661d02cb988ba23b16a42a
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
drm/rockchip: dw_hdmi_qp: Use local dev variable consistently in bind()

This commit replaces indirect struct device accesses via hdmi->dev and
pdev->dev with the local dev parameter in dw_hdmi_qp_rockchip_bind().
This improves consistency and readability.

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index 369f1066dabe8..2aceff11d14b5 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
[ ... ]
> @@ -622,37 +622,36 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
>  						       "rockchip,grf");
>  	if (IS_ERR(hdmi->regmap))
> -		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->regmap),
> +		return dev_err_probe(dev, PTR_ERR(hdmi->regmap),
>  				     "Unable to get rockchip,grf\n");
>  
>  	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
>  							  "rockchip,vo-grf");
>  	if (IS_ERR(hdmi->vo_regmap))
> -		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->vo_regmap),
> +		return dev_err_probe(dev, PTR_ERR(hdmi->vo_regmap),
>  				     "Unable to get rockchip,vo-grf\n");
>  
> -	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
> +	ret = devm_clk_bulk_get_all_enabled(dev, &clks);

[Severity: High]
This is a pre-existing issue, but won't using devm_ helpers in a component
.bind() callback lead to resource leaks if bind fails or unbind is called?

Because devm_ resources are tied to the driver probe/remove lifecycle, they
won't be freed if dw_hdmi_qp_rockchip_bind() returns an error or if
dw_hdmi_qp_rockchip_unbind() is called.

If a probe deferral causes .bind() to fail, won't the next bind attempt fail
with -EBUSY on devm_request_threaded_irq() since the first allocation was
never freed?

>  	if (ret < 0)
> -		return dev_err_probe(hdmi->dev, ret, "Failed to get clocks\n");
> +		return dev_err_probe(dev, ret, "Failed to get clocks\n");
[ ... ]
> @@ -681,7 +680,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  	drm_encoder_helper_add(encoder, &dw_hdmi_qp_rockchip_encoder_helper_funcs);
>  	ret = drmm_encoder_init(drm, encoder, NULL, DRM_MODE_ENCODER_TMDS, NULL);
>  	if (ret)
> -		return dev_err_probe(hdmi->dev, ret, "Failed to init encoder\n");
> +		return dev_err_probe(dev, ret, "Failed to init encoder\n");
>  
>  	platform_set_drvdata(pdev, hdmi);
>  
> @@ -689,12 +688,12 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  	if (IS_ERR(hdmi->hdmi)) {
>  		ret = PTR_ERR(hdmi->hdmi);
>  		hdmi->hdmi = NULL;
> -		return dev_err_probe(hdmi->dev, ret, "Failed to bind dw-hdmi-qp\n");
> +		return dev_err_probe(dev, ret, "Failed to bind dw-hdmi-qp\n");
>  	}

[Severity: High]
This is a pre-existing issue, but does this error path leave a dangling
pointer in drvdata?

If dw_hdmi_qp_bind() fails, the function returns without clearing drvdata.
Because hdmi is allocated earlier with drmm_kzalloc(), it will be
automatically freed when the master DRM device cleans up following the bind
failure.

However, the component device remains bound to its driver, and its drvdata
still points to the freed hdmi struct. If the system then suspends, won't
dw_hdmi_qp_rockchip_suspend() retrieve this dangling pointer and dereference
it via hdmi->hdmi?

>  
>  	connector = drm_bridge_connector_init(drm, encoder);
>  	if (IS_ERR(connector))
> -		return dev_err_probe(hdmi->dev, PTR_ERR(connector),
> +		return dev_err_probe(dev, PTR_ERR(connector),
>  				     "Failed to init bridge connector\n");
>  
>  	return 0;
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index 369f1066dabe..2aceff11d14b 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -563,7 +563,7 @@  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	struct clk *ref_clk;
 	int ret, irq, i;
 
-	if (!pdev->dev.of_node)
+	if (!dev->of_node)
 		return -ENODEV;
 
 	hdmi = drmm_kzalloc(drm, sizeof(*hdmi), GFP_KERNEL);
@@ -583,7 +583,7 @@  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		return dev_err_probe(dev, -ENODEV, "Missing platform ctrl ops\n");
 
 	hdmi->ctrl_ops = cfg->ctrl_ops;
-	hdmi->dev = &pdev->dev;
+	hdmi->dev = dev;
 	hdmi->port_id = -ENODEV;
 
 	/* Identify port ID by matching base IO address */
@@ -594,7 +594,7 @@  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		}
 	}
 	if (hdmi->port_id < 0)
-		return dev_err_probe(hdmi->dev, hdmi->port_id,
+		return dev_err_probe(dev, hdmi->port_id,
 				     "Failed to match HDMI port ID\n");
 
 	plat_data.phy_ops = cfg->phy_ops;
@@ -622,37 +622,36 @@  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 						       "rockchip,grf");
 	if (IS_ERR(hdmi->regmap))
-		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->regmap),
+		return dev_err_probe(dev, PTR_ERR(hdmi->regmap),
 				     "Unable to get rockchip,grf\n");
 
 	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 							  "rockchip,vo-grf");
 	if (IS_ERR(hdmi->vo_regmap))
-		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->vo_regmap),
+		return dev_err_probe(dev, PTR_ERR(hdmi->vo_regmap),
 				     "Unable to get rockchip,vo-grf\n");
 
-	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
+	ret = devm_clk_bulk_get_all_enabled(dev, &clks);
 	if (ret < 0)
-		return dev_err_probe(hdmi->dev, ret, "Failed to get clocks\n");
+		return dev_err_probe(dev, ret, "Failed to get clocks\n");
 
-	ref_clk = clk_get(hdmi->dev, "ref");
+	ref_clk = clk_get(dev, "ref");
 	if (IS_ERR(ref_clk))
-		return dev_err_probe(hdmi->dev, PTR_ERR(ref_clk),
+		return dev_err_probe(dev, PTR_ERR(ref_clk),
 				     "Failed to get ref clock\n");
 
 	plat_data.ref_clk_rate = clk_get_rate(ref_clk);
 	clk_put(ref_clk);
 
-	hdmi->frl_enable_gpio = devm_gpiod_get_optional(hdmi->dev, "frl-enable",
+	hdmi->frl_enable_gpio = devm_gpiod_get_optional(dev, "frl-enable",
 							GPIOD_OUT_LOW);
 	if (IS_ERR(hdmi->frl_enable_gpio))
-		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->frl_enable_gpio),
+		return dev_err_probe(dev, PTR_ERR(hdmi->frl_enable_gpio),
 				     "Failed to request FRL enable GPIO\n");
 
 	hdmi->phy = devm_of_phy_get_by_index(dev, dev->of_node, 0);
 	if (IS_ERR(hdmi->phy))
-		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->phy),
-				     "Failed to get phy\n");
+		return dev_err_probe(dev, PTR_ERR(hdmi->phy), "Failed to get phy\n");
 
 	cfg->ctrl_ops->io_init(hdmi);
 
@@ -670,7 +669,7 @@  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	if (irq < 0)
 		return irq;
 
-	ret = devm_request_threaded_irq(hdmi->dev, irq,
+	ret = devm_request_threaded_irq(dev, irq,
 					cfg->ctrl_ops->hardirq_callback,
 					cfg->ctrl_ops->irq_callback,
 					IRQF_SHARED, "dw-hdmi-qp-hpd",
@@ -681,7 +680,7 @@  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	drm_encoder_helper_add(encoder, &dw_hdmi_qp_rockchip_encoder_helper_funcs);
 	ret = drmm_encoder_init(drm, encoder, NULL, DRM_MODE_ENCODER_TMDS, NULL);
 	if (ret)
-		return dev_err_probe(hdmi->dev, ret, "Failed to init encoder\n");
+		return dev_err_probe(dev, ret, "Failed to init encoder\n");
 
 	platform_set_drvdata(pdev, hdmi);
 
@@ -689,12 +688,12 @@  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	if (IS_ERR(hdmi->hdmi)) {
 		ret = PTR_ERR(hdmi->hdmi);
 		hdmi->hdmi = NULL;
-		return dev_err_probe(hdmi->dev, ret, "Failed to bind dw-hdmi-qp\n");
+		return dev_err_probe(dev, ret, "Failed to bind dw-hdmi-qp\n");
 	}
 
 	connector = drm_bridge_connector_init(drm, encoder);
 	if (IS_ERR(connector))
-		return dev_err_probe(hdmi->dev, PTR_ERR(connector),
+		return dev_err_probe(dev, PTR_ERR(connector),
 				     "Failed to init bridge connector\n");
 
 	return 0;