Hi Dan,
Thank you for your review.
On 1/31/25 14:48, Dan Carpenter wrote:
Unrelated to this patch but from a naive reading of the code the
conn->hpd_irq = gpiod_to_irq(conn->hpd_gpio);
assignment can fail with -EPROBE_DEFER if CONFIG_GPIOLIB_IRQCHIP is
enabled. The driver can function without an IRQ so we just ignore
the error, but probably it would be better to propogate it back?
Makes sense. I will write a patch addressing this issue if anyone thinks
the EPROBE_DEFER error should not be ignored. Please let me know.
On Fri, Jan 31, 2025 at 02:19:18PM +0900, Joe Hattori wrote:
Current implementation of .probe() leaks a reference of i2c_adapter.
Implement an error path and call put_device() on the obtained
i2c_adapter in it to fix this refcount bug.
This bug was found by an experimental static analysis tool that I am
developing.
Fixes: 0c275c30176b ("drm/bridge: Add bridge driver for display connectors")
Signed-off-by: Joe Hattori <joe@xxxxxxxxxxxxxxxxxxxxx>
---
Changes in V3:
- Removed shadowed variables (ret).
Changes in V2:
- Omit the null check before calling i2c_put_adapter().
---
drivers/gpu/drm/bridge/display-connector.c | 31 +++++++++++++---------
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
index 72bc508d4e6e..411f9372e064 100644
--- a/drivers/gpu/drm/bridge/display-connector.c
+++ b/drivers/gpu/drm/bridge/display-connector.c
@@ -329,35 +329,39 @@ static int display_connector_probe(struct platform_device *pdev)
/* Get the DP PWR for DP connector. */
if (type == DRM_MODE_CONNECTOR_DisplayPort) {
- int ret;
-
ret = display_connector_get_supply(pdev, conn, "dp-pwr");
- if (ret < 0)
- return dev_err_probe(&pdev->dev, ret, "failed to get DP PWR regulator\n");
+ if (ret < 0) {
+ ret = dev_err_probe(&pdev->dev, ret,
^^^ ^^^
This is a "ret = ret" assignment. There is no need for that. Just
call dev_err_probe() without saving the return value.
Handled in the V4 patch [1].
+ "failed to get DP PWR regulator\n");
+ goto err_put;
+ }
}
/* enable DDC */
if (type == DRM_MODE_CONNECTOR_HDMIA) {
- int ret;
-
conn->ddc_en = devm_gpiod_get_optional(&pdev->dev, "ddc-en",
GPIOD_OUT_HIGH);
if (IS_ERR(conn->ddc_en)) {
dev_err(&pdev->dev, "Couldn't get ddc-en gpio\n");
- return PTR_ERR(conn->ddc_en);
+ ret = PTR_ERR(conn->ddc_en);
+ goto err_put;
}
ret = display_connector_get_supply(pdev, conn, "hdmi-pwr");
- if (ret < 0)
- return dev_err_probe(&pdev->dev, ret, "failed to get HDMI +5V Power regulator\n");
+ if (ret < 0) {
+ ret = dev_err_probe(
+ &pdev->dev, ret,
+ "failed to get HDMI +5V Power regulator\n");
Same.
+ goto err_put;
+ }
}
if (conn->supply) {
ret = regulator_enable(conn->supply);
if (ret) {
dev_err(&pdev->dev, "failed to enable PWR regulator: %d\n", ret);
- return ret;
+ goto err_put;
}
}
@@ -383,6 +387,10 @@ static int display_connector_probe(struct platform_device *pdev)
drm_bridge_add(&conn->bridge);
return 0;
+
+err_put:
+ i2c_put_adapter(conn->bridge.ddc);
+ return ret;
}
static void display_connector_remove(struct platform_device *pdev)
@@ -397,8 +405,7 @@ static void display_connector_remove(struct platform_device *pdev)
drm_bridge_remove(&conn->bridge);
- if (!IS_ERR(conn->bridge.ddc))
- i2c_put_adapter(conn->bridge.ddc);
+ i2c_put_adapter(conn->bridge.ddc);
This change is a nice cleanup and perhaps it silences a warning in your
static checker? It should be mentioned in the commit message.
Reflected in the commit message of v4. (This change was not detected by
my verifier, but yes, should have been mentioned in the message anyway)
regards,
dan carpenter
[1]:
https://lore.kernel.org/dri-devel/20250131073453.551252-1-joe@xxxxxxxxxxxxxxxxxxxxx/
Best,
Joe
