Re: [PATCH v3 07/19] drm/bridge: inno-hdmi: Split probe out of bind

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Using `devm_` APIs during the component bind phase causes permanent resource leaks and breaks EPROBE_DEFER.
- [High] The hotplug interrupt handler `inno_hdmi_irq` can fire before the bridge is attached, leading to a NULL pointer dereference.
- [High] The I2C adapter is exposed before its private data is initialized, leading to a NULL pointer dereference on concurrent access.
--

commit 72f1334365740cfcdf0865d9dfd6562b9c3b40a8
Author: Michal Wilczynski <m.wilczynski@samsung.com>
Subject: drm/bridge: inno-hdmi: Split probe out of bind

This commit splits the hardware setup of the inno-hdmi driver into a new
inno_hdmi_probe() function. This separates the bridge registration from its
attachment in inno_hdmi_bind(), allowing decoupled MFD/glue drivers to manage
them independently.

> diff --git a/drivers/gpu/drm/bridge/inno-hdmi.c b/drivers/gpu/drm/bridge/inno-hdmi.c
> index dd35f5a875d3d..aab474740f7f5 100644
> --- a/drivers/gpu/drm/bridge/inno-hdmi.c
> +++ b/drivers/gpu/drm/bridge/inno-hdmi.c
> @@ -1061,11 +1061,24 @@ static struct i2c_adapter *inno_hdmi_i2c_adapter(struct inno_hdmi *hdmi)
>  	return adap;
>  }

[Severity: High]
This is a pre-existing issue, but the I2C adapter is exposed before its private
data is initialized, which can lead to a NULL pointer dereference on concurrent
access.

Just above this line, devm_i2c_add_adapter() makes the bus visible to the system
before hdmi->i2c is assigned:

    ret = devm_i2c_add_adapter(hdmi->dev, adap);
    if (ret) {
        ...
    }

    hdmi->i2c = i2c;

If a concurrent I2C transfer is initiated (e.g. via udev running i2cdetect),
inno_hdmi_i2c_xfer() can be invoked. It will read the uninitialized hdmi->i2c
and attempt to lock its mutex:

drivers/gpu/drm/bridge/inno-hdmi.c:inno_hdmi_i2c_xfer() {
    struct inno_hdmi_i2c *i2c = hdmi->i2c;
    ...
    mutex_lock(&i2c->lock);
    ...
}

Could hdmi->i2c be assigned before calling devm_i2c_add_adapter() to prevent
this race?

> -struct inno_hdmi *inno_hdmi_bind(struct device *dev,
> -				 struct drm_encoder *encoder,
> -				 const struct inno_hdmi_plat_data *plat_data)
> +/**
> + * inno_hdmi_probe - Internal helper to perform common setup
> + * @pdev: platform device
> + * @plat_data: SoC-specific platform data
> + *
> + * This function handles all the common hardware setup: allocating the main
> + * struct, mapping registers, getting clocks, initializing the hardware,
> + * setting up the IRQ, and initializing the DDC adapter and bridge struct.
> + * It returns a pointer to the inno_hdmi struct on success, or an ERR_PTR
> + * on failure.
> + *
> + * This function is used by modern, decoupled MFD/glue drivers. It registers
> + * the bridge but does not attach it.
> + */
> +struct inno_hdmi *inno_hdmi_probe(struct platform_device *pdev,
> +				  const struct inno_hdmi_plat_data *plat_data)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but using devm_ APIs in the
component bind phase (now moved to inno_hdmi_probe()) causes permanent
resource leaks and breaks EPROBE_DEFER.

Because inno_hdmi_bind() is called from inno_hdmi_rockchip_bind() (a
component bind callback), the platform device is not removed when a component
unbinds. If another component in the DRM pipeline defers, the devm_ resources
are not freed.

Subsequent bind attempts will fail with -EBUSY when re-requesting the IRQ or
I/O regions:

drivers/gpu/drm/bridge/inno-hdmi.c:inno_hdmi_probe() {
    ...
    ret = devm_request_threaded_irq(dev, irq, inno_hdmi_hardirq,
                                    inno_hdmi_irq, IRQF_SHARED,
                                    dev_name(dev), hdmi);
    ...
}

Should this driver be converted to request resources during the actual probe
phase rather than bind?

[ ... ]

> +struct inno_hdmi *inno_hdmi_bind(struct device *dev,
> +				 struct drm_encoder *encoder,
> +				 const struct inno_hdmi_plat_data *plat_data)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct inno_hdmi *hdmi;
> +	int ret;
> +
> +	hdmi = inno_hdmi_probe(pdev, plat_data);
> +	if (IS_ERR(hdmi))
> +		return hdmi;
> +
> +	ret = drm_bridge_attach(encoder, &hdmi->bridge, NULL,
> +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);

[Severity: High]
This is a pre-existing issue, but this refactoring significantly widens the race
window for a NULL pointer dereference. The hotplug interrupt handler can fire
before the bridge is attached.

inno_hdmi_probe() unmasks the HPD interrupt and registers the IRQ handler with
devm_request_threaded_irq(). If a monitor is physically connected, the
interrupt can fire immediately.

The IRQ handler then attempts to dereference hdmi->bridge.dev:

drivers/gpu/drm/bridge/inno-hdmi.c:inno_hdmi_irq() {
    ...
    drm_helper_hpd_irq_event(hdmi->bridge.dev);
    ...
}

However, hdmi->bridge.dev is only assigned when drm_bridge_attach() is called.
Since this patch decouples probe from attach for modern MFD/glue drivers, the
attach phase happens much later, leaving a dangerous race window.

Should the IRQ registration be moved to after the bridge is attached, or should
the IRQ handler check if the bridge is attached before proceeding?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-jh7110-clean-send-v3-0-484f9ae72715@samsung.com?part=7