Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm: list corruption in privobj_list during dynamic bridge detachment --
commit 77c9ca5601fd825a1740e548bb2319aadc1fb84a Author: Luca Ceresoli <[email protected]> drm/encoder: add drm_encoder_cleanup_from() Supporting hardware whose final part of the DRM pipeline can be physically removed requires the ability to detach all bridges from a given point to the end of the pipeline. Introduce a variant of drm_encoder_cleanup() for this. > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c > index 0d5dbed06db44..40ece477b3025 100644 > --- a/drivers/gpu/drm/drm_encoder.c > +++ b/drivers/gpu/drm/drm_encoder.c [ ... ] > @@ -179,6 +179,44 @@ int drm_encoder_init(struct drm_device *dev, > } > EXPORT_SYMBOL(drm_encoder_init); > > +/** > + * drm_encoder_cleanup_from - remove a given bridge and all the following > + * @encoder: encoder whole list of bridges shall be pruned > + * @bridge: first bridge to remove > + * > + * Removes from an encoder all the bridges starting with a given bridge > + * and until the end of the chain. > + * > + * Does nothing if the bridge is not attached to an encoder chain. > + * > + * This should not be used in "normal" DRM pipelines. It is only useful for > + * devices whose final part of the DRM chain can be physically removed and > + * later reconnected (possibly with different hardware). > + */ > +void drm_encoder_cleanup_from(struct drm_encoder *encoder, struct drm_bridge > *bridge) > +{ > + struct drm_bridge *next; > + LIST_HEAD(tmplist); > + > + /* > + * We need the bridge_chain_mutex to modify the chain, but > + * drm_bridge_detach() will call DRM_MODESET_LOCK_ALL_BEGIN() (in > + * drm_modeset_lock_fini()), resulting in a possible ABBA circular > + * deadlock. Avoid it by first moving all the bridges to a > + * temporary list holding the lock, and then calling > + * drm_bridge_detach() without the lock. > + */ > + mutex_lock(&encoder->bridge_chain_mutex); > + if (!list_empty(&bridge->chain_node)) > + list_for_each_entry_safe_from(bridge, next, > &encoder->bridge_chain, chain_node) > + list_move_tail(&bridge->chain_node, &tmplist); > + mutex_unlock(&encoder->bridge_chain_mutex); > + > + while (!list_empty(&tmplist)) > + drm_bridge_detach(list_first_entry(&tmplist, struct drm_bridge, > chain_node)); [Severity: High] Does this dynamically detach the bridge without acquiring the connection_mutex? If a hardware bridge unplug triggers drm_bridge_remove(), this calls: drm_encoder_cleanup_from() -> drm_bridge_detach() -> drm_atomic_private_obj_fini() In drm_atomic_private_obj_fini(), it executes: list_del(&obj->head); on dev->mode_config.privobj_list without holding connection_mutex or any other list-protecting lock. Can this create a fatal race condition with concurrent lockless readers? Concurrent userspace DRM IOCTLs (like modeset or atomic commits) can invoke drm_modeset_lock_all_ctx(), which iterates over privobj_list locklessly using list_for_each_entry(). The lockless iteration would race with the un-synchronized list_del() triggered here, potentially corrupting the list traversal for the reader and causing a crash. > +} > +EXPORT_SYMBOL(drm_encoder_cleanup_from); > + -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
