Re: [PATCH 2/4] drm/bridge: display-connector: Fix I2C adapter resource leak

[Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>]

Hello,

This is the friendly mail bot of Laurent Pinchart. You have sent him
what appears to be an e-mail generated by an LLM. In order to preserve
his mental health, the e-mail has been automatically ignored.

Thank you,

Laurent's spam bot

On Sun, 05 Jul 2026 21:48:25 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Use-After-Free of devm-managed resources due to DRM bridge lifetime mismatch
> - [Medium] Hardware state leak of `ddc_en` GPIO on probe error paths
> --
> 
> commit 9c76898e592dfddd3f50beaa692cc32ae295e2c7
> Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> drm/bridge: display-connector: Fix I2C adapter resource leak
> 
> If the probe function returns an error after getting the I2C adapter for
> DDC, the reference to the adapter is never released. Fix it by releasing
> it in the bridge .destroy() handler.
> 
> > diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
> > index d1e74dd2e0518..eb3412ec30a07 100644
> > --- a/drivers/gpu/drm/bridge/display-connector.c
> > +++ b/drivers/gpu/drm/bridge/display-connector.c
> > @@ -43,6 +43,13 @@ static int display_connector_attach(struct drm_bridge *bridge,
> >  	return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL;
> >  }
> >  
> > +static void display_connector_destroy(struct drm_bridge *bridge)
> > +{
> > +	struct display_connector *conn = to_display_connector(bridge);
> > +
> > +	i2c_put_adapter(conn->bridge.ddc);
> > +}
> > +
> >  static enum drm_connector_status display_connector_detect(struct drm_bridge *bridge)
> >  {
> >  	struct display_connector *conn = to_display_connector(bridge);
> 
> [ ... ]
> 
> > @@ -439,9 +447,6 @@ static void display_connector_remove(struct platform_device *pdev)
> >  		regulator_disable(conn->supply);
> >  
> >  	drm_bridge_remove(&conn->bridge);
> > -
> > -	if (!IS_ERR(conn->bridge.ddc))
> > -		i2c_put_adapter(conn->bridge.ddc);
> >  }
> >  
> >  static const struct of_device_id display_connector_match[] = {
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does the bridge lifetime 
> mismatch with devm resources cause a use-after-free?
> 
> In display_connector_probe(), the bridge is allocated with 
> devm_drm_bridge_alloc(), tying its lifetime to the DRM bridge kref:
> 
> display_connector_probe() {
>     ...
>     conn = devm_drm_bridge_alloc(&pdev->dev, struct display_connector, bridge,
>                                  &display_connector_bridge_funcs);
>     ...
> }
> 
> However, devm-managed resources like the HPD GPIO and IRQ are tied to the 
> platform device unbind. If the device is unbound via sysfs while the DRM 
> core still holds a reference to the bridge, devres will free the GPIO and 
> IRQ resources.
> 
> If the DRM core later invokes callbacks like detect or hpd_disable, would it 
> dereference the freed GPIO or attempt to disable the freed IRQ?
> 
> display_connector_detect() {
>     ...
>     if (gpiod_get_value_cansleep(conn->hpd_gpio))
>     ...
> }
> 
> display_connector_hpd_disable() {
>     ...
>     disable_irq(conn->hpd_irq);
>     ...
> }
> 
> [Severity: Medium]
> This is a pre-existing issue, but is there a hardware state leak on the 
> probe error path?
> 
> In display_connector_probe(), the DDC enable GPIO is acquired and 
> immediately asserted high:
> 
> display_connector_probe() {
>     ...
>     if (type == DRM_MODE_CONNECTOR_HDMIA) {
>         ...
>         conn->ddc_en = devm_gpiod_get_optional(&pdev->dev, "ddc-en",
>                                                GPIOD_OUT_HIGH);
> 
>         if (IS_ERR(conn->ddc_en)) {
>             ...
>         }
> 
>         ret = display_connector_get_supply(pdev, conn, "hdmi-pwr");
>         if (ret < 0)
>             return dev_err_probe(...);
>     ...
> }
> 
> If a subsequent step like display_connector_get_supply() or 
> regulator_enable() fails, the function returns an error. While devres will 
> free the GPIO descriptor, it doesn't revert the physical pin state.
> 
> Unlike display_connector_remove() which explicitly sets the GPIO to 0, does 
> this error path leave the DDC bus permanently enabled?
>