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

Luca Ceresoli
2026-04-28T13:48:02Z

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.

>
> if (IS_ERR(adv7511->bridge.next_bridge)) {
> int err = PTR_ERR(adv7511->bridge.next_bridge);
> adv7511->bridge.next_bridge = NULL;
> return err;
> }

Is this if() condition wrong? The driver needs to accept the -ENODEV return
value, the next_bridge is conditional in the curent driver code.

>
> Cheers,
> Biju
>
>> 2. let nexxt_bridge hold -ENODEV but ignore it when it is used (only in
>> the attach op, for all 3 drivers):
>>
>> - if (adv->bridge.next_bridge) {
>> + if (!IS_ERR_OR_NULL(adv->bridge.next_bridge)) {
>>
>> While the latter approach involves less code, it might let errors sneak in in case new usages of
>> next_bridge are added with just a NULL check.
>>
>> Opinions about the two?

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering