| 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
| ||
|
Message-ID: <aff6f95e-abdc-4253-8724-eec4a98bb268@nxp.com>
Date: Thu, 29 Jan 2026 16:18:32 +0800
From: Liu Ying <victor.liu@....com>
To: Luca Ceresoli <luca.ceresoli@...tlin.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
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
Powered by blists - more mailing lists