linux-kernel - Re: [PATCH v4 4/4] drm/bridge: imx8qxp-pixel-link: get/put the next bridge

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <DFYUVCNVLBND.3MHWLTF20L4TS@bootlin.com>
Date: Mon, 26 Jan 2026 22:57:35 +0100
From: "Luca Ceresoli" <luca.ceresoli@...tlin.com>
To: "Luca Ceresoli" <luca.ceresoli@...tlin.com>, "Liu Ying"
 <victor.liu@....com>, "Andrzej Hajda" <andrzej.hajda@...el.com>, "Neil
 Armstrong" <neil.armstrong@...aro.org>, "Robert Foss" <rfoss@...nel.org>,
 "Laurent Pinchart" <Laurent.pinchart@...asonboard.com>, "Jonas Karlman"
 <jonas@...boo.se>, "Jernej Skrabec" <jernej.skrabec@...il.com>, "Maarten
 Lankhorst" <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard"
 <mripard@...nel.org>, "Thomas Zimmermann" <tzimmermann@...e.de>, "David
 Airlie" <airlied@...il.com>, "Simona Vetter" <simona@...ll.ch>, "Shawn Guo"
 <shawnguo@...nel.org>, "Sascha Hauer" <s.hauer@...gutronix.de>,
 "Pengutronix Kernel Team" <kernel@...gutronix.de>, "Fabio Estevam"
 <festevam@...il.com>
Cc: "Hui Pu" <Hui.Pu@...ealthcare.com>, "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>, <dri-devel@...ts.freedesktop.org>,
 <imx@...ts.linux.dev>, <linux-arm-kernel@...ts.infradead.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 4/4] drm/bridge: imx8qxp-pixel-link: get/put the next
 bridge

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