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

Biju Das
2026-04-28T13:32:16Z

Hi Luca,

> -----Original Message-----
> From: Luca Ceresoli <luca.ceresoli@bootlin.com>
> Sent: 28 April 2026 14:20
> Subject: Re: [PATCH v2 08/11] drm/bridge: adv7511: switch to of_drm_get_bridge_by_endpoint()
>
> 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);

The point is you cannot return PTR_ERR as it will lead to crash, if it is
direct assignment.

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

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