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

[Biju Das]

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.

Cheers,
Biju