Messages in this thread |  | | Date | Tue, 28 Apr 2026 15:19:35 +0200 | | Subject | Re: [PATCH v2 08/11] drm/bridge: adv7511: switch to of_drm_get_bridge_by_endpoint() | | From | "Luca Ceresoli" <> |
| |
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
|  |