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

[Maxime Ripard <mripard@kernel.org>]

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