linux-kernel - Re: [PATCH v2 1/7] drm/encoder: add mutex to protect the bridge chain

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: <DDC59CH6JUWR.GRJ77V41K2DC@bootlin.com>
Date: Tue, 07 Oct 2025 16:02:47 +0200
From: "Luca Ceresoli" <luca.ceresoli@...tlin.com>
To: "Luca Ceresoli" <luca.ceresoli@...tlin.com>, "Dmitry Baryshkov"
 <dmitry.baryshkov@....qualcomm.com>
Cc: "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>, "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>, "Hui Pu"
 <Hui.Pu@...ealthcare.com>, "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>, <dri-devel@...ts.freedesktop.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/7] drm/encoder: add mutex to protect the bridge
 chain

Hi Dmitry,

On Tue Oct 7, 2025 at 11:45 AM CEST, Luca Ceresoli wrote:
> Hi Dmitry,
>
> On Sat Oct 4, 2025 at 11:47 AM CEST, Dmitry Baryshkov wrote:
>
>>> @@ -319,6 +323,41 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
>>>  	return mo ? obj_to_encoder(mo) : NULL;
>>>  }
>>>  
>>> +/**
>>> + * drm_encoder_chain_lock - lock the encoder bridge chain
>>> + * @encoder: encoder whose bridge chain must be locked
>>> + *
>>> + * Locks the mutex protecting the bridge chain from concurrent access.
>>> + * To be called by code modifying ot iterating over the bridge chain to
>>> + * prevent the list from changing while iterating over it.
>>> + * Call drm_encoder_chain_unlock() when done to unlock the mutex.
>>> + *
>>> + * Returns:
>>> + * Pointer to @encoder. Useful to lock the chain and then operate on the
>>> + * in the same statement, e.g.:
>>> + * list_first_entry_or_null(&drm_encoder_chain_lock(encoder)->bridge_chain)
>>> + */
>>> +static inline struct drm_encoder *drm_encoder_chain_lock(struct drm_encoder *encoder)
>>
>> What is the use case for these wrappers? I'm asking especially since
>> you almost never use the return value of the _lock() one. I think with
>> scoped_guard you can get the same kind of code without needing extra API
>> or extra wrappers.
>
> For two reasons.
>
> One is to avoid drm_encoder users to need to access internal fields
> (encapsulation, in object-oriented jargon). But if I read correctly between
> the lines of your question, it is not worth because drm_bridge and
> drm_encoder are already interdependent?
>
> The second is that the C language spec sets tight constraints to the
> drm_for_each_bridge_in_chain_scoped(). The macro must look like:
>
>   #define drm_for_each_bridge_in_chain_scoped(encoder, bridge) \
>           for (struct drm_bridge *bridge = <FOO>; clause-2; clause-3)
> 	       '----------- clause-1 ----------'
>
> clause-1 must:
>
>  * declare a 'struct drm_bridge *' variable (the loop cursor)
>  * initialize it via <FOO> which thus must be a rvalue of type
>    'struct drm_bridge *' (<FOO> must be a function or a macro, as a
>    variable with the correct value is not available)
>  * use the struct drm_encoder * as its sole input
>  * lock the encoder chain mutex
>  * get a reference to the bridge (as Maxime requested)
>  * ensure the bridge reference is put and the mutex is released on break
>    and return (clause-3 can't do that)
>
> Given the above, we still need a function that locks the encoder chain
> mutex and returns the encoder (bullets 3 and 4), like
> drm_encoder_chain_lock(). I'm OK with removing drm_encoder_chain_lock() and
> replace it with an internal macro or function in drm_bridge.h though.
>
> However I'm not sure how to use scoped_guard here because it doesn't return
> a pointer that can then be passed further. Basically we are constrained to
> have a chain of function or macro calls, each eating the result of the
> inner one, with the outer one returning a bridge pointer for the loop
> cursor variable. There might be some macro magic I'm missing, in such case
> don't hesitate to mention that.

I realized I was not fully clear, sorry about that. The inability to use
scoped_guard refers to the drm_for_each_bridge_in_chain_scoped()
implementation, being a macro defining a for loop. However scoped_guard can
be used in normal locking code such as the changes in patches 3, 6 and 7.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com