[PATCH 00/32] drm/mipi-dsi: avoid DSI host drivers to have pointers to DSI devices

[PATCH 00/32] drm/mipi-dsi: avoid DSI host drivers to have pointers to DSI devices

[Luca Ceresoli]

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.

Limitations of this series
==========================

I could not convert a few drivers is an obvious way, due to the use they
make of the device pointer. Those are not converted in this series (thus
the "draft" status mentioned above). They are described in a dedicated
section below.

Also, this series only address the .attach op. if the approach is generally
agreed I will proceed to .detach as well, which I estimate to be much
easier.

It is important to note that while the DSI bus specification allows
connecting multiple devices to the same bus using different channels, none
of the drivers in the current Linux kernel appears to support it, and this
series does not try to fill that gap.

Series layout
=============

The core of this work is in part 4 (adding .attach_new) and the driver
conversion in part 5.

The first two parts are generic cleanups that I deem generally a good
improvement and can be applied independently. They are in the same series
only to avoid patch conflicts. Part 3 is also quite orthogonal to the main
topic of the series. 

The above results in lots of patches and lots of files, so I had to trim
the recipients list in order to send a series that is comprehensive enough
to show the main idea. I'm OK with splitting it in smaller series if the
principle is OK. It would be nice if these 3 parts could be applied quickly
though, to shave the number of simple patches to maintain over iterations.

 * Generic cleanup: add lane number check in the core

     drm/mipi-dsi: add sanity check of lane number in mipi_dsi_attach()
     drm/hisilicon/kirin: remove redundant lanes number check
     drm/bridge: nwl-dsi: remove redundant lanes number check
     drm/mcde: remove redundant lanes number check

 * Generic cleanup: move attach/detach logging to the core

     drm/mipi-dsi: log DSI device attach and detach
     drm/bridge: samsung-dsim: remove redundant logging
     drm/bridge: nwl-dsi: remove redundant logging
     drm/bridge: cdns-dsi: remove redundant logging
     drm/mcde: remove redundant logging
     drm/sun4i: dsi: remove redundant logging

 * Remove some one-fo-a-kind mipi_dsi_device dereferences
 
     drm/bridge: synopsys/dsi2: remove DSI device pointer from private callbacks
     [RFC!] drm/meson: dsi: remove unneeded DSI device check

 * Introduce .attach_new host op
 
     drm/mipi-dsi: move format define above
     drm/mipi-dsi: add .attach_new to mipi_dsi_host_ops

 * Convert most drivers to attach_new (in increasing order of complexity)

     drm: adp: mipi: convert to the .attach_new op
     drm/kmb: dsi: convert to the .attach_new op
     drm/i915/dsi: convert to the .attach_new op
     drm/hisilicon/kirin: convert to the .attach_new op
     drm/bridge: synopsys/dsi2: convert to the .attach_new op
     drm/msm/dsi: convert to the .attach_new op
     drm/rcar-du: dsi: convert to the .attach_new op
     drm: renesas: rz-du: rzg2l_mipi_dsi: convert to the .attach_new op
     drm/vc4: dsi: convert to the .attach_new op
     drm/mediatek: dsi: convert to the .attach_new op
     drm/bridge: nwl-dsi: convert to the .attach_new op
     drm/bridge: cdns-dsi: convert to the .attach_new op
     drm/bridge: tc358768: convert to the .attach_new op
     drm/sprd: convert to the .attach_new op
     drm/bridge: synopsys: dw-mipi-dsi: convert to the .attach_new op
     drm/mcde: store a pointer to mipi_dsi_host to perform TE requests
     drm/mcde: use the DSI host pointer in mcde_dsi_irq
     drm/mcde: convert to the .attach_new op

Problematic cases, not yet converted
====================================

Drivers requesting an IRQ in command mode
-----------------------------------------

Two host drivers register an IRQ when in DSI command mode, both using
mipi_dsi_device.dev to get the TE GPIO.

 * samsung-dsim.c (samsung_dsim_host_attach), which has an interesting
   comment saying this should be changed:
   
 	/*
 	 * This is a temporary solution and should be made by more generic way.
 	 *
 	 * If attached panel device is for command mode one, dsi should register
 	 * TE interrupt handler.
 	 */
 	if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
		ret = samsung_dsim_register_te_irq(dsi, &device->dev);

 * omapdrm/dss/dsi.c (omap_dsi_host_attach), doing similarly but without
   comments:
 
 	if (client->mode_flags & MIPI_DSI_MODE_VIDEO) {
 		dsi->mode = OMAP_DSS_DSI_VIDEO_MODE;
 	} else {
		r = omap_dsi_register_te_irq(dsi, client);

Maybe this could be handled by the core?

Drivers looking for a panel
---------------------------

Two host drivers use mipi_dsi_device->dev.of_node to look up a panel via
of_drm_find_panel() in .attach:

 * sun6i_mipi_dsi.c (sun6i_dsi_attach):
 
 	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
	struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);

 * tegra/dsi.c, which additionally does some checks in .detach:
 
    - tegra_dsi_host_attach:

 	if (!dsi->master) {
 		struct tegra_output *output = &dsi->output;
 
		output->panel = of_drm_find_panel(device->dev.of_node);
 		if (IS_ERR(output->panel))
 			output->panel = NULL;
		...	
 	}
	
    - tegra_dsi_host_detach -- not sure this check is really needed,
      similarly to patch "[RFC!] drm/meson: dsi: remove unneeded DSI device
      check" in this series:

 	struct tegra_dsi *dsi = host_to_tegra(host);
 	struct tegra_output *output = &dsi->output;
 
	if (output->panel && &device->dev == output->panel->dev) {
 		output->panel = NULL;
 
 		if (output->connector.dev)
 			drm_helper_hpd_irq_event(output->connector.dev);
 	}

Anusha, Maxime, do you think the ongoing work on panel lifetime and the
panel_bridge can interact with this? DO you see any short-term soliution
while that progresses?

Best regards,
Luca

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (32):
      drm/mipi-dsi: add sanity check of lane number in mipi_dsi_attach()
      drm/hisilicon/kirin: remove redundant lanes number check
      drm/bridge: nwl-dsi: remove redundant lanes number check
      drm/mcde: remove redundant lanes number check
      drm/mipi-dsi: log DSI device attach and detach
      drm/bridge: samsung-dsim: remove redundant logging
      drm/bridge: nwl-dsi: remove redundant logging
      drm/bridge: cdns-dsi: remove redundant logging
      drm/mcde: remove redundant logging
      drm/sun4i: dsi: remove redundant logging
      drm/bridge: synopsys/dsi2: remove DSI device pointer from private callbacks
      [RFC] drm/meson: dsi: remove unneeded DSI device check
      drm/mipi-dsi: move format define above
      drm/mipi-dsi: add .attach_new to mipi_dsi_host_ops
      drm: adp: mipi: convert to the .attach_new op
      drm/kmb: dsi: convert to the .attach_new op
      drm/i915/dsi: convert to the .attach_new op
      drm/hisilicon/kirin: convert to the .attach_new op
      drm/bridge: synopsys/dsi2: convert to the .attach_new op
      drm/msm/dsi: convert to the .attach_new op
      drm/rcar-du: dsi: convert to the .attach_new op
      drm: renesas: rz-du: rzg2l_mipi_dsi: convert to the .attach_new op
      drm/vc4: dsi: convert to the .attach_new op
      drm/mediatek: dsi: convert to the .attach_new op
      drm/bridge: nwl-dsi: convert to the .attach_new op
      drm/bridge: cdns-dsi: convert to the .attach_new op
      drm/bridge: tc358768: convert to the .attach_new op
      drm/sprd: convert to the .attach_new op
      drm/bridge: synopsys: dw-mipi-dsi: convert to the .attach_new op
      drm/mcde: store a pointer to mipi_dsi_host to perform TE requests
      drm/mcde: use the DSI host pointer in mcde_dsi_irq
      drm/mcde: convert to the .attach_new op

 drivers/gpu/drm/adp/adp-mipi.c                   |   4 +-
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c   |  66 +++++++-------
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h   |   2 +-
 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c      |   4 +-
 drivers/gpu/drm/bridge/nwl-dsi.c                 |  16 ++--
 drivers/gpu/drm/bridge/samsung-dsim.c            |   5 --
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c    |  18 ++--
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi2.c   |  20 ++---
 drivers/gpu/drm/bridge/tc358768.c                |  40 ++++-----
 drivers/gpu/drm/drm_mipi_dsi.c                   |  38 +++++++-
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c     |  12 +--
 drivers/gpu/drm/i915/display/icl_dsi.c           |   4 +-
 drivers/gpu/drm/i915/display/vlv_dsi.c           |   4 +-
 drivers/gpu/drm/kmb/kmb_dsi.c                    |   4 +-
 drivers/gpu/drm/mcde/mcde_display.c              |  18 ++--
 drivers/gpu/drm/mcde/mcde_drm.h                  |   8 +-
 drivers/gpu/drm/mcde/mcde_dsi.c                  |  97 ++++++++++-----------
 drivers/gpu/drm/mediatek/mtk_dsi.c               |  10 +--
 drivers/gpu/drm/meson/meson_dw_mipi_dsi.c        |  21 ++---
 drivers/gpu/drm/msm/dsi/dsi_host.c               |  18 ++--
 drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c  |  10 +--
 drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c   |  18 ++--
 drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c  |   2 +-
 drivers/gpu/drm/rockchip/dw-mipi-dsi2-rockchip.c |   6 +-
 drivers/gpu/drm/sprd/megacores_pll.c             |   2 +-
 drivers/gpu/drm/sprd/sprd_dpu.c                  |   2 +-
 drivers/gpu/drm/sprd/sprd_dsi.c                  |  28 +++---
 drivers/gpu/drm/sprd/sprd_dsi.h                  |   2 +-
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c           |   2 -
 drivers/gpu/drm/vc4/vc4_dsi.c                    |  12 +--
 include/drm/bridge/dw_mipi_dsi.h                 |   3 +-
 include/drm/bridge/dw_mipi_dsi2.h                |   6 +-
 include/drm/drm_mipi_dsi.h                       | 105 ++++++++++++++---------
 33 files changed, 320 insertions(+), 287 deletions(-)
---
base-commit: 1174bf15bd601f17556f721798cd9183e169549a
change-id: 20250625-drm-dsi-host-no-device-ptr-255f8fd00948

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH RFC 12/32] drm/meson: dsi: remove unneeded DSI device check

[Luca Ceresoli]

This check is not really needed: there is no reason the detaching client
can be different from the attached one. Should this happen, that would be a
bug elsewhere.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

**NOTE**: I'm not 100% sure this is correct, but it appears so, and other
drivers have no such check. And anyway it appears that such a check should
belong to the core, not to individual drivers. Comments welcome!

To: Kevin Hilman <khilman@baylibre.com>
To: Jerome Brunet <jbrunet@baylibre.com>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: linux-amlogic@lists.infradead.org
---
 drivers/gpu/drm/meson/meson_dw_mipi_dsi.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c b/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c
index 66c73c512b0e68ff0e9dbbfaba5f8bf2d347e6b1..4dc726cef5455075def7927a469ae23020ebfec7 100644
--- a/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c
+++ b/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c
@@ -252,11 +252,6 @@ static int meson_dw_mipi_dsi_host_detach(void *priv_data,
 {
 	struct meson_dw_mipi_dsi *mipi_dsi = priv_data;
 
-	if (device == mipi_dsi->dsi_device)
-		mipi_dsi->dsi_device = NULL;
-	else
-		return -EINVAL;
-
 	return phy_exit(mipi_dsi->phy);
 }
 

-- 
2.49.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 00/32] drm/mipi-dsi: avoid DSI host drivers to have pointers to DSI devices

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 5009 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 00/32] drm/mipi-dsi: avoid DSI host drivers to have pointers to DSI devices

[Luca Ceresoli]

On Mon, 7 Jul 2025 08:16:49 +0200
Maxime Ripard <mripard@kernel.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:


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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 00/32] drm/mipi-dsi: avoid DSI host drivers to have pointers to DSI devices

[Luca Ceresoli]

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@bootlin.com> wrote:

> On Mon, 7 Jul 2025 08:16:49 +0200
> Maxime Ripard <mripard@kernel.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.

Best regards,
Luca

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 00/32] drm/mipi-dsi: avoid DSI host drivers to have pointers to DSI devices

[Luca Ceresoli]

Hi Maxime,

On Mon, 7 Jul 2025 12:13:19 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

...

> > 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 haven't received any reply to this e-mail. Should this be due to the
fuzzyness of what I wrote, you're perfectly understood. :-)

Let me try to start cleaner, and focus only on the question quoted here
above. It is very relevant to the hotplug work, so I'd like any informed
opinions about it in the first place. Many other things depend on it.

The samsung-dsim driver, which is in the hardware I'm working on, falls
in the A) case, and this is problematic.

> 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.

I managed to try today and test on hardware, and I can confirm that the
samsung-dsim driver can be moved from A) to B). In other words the
drm_bridge_add() call to add the samsung-dsim bridge can be moved from
the mipi_dsi_host_ops.attach op to the probe function, but this
requires an additional change, at least when using the imx8mp LCDIF:

@@ -1645,6 +1645,9 @@ static int samsung_dsim_attach(struct drm_bridge *bridge,
 {
        struct samsung_dsim *dsi = bridge_to_dsi(bridge);
 
+       if (!dsi->out_bridge)
+               return -EPROBE_DEFER;
+
        return drm_bridge_attach(encoder, dsi->out_bridge, bridge,
                                 flags);
 }

Without the above change, the mxsfb driver will hard-fail because
mxsfb_attach_bridge() [0] finds the next bridge (the samsung-dsim
bridge, which is now added earlier, in probe) but cannot attach it to
the encoder chain (the samsung-dsim bridge still hasn't got an
out_bridge).

I have a working draft of the above samsung-dsim changes working with
the hotplug-bridge, and it makes the hotplug code one relevant step
simpler.

Your opinion would be appreciated before I proceed to cleaning up and
sending such change.

[0] https://elixir.bootlin.com/linux/v6.16-rc5/source/drivers/gpu/drm/mxsfb/mxsfb_drv.c#L128-L145

Luca

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 00/32] drm/mipi-dsi: avoid DSI host drivers to have pointers to DSI devices

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 13401 bytes --]

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@bootlin.com> wrote:
> 
> > On Mon, 7 Jul 2025 08:16:49 +0200
> > Maxime Ripard <mripard@kernel.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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 00/32] drm/mipi-dsi: avoid DSI host drivers to have pointers to DSI devices

[Luca Ceresoli]

Hi Maxime,

On Fri, 25 Jul 2025 17:22:47 +0200
Maxime Ripard <mripard@kernel.org> wrote:

...

> > 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.

Thanks for taking time to reply this!

I just sent a patch to do the mentioned change in samsung-dsim [0] to
start discussion on real code. I'd appreciate if you could prioritize
that patch over all other patches I have sent.

[0] https://lore.kernel.org/all/20250725-drm-bridge-samsung-dsim-add-in-probe-v1-1-b23d29c23fbd@bootlin.com/

Best regards,
Luca

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 00/32] drm/mipi-dsi: avoid DSI host drivers to have pointers to DSI devices

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 13518 bytes --]

Hi Luca,

On Mon, Jul 07, 2025 at 11:58:53AM +0200, Luca Ceresoli wrote:
> On Mon, 7 Jul 2025 08:16:49 +0200
> Maxime Ripard <mripard@kernel.org> wrote:
> > 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).

So, I know that you're not going to be super happy about it, but some
parts of mipi_dsi_device shouldn't really belong in mipi_dsi_device at
the moment.

The virtual channel definitely belongs there, but formats for example
aren't really fixed by the hardware itself. If we're looking at bridges
as a whole, it would make much more sense for formats to be exposed
through the bus format API bridges provide.

Lanes are kind of like that too, since they somewhat depend on the rate.

> 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 │
> └─────────┘     └──────────────────┘     └───────────┘

So, in this case, the DSI bridge is both a DSI device on bus A, and a
DSI host on bus B?

Do we have hardware like that?

> 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.

Another way out would be to evaluate a different solution to that
problem that wouldn't involve creating an intermediate bridge.

> 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().

I guess in this case, you mean a DSI host that is a device of a !DSI
bus, right?

> 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.

I guess that's one thing that would be solved by moving the formats out
of mipi_dsi_device. The other bridges compute this at atomic_check time,
so way after the bridge has been attached, and thus this wouldn't be a
worry anymore.

> [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.

Most importantly, looking at a couple of DSI drivers, it doesn't look
like any use the format outside of atomic_pre_enable / atomic_enable, so
if the format was provided in the bridge state, it would work fine.

> 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'm not too worried about pointers. I mean, yes, it's a footgun, yes,
you can create unsafe behaviors with them, but as long as we have some
kind of mechanism to essentially give you a pointer and revoke it, which
attach / detach is, it's fine.

We'll catch the rest during review, just like access to freed pointers,
or pointers stored without taking a reference.

> 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:

That's certainly surprising to me, but I know that the DSI setup can be
a pain. I struggled in the past with the ordering with the component
framework, and I wouldn't be too surprised if those were attempts to
work around some of these issues.

vc4_dsi seems to have a similar construct. Understanding why they both
do something like this, and updating the doc[1] to reflect it would be a
good idea I think, so we can frame the problem properly, and then try to
find a solution for it.

Maxime

1: https://docs.kernel.org/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic