Message-ID: <20250707-strange-warm-bear-cb4ee8@houat>
Date: Mon, 7 Jul 2025 08:16:49 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Luca Ceresoli <luca.ceresoli@...tlin.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	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>, Inki Dae <inki.dae@...sung.com>, 
	Jagan Teki <jagan@...rulasolutions.com>, Marek Szyprowski <m.szyprowski@...sung.com>, 
	Jani Nikula <jani.nikula@...ux.intel.com>, Dmitry Baryshkov <lumag@...nel.org>, 
	Hui Pu <Hui.Pu@...ealthcare.com>, Thomas Petazzoni <thomas.petazzoni@...tlin.com>, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, linux-sunxi@...ts.linux.dev, 
	Kevin Hilman <khilman@...libre.com>, Jerome Brunet <jbrunet@...libre.com>, 
	Martin Blumenstingl <martin.blumenstingl@...glemail.com>, linux-amlogic@...ts.infradead.org
Subject: Re: [PATCH 00/32] drm/mipi-dsi: avoid DSI host drivers to have
 pointers to DSI devices

Hi Luca,

On Wed, Jun 25, 2025 at 06:45:04PM +0200, Luca Ceresoli wrote:
> This series is the first attempt at avoiding DSI host drivers to have
> pointers to DSI devices (struct mipi_dsi_device), as discussed during the
> Linux Plumbers Conference 2024 with Maxime and Dmitry.
> 
> It is working, but I consider this a draft in order to discuss and
> challenge the proposed approach.
> 
> Overall work
> ============
> 
> This is part of the work towards removal of bridges from a still existing
> DRM pipeline without use-after-free. The grand plan as discussed in [1].
> Here's the work breakdown (➜ marks the current series):
> 
>  1. … add refcounting to DRM bridges (struct drm_bridge)
>     (based on devm_drm_bridge_alloc() [0])
>     A. ✔ add new alloc API and refcounting (in v6.16-rc1)
>     B. ✔ convert all bridge drivers to new API (now in drm-misc-next)
>     C. ✔ kunit tests (now in drm-misc-next)
>     D. … add get/put to drm_bridge_add/remove() + attach/detach()
>          and warn on old allocation pattern (under review)
>     E. … add get/put on drm_bridge accessors
>        1. … drm_bridge_chain_get_first_bridge() + add a cleanup action
>        2. … drm_bridge_chain_get_last_bridge()
>        3. drm_bridge_get_prev_bridge()
>        4. drm_bridge_get_next_bridge()
>        5. drm_for_each_bridge_in_chain()
>        6. drm_bridge_connector_init
>        7. of_drm_find_bridge
>        8. drm_of_find_panel_or_bridge, *_of_get_bridge
>     F. debugfs improvements
>  2. handle gracefully atomic updates during bridge removal
>  3. ➜ avoid DSI host drivers to have dangling pointers to DSI devices
>       (this series)
>  4. finish the hotplug bridge work, removing the "always-disconnected"
>     connector, moving code to the core and potentially removing the
>     hotplug-bridge itself (this needs to be clarified as points 1-3 are
>     developed)
> 
> [0] https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/0cc6aadd7fc1e629b715ea3d1ba537ef2da95eec
> [1] https://lore.kernel.org/lkml/20250206-hotplug-drm-bridge-v6-0-9d6f2c9c3058@bootlin.com/t/#u
> 
> Motivation
> ==========
> 
> The motivation for this series is that with hot-pluggable hardware a DSI
> device can be disconnected from the DSI host at runtime, and later on
> reconnected, potentially with a different model having different bus
> parameters.
> 
> DSI host drivers currently receive a struct mipi_dsi_device pointer in the
> attach callback and some store it permanently for later access to the bur
> format data (lanes, channel, pixel format etc). The stored pointer can
> become dangling if the device is removed, leading to a use-after-free.
> 
> Currently the data exchange between DSI host and device happens primarily
> by two means:
> 
>  * the device requests attach, detach and message transfer to the host by
>    calling mipi_dsi_attach/detach/transfer which in turn call the callbacks
>    in struct mipi_dsi_host_ops
>     - for this to work, struct mipi_dsi_device has a pointer to the host:
>       this is OK because the goal is supporting hotplug of the "remote"
>       part of the DRM pipeline
>  * the host accesses directly the fields of struct mipi_dsi_device, to
>    which it receives a pointer in the .attach and .detach callbacks
> 
> The second bullet is the problematic one, which we want to remove.
> 
> Strategy
> ========
> 
> I devised two possible strategies to address it:
> 
>  1. change the host ops to not pass a struct mipi_dsi_device, but instead
>     to pass only a copy of the needed information (bus format mainly), so
>     the host driver does never access any info from the device
>     
>  2. let the host get info from the device as needed, but without having a
>     pointer to it; this is be based on:
>      - storing a __private mipi_dsi_device pointer in struct mipi_dsi_host
>      - adding getters to the DSI core for the host to query the needed
>        info, e.g. drm_mipi_dsi_host_get_device_lanes(host) (the getters
>        would be allowed to dereference the device pointer)
> 
> This series implements strategy 1. It does so by adding a .attach_new host
> op, which does not take a mipi_dsi_device pointer, and converting most host
> drivers to it. Once all drivers are converted, the old op can be removed,
> and .attach_new renamed to .attach.

I don't recall discussing this particular aspect at Plumbers, so sorry
if we're coming back to the same discussion we had.

I'm not necessarily opposed to changing the MIPI-DSI bus API, but I
don't think changing the semantics to remove the fact that a particular
device is connected or not is a good idea.

I would have expected to have bus driver (maybe) take a device pointer
at attach, and drop it at detach.

Then, when we detect the hotplug of a DSI device, we detach it from its
parent, and we're done.

What prevents us from using that approach?

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)

Powered by blists - more mailing lists