* [PATCH 0/2] drm/atomic: protect bridge private_obj during bridge removal
@ 2025-10-13 16:24 Luca Ceresoli
2025-10-13 16:24 ` [PATCH 1/2] drm/atomic: pass drm_device pointer to drm_atomic_private_obj_fini() Luca Ceresoli
2025-10-13 16:24 ` [PATCH 2/2] drm_atomic_private_obj_fini: protect private_obj removal from list Luca Ceresoli
0 siblings, 2 replies; 5+ messages in thread
From: Luca Ceresoli @ 2025-10-13 16:24 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, David Airlie, Simona Vetter, Liviu Dudau,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Paul Cercueil, Rob Clark,
Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang, Sean Paul,
Marijn Suijten, Tomi Valkeinen, Thierry Reding, Mikko Perttunen,
Jonathan Hunter, Dave Stevenson, Maíra Canal,
Raspberry Pi Kernel Maintenance
Cc: Hui Pu, Thomas Petazzoni, amd-gfx, dri-devel, linux-kernel,
linux-mips, linux-arm-msm, freedreno, linux-tegra, Luca Ceresoli
This series avoids a race between DRM bridge removal and usage of the
bridge private_obj during DRM_MODESET_LOCK_ALL_BEGIN/END() and other
locking operations.
This is part of the work towards removal of bridges from a still existing
DRM pipeline without use-after-free. The grand plan was discussed in [0].
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 (v6.16)
B. ✔ convert all bridge drivers to new API (v6.17)
C. ✔ kunit tests (v6.17)
D. ✔ add get/put to drm_bridge_add/remove() + attach/detach()
and warn on old allocation pattern (v6.17)
E. … add get/put on drm_bridge accessors
1. ✔ drm_bridge_chain_get_first_bridge(), add cleanup action (v6.18)
2. ✔ drm_bridge_get_prev_bridge() (v6.18)
3. ✔ drm_bridge_get_next_bridge() (v6.19)
4. ✔ drm_for_each_bridge_in_chain() (v6.19)
5. ✔ drm_bridge_connector_init (v6.19)
6. … protect encoder bridge chain with a mutex
7. of_drm_find_bridge
8. drm_of_find_panel_or_bridge, *_of_get_bridge
9. … enforce drm_bridge_add before drm_bridge_attach
F. ✔ debugfs improvements
1. ✔ add top-level 'bridges' file (v6.16)
2. ✔ show refcount and list lingering bridges (v6.19)
2. ➜ handle gracefully atomic updates during bridge removal
A. … Add drm_dev_enter/exit() to protect device resources
B. ➜ protect private_obj removal from list
3. … DSI host-device driver interaction
4. ✔ removing the need for the "always-disconnected" connector
5. finish the hotplug bridge work, 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://lore.kernel.org/lkml/20250206-hotplug-drm-bridge-v6-0-9d6f2c9c3058@bootlin.com/#t
The need for this series emerged during testing of DRM bridge
hot-plugging. Very rarely on hot-unplug the following warning has appeared:
WARNING: CPU: 0 PID: 123 at include/drm/drm_modeset_lock.h:114 drm_atomic_private_obj_fini+0x64/0x80
...
Call trace:
drm_atomic_private_obj_fini+0x64/0x80
drm_bridge_detach+0x38/0x98
The actual change is in patch 2 along with a detailed explanation.
Patch 1 is just a preparation step.
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (2):
drm/atomic: pass drm_device pointer to drm_atomic_private_obj_fini()
drm_atomic_private_obj_fini: protect private_obj removal from list
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c | 2 +-
drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 +-
drivers/gpu/drm/display/drm_dp_tunnel.c | 2 +-
drivers/gpu/drm/drm_atomic.c | 9 ++++++++-
drivers/gpu/drm/drm_bridge.c | 2 +-
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 +-
drivers/gpu/drm/ingenic/ingenic-ipu.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +-
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 2 +-
drivers/gpu/drm/omapdrm/omap_drv.c | 2 +-
drivers/gpu/drm/tegra/hub.c | 2 +-
drivers/gpu/drm/vc4/vc4_kms.c | 6 +++---
include/drm/drm_atomic.h | 3 ++-
14 files changed, 24 insertions(+), 16 deletions(-)
---
base-commit: 3b80ba4fb2d81c77cfef535b202162cbb8aa1f6e
change-id: 20251013-drm-bridge-atomic-vs-remove-private_obj-d792805bebdc
Best regards,
--
Luca Ceresoli <luca.ceresoli@bootlin.com>
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 1/2] drm/atomic: pass drm_device pointer to drm_atomic_private_obj_fini() 2025-10-13 16:24 [PATCH 0/2] drm/atomic: protect bridge private_obj during bridge removal Luca Ceresoli @ 2025-10-13 16:24 ` Luca Ceresoli 2025-10-14 7:44 ` Maxime Ripard 2025-10-13 16:24 ` [PATCH 2/2] drm_atomic_private_obj_fini: protect private_obj removal from list Luca Ceresoli 1 sibling, 1 reply; 5+ messages in thread From: Luca Ceresoli @ 2025-10-13 16:24 UTC (permalink / raw) To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher, Christian König, David Airlie, Simona Vetter, Liviu Dudau, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Paul Cercueil, Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang, Sean Paul, Marijn Suijten, Tomi Valkeinen, Thierry Reding, Mikko Perttunen, Jonathan Hunter, Dave Stevenson, Maíra Canal, Raspberry Pi Kernel Maintenance Cc: Hui Pu, Thomas Petazzoni, amd-gfx, dri-devel, linux-kernel, linux-mips, linux-arm-msm, freedreno, linux-tegra, Luca Ceresoli In preparation for creating a drm_modeset_acquire_ctx for locking insode drm_atomic_private_obj_fini(), pass a pointer to the struct drm_device which is needed to create such context. Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c | 2 +- drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 +- drivers/gpu/drm/display/drm_dp_tunnel.c | 2 +- drivers/gpu/drm/drm_atomic.c | 3 ++- drivers/gpu/drm/drm_bridge.c | 2 +- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 +- drivers/gpu/drm/ingenic/ingenic-ipu.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 2 +- drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- drivers/gpu/drm/tegra/hub.c | 2 +- drivers/gpu/drm/vc4/vc4_kms.c | 6 +++--- include/drm/drm_atomic.h | 3 ++- 14 files changed, 18 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index fe41494635c54ee58354b51b53449835a7f47328..e5391dc5a69c7266e6902bd6ea01a229b002ffc6 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5491,7 +5491,7 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev) static void amdgpu_dm_destroy_drm_device(struct amdgpu_display_manager *dm) { if (dm->atomic_obj.state) - drm_atomic_private_obj_fini(&dm->atomic_obj); + drm_atomic_private_obj_fini(dm->ddev, &dm->atomic_obj); } /****************************************************************************** diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c b/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c index 914400c4af73824e52dda76425a73a74e681a146..08b1f35727ded45b7b00c727199f0f952f9c5108 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c @@ -428,5 +428,5 @@ void komeda_kms_cleanup_private_objs(struct komeda_kms_dev *kms) struct drm_private_obj *obj, *next; list_for_each_entry_safe(obj, next, &config->privobj_list, head) - drm_atomic_private_obj_fini(obj); + drm_atomic_private_obj_fini(&kms->base, obj); } diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c index 64e5c176d5cce9df9314f77a0b4c97662c30c070..b1ab7b8e4bf9c9b3fe46ad90558edf48b5521ed2 100644 --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c @@ -5773,9 +5773,9 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr) destroy_workqueue(mgr->delayed_destroy_wq); mgr->delayed_destroy_wq = NULL; } + drm_atomic_private_obj_fini(mgr->dev, &mgr->base); mgr->dev = NULL; mgr->aux = NULL; - drm_atomic_private_obj_fini(&mgr->base); mgr->funcs = NULL; mutex_destroy(&mgr->delayed_destroy_lock); diff --git a/drivers/gpu/drm/display/drm_dp_tunnel.c b/drivers/gpu/drm/display/drm_dp_tunnel.c index 43f13a7c79b931beb230f8afe20afa0ebcf5ed8d..407eda40981c687a0e8bc3a7d61c6fbda0c61100 100644 --- a/drivers/gpu/drm/display/drm_dp_tunnel.c +++ b/drivers/gpu/drm/display/drm_dp_tunnel.c @@ -1601,7 +1601,7 @@ static bool init_group(struct drm_dp_tunnel_mgr *mgr, struct drm_dp_tunnel_group static void cleanup_group(struct drm_dp_tunnel_group *group) { - drm_atomic_private_obj_fini(&group->base); + drm_atomic_private_obj_fini(group->mgr->dev, &group->base); } #ifdef CONFIG_DRM_DISPLAY_DP_TUNNEL_STATE_DEBUG diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index be2cb6e43cb07fbe553d1ab875911253be628d1a..7910dacb269c03a0f3e1785bb864d228a693a1aa 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -807,7 +807,8 @@ EXPORT_SYMBOL(drm_atomic_private_obj_init); * Finalize the private object. */ void -drm_atomic_private_obj_fini(struct drm_private_obj *obj) +drm_atomic_private_obj_fini(struct drm_device *dev, + struct drm_private_obj *obj) { list_del(&obj->head); obj->funcs->atomic_destroy_state(obj, obj->state); diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 53e7ece36dd940aabd1c0880f296fce7224a12ac..0997a17a6793f5e42a488d81d8e57b93c5f425a3 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -512,7 +512,7 @@ void drm_bridge_detach(struct drm_bridge *bridge) return; if (drm_bridge_is_atomic(bridge)) - drm_atomic_private_obj_fini(&bridge->base); + drm_atomic_private_obj_fini(bridge->dev, &bridge->base); if (bridge->funcs->detach) bridge->funcs->detach(bridge); diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index d3213fbf22be14b177fc1b7100c5b721d5f17924..9d7f978eeefd317c07e6b0d328b9f59a0048e073 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -1081,7 +1081,7 @@ static void ingenic_drm_configure_hwdesc_plane(struct ingenic_drm *priv, static void ingenic_drm_atomic_private_obj_fini(struct drm_device *drm, void *private_obj) { - drm_atomic_private_obj_fini(private_obj); + drm_atomic_private_obj_fini(drm, private_obj); } static int ingenic_drm_bind(struct device *dev, bool has_components) diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c index 32638a713241abbd4eaed09f0aaec2b790650cc9..c89dd4ce73e277e382b244e73b4d1474ad544c6b 100644 --- a/drivers/gpu/drm/ingenic/ingenic-ipu.c +++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c @@ -908,7 +908,7 @@ static void ingenic_ipu_unbind(struct device *dev, { struct ingenic_ipu *ipu = dev_get_drvdata(dev); - drm_atomic_private_obj_fini(&ipu->private_obj); + drm_atomic_private_obj_fini(ipu->drm, &ipu->private_obj); clk_unprepare(ipu->clk); } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 4e5a8ecd31f7570beb45fd1629a131e70aaefea8..6f03e2e11d255b734f6212bc57bf2d5660d89269 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -400,7 +400,7 @@ static int dpu_kms_global_obj_init(struct dpu_kms *dpu_kms) static void dpu_kms_global_obj_fini(struct dpu_kms *dpu_kms) { - drm_atomic_private_obj_fini(&dpu_kms->global_state); + drm_atomic_private_obj_fini(dpu_kms->dev, &dpu_kms->global_state); } static int dpu_kms_parse_data_bus_icc_path(struct dpu_kms *dpu_kms) diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c index 61edf6864092664afe474cc8d1fd097ca495ebb8..57ecc2ca8bb9d8e2ce38f3eff4cc67c179c98438 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c @@ -571,7 +571,7 @@ static void mdp5_destroy(struct mdp5_kms *mdp5_kms) if (mdp5_kms->rpm_enabled) pm_runtime_disable(&mdp5_kms->pdev->dev); - drm_atomic_private_obj_fini(&mdp5_kms->glob_state); + drm_atomic_private_obj_fini(mdp5_kms->dev, &mdp5_kms->glob_state); } static int construct_pipes(struct mdp5_kms *mdp5_kms, int cnt, diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 794267f0f007850e43949f93be5c98d0e32a84ea..37b9cf58d4b3776a484d9923c291e49a9d899cf4 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -295,7 +295,7 @@ static int omap_global_obj_init(struct drm_device *dev) static void omap_global_obj_fini(struct omap_drm_private *priv) { - drm_atomic_private_obj_fini(&priv->glob_obj); + drm_atomic_private_obj_fini(priv->ddev, &priv->glob_obj); } static void omap_disconnect_pipelines(struct drm_device *ddev) diff --git a/drivers/gpu/drm/tegra/hub.c b/drivers/gpu/drm/tegra/hub.c index 8f779f23dc0904d38b14d3f3a928a07fc9e601ad..b468be737273b4ba7682e8b4cc5bffd061b4914e 100644 --- a/drivers/gpu/drm/tegra/hub.c +++ b/drivers/gpu/drm/tegra/hub.c @@ -959,7 +959,7 @@ static int tegra_display_hub_exit(struct host1x_client *client) struct drm_device *drm = dev_get_drvdata(client->host); struct tegra_drm *tegra = drm->dev_private; - drm_atomic_private_obj_fini(&tegra->hub->base); + drm_atomic_private_obj_fini(drm, &tegra->hub->base); tegra->hub = NULL; return 0; diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 8f983edb81ff0e3b11bbc8465e69f838050f0d07..2150d4c28cbe4f16ed089a04215bb4b4bfa48985 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -93,7 +93,7 @@ static void vc4_ctm_obj_fini(struct drm_device *dev, void *unused) { struct vc4_dev *vc4 = to_vc4_dev(dev); - drm_atomic_private_obj_fini(&vc4->ctm_manager); + drm_atomic_private_obj_fini(dev, &vc4->ctm_manager); } static int vc4_ctm_obj_init(struct vc4_dev *vc4) @@ -726,7 +726,7 @@ static void vc4_load_tracker_obj_fini(struct drm_device *dev, void *unused) { struct vc4_dev *vc4 = to_vc4_dev(dev); - drm_atomic_private_obj_fini(&vc4->load_tracker); + drm_atomic_private_obj_fini(dev, &vc4->load_tracker); } static int vc4_load_tracker_obj_init(struct vc4_dev *vc4) @@ -809,7 +809,7 @@ static void vc4_hvs_channels_obj_fini(struct drm_device *dev, void *unused) { struct vc4_dev *vc4 = to_vc4_dev(dev); - drm_atomic_private_obj_fini(&vc4->hvs_channels); + drm_atomic_private_obj_fini(dev, &vc4->hvs_channels); } static int vc4_hvs_channels_obj_init(struct vc4_dev *vc4) diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 155e82f87e4d47161475b57fc28762d7ba8fd206..d9a26da24fa89ccb6ab351c9dbeb4978ee117f7d 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -672,7 +672,8 @@ void drm_atomic_private_obj_init(struct drm_device *dev, struct drm_private_obj *obj, struct drm_private_state *state, const struct drm_private_state_funcs *funcs); -void drm_atomic_private_obj_fini(struct drm_private_obj *obj); +void drm_atomic_private_obj_fini(struct drm_device *dev, + struct drm_private_obj *obj); struct drm_private_state * __must_check drm_atomic_get_private_obj_state(struct drm_atomic_state *state, -- 2.51.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] drm/atomic: pass drm_device pointer to drm_atomic_private_obj_fini() 2025-10-13 16:24 ` [PATCH 1/2] drm/atomic: pass drm_device pointer to drm_atomic_private_obj_fini() Luca Ceresoli @ 2025-10-14 7:44 ` Maxime Ripard 2025-10-14 8:04 ` Luca Ceresoli 0 siblings, 1 reply; 5+ messages in thread From: Maxime Ripard @ 2025-10-14 7:44 UTC (permalink / raw) To: Luca Ceresoli Cc: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher, Christian König, David Airlie, Simona Vetter, Liviu Dudau, Maarten Lankhorst, Thomas Zimmermann, Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Paul Cercueil, Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang, Sean Paul, Marijn Suijten, Tomi Valkeinen, Thierry Reding, Mikko Perttunen, Jonathan Hunter, Dave Stevenson, Maíra Canal, Raspberry Pi Kernel Maintenance, Hui Pu, Thomas Petazzoni, amd-gfx, dri-devel, linux-kernel, linux-mips, linux-arm-msm, freedreno, linux-tegra [-- Attachment #1: Type: text/plain, Size: 11160 bytes --] On Mon, Oct 13, 2025 at 06:24:22PM +0200, Luca Ceresoli wrote: > In preparation for creating a drm_modeset_acquire_ctx for locking insode > drm_atomic_private_obj_fini(), pass a pointer to the struct drm_device > which is needed to create such context. > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- > drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c | 2 +- > drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 +- > drivers/gpu/drm/display/drm_dp_tunnel.c | 2 +- > drivers/gpu/drm/drm_atomic.c | 3 ++- > drivers/gpu/drm/drm_bridge.c | 2 +- > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 +- > drivers/gpu/drm/ingenic/ingenic-ipu.c | 2 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 2 +- > drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- > drivers/gpu/drm/tegra/hub.c | 2 +- > drivers/gpu/drm/vc4/vc4_kms.c | 6 +++--- > include/drm/drm_atomic.h | 3 ++- > 14 files changed, 18 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index fe41494635c54ee58354b51b53449835a7f47328..e5391dc5a69c7266e6902bd6ea01a229b002ffc6 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -5491,7 +5491,7 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev) > static void amdgpu_dm_destroy_drm_device(struct amdgpu_display_manager *dm) > { > if (dm->atomic_obj.state) > - drm_atomic_private_obj_fini(&dm->atomic_obj); > + drm_atomic_private_obj_fini(dm->ddev, &dm->atomic_obj); > } > > /****************************************************************************** > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c b/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c > index 914400c4af73824e52dda76425a73a74e681a146..08b1f35727ded45b7b00c727199f0f952f9c5108 100644 > --- a/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c > @@ -428,5 +428,5 @@ void komeda_kms_cleanup_private_objs(struct komeda_kms_dev *kms) > struct drm_private_obj *obj, *next; > > list_for_each_entry_safe(obj, next, &config->privobj_list, head) > - drm_atomic_private_obj_fini(obj); > + drm_atomic_private_obj_fini(&kms->base, obj); > } > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c > index 64e5c176d5cce9df9314f77a0b4c97662c30c070..b1ab7b8e4bf9c9b3fe46ad90558edf48b5521ed2 100644 > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c > @@ -5773,9 +5773,9 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr) > destroy_workqueue(mgr->delayed_destroy_wq); > mgr->delayed_destroy_wq = NULL; > } > + drm_atomic_private_obj_fini(mgr->dev, &mgr->base); > mgr->dev = NULL; > mgr->aux = NULL; > - drm_atomic_private_obj_fini(&mgr->base); > mgr->funcs = NULL; > > mutex_destroy(&mgr->delayed_destroy_lock); > diff --git a/drivers/gpu/drm/display/drm_dp_tunnel.c b/drivers/gpu/drm/display/drm_dp_tunnel.c > index 43f13a7c79b931beb230f8afe20afa0ebcf5ed8d..407eda40981c687a0e8bc3a7d61c6fbda0c61100 100644 > --- a/drivers/gpu/drm/display/drm_dp_tunnel.c > +++ b/drivers/gpu/drm/display/drm_dp_tunnel.c > @@ -1601,7 +1601,7 @@ static bool init_group(struct drm_dp_tunnel_mgr *mgr, struct drm_dp_tunnel_group > > static void cleanup_group(struct drm_dp_tunnel_group *group) > { > - drm_atomic_private_obj_fini(&group->base); > + drm_atomic_private_obj_fini(group->mgr->dev, &group->base); > } > > #ifdef CONFIG_DRM_DISPLAY_DP_TUNNEL_STATE_DEBUG > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index be2cb6e43cb07fbe553d1ab875911253be628d1a..7910dacb269c03a0f3e1785bb864d228a693a1aa 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -807,7 +807,8 @@ EXPORT_SYMBOL(drm_atomic_private_obj_init); > * Finalize the private object. > */ > void > -drm_atomic_private_obj_fini(struct drm_private_obj *obj) > +drm_atomic_private_obj_fini(struct drm_device *dev, > + struct drm_private_obj *obj) > { > list_del(&obj->head); > obj->funcs->atomic_destroy_state(obj, obj->state); > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 53e7ece36dd940aabd1c0880f296fce7224a12ac..0997a17a6793f5e42a488d81d8e57b93c5f425a3 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -512,7 +512,7 @@ void drm_bridge_detach(struct drm_bridge *bridge) > return; > > if (drm_bridge_is_atomic(bridge)) > - drm_atomic_private_obj_fini(&bridge->base); > + drm_atomic_private_obj_fini(bridge->dev, &bridge->base); > > if (bridge->funcs->detach) > bridge->funcs->detach(bridge); > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > index d3213fbf22be14b177fc1b7100c5b721d5f17924..9d7f978eeefd317c07e6b0d328b9f59a0048e073 100644 > --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > @@ -1081,7 +1081,7 @@ static void ingenic_drm_configure_hwdesc_plane(struct ingenic_drm *priv, > > static void ingenic_drm_atomic_private_obj_fini(struct drm_device *drm, void *private_obj) > { > - drm_atomic_private_obj_fini(private_obj); > + drm_atomic_private_obj_fini(drm, private_obj); > } > > static int ingenic_drm_bind(struct device *dev, bool has_components) > diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c > index 32638a713241abbd4eaed09f0aaec2b790650cc9..c89dd4ce73e277e382b244e73b4d1474ad544c6b 100644 > --- a/drivers/gpu/drm/ingenic/ingenic-ipu.c > +++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c > @@ -908,7 +908,7 @@ static void ingenic_ipu_unbind(struct device *dev, > { > struct ingenic_ipu *ipu = dev_get_drvdata(dev); > > - drm_atomic_private_obj_fini(&ipu->private_obj); > + drm_atomic_private_obj_fini(ipu->drm, &ipu->private_obj); > clk_unprepare(ipu->clk); > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index 4e5a8ecd31f7570beb45fd1629a131e70aaefea8..6f03e2e11d255b734f6212bc57bf2d5660d89269 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -400,7 +400,7 @@ static int dpu_kms_global_obj_init(struct dpu_kms *dpu_kms) > > static void dpu_kms_global_obj_fini(struct dpu_kms *dpu_kms) > { > - drm_atomic_private_obj_fini(&dpu_kms->global_state); > + drm_atomic_private_obj_fini(dpu_kms->dev, &dpu_kms->global_state); > } > > static int dpu_kms_parse_data_bus_icc_path(struct dpu_kms *dpu_kms) > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > index 61edf6864092664afe474cc8d1fd097ca495ebb8..57ecc2ca8bb9d8e2ce38f3eff4cc67c179c98438 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > @@ -571,7 +571,7 @@ static void mdp5_destroy(struct mdp5_kms *mdp5_kms) > if (mdp5_kms->rpm_enabled) > pm_runtime_disable(&mdp5_kms->pdev->dev); > > - drm_atomic_private_obj_fini(&mdp5_kms->glob_state); > + drm_atomic_private_obj_fini(mdp5_kms->dev, &mdp5_kms->glob_state); > } > > static int construct_pipes(struct mdp5_kms *mdp5_kms, int cnt, > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c > index 794267f0f007850e43949f93be5c98d0e32a84ea..37b9cf58d4b3776a484d9923c291e49a9d899cf4 100644 > --- a/drivers/gpu/drm/omapdrm/omap_drv.c > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c > @@ -295,7 +295,7 @@ static int omap_global_obj_init(struct drm_device *dev) > > static void omap_global_obj_fini(struct omap_drm_private *priv) > { > - drm_atomic_private_obj_fini(&priv->glob_obj); > + drm_atomic_private_obj_fini(priv->ddev, &priv->glob_obj); > } > > static void omap_disconnect_pipelines(struct drm_device *ddev) > diff --git a/drivers/gpu/drm/tegra/hub.c b/drivers/gpu/drm/tegra/hub.c > index 8f779f23dc0904d38b14d3f3a928a07fc9e601ad..b468be737273b4ba7682e8b4cc5bffd061b4914e 100644 > --- a/drivers/gpu/drm/tegra/hub.c > +++ b/drivers/gpu/drm/tegra/hub.c > @@ -959,7 +959,7 @@ static int tegra_display_hub_exit(struct host1x_client *client) > struct drm_device *drm = dev_get_drvdata(client->host); > struct tegra_drm *tegra = drm->dev_private; > > - drm_atomic_private_obj_fini(&tegra->hub->base); > + drm_atomic_private_obj_fini(drm, &tegra->hub->base); > tegra->hub = NULL; > > return 0; > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index 8f983edb81ff0e3b11bbc8465e69f838050f0d07..2150d4c28cbe4f16ed089a04215bb4b4bfa48985 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -93,7 +93,7 @@ static void vc4_ctm_obj_fini(struct drm_device *dev, void *unused) > { > struct vc4_dev *vc4 = to_vc4_dev(dev); > > - drm_atomic_private_obj_fini(&vc4->ctm_manager); > + drm_atomic_private_obj_fini(dev, &vc4->ctm_manager); > } > > static int vc4_ctm_obj_init(struct vc4_dev *vc4) > @@ -726,7 +726,7 @@ static void vc4_load_tracker_obj_fini(struct drm_device *dev, void *unused) > { > struct vc4_dev *vc4 = to_vc4_dev(dev); > > - drm_atomic_private_obj_fini(&vc4->load_tracker); > + drm_atomic_private_obj_fini(dev, &vc4->load_tracker); > } > > static int vc4_load_tracker_obj_init(struct vc4_dev *vc4) > @@ -809,7 +809,7 @@ static void vc4_hvs_channels_obj_fini(struct drm_device *dev, void *unused) > { > struct vc4_dev *vc4 = to_vc4_dev(dev); > > - drm_atomic_private_obj_fini(&vc4->hvs_channels); > + drm_atomic_private_obj_fini(dev, &vc4->hvs_channels); > } > > static int vc4_hvs_channels_obj_init(struct vc4_dev *vc4) > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index 155e82f87e4d47161475b57fc28762d7ba8fd206..d9a26da24fa89ccb6ab351c9dbeb4978ee117f7d 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -672,7 +672,8 @@ void drm_atomic_private_obj_init(struct drm_device *dev, > struct drm_private_obj *obj, > struct drm_private_state *state, > const struct drm_private_state_funcs *funcs); > -void drm_atomic_private_obj_fini(struct drm_private_obj *obj); > +void drm_atomic_private_obj_fini(struct drm_device *dev, > + struct drm_private_obj *obj); It's redundant with https://lore.kernel.org/dri-devel/20251008-drm-private-obj-reset-v1-1-805ab43ae65a@kernel.org/ Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 273 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] drm/atomic: pass drm_device pointer to drm_atomic_private_obj_fini() 2025-10-14 7:44 ` Maxime Ripard @ 2025-10-14 8:04 ` Luca Ceresoli 0 siblings, 0 replies; 5+ messages in thread From: Luca Ceresoli @ 2025-10-14 8:04 UTC (permalink / raw) To: Maxime Ripard Cc: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher, Christian König, David Airlie, Simona Vetter, Liviu Dudau, Maarten Lankhorst, Thomas Zimmermann, Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Paul Cercueil, Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang, Sean Paul, Marijn Suijten, Tomi Valkeinen, Thierry Reding, Mikko Perttunen, Jonathan Hunter, Dave Stevenson, Maíra Canal, Raspberry Pi Kernel Maintenance, Hui Pu, Thomas Petazzoni, amd-gfx, dri-devel, linux-kernel, linux-mips, linux-arm-msm, freedreno, linux-tegra Hi Maxime, On Tue Oct 14, 2025 at 9:44 AM CEST, Maxime Ripard wrote: >> --- a/include/drm/drm_atomic.h >> +++ b/include/drm/drm_atomic.h >> @@ -672,7 +672,8 @@ void drm_atomic_private_obj_init(struct drm_device *dev, >> struct drm_private_obj *obj, >> struct drm_private_state *state, >> const struct drm_private_state_funcs *funcs); >> -void drm_atomic_private_obj_fini(struct drm_private_obj *obj); >> +void drm_atomic_private_obj_fini(struct drm_device *dev, >> + struct drm_private_obj *obj); > > It's redundant with > > https://lore.kernel.org/dri-devel/20251008-drm-private-obj-reset-v1-1-805ab43ae65a@kernel.org/ What a timing! Thanks for the pointer, I hadn't noticed that series. It looks useful for my work, I'm definitely reviewing and testing it very soon. Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] drm_atomic_private_obj_fini: protect private_obj removal from list 2025-10-13 16:24 [PATCH 0/2] drm/atomic: protect bridge private_obj during bridge removal Luca Ceresoli 2025-10-13 16:24 ` [PATCH 1/2] drm/atomic: pass drm_device pointer to drm_atomic_private_obj_fini() Luca Ceresoli @ 2025-10-13 16:24 ` Luca Ceresoli 1 sibling, 0 replies; 5+ messages in thread From: Luca Ceresoli @ 2025-10-13 16:24 UTC (permalink / raw) To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher, Christian König, David Airlie, Simona Vetter, Liviu Dudau, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Paul Cercueil, Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang, Sean Paul, Marijn Suijten, Tomi Valkeinen, Thierry Reding, Mikko Perttunen, Jonathan Hunter, Dave Stevenson, Maíra Canal, Raspberry Pi Kernel Maintenance Cc: Hui Pu, Thomas Petazzoni, amd-gfx, dri-devel, linux-kernel, linux-mips, linux-arm-msm, freedreno, linux-tegra, Luca Ceresoli Currently drm_bridge_detach() expects that the bridge private_obj is not locked by a drm_modeset_acquire_ctx, and it warns in case that happens: drm_bridge_detach() -> drm_atomic_private_obj_fini() -> list_del(&obj->head) // removes priv_obj from // dev->mode_config.privobj_list -> obj->funcs->atomic_destroy_state() -> drm_modeset_lock_fini(&obj->lock) -> WARN_ON(!list_empty(&lock->head)) // warn if priv_obj->lock // is still in ctx->locked The expectation is not respected when adding bridge hot-plugging. In such case the warning triggers if the bridge is being removed concurrently to an operation that locks the private object using a drm_modeset_acquire_ctx, such as in this execution scenario: CPU0: drm_mode_obj_get_properties_ioctl() // userspace request -> DRM_MODESET_LOCK_ALL_BEGIN() . -> drm_for_each_privobj() // loop on dev->mode_config.privobj_list . - lock the privobj mutex . - add priv_obj->lock to ctx->locked . (list of locks to be released) . . CPU1: . drm_bridge_detach() // bridge hot-unplug . -> WARN triggers! . -> DRM_MODESET_LOCK_ALL_END() -> for each lock in ctx->locked - remove priv_obj->lock from ctx->locked - unlock the privobj mutex Fix this by using DRM_MODESET_LOCK_ALL_BEGIN/END() around the list removal in drm_atomic_private_obj_fini(). This ensures that exactly one of these happens: * the concurrent code (e.g. drm_mode_obj_get_properties_ioctl()) acquires all the locks first, so it can execute fully and release the privobj->lock before drm_atomic_private_obj_fini() calls list_del() and before the WARN_ON() * drm_atomic_private_obj_fini() acquires all the locks first, so it removes its privobj->lock from the dev->mode_config.privobj_list; the concurrent code will run afterwards and not acquire that lock because it is not present anymore Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- drivers/gpu/drm/drm_atomic.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7910dacb269c03a0f3e1785bb864d228a693a1aa..aa13389a8efe06b0f67cdce4699d403906b282be 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -810,7 +810,13 @@ void drm_atomic_private_obj_fini(struct drm_device *dev, struct drm_private_obj *obj) { + struct drm_modeset_acquire_ctx ctx; + int ret = 0; + + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); list_del(&obj->head); + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); + obj->funcs->atomic_destroy_state(obj, obj->state); drm_modeset_lock_fini(&obj->lock); } -- 2.51.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-10-14 8:04 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-13 16:24 [PATCH 0/2] drm/atomic: protect bridge private_obj during bridge removal Luca Ceresoli 2025-10-13 16:24 ` [PATCH 1/2] drm/atomic: pass drm_device pointer to drm_atomic_private_obj_fini() Luca Ceresoli 2025-10-14 7:44 ` Maxime Ripard 2025-10-14 8:04 ` Luca Ceresoli 2025-10-13 16:24 ` [PATCH 2/2] drm_atomic_private_obj_fini: protect private_obj removal from list Luca Ceresoli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).