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

Luca Ceresoli
2026-04-28T13:20:04Z

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