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

Luca Ceresoli
2026-04-28T15:20:03Z

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