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

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

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
https://bootlin.com