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

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

Hello Biju,

On Tue Apr 28, 2026 at 4:45 PM CEST, Biju Das wrote:
> Hi Luca,
>
>> -----Original Message-----
>> From: dri-devel <dri-devel-bounces@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@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.

Very quick feedback loop! Thanks a lot Biju.

Sending v3 in a moment.

Luca

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