Re: [PATCH v3 3/3] drm/bridge: megachips: remove bridge when irq request fails

[Osama Abdelkader]

Hello Luca,

On Wed, Apr 29, 2026 at 01:48:33PM +0200, Luca Ceresoli wrote:
> On Thu Apr 23, 2026 at 10:06 PM CEST, Osama Abdelkader wrote:
> > If devm_request_threaded_irq() fails after drm_bridge_add(), remove the
> > bridge before returning.
> >
> > Keep drm_bridge_add() rather than devm_drm_bridge_add(): registration is
> > tied to the STDP4028 device while ge_b850v3_register() may complete from
> > either I2C probe; devm would not unwind the bridge if the other client's
> > probe fails.
> >
> > Signed-off-by: Osama Abdelkader <osama.abdelkader@xxxxxxxxx>
> > Fixes: a68ee76f4a28 ("drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: Fix bridge initialization")
> 
> That commit only moved the bug to a slightly different location. The bug
> was present even before, since commit fcfa0ddc18ed ("drm/bridge: Drivers
> for megachips-stdpxxxx-ge-b850v3-fw (LVDS-DP++)"), so you should update
> your Fixes line to point to it.

Updated in v4, thanks.

> 
> 
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> > v3: add Fixes and Cc tags
> > v2: IRQ failure path only (explicit drm_bridge_remove)
> > ---
> >  .../drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> > index c9e6505cbd88..2d02cc69f237 100644
> > --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> > +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> > @@ -251,7 +251,6 @@ static void ge_b850v3_lvds_remove(void)
> >  		goto out;
> >
> >  	drm_bridge_remove(&ge_b850v3_lvds_ptr->bridge);
> > -
> >  	ge_b850v3_lvds_ptr = NULL;
> >  out:
> >  	mutex_unlock(&ge_b850v3_lvds_dev_mutex);
> > @@ -261,6 +260,7 @@ static int ge_b850v3_register(void)
> >  {
> >  	struct i2c_client *stdp4028_i2c = ge_b850v3_lvds_ptr->stdp4028_i2c;
> >  	struct device *dev = &stdp4028_i2c->dev;
> > +	int ret;
> >
> >  	/* drm bridge initialization */
> >  	ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT |
> > @@ -277,11 +277,15 @@ static int ge_b850v3_register(void)
> >  	if (!stdp4028_i2c->irq)
> >  		return 0;
> >
> > -	return devm_request_threaded_irq(&stdp4028_i2c->dev,
> > -			stdp4028_i2c->irq, NULL,
> > -			ge_b850v3_lvds_irq_handler,
> > -			IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > -			"ge-b850v3-lvds-dp", ge_b850v3_lvds_ptr);
> > +	ret = devm_request_threaded_irq(&stdp4028_i2c->dev,
> > +					stdp4028_i2c->irq, NULL,
> > +					ge_b850v3_lvds_irq_handler,
> > +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > +					"ge-b850v3-lvds-dp", ge_b850v3_lvds_ptr);
> > +	if (ret)
> > +		drm_bridge_remove(&ge_b850v3_lvds_ptr->bridge);
> 
> Why not just using devm_drm_bridge_add() and keep everything else clean, as
> you did in other patches in the series?

Because registration is tied to the STDP4028 device while ge_b850v3_register()
may complete from either I2C probe; so devm would not unwind the bridge if the 
other client's probe fails.

> 
> Luca
> 
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com