linux-kernel - Re: [PATCH v2] drm/bridge: Fix assignment of the of

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241030193928.GC920@pendragon.ideasonboard.com>
Date: Wed, 30 Oct 2024 21:39:28 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Sui Jingfeng <sui.jingfeng@...ux.dev>
Cc: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
	Abel Vesa <abel.vesa@...aro.org>,
	Andrzej Hajda <andrzej.hajda@...el.com>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Robert Foss <rfoss@...nel.org>, 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>,
	Johan Hovold <johan@...nel.org>, dri-devel@...ts.freedesktop.org,
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the
 parent to aux bridge

On Thu, Oct 31, 2024 at 12:45:24AM +0800, Sui Jingfeng wrote:
> Hi,
> 
> On 2024/10/18 23:43, Dmitry Baryshkov wrote:
> > On Fri, Oct 18, 2024 at 03:49:34PM +0300, Abel Vesa wrote:
> >> The assignment of the of_node to the aux bridge needs to mark the
> >> of_node as reused as well, otherwise resource providers like pinctrl will
> >> report a gpio as already requested by a different device when both pinconf
> >> and gpios property are present.
> >> Fix that by using the device_set_of_node_from_dev() helper instead.
> >>
> >> Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers")
> >> Cc: stable@...r.kernel.org      # 6.8
> >> Cc: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> >> Signed-off-by: Abel Vesa <abel.vesa@...aro.org>
> >> ---
> >> Changes in v2:
> >> - Re-worded commit to be more explicit of what it fixes, as Johan suggested
> >> - Used device_set_of_node_from_dev() helper, as per Johan's suggestion
> >> - Added Fixes tag and cc'ed stable
> >> - Link to v1: https://lore.kernel.org/r/20241017-drm-aux-bridge-mark-of-node-reused-v1-1-7cd5702bb4f2@linaro.org
> >> ---
> >>   drivers/gpu/drm/bridge/aux-bridge.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> 
> Technically speaking, your driver just move the burden to its caller.
> Because this driver requires its user call drm_aux_bridge_register()
> to create an AUX child device manually, you need it call ida_alloc()
> to generate a unique id.

There's a relevant discussion for a ti-sn65dsi86 patch, see
https://lore.kernel.org/r/20241030102846.GB14276@pendragon.ideasonboard.com

I agree it shouldn't be the responsibility of each caller to generate
unique IDs.

> Functions symbols still have to leak to other subsystems, which is
> not really preserve coding sharing.
> 
> What's worse, the action that allocating unique device id traditionally
> is the duty of driver core. Why breaks (so called) perfect device driver
> model by moving that out of core. Especially in the DT world that the
> core knows very well how to populate device instance and manage the
> reference counter.
> 
> HPD handling is traditionally belongs to connector, create standalone
> driver like this one *abuse* to both Maxime's simple bridge driver and
> Laurent's display-connector bridge driver or drm_bridge_connector or
> whatever. Why those work can't satisfy you? At least, their drivers
> are able to passing the mode setting states to the next bridge.
> 
> Basically those AUX drivers implementation abusing the definition of
> bridge, abusing the definition of connector and abusing the DT.
> Its just manually populate instances across drivers.

-- 
Regards,

Laurent Pinchart