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
