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

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

On Tue Apr 28, 2026 at 1:58 PM CEST, Biju Das wrote:
> Hi all,
>
>> -----Original Message-----
>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Biju Das
>> Sent: 28 April 2026 12:50
>> Subject: RE: [PATCH v2 08/11] drm/bridge: adv7511: switch to of_drm_get_bridge_by_endpoint()
>>
>>
>> Hi,
>>
>> Thanks for the patch.
>>
>> > -----Original Message-----
>> > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
>> > Luca Ceresoli
>> > Sent: 28 April 2026 10:16
>> > Subject: [PATCH v2 08/11] drm/bridge: adv7511: switch to
>> > of_drm_get_bridge_by_endpoint()
>> >
>> > This driver calls drm_of_find_panel_or_bridge() with a NULL pointer in
>> > the @panel parameter, thus using a reduced feature set of that function.
>> > Replace this call with the simpler of_drm_get_bridge_by_endpoint().
>> >
>> > Since of_drm_get_bridge_by_endpoint() increases the refcount of the
>> > returned bridge, ensure it is put on removal. To achieve this, instead
>> > of adding an explicit drm_bridge_put(), migrate to the bridge::next_bridge pointer which is
>> automatically put when the bridge is eventually freed.
>> >
>> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>> > ---
>> >  drivers/gpu/drm/bridge/adv7511/adv7511.h     |  1 -
>> >  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 11 +++++------
>> >  2 files changed, 5 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> > b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> > index 8be7266fd4f4..12c95198d9a4 100644
>> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> > @@ -354,7 +354,6 @@ struct adv7511 {
>> >  	enum drm_connector_status status;
>> >  	bool powered;
>> >
>> > -	struct drm_bridge *next_bridge;
>> >  	struct drm_display_mode curr_mode;
>> >
>> >  	unsigned int f_tmds;
>> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> > index 6bd76c1fb007..498e38579a0f 100644
>> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> > @@ -851,8 +851,8 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge,
>> >  	struct adv7511 *adv = bridge_to_adv7511(bridge);
>> >  	int ret = 0;
>> >
>> > -	if (adv->next_bridge) {
>> > -		ret = drm_bridge_attach(encoder, adv->next_bridge, bridge,
>> > +	if (adv->bridge.next_bridge) {
>> > +		ret = drm_bridge_attach(encoder, adv->bridge.next_bridge, bridge,
>> >  					flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>> >  		if (ret)
>> >  			return ret;
>> > @@ -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);
      +       }

 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