This driver obtains a bridge pointer from of_drm_find_bridge() in the probe
function and stores it until driver removal. of_drm_find_bridge() is
deprecated. Move to of_drm_find_and_get_bridge() for the bridge to be
refcounted and use bridge->next_bridge to put the reference on
deallocation.
This needs to be handled in various steps:
* the bridge returned of_drm_get_bridge() is stored in the local temporary
variable next_bridge whose scope is the for loop, so a cleanup action is
enough
* the value of next_bridge is copied into selected_bridge, potentially
more than once, so a cleanup action at function scope plus a
drm_bridge_put() in case of reassignment are enough
* on successful return selected_bridge is stored in bridge->next_bridge,
which ensures it is put when the bridge is deallocated
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
index 91e4f4d55469..b3050310a7f0 100644
--- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
+++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
@@ -23,7 +23,6 @@
struct imx8qxp_pixel_link {
struct drm_bridge bridge;
- struct drm_bridge *next_bridge;
struct device *dev;
struct imx_sc_ipc *ipc_handle;
u8 stream_id;
@@ -140,7 +139,7 @@ static int imx8qxp_pixel_link_bridge_attach(struct drm_bridge *bridge,
}
return drm_bridge_attach(encoder,
- pl->next_bridge, bridge,
+ pl->bridge.next_bridge, bridge,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
}
@@ -260,7 +259,7 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
{
struct device_node *np = pl->dev->of_node;
struct device_node *port;
- struct drm_bridge *selected_bridge = NULL;
+ struct drm_bridge *selected_bridge __free(drm_bridge_put) = NULL;
u32 port_id;
bool found_port = false;
int reg;
@@ -297,7 +296,8 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
continue;
}
- struct drm_bridge *next_bridge = of_drm_find_bridge(remote);
+ struct drm_bridge *next_bridge __free(drm_bridge_put) =
+ of_drm_find_and_get_bridge(remote);
if (!next_bridge)
return -EPROBE_DEFER;
@@ -305,12 +305,14 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
* Select the next bridge with companion PXL2DPI if
* present, otherwise default to the first bridge
*/
- if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi"))
- selected_bridge = next_bridge;
+ if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
+ drm_bridge_put(selected_bridge);
+ selected_bridge = drm_bridge_get(next_bridge);
+ }
}
pl->mst_addr = port_id - 1;
- pl->next_bridge = selected_bridge;
+ pl->bridge.next_bridge = drm_bridge_get(selected_bridge);
return 0;
}
--
2.52.0On Wed, Jan 07, 2026 at 10:56:29AM +0100, Luca Ceresoli wrote: [...] > * the bridge returned of_drm_get_bridge() is stored in the local temporary There is no of_drm_get_bridge() defined, so s/of_drm_get_bridge/of_drm_find_and_get_bridge/ ? -- Regards, Liu Ying
On Mon Jan 26, 2026 at 9:14 AM CET, Liu Ying wrote: > > > On Wed, Jan 07, 2026 at 10:56:29AM +0100, Luca Ceresoli wrote: > > [...] > >> * the bridge returned of_drm_get_bridge() is stored in the local temporary > > There is no of_drm_get_bridge() defined, so > s/of_drm_get_bridge/of_drm_find_and_get_bridge/ ? Ah, well spotted, fixing that. Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi Luca,
On Wed, Jan 07, 2026 at 10:56:29AM +0100, Luca Ceresoli wrote:
> This driver obtains a bridge pointer from of_drm_find_bridge() in the probe
> function and stores it until driver removal. of_drm_find_bridge() is
> deprecated. Move to of_drm_find_and_get_bridge() for the bridge to be
> refcounted and use bridge->next_bridge to put the reference on
> deallocation.
>
> This needs to be handled in various steps:
>
> * the bridge returned of_drm_get_bridge() is stored in the local temporary
> variable next_bridge whose scope is the for loop, so a cleanup action is
> enough
> * the value of next_bridge is copied into selected_bridge, potentially
> more than once, so a cleanup action at function scope plus a
> drm_bridge_put() in case of reassignment are enough
> * on successful return selected_bridge is stored in bridge->next_bridge,
> which ensures it is put when the bridge is deallocated
>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
> drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
> index 91e4f4d55469..b3050310a7f0 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
> @@ -23,7 +23,6 @@
>
> struct imx8qxp_pixel_link {
> struct drm_bridge bridge;
> - struct drm_bridge *next_bridge;
> struct device *dev;
> struct imx_sc_ipc *ipc_handle;
> u8 stream_id;
> @@ -140,7 +139,7 @@ static int imx8qxp_pixel_link_bridge_attach(struct drm_bridge *bridge,
> }
>
> return drm_bridge_attach(encoder,
> - pl->next_bridge, bridge,
> + pl->bridge.next_bridge, bridge,
> DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> }
>
> @@ -260,7 +259,7 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
> {
> struct device_node *np = pl->dev->of_node;
> struct device_node *port;
> - struct drm_bridge *selected_bridge = NULL;
> + struct drm_bridge *selected_bridge __free(drm_bridge_put) = NULL;
> u32 port_id;
> bool found_port = false;
> int reg;
> @@ -297,7 +296,8 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
> continue;
> }
>
> - struct drm_bridge *next_bridge = of_drm_find_bridge(remote);
> + struct drm_bridge *next_bridge __free(drm_bridge_put) =
> + of_drm_find_and_get_bridge(remote);
> if (!next_bridge)
> return -EPROBE_DEFER;
>
> @@ -305,12 +305,14 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
> * Select the next bridge with companion PXL2DPI if
> * present, otherwise default to the first bridge
> */
> - if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi"))
> - selected_bridge = next_bridge;
> + if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
> + drm_bridge_put(selected_bridge);
> + selected_bridge = drm_bridge_get(next_bridge);
Considering selecting the first bridge without the companion pxl2dpi,
there would be a superfluous refcount for the selected bridge:
1) of_drm_find_and_get_bridge: refcount = 1
2) drm_bridge_put: noop, since selected_bridge is NULL, refcount = 1
3) drm_bridge_get: refcount = 2
4) drm_bridge_put(__free): refcount = 1
5) drm_bridge_get: for the pl->bridge.next_bridge, refcount = 2
I think the below snippet would be the right thing to do:
-8<-
{
...
struct drm_bridge *next_bridge __free(drm_bridge_put) =
of_drm_find_and_get_bridge(remote);
if (!next_bridge)
return -EPROBE_DEFER;
/*
* Select the next bridge with companion PXL2DPI if
* present, otherwise default to the first bridge
*/
if (!selected_bridge)
selected_bridge = drm_bridge_get(next_bridge);
if (of_property_present(remote, "fsl,companion-pxl2dpi")) {
if (selected_bridge)
drm_bridge_put(selected_bridge);
selected_bridge = drm_bridge_get(next_bridge);
}
}
...
pl->bridge.next_bridge = selected_bridge;
-8<-
Make sense?
> + }
> }
>
> pl->mst_addr = port_id - 1;
> - pl->next_bridge = selected_bridge;
> + pl->bridge.next_bridge = drm_bridge_get(selected_bridge);
>
> return 0;
> }
>
--
Regards,
Liu Ying
Hello Liu,
On Mon Jan 26, 2026 at 9:06 AM CET, Liu Ying wrote:
> Hi Luca,
>
> On Wed, Jan 07, 2026 at 10:56:29AM +0100, Luca Ceresoli wrote:
>> This driver obtains a bridge pointer from of_drm_find_bridge() in the probe
>> function and stores it until driver removal. of_drm_find_bridge() is
>> deprecated. Move to of_drm_find_and_get_bridge() for the bridge to be
>> refcounted and use bridge->next_bridge to put the reference on
>> deallocation.
>>
>> This needs to be handled in various steps:
>>
>> * the bridge returned of_drm_get_bridge() is stored in the local temporary
>> variable next_bridge whose scope is the for loop, so a cleanup action is
>> enough
>> * the value of next_bridge is copied into selected_bridge, potentially
>> more than once, so a cleanup action at function scope plus a
>> drm_bridge_put() in case of reassignment are enough
>> * on successful return selected_bridge is stored in bridge->next_bridge,
>> which ensures it is put when the bridge is deallocated
>>
>> Reviewed-by: Maxime Ripard <mripard@kernel.org>
>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Thanks for having found the time to go into the details and provide a
careful review of this series!
>> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
>> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
>> @@ -23,7 +23,6 @@
>>
>> struct imx8qxp_pixel_link {
>> struct drm_bridge bridge;
>> - struct drm_bridge *next_bridge;
>> struct device *dev;
>> struct imx_sc_ipc *ipc_handle;
>> u8 stream_id;
>> @@ -140,7 +139,7 @@ static int imx8qxp_pixel_link_bridge_attach(struct drm_bridge *bridge,
>> }
>>
>> return drm_bridge_attach(encoder,
>> - pl->next_bridge, bridge,
>> + pl->bridge.next_bridge, bridge,
>> DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>> }
>>
>> @@ -260,7 +259,7 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>> {
>> struct device_node *np = pl->dev->of_node;
>> struct device_node *port;
>> - struct drm_bridge *selected_bridge = NULL;
>> + struct drm_bridge *selected_bridge __free(drm_bridge_put) = NULL;
>> u32 port_id;
>> bool found_port = false;
>> int reg;
>> @@ -297,7 +296,8 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>> continue;
>> }
>>
>> - struct drm_bridge *next_bridge = of_drm_find_bridge(remote);
>> + struct drm_bridge *next_bridge __free(drm_bridge_put) =
>> + of_drm_find_and_get_bridge(remote);
>> if (!next_bridge)
>> return -EPROBE_DEFER;
>>
>> @@ -305,12 +305,14 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>> * Select the next bridge with companion PXL2DPI if
>> * present, otherwise default to the first bridge
>> */
>> - if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi"))
>> - selected_bridge = next_bridge;
>> + if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
>> + drm_bridge_put(selected_bridge);
>> + selected_bridge = drm_bridge_get(next_bridge);
>
> Considering selecting the first bridge without the companion pxl2dpi,
> there would be a superfluous refcount for the selected bridge:
>
> 1) of_drm_find_and_get_bridge: refcount = 1
> 2) drm_bridge_put: noop, since selected_bridge is NULL, refcount = 1
> 3) drm_bridge_get: refcount = 2
> 4) drm_bridge_put(__free): refcount = 1
> 5) drm_bridge_get: for the pl->bridge.next_bridge, refcount = 2
Here you are missing one put. There are two drm_bridge_put(__free), one for
next_bridge and one for selected_bridge. So your list should rather be:
1) next_bridge = of_drm_find_and_get_bridge: refcount = 1
2) drm_bridge_put(selected_bridge): noop, since selected_bridge is NULL, refcount = 1
3) selected_bridge = drm_bridge_get: refcount = 2
4) drm_bridge_put(next_bridge) [__free at loop scope end]: refcount = 1
5) pl->bridge.next_bridge = drm_bridge_get(), refcount = 2
6) drm_bridge_put(selected_bridge) [__free at function scope end]: refcount = 1
The idea is that for each pointer (which is a reference) we get a reference
(refcount++) when the pointer is set and put the reference when that same
pointer goes out of scope or is reset to NULL. "the pointer is set" can be
either of_drm_find_and_get_bridge() or an assignment, as each of these
operations creates another reference (pointer) to the same bridge.
Does it look correct?
> I think the below snippet would be the right thing to do:
> -8<-
> {
> ...
>
> struct drm_bridge *next_bridge __free(drm_bridge_put) =
> of_drm_find_and_get_bridge(remote);
> if (!next_bridge)
> return -EPROBE_DEFER;
>
> /*
> * Select the next bridge with companion PXL2DPI if
> * present, otherwise default to the first bridge
> */
> if (!selected_bridge)
> selected_bridge = drm_bridge_get(next_bridge);
>
> if (of_property_present(remote, "fsl,companion-pxl2dpi")) {
> if (selected_bridge)
> drm_bridge_put(selected_bridge);
>
> selected_bridge = drm_bridge_get(next_bridge);
> }
> }
Your version of the code looks OK as well so far, but totally equivalent to
what my patch proposes.
Do you think splitting the if() into two if()s is clearer? Would you like
me to change this?
> ...
> pl->bridge.next_bridge = selected_bridge;
Based on the logic above the drm_bridge_get() is still needed here (both
with the single if() or the split if()s) because at function exit the
selected_bridge reference will be put.
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Mon, Jan 26, 2026 at 07:18:47PM +0100, Luca Ceresoli wrote:
> Hello Liu,
Hello Luca,
>
> On Mon Jan 26, 2026 at 9:06 AM CET, Liu Ying wrote:
>> Hi Luca,
>>
>> On Wed, Jan 07, 2026 at 10:56:29AM +0100, Luca Ceresoli wrote:
>>> This driver obtains a bridge pointer from of_drm_find_bridge() in the probe
>>> function and stores it until driver removal. of_drm_find_bridge() is
>>> deprecated. Move to of_drm_find_and_get_bridge() for the bridge to be
>>> refcounted and use bridge->next_bridge to put the reference on
>>> deallocation.
>>>
>>> This needs to be handled in various steps:
>>>
>>> * the bridge returned of_drm_get_bridge() is stored in the local temporary
>>> variable next_bridge whose scope is the for loop, so a cleanup action is
>>> enough
>>> * the value of next_bridge is copied into selected_bridge, potentially
>>> more than once, so a cleanup action at function scope plus a
>>> drm_bridge_put() in case of reassignment are enough
>>> * on successful return selected_bridge is stored in bridge->next_bridge,
>>> which ensures it is put when the bridge is deallocated
>>>
>>> Reviewed-by: Maxime Ripard <mripard@kernel.org>
>>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>
> Thanks for having found the time to go into the details and provide a
> careful review of this series!
>
>>> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
>>> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
>>> @@ -23,7 +23,6 @@
>>>
>>> struct imx8qxp_pixel_link {
>>> struct drm_bridge bridge;
>>> - struct drm_bridge *next_bridge;
>>> struct device *dev;
>>> struct imx_sc_ipc *ipc_handle;
>>> u8 stream_id;
>>> @@ -140,7 +139,7 @@ static int imx8qxp_pixel_link_bridge_attach(struct drm_bridge *bridge,
>>> }
>>>
>>> return drm_bridge_attach(encoder,
>>> - pl->next_bridge, bridge,
>>> + pl->bridge.next_bridge, bridge,
>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>> }
>>>
>>> @@ -260,7 +259,7 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>> {
>>> struct device_node *np = pl->dev->of_node;
>>> struct device_node *port;
>>> - struct drm_bridge *selected_bridge = NULL;
>>> + struct drm_bridge *selected_bridge __free(drm_bridge_put) = NULL;
>>> u32 port_id;
>>> bool found_port = false;
>>> int reg;
>>> @@ -297,7 +296,8 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>> continue;
>>> }
>>>
>>> - struct drm_bridge *next_bridge = of_drm_find_bridge(remote);
>>> + struct drm_bridge *next_bridge __free(drm_bridge_put) =
>>> + of_drm_find_and_get_bridge(remote);
>>> if (!next_bridge)
>>> return -EPROBE_DEFER;
>>>
>>> @@ -305,12 +305,14 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>> * Select the next bridge with companion PXL2DPI if
>>> * present, otherwise default to the first bridge
>>> */
>>> - if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi"))
>>> - selected_bridge = next_bridge;
>>> + if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
>>> + drm_bridge_put(selected_bridge);
>>> + selected_bridge = drm_bridge_get(next_bridge);
>>
>> Considering selecting the first bridge without the companion pxl2dpi,
>> there would be a superfluous refcount for the selected bridge:
>>
>> 1) of_drm_find_and_get_bridge: refcount = 1
>> 2) drm_bridge_put: noop, since selected_bridge is NULL, refcount = 1
>> 3) drm_bridge_get: refcount = 2
>> 4) drm_bridge_put(__free): refcount = 1
>> 5) drm_bridge_get: for the pl->bridge.next_bridge, refcount = 2
>
> Here you are missing one put. There are two drm_bridge_put(__free), one for
> next_bridge and one for selected_bridge. So your list should rather be:
>
> 1) next_bridge = of_drm_find_and_get_bridge: refcount = 1
> 2) drm_bridge_put(selected_bridge): noop, since selected_bridge is NULL, refcount = 1
> 3) selected_bridge = drm_bridge_get: refcount = 2
> 4) drm_bridge_put(next_bridge) [__free at loop scope end]: refcount = 1
> 5) pl->bridge.next_bridge = drm_bridge_get(), refcount = 2
> 6) drm_bridge_put(selected_bridge) [__free at function scope end]: refcount = 1
Ah, right, I did miss this last put because selected_bridge is declared with
__free a bit far away from the loop at the very beginning of
imx8qxp_pixel_link_find_next_bridge() - that's my problem I guess, but I'm
not even sure if I'll fall into this same pitfall again after a while, which
makes the driver difficult to maintain.
Also, it seems that the refcount dance(back and forth bewteen 1 and 2) is not
something straightforward for driver readers to follow.
>
> The idea is that for each pointer (which is a reference) we get a reference
> (refcount++) when the pointer is set and put the reference when that same
> pointer goes out of scope or is reset to NULL. "the pointer is set" can be
> either of_drm_find_and_get_bridge() or an assignment, as each of these
> operations creates another reference (pointer) to the same bridge.
>
> Does it look correct?
Lol, I guess I need more coffee to read your logic of refcount get/put.
>
>> I think the below snippet would be the right thing to do:
>> -8<-
>> {
>> ...
>>
>> struct drm_bridge *next_bridge __free(drm_bridge_put) =
>> of_drm_find_and_get_bridge(remote);
>> if (!next_bridge)
>> return -EPROBE_DEFER;
>>
>> /*
>> * Select the next bridge with companion PXL2DPI if
>> * present, otherwise default to the first bridge
>> */
>> if (!selected_bridge)
>> selected_bridge = drm_bridge_get(next_bridge);
>>
>> if (of_property_present(remote, "fsl,companion-pxl2dpi")) {
>> if (selected_bridge)
>> drm_bridge_put(selected_bridge);
>>
>> selected_bridge = drm_bridge_get(next_bridge);
>> }
>> }
>
> Your version of the code looks OK as well so far, but totally equivalent to
> what my patch proposes.
>
> Do you think splitting the if() into two if()s is clearer? Would you like
> me to change this?
Yes, please. Two if()s are easier for me to read. Also I think the
"if (selected_bridge)" before "drm_bridge_put(selected_bridge)" improves
readability, though I know drm_bridge_put() checks input parameter bridge
for now.
>
>> ...
>> pl->bridge.next_bridge = selected_bridge;
>
> Based on the logic above the drm_bridge_get() is still needed here (both
> with the single if() or the split if()s) because at function exit the
> selected_bridge reference will be put.
Can the refcount dance be simplified a bit by dropping the put at
function exit? This snippet is what I'd propose if not too scary:
-8<-
struct drm_bridge *selected_bridge = NULL;
...
{
...
struct drm_bridge *next_bridge __free(drm_bridge_put) =
of_drm_find_and_get_bridge(remote);
if (!next_bridge)
return -EPROBE_DEFER;
/*
* Select the next bridge with companion PXL2DPI if
* present, otherwise default to the first bridge
*/
if (!selected_bridge)
selected_bridge = drm_bridge_get(next_bridge);
if (of_property_present(remote, "fsl,companion-pxl2dpi")) {
if (selected_bridge)
drm_bridge_put(selected_bridge);
selected_bridge = drm_bridge_get(next_bridge);
}
}
...
pl->bridge.next_bridge = selected_bridge;
-8<-
>
> Luca
>
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com/
--
Regards,
Liu Ying
On Tue Jan 27, 2026 at 4:54 AM CET, Liu Ying wrote:
...
>>>> @@ -260,7 +259,7 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>>> {
>>>> struct device_node *np = pl->dev->of_node;
>>>> struct device_node *port;
>>>> - struct drm_bridge *selected_bridge = NULL;
>>>> + struct drm_bridge *selected_bridge __free(drm_bridge_put) = NULL;
>>>> u32 port_id;
>>>> bool found_port = false;
>>>> int reg;
>>>> @@ -297,7 +296,8 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>>> continue;
>>>> }
>>>>
>>>> - struct drm_bridge *next_bridge = of_drm_find_bridge(remote);
>>>> + struct drm_bridge *next_bridge __free(drm_bridge_put) =
>>>> + of_drm_find_and_get_bridge(remote);
>>>> if (!next_bridge)
>>>> return -EPROBE_DEFER;
>>>>
>>>> @@ -305,12 +305,14 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>>> * Select the next bridge with companion PXL2DPI if
>>>> * present, otherwise default to the first bridge
>>>> */
>>>> - if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi"))
>>>> - selected_bridge = next_bridge;
>>>> + if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
>>>> + drm_bridge_put(selected_bridge);
>>>> + selected_bridge = drm_bridge_get(next_bridge);
>>>
>>> Considering selecting the first bridge without the companion pxl2dpi,
>>> there would be a superfluous refcount for the selected bridge:
>>>
>>> 1) of_drm_find_and_get_bridge: refcount = 1
>>> 2) drm_bridge_put: noop, since selected_bridge is NULL, refcount = 1
>>> 3) drm_bridge_get: refcount = 2
>>> 4) drm_bridge_put(__free): refcount = 1
>>> 5) drm_bridge_get: for the pl->bridge.next_bridge, refcount = 2
>>
>> Here you are missing one put. There are two drm_bridge_put(__free), one for
>> next_bridge and one for selected_bridge. So your list should rather be:
>>
>> 1) next_bridge = of_drm_find_and_get_bridge: refcount = 1
>> 2) drm_bridge_put(selected_bridge): noop, since selected_bridge is NULL, refcount = 1
>> 3) selected_bridge = drm_bridge_get: refcount = 2
>> 4) drm_bridge_put(next_bridge) [__free at loop scope end]: refcount = 1
>> 5) pl->bridge.next_bridge = drm_bridge_get(), refcount = 2
>> 6) drm_bridge_put(selected_bridge) [__free at function scope end]: refcount = 1
>
> Ah, right, I did miss this last put because selected_bridge is declared with
> __free a bit far away from the loop at the very beginning of
> imx8qxp_pixel_link_find_next_bridge() - that's my problem I guess, but I'm
> not even sure if I'll fall into this same pitfall again after a while, which
> makes the driver difficult to maintain.
>
> Also, it seems that the refcount dance(back and forth bewteen 1 and 2) is not
> something straightforward for driver readers to follow.
I thing the whole logic becomes straightforward if you think it this way:
* when a pointer is assigned = a new reference starts existing -> refcount++
* when a pointer is cleared/overwritten or goes out of scope = a reference
stops existing -> refcount--
In short: one pointer, one reference, one refcount.
If you re-read the patch with this in mind, does it become clearer?
>>> I think the below snippet would be the right thing to do:
>>> -8<-
>>> {
>>> ...
>>>
>>> struct drm_bridge *next_bridge __free(drm_bridge_put) =
>>> of_drm_find_and_get_bridge(remote);
>>> if (!next_bridge)
>>> return -EPROBE_DEFER;
>>>
>>> /*
>>> * Select the next bridge with companion PXL2DPI if
>>> * present, otherwise default to the first bridge
>>> */
>>> if (!selected_bridge)
>>> selected_bridge = drm_bridge_get(next_bridge);
>>>
>>> if (of_property_present(remote, "fsl,companion-pxl2dpi")) {
>>> if (selected_bridge)
>>> drm_bridge_put(selected_bridge);
>>>
>>> selected_bridge = drm_bridge_get(next_bridge);
>>> }
>>> }
>>
>> Your version of the code looks OK as well so far, but totally equivalent to
>> what my patch proposes.
>>
>> Do you think splitting the if() into two if()s is clearer? Would you like
>> me to change this?
>
> Yes, please. Two if()s are easier for me to read.
OK, will do.
> Also I think the
> "if (selected_bridge)" before "drm_bridge_put(selected_bridge)" improves
> readability, though I know drm_bridge_put() checks input parameter bridge
> for now.
I was about to reply "the NULL check in drm_bridge_put() is part of the API
contract as its documentation says", but then realized the documentation
does not say that. My bad, I was convinced I had documented that behaviour
to make it part of the contract so users can rely on it. I'm sending a
patch ASAP to document that.
>
>>
>>> ...
>>> pl->bridge.next_bridge = selected_bridge;
>>
>> Based on the logic above the drm_bridge_get() is still needed here (both
>> with the single if() or the split if()s) because at function exit the
>> selected_bridge reference will be put.
>
> Can the refcount dance be simplified a bit by dropping the put at
> function exit? This snippet is what I'd propose if not too scary:
>
> -8<-
> struct drm_bridge *selected_bridge = NULL;
> ...
>
> {
> ...
>
> struct drm_bridge *next_bridge __free(drm_bridge_put) =
> of_drm_find_and_get_bridge(remote);
> if (!next_bridge)
> return -EPROBE_DEFER;
>
> /*
> * Select the next bridge with companion PXL2DPI if
> * present, otherwise default to the first bridge
> */
> if (!selected_bridge)
> selected_bridge = drm_bridge_get(next_bridge);
>
> if (of_property_present(remote, "fsl,companion-pxl2dpi")) {
> if (selected_bridge)
> drm_bridge_put(selected_bridge);
>
> selected_bridge = drm_bridge_get(next_bridge);
> }
> }
>
> ...
> pl->bridge.next_bridge = selected_bridge;
> -8<-
Based on the "one pointer, one reference, one refcount" logic I explained
above, I find this version more complex to understand. I read it as:
selected_bridge and pl->bridge.next_bridge are two pointers sharing a
single reference, and we know that would not create bugs because by careful
code inspection we realize that the life of the two references is
overlapped without a hole inbetween. I'm not a fan of this.
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Wed, Jan 28, 2026 at 04:58:18PM +0100, Luca Ceresoli wrote:
> On Tue Jan 27, 2026 at 4:54 AM CET, Liu Ying wrote:
>
> ...
>
>>>>> @@ -260,7 +259,7 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>>>> {
>>>>> struct device_node *np = pl->dev->of_node;
>>>>> struct device_node *port;
>>>>> - struct drm_bridge *selected_bridge = NULL;
>>>>> + struct drm_bridge *selected_bridge __free(drm_bridge_put) = NULL;
>>>>> u32 port_id;
>>>>> bool found_port = false;
>>>>> int reg;
>>>>> @@ -297,7 +296,8 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>>>> continue;
>>>>> }
>>>>>
>>>>> - struct drm_bridge *next_bridge = of_drm_find_bridge(remote);
>>>>> + struct drm_bridge *next_bridge __free(drm_bridge_put) =
>>>>> + of_drm_find_and_get_bridge(remote);
>>>>> if (!next_bridge)
>>>>> return -EPROBE_DEFER;
>>>>>
>>>>> @@ -305,12 +305,14 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>>>> * Select the next bridge with companion PXL2DPI if
>>>>> * present, otherwise default to the first bridge
>>>>> */
>>>>> - if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi"))
>>>>> - selected_bridge = next_bridge;
>>>>> + if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
>>>>> + drm_bridge_put(selected_bridge);
>>>>> + selected_bridge = drm_bridge_get(next_bridge);
>>>>
>>>> Considering selecting the first bridge without the companion pxl2dpi,
>>>> there would be a superfluous refcount for the selected bridge:
>>>>
>>>> 1) of_drm_find_and_get_bridge: refcount = 1
>>>> 2) drm_bridge_put: noop, since selected_bridge is NULL, refcount = 1
>>>> 3) drm_bridge_get: refcount = 2
>>>> 4) drm_bridge_put(__free): refcount = 1
>>>> 5) drm_bridge_get: for the pl->bridge.next_bridge, refcount = 2
>>>
>>> Here you are missing one put. There are two drm_bridge_put(__free), one for
>>> next_bridge and one for selected_bridge. So your list should rather be:
>>>
>>> 1) next_bridge = of_drm_find_and_get_bridge: refcount = 1
>>> 2) drm_bridge_put(selected_bridge): noop, since selected_bridge is NULL, refcount = 1
>>> 3) selected_bridge = drm_bridge_get: refcount = 2
>>> 4) drm_bridge_put(next_bridge) [__free at loop scope end]: refcount = 1
>>> 5) pl->bridge.next_bridge = drm_bridge_get(), refcount = 2
>>> 6) drm_bridge_put(selected_bridge) [__free at function scope end]: refcount = 1
>>
>> Ah, right, I did miss this last put because selected_bridge is declared with
>> __free a bit far away from the loop at the very beginning of
>> imx8qxp_pixel_link_find_next_bridge() - that's my problem I guess, but I'm
>> not even sure if I'll fall into this same pitfall again after a while, which
>> makes the driver difficult to maintain.
>>
>> Also, it seems that the refcount dance(back and forth bewteen 1 and 2) is not
>> something straightforward for driver readers to follow.
>
> I thing the whole logic becomes straightforward if you think it this way:
>
> * when a pointer is assigned = a new reference starts existing -> refcount++
> * when a pointer is cleared/overwritten or goes out of scope = a reference
> stops existing -> refcount--
>
> In short: one pointer, one reference, one refcount.
>
> If you re-read the patch with this in mind, does it become clearer?
Thanks for more explaination, maybe it becomes a bit clearer, I'm not sure:/
Anyway, to simplify things with another try, I came up with the below
snippet in that loop, which drops the two intermediate bridges(local
next_bridge and selected_bridge) and uses pl->next_bridge only.
It looks ok to me(at least, refcount dance is much simpler).
-8<-
if (!pl->next_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
drm_bridge_put(pl->next_bridge);
pl->next_bridge = of_drm_find_and_get_bridge(remote);
if (!pl->next_bridge)
return -EPROBE_DEFER;
}
-8<-
What do you think?
--
Regards,
Liu Ying
On Thu, Jan 29, 2026 at 03:49:38PM +0800, Liu Ying wrote:
>
>
> On Wed, Jan 28, 2026 at 04:58:18PM +0100, Luca Ceresoli wrote:
>> On Tue Jan 27, 2026 at 4:54 AM CET, Liu Ying wrote:
>>
>> ...
>>
>>>>>> @@ -260,7 +259,7 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>>>>> {
>>>>>> struct device_node *np = pl->dev->of_node;
>>>>>> struct device_node *port;
>>>>>> - struct drm_bridge *selected_bridge = NULL;
>>>>>> + struct drm_bridge *selected_bridge __free(drm_bridge_put) = NULL;
>>>>>> u32 port_id;
>>>>>> bool found_port = false;
>>>>>> int reg;
>>>>>> @@ -297,7 +296,8 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>>>>> continue;
>>>>>> }
>>>>>>
>>>>>> - struct drm_bridge *next_bridge = of_drm_find_bridge(remote);
>>>>>> + struct drm_bridge *next_bridge __free(drm_bridge_put) =
>>>>>> + of_drm_find_and_get_bridge(remote);
>>>>>> if (!next_bridge)
>>>>>> return -EPROBE_DEFER;
>>>>>>
>>>>>> @@ -305,12 +305,14 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>>>>> * Select the next bridge with companion PXL2DPI if
>>>>>> * present, otherwise default to the first bridge
>>>>>> */
>>>>>> - if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi"))
>>>>>> - selected_bridge = next_bridge;
>>>>>> + if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
>>>>>> + drm_bridge_put(selected_bridge);
>>>>>> + selected_bridge = drm_bridge_get(next_bridge);
>>>>>
>>>>> Considering selecting the first bridge without the companion pxl2dpi,
>>>>> there would be a superfluous refcount for the selected bridge:
>>>>>
>>>>> 1) of_drm_find_and_get_bridge: refcount = 1
>>>>> 2) drm_bridge_put: noop, since selected_bridge is NULL, refcount = 1
>>>>> 3) drm_bridge_get: refcount = 2
>>>>> 4) drm_bridge_put(__free): refcount = 1
>>>>> 5) drm_bridge_get: for the pl->bridge.next_bridge, refcount = 2
>>>>
>>>> Here you are missing one put. There are two drm_bridge_put(__free), one for
>>>> next_bridge and one for selected_bridge. So your list should rather be:
>>>>
>>>> 1) next_bridge = of_drm_find_and_get_bridge: refcount = 1
>>>> 2) drm_bridge_put(selected_bridge): noop, since selected_bridge is NULL, refcount = 1
>>>> 3) selected_bridge = drm_bridge_get: refcount = 2
>>>> 4) drm_bridge_put(next_bridge) [__free at loop scope end]: refcount = 1
>>>> 5) pl->bridge.next_bridge = drm_bridge_get(), refcount = 2
>>>> 6) drm_bridge_put(selected_bridge) [__free at function scope end]: refcount = 1
>>>
>>> Ah, right, I did miss this last put because selected_bridge is declared with
>>> __free a bit far away from the loop at the very beginning of
>>> imx8qxp_pixel_link_find_next_bridge() - that's my problem I guess, but I'm
>>> not even sure if I'll fall into this same pitfall again after a while, which
>>> makes the driver difficult to maintain.
>>>
>>> Also, it seems that the refcount dance(back and forth bewteen 1 and 2) is not
>>> something straightforward for driver readers to follow.
>>
>> I thing the whole logic becomes straightforward if you think it this way:
>>
>> * when a pointer is assigned = a new reference starts existing -> refcount++
>> * when a pointer is cleared/overwritten or goes out of scope = a reference
>> stops existing -> refcount--
>>
>> In short: one pointer, one reference, one refcount.
>>
>> If you re-read the patch with this in mind, does it become clearer?
>
> Thanks for more explaination, maybe it becomes a bit clearer, I'm not sure:/
>
> Anyway, to simplify things with another try, I came up with the below
> snippet in that loop, which drops the two intermediate bridges(local
> next_bridge and selected_bridge) and uses pl->next_bridge only.
Fix a typo:
s/pl->next_bridge/pl->bridge.next_bridge/
> It looks ok to me(at least, refcount dance is much simpler).
>
> -8<-
> if (!pl->next_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
> drm_bridge_put(pl->next_bridge);
> pl->next_bridge = of_drm_find_and_get_bridge(remote);
> if (!pl->next_bridge)
> return -EPROBE_DEFER;
> }
> -8<-
-8<-
if (!pl->bridge.next_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
drm_bridge_put(pl->bridge.next_bridge);
pl->bridge.next_bridge = of_drm_find_and_get_bridge(remote);
if (!pl->bridge.next_bridge)
return -EPROBE_DEFER;
}
-8<-
>
> What do you think?
>
--
Regards,
Liu Ying
On Thu Jan 29, 2026 at 9:18 AM CET, Liu Ying wrote:
>
>
> On Thu, Jan 29, 2026 at 03:49:38PM +0800, Liu Ying wrote:
>>
>>
>> On Wed, Jan 28, 2026 at 04:58:18PM +0100, Luca Ceresoli wrote:
>>> On Tue Jan 27, 2026 at 4:54 AM CET, Liu Ying wrote:
>>>
>>> ...
>>>
>>>>>>> @@ -260,7 +259,7 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>>>>>> {
>>>>>>> struct device_node *np = pl->dev->of_node;
>>>>>>> struct device_node *port;
>>>>>>> - struct drm_bridge *selected_bridge = NULL;
>>>>>>> + struct drm_bridge *selected_bridge __free(drm_bridge_put) = NULL;
>>>>>>> u32 port_id;
>>>>>>> bool found_port = false;
>>>>>>> int reg;
>>>>>>> @@ -297,7 +296,8 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>>>>>> continue;
>>>>>>> }
>>>>>>>
>>>>>>> - struct drm_bridge *next_bridge = of_drm_find_bridge(remote);
>>>>>>> + struct drm_bridge *next_bridge __free(drm_bridge_put) =
>>>>>>> + of_drm_find_and_get_bridge(remote);
>>>>>>> if (!next_bridge)
>>>>>>> return -EPROBE_DEFER;
>>>>>>>
>>>>>>> @@ -305,12 +305,14 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>>>>>> * Select the next bridge with companion PXL2DPI if
>>>>>>> * present, otherwise default to the first bridge
>>>>>>> */
>>>>>>> - if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi"))
>>>>>>> - selected_bridge = next_bridge;
>>>>>>> + if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
>>>>>>> + drm_bridge_put(selected_bridge);
>>>>>>> + selected_bridge = drm_bridge_get(next_bridge);
>>>>>>
>>>>>> Considering selecting the first bridge without the companion pxl2dpi,
>>>>>> there would be a superfluous refcount for the selected bridge:
>>>>>>
>>>>>> 1) of_drm_find_and_get_bridge: refcount = 1
>>>>>> 2) drm_bridge_put: noop, since selected_bridge is NULL, refcount = 1
>>>>>> 3) drm_bridge_get: refcount = 2
>>>>>> 4) drm_bridge_put(__free): refcount = 1
>>>>>> 5) drm_bridge_get: for the pl->bridge.next_bridge, refcount = 2
>>>>>
>>>>> Here you are missing one put. There are two drm_bridge_put(__free), one for
>>>>> next_bridge and one for selected_bridge. So your list should rather be:
>>>>>
>>>>> 1) next_bridge = of_drm_find_and_get_bridge: refcount = 1
>>>>> 2) drm_bridge_put(selected_bridge): noop, since selected_bridge is NULL, refcount = 1
>>>>> 3) selected_bridge = drm_bridge_get: refcount = 2
>>>>> 4) drm_bridge_put(next_bridge) [__free at loop scope end]: refcount = 1
>>>>> 5) pl->bridge.next_bridge = drm_bridge_get(), refcount = 2
>>>>> 6) drm_bridge_put(selected_bridge) [__free at function scope end]: refcount = 1
>>>>
>>>> Ah, right, I did miss this last put because selected_bridge is declared with
>>>> __free a bit far away from the loop at the very beginning of
>>>> imx8qxp_pixel_link_find_next_bridge() - that's my problem I guess, but I'm
>>>> not even sure if I'll fall into this same pitfall again after a while, which
>>>> makes the driver difficult to maintain.
>>>>
>>>> Also, it seems that the refcount dance(back and forth bewteen 1 and 2) is not
>>>> something straightforward for driver readers to follow.
>>>
>>> I thing the whole logic becomes straightforward if you think it this way:
>>>
>>> * when a pointer is assigned = a new reference starts existing -> refcount++
>>> * when a pointer is cleared/overwritten or goes out of scope = a reference
>>> stops existing -> refcount--
>>>
>>> In short: one pointer, one reference, one refcount.
>>>
>>> If you re-read the patch with this in mind, does it become clearer?
>>
>> Thanks for more explaination, maybe it becomes a bit clearer, I'm not sure:/
>>
>> Anyway, to simplify things with another try, I came up with the below
>> snippet in that loop, which drops the two intermediate bridges(local
>> next_bridge and selected_bridge) and uses pl->next_bridge only.
>
> Fix a typo:
> s/pl->next_bridge/pl->bridge.next_bridge/
>
>> It looks ok to me(at least, refcount dance is much simpler).
>>
>> -8<-
>> if (!pl->next_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
>> drm_bridge_put(pl->next_bridge);
>> pl->next_bridge = of_drm_find_and_get_bridge(remote);
>> if (!pl->next_bridge)
>> return -EPROBE_DEFER;
>> }
>> -8<-
>
> -8<-
> if (!pl->bridge.next_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
> drm_bridge_put(pl->bridge.next_bridge);
> pl->bridge.next_bridge = of_drm_find_and_get_bridge(remote);
> if (!pl->bridge.next_bridge)
> return -EPROBE_DEFER;
> }
> -8<-
It's OK enough, so in v5 I'm going to split the if() and skip the
intermediate selected_bridge variable.
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hello Liu,
On Mon Jan 26, 2026 at 7:18 PM CET, Luca Ceresoli wrote:
>>> @@ -260,7 +259,7 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>> {
>>> struct device_node *np = pl->dev->of_node;
>>> struct device_node *port;
>>> - struct drm_bridge *selected_bridge = NULL;
>>> + struct drm_bridge *selected_bridge __free(drm_bridge_put) = NULL;
>>> u32 port_id;
>>> bool found_port = false;
>>> int reg;
>>> @@ -297,7 +296,8 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>> continue;
>>> }
>>>
>>> - struct drm_bridge *next_bridge = of_drm_find_bridge(remote);
>>> + struct drm_bridge *next_bridge __free(drm_bridge_put) =
>>> + of_drm_find_and_get_bridge(remote);
>>> if (!next_bridge)
>>> return -EPROBE_DEFER;
>>>
>>> @@ -305,12 +305,14 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>> * Select the next bridge with companion PXL2DPI if
>>> * present, otherwise default to the first bridge
>>> */
>>> - if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi"))
>>> - selected_bridge = next_bridge;
>>> + if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
>>> + drm_bridge_put(selected_bridge);
>>> + selected_bridge = drm_bridge_get(next_bridge);
>>
>> Considering selecting the first bridge without the companion pxl2dpi,
>> there would be a superfluous refcount for the selected bridge:
>>
>> 1) of_drm_find_and_get_bridge: refcount = 1
>> 2) drm_bridge_put: noop, since selected_bridge is NULL, refcount = 1
>> 3) drm_bridge_get: refcount = 2
>> 4) drm_bridge_put(__free): refcount = 1
>> 5) drm_bridge_get: for the pl->bridge.next_bridge, refcount = 2
>
> Here you are missing one put. There are two drm_bridge_put(__free), one for
> next_bridge and one for selected_bridge. So your list should rather be:
>
> 1) next_bridge = of_drm_find_and_get_bridge: refcount = 1
> 2) drm_bridge_put(selected_bridge): noop, since selected_bridge is NULL, refcount = 1
> 3) selected_bridge = drm_bridge_get: refcount = 2
> 4) drm_bridge_put(next_bridge) [__free at loop scope end]: refcount = 1
> 5) pl->bridge.next_bridge = drm_bridge_get(), refcount = 2
> 6) drm_bridge_put(selected_bridge) [__free at function scope end]: refcount = 1
>
> The idea is that for each pointer (which is a reference) we get a reference
> (refcount++) when the pointer is set and put the reference when that same
> pointer goes out of scope or is reset to NULL. "the pointer is set" can be
> either of_drm_find_and_get_bridge() or an assignment, as each of these
> operations creates another reference (pointer) to the same bridge.
>
> Does it look correct?
Based on this discussion I thought the commit message could be clearer, and
rewrote it as:
-----[no changes from here...]-----
drm/bridge: imx8qxp-pixel-link: get/put the next bridge
This driver obtains a bridge pointer from of_drm_find_bridge() in the probe
function and stores it until driver removal. of_drm_find_bridge() is
deprecated. Move to of_drm_find_and_get_bridge() for the bridge to be
refcounted and use bridge->next_bridge to put the reference on
deallocation.
-----[...to here]-----
To keep the code as simple and reliable as possible, get a reference for
each pointer that stores a drm_bridge address when it is stored and release
when the pointer is set to NULL or goes out of scope. The involved pointers
are:
* next_bridge loop-local variable:
- get reference by of_drm_find_and_get_bridge()
- put reference at the end of the loop iteration (__free)
* selected_bridge function-local variable:
- get reference when written (by copy from next_bridge)
- put reference at function exit (__free)
- put reference before reassignment too
* pl->bridge.next_bridge, tied to struct imx8qxp_pixel_link lifetime:
- get reference when written (by copy from selected_bridge)
- put reference when the struct imx8qxp_pixel_link embedding the
struct drm_bridge is destroyed (struct drm_bridge::next_bridge)
Do you think it's better now?
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
© 2016 - 2026 Red Hat, Inc.