Message-ID: <20250725-juicy-falcon-of-climate-d87ea6@houat>
Date: Fri, 25 Jul 2025 17:22:47 +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

On Mon, Jul 07, 2025 at 12:13:19PM +0200, Luca Ceresoli wrote:
> Hi Maxime,
> 
> ouch, e-mail sent by mistake unfinished and without proof-reading...
> well, let me continue it below.
> 
> On Mon, 7 Jul 2025 11:58:53 +0200
> Luca Ceresoli <luca.ceresoli@...tlin.com> wrote:
> 
> > On Mon, 7 Jul 2025 08:16:49 +0200
> > Maxime Ripard <mripard@...nel.org> wrote:
> > 
> > > 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?  
> > 
> > I probably should have done a recap of the whole discussion, so let me
> > do it now.
> > 
> > It all starts with one fact: a DSI device can be disconnected and then
> > a different one connected later on, having a different DSI bus format
> > (lanes, channel, mode flags, whatever). A detach/attach sequence would
> > handle that, but only in the simple case when there is a host/device
> > pair. Let's how consider this topology:
> >                                                       
> >                 ┌──────────────────┐                  
> >                 │    DSI bridge    │                  
> > ┌─────────┐  A  │                  │  B  ┌───────────┐
> > │ DSI host├────►│device        host├────►│DSI device │
> > └─────────┘     └──────────────────┘     └───────────┘
> >                                                       
> > Here link A is always connected, link B is hot-pluggable. When the tail
> > device is removed and a different one plugged, a detach/attach sequence
> > can update the bus format on the DSI bridge, but then the DSI bridge
> > cannot update the format on the first host without faking a
> > detach/attach that does not map a real event.
> > 
> > The above topology is probably not common, but it is exactly what the
> > hotplug-bridge introduces [0]. Whether the hotplug-bridge will have to
> > eventually exist or not to support hotplug is still to be defined, but
> > regardless there is another problematic aspect.
> > 
> > The second problematic aspect is that several DSI host drivers will not
> > even drm_bridge_add() until they have an attached DSI device. One such
> > example is samsung-dsim, which calls drm_bridge_add()
> > in samsung_dsim_host_attach(). When such a driver implements the first
> > DSI host, the DSI bridge must register a DSI device before the DRM card
> > can be instantiated. See the lengthy comment before
> > hotplug_bridge_dsi_attach() in [0] for more gory details, but the
> > outcome is that the hotplug-bridge needs to attach a DSI device with
> > a fake format once initially just to let the DRM card probe, and the
> > detach and reattach with the correct format once an actual DSI device
> > is connected at the tail.
> > 
> > [0] https://lore.kernel.org/all/20240917-hotplug-drm-bridge-v4-4-bc4dfee61be6@bootlin.com/
> > 
> > The above would be improved if the DSI host API provided a way to
> > notify to the host about a bus format change, which is however not
> > present currently.
> > 
> > The naive solution would be adding a new DSI host op:
> > 
> >  struct mipi_dsi_host_ops {
> >  	int (*attach)(struct mipi_dsi_host *host,
> >  		      struct mipi_dsi_device *dsi);
> >  	int (*detach)(struct mipi_dsi_host *host,
> >  		      struct mipi_dsi_device *dsi);
> > +	int (*bus_fmt_changed)(struct mipi_dsi_host *host,
> > + 		      struct mipi_dsi_device *dsi);
> >  	ssize_t (*transfer)(struct mipi_dsi_host *host,
> >  			    const struct mipi_dsi_msg *msg);
> >  };
> > 
> > This would allow reduce the current sequence:
> >  1. attach with dummy format (no tail device yet)
> >  2. fake detach
> >  3. attach
> > 
> > with:
> >  1. attach with dummy format (no tail device yet)
> >  2. update format
> > 
> > Adding such a new op would be part of chapter 4 of this work, being it
> > quite useless without hotplug.
> > 
> > However while reasoning about this I noticed the DSI host drivers peek
> > into the struct mipi_dsi_device fields to read the format, so there is
> > no sort of isolation between host and device. Introducing a struct to
> > contain all the format fields looked like a good improvement in terms
> > of code organization.
> > 
> > Yet another aspect is that several host drivers keep a pointer to the
> > device, and thus in case of format change in the DSI device they might
> > be reading different fields at different moments, ending up with an
> > inconsistent format.
> > 
> > The above considerations, which are all partially overlapped, led me to
> > the idea of introducing a struct to exchange a DSI bus format, to be
> > exchanged as a whole ("atomically") between host and device. What's
> > your opinion about introducing such a struct?
> > 
> > The second aspect of this series is not passing pointers, and that's
> > the core topic you questioned. I realize it is not strictly necessary
> > to reach the various goals discussed in this e-mail. The work I'm doing
> > on the drm_bridge struct is actually a way to store a pointer while
> > avoiding use-after-free, so that can obviously be done for a simpler
> > scenario such as DSI host-device. However I thought not passing a
> > pointer would be a more radical solution: if a driver receives no
> > pointer, then it cannot by mistake keep it stored when it shouldn't,
> > maybe in a rare case within a complex driver where it is hard to spot.
> > 
> > I'll be OK to change the approach and keep the pointer passed in the
> > attach/detach ops, if that is the best option. However I'd like to have
> > your opinion about the above topics before working towards that
> > direction, and ensure I fully grasp the usefulness of keeping the
> > pointer.
> > 
> > Post scriptum. The very initial issue that led to all this discussion
> > when writing the hotplug-bridge driver is that the samsung-dsim driver
> > will not drm_bridge_add() until a DSI device does .attach to it. Again,
> > see the comments before hotplug_bridge_dsi_attach() in [0] for details.
> > However by re-examining the driver for the N-th time now from a new
> > POV, I _think_ this is not correct and potentially easy to solve. But this leads to one fundamental question:
> 
> The question is: should a DSI host bridge driver:
> 
>  A) wait for a DSI device to .attach before drm_bridge_add()ing itself,
>     or
>  B) drm_bridge_add() itself unconditionally, and let the DSI device
>     .attach whenever it happens?
> 
> A) is what many drivers (IIRC the majority) does. It implies the card
> will not be populated until .attach, which in the hotplug case could
> happen very late
> 
> B) is done by a few drivers and allows the card to appear in the
> hotplug case without the device, which is needed for hotplug.
> 
> I had tried simply moving drm_bridge_add() from .attach to probe in
> the samsung-dsim driver in the pase but that would not work. Now I did
> yet another check at the code and I suspect it can be done with a small
> additional change, but cannot access the hardware to test it currently.
> 
> Answering this last question might change and simplify the requirements
> discussed in the (very lengthy, sorry about that) discussion above.

You're not going to like the answer too much, but I think that it is
that "nobody knows".

The bad news is that I can't give you an answer, but the good one is
that it gives us some leeway.

As I said in my previous mail, we should probably figure it out,
document it, and then make it work for your drivers. Once we have a
documentation we can point to, the rest will fall in line.

Maxime

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

Powered by blists - more mailing lists