[PATCH v2 08/11] drm/bridge: adv7511: switch to of_drm_get_bridge_by_endpoint()

[Biju Das]

Hi Luca,

> -----Original Message-----
> From: dri-devel <dri-devel-bounces at lists.freedesktop.org> On Behalf Of Biju Das
> Sent: 28 April 2026 15:39
> Subject: RE: [PATCH v2 08/11] drm/bridge: adv7511: switch to of_drm_get_bridge_by_endpoint()
> 
> 
> 
> > -----Original Message-----
> > From: Biju Das
> > Sent: 28 April 2026 15:02
> > Subject: RE: [PATCH v2 08/11] drm/bridge: adv7511: switch to
> > of_drm_get_bridge_by_endpoint()
> >
> > Hi Luca,
> >
> > > -----Original Message-----
> > > From: Luca Ceresoli <luca.ceresoli at bootlin.com>
> > > Sent: 28 April 2026 14:48
> > > Subject: Re: [PATCH v2 08/11] drm/bridge: adv7511: switch to
> > > of_drm_get_bridge_by_endpoint()
> > >
> > > Hello,
> > >
> > > On Tue Apr 28, 2026 at 3:31 PM CEST, Biju Das wrote:
> > > >> >> > @@ -1251,10 +1251,9 @@ static int adv7511_probe(struct
> > > >> >> > i2c_client
> > > >> >> > *i2c)
> > > >> >> >
> > > >> >> >  	memset(&link_config, 0, sizeof(link_config));
> > > >> >> >
> > > >> >> > -	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, -1, NULL,
> > > >> >> > -					  &adv7511->next_bridge);
> > > >> >> > -	if (ret && ret != -ENODEV)
> > > >> >> > -		return ret;
> > > >> >> > +	adv7511->bridge.next_bridge = of_drm_get_bridge_by_endpoint(dev->of_node, 1, -1);
> > > >> >> > +	if (IS_ERR(adv7511->bridge.next_bridge) && PTR_ERR(adv7511->bridge.next_bridge) != -
> ENODEV)
> > > >> >> > +		return PTR_ERR(adv7511->bridge.next_bridge);
> > > >> >>
> > > >> >> Does it crash, if the error is  -EPROBE_DEFER?
> > > >> >
> > > >> > I see a crash with patch [1], which is fixed by avoiding the direct assignment.
> > > >>
> > > >> Ah, dammit! Good catch, thanks for the quick testing and follow-up!
> > > >>
> > > >> Indeed this driver assumes next_bridge is either NULL or a valid
> > > >> pointer, but due to the 'if(IS_ERR() && some_other_condition)'
> > > >> now it can also be -ENODEV (not -
> > > EPROBE_DEFER, but that's irrelevant).
> > > >>
> > > >> This affects the msm and zynqmp_dp patches equally.
> > > >>
> > > >> I'm sending a v3 soon with these fixed. I'm just not sure which
> > > >> approach to use to fix (same for all the 3 patches). Alternatives are:
> > > >>
> > > >>  1. -ENODEV is accepted, set next_bridge to NULL when it happens:
> > > >>
> > > >>       -       if (IS_ERR(adv7511->bridge.next_bridge) && PTR_ERR(adv7511->bridge.next_bridge) !=
> -
> > > >> ENODEV)
> > > >>       -               return PTR_ERR(adv7511->bridge.next_bridge);
> > > >>       +       if (IS_ERR(adv7511->bridge.next_bridge)) {
> > > >>       +               if (PTR_ERR(adv7511->bridge.next_bridge) == -ENODEV)
> > > >>       +                       adv7511->bridge.next_bridge = NULL;
> > > >>       +               else
> > > >>       +                       return PTR_ERR(adv7511->bridge.next_bridge);
> > > >
> > > > The point is you cannot return PTR_ERR as it will lead to crash,
> > > > if it is direct assignment.
> > >
> > > It would definitely crash when the next_bridge is dereferenced
> > > (which happens in
> > > adv7511_bridge_attach()) but I don't see how it can crash here. Here
> > > it _can_ be assigned -ENODEV, but it would be immediately be cleared
> > > to NULL, or to enother error, but we'd immediately return. And in
> > > case of return, when next_bridge is cleared by __drm_bridge_free ->
> > > drm_bridge_put, the error value would
> > be ignored thanks to patch 1.
> >
> > OK, I haven't noticed the newly introduced check in drm_bridge_put() in patch#1.
> >
> > I guess, we should avoid putting error values in bridge.next_bridge.
> > It should be either NULL or Valid pointer, not PTR_ERR.
> 
> FTR, I get a crash in attach. I will apply the suggested changes and will let you know the result.
> 
> [   18.957324] pc : drm_bridge_attach+0x34/0x210 [drm]
> [   18.969425] lr : adv7511_bridge_attach+0x38/0xb8 [adv7511]
> 
> [   18.969610]  drm_bridge_attach+0x34/0x210 [drm] (P)
> [   18.969845]  adv7511_bridge_attach+0x38/0xb8 [adv7511]
> [   18.969867]  drm_bridge_attach+0xf0/0x210 [drm]
> [   18.970042]  rzg2l_mipi_dsi_attach+0x24/0x3c [rzg2l_mipi_dsi]
> [   18.970064]  drm_bridge_attach+0xf0/0x210 [drm]
> [   18.970262]  rzg2l_du_encoder_init+0x9c/0x250 [rzg2l_du_drm]
> [   18.970293]  rzg2l_du_modeset_init+0x30c/0x4d0 [rzg2l_du_drm]
> [   18.970307]  rzg2l_du_probe+0xc8/0x174 [rzg2l_du_drm]
> [   18.970321]  platform_probe+0x5c/0xa4
> [   18.970336]  really_probe+0xbc/0x2c0
> [   18.970348]  __driver_probe_device+0x80/0x14c
> [   18.970359]  driver_probe_device+0x3c/0x164
> [   18.970369]  __driver_attach+0x90/0x1a4
> [   18.970379]  bus_for_each_dev+0x7c/0xdc
> [   18.970388]  driver_attach+0x24/0x30
> [   18.970397]  bus_add_driver+0xe4/0x208
> [   18.970406]  driver_register+0x68/0x130
> [   18.970416]  __platform_driver_register+0x24/0x30
> 

I confirm the crash is fixed by your suggested changes for V3.

Cheers,
Biju