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-9d6f2c9c3...@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
signature.asc
Description: PGP signature
