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 <[email protected]> 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/[email protected]?part=7
