From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtpout-03.galae.net (smtpout-03.galae.net [185.246.85.4]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 441C43B0AE8 for ; Thu, 9 Apr 2026 09:56:09 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.246.85.4 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775728574; cv=none; b=Rs7wPv+t99vwkLfbc8328hLkH8bK9b1wrhem7RPAd2DI2TUjqlwM4NRLam9s91r57N41zviRvKC0B2DAd4lhmb5lkTSwZylgXNEtXEWSRIYAm73U9c+kjN85UjDB1P0i+j8n6VUCEjb7wMi2MThZ6w53oivT+chzFQ0Z0/MQl1M= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775728574; c=relaxed/simple; bh=aNo6jVn7+Vka7OMbz7VtJg1v2Zf93r7/GyWTdW6NTXo=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=PeovYcKnN3YO9OxxHi8MIdAn2hUy/CkCZhFfKBOnpceGPuefG+55+ecaPU6zIr+oYEqhBmpN+HadSmcKM63+PMWJhhWCieKIxh0Cte6I5LFlCr69AMhHsRVUlrLVLk4QvwoosHkRNrtrkCZk5r5I9w+tra2yjEANyW0Mfc6l1/s= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com; spf=pass smtp.mailfrom=bootlin.com; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b=TzNNurnS; arc=none smtp.client-ip=185.246.85.4 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=bootlin.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b="TzNNurnS" Received: from smtpout-01.galae.net (smtpout-01.galae.net [212.83.139.233]) by smtpout-03.galae.net (Postfix) with ESMTPS id 1953E4E429A2; Thu, 9 Apr 2026 09:56:02 +0000 (UTC) Received: from mail.galae.net (mail.galae.net [212.83.136.155]) by smtpout-01.galae.net (Postfix) with ESMTPS id E077B5FDEB; Thu, 9 Apr 2026 09:56:01 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id A772A104500DC; Thu, 9 Apr 2026 11:55:55 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim; t=1775728560; h=from:subject:date:message-id:to:cc:mime-version:content-type: content-transfer-encoding:content-language:in-reply-to:references; bh=FpD0twZLmQedKKeyHHoEMyO31Guiy152fgQmnxDwZLY=; b=TzNNurnS0aTVUs8upEFAuALzgLeq4YxSFdxHEECbVXSrCdMnos0hlcwkQ1czYCMoCfThsk 0xjJIp4o0E+lnsAFzSmy+gTkdHcx/WQpiFJV2DkLefi52qg+YqAnY8PAGuqyyXM043txN2 +ID5x0kVbFKvAdQBidDC3U2Q+/Fx1Ca4hMcTJG8YFdqlFVKLRs1i0Ow7i0v2baDhj4RIee 7AnE88wehUWrdDWsBJIKGa1ftrsX/A1+0pZ0Z9cZ7R4piOUcW4E9Jy1mb30w+2QHsr6BAe LfjC3V9V5BRV0FN2ye9pI6joBc/WJ+x07gnDQcKLPQaAa7xwTMYA+j6NeenI9w== Message-ID: Date: Thu, 9 Apr 2026 11:55:52 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 2/7] drm/encoder: drm_encoder_cleanup: lock the encoder chain mutex during removal To: Luca Ceresoli , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , Andrzej Hajda , Neil Armstrong , Robert Foss , Laurent Pinchart , Jonas Karlman , Jernej Skrabec Cc: Hui Pu , Thomas Petazzoni , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Ian Ray References: <20260324-drm-bridge-alloc-encoder-chain-mutex-v5-0-8bf786c5c7e6@bootlin.com> <20260324-drm-bridge-alloc-encoder-chain-mutex-v5-2-8bf786c5c7e6@bootlin.com> Content-Language: en-US From: Louis Chauvet In-Reply-To: <20260324-drm-bridge-alloc-encoder-chain-mutex-v5-2-8bf786c5c7e6@bootlin.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Last-TLS-Session-Version: TLSv1.3 On 3/24/26 9:58 AM, Luca Ceresoli wrote: > drm_encoder_cleanup() modifies the encoder chain by removing bridges via > drm_bridge_detach(). Protect this whole operation by taking the mutex, so > that: > > * any users iterating over the chain will not access it during the change > * other code willing to modify the list (drm_bridge_attach()) will wait > until drm_encoder_cleanup() is done > > Note that the _safe macro in use here is providing a different and > orthogonal kind of protection than the mutex: > > 1. list_for_each_entry_safe() allows removing the current entry from the > list it is iterating on, synchronously; the non-safe version would be > unable to find the next entry after the current entry has been removed > 2. the mutex being added allows to ensure that the list is not used > asynchronously by other code while it is being modified; this prevents > such other concurrent code to derail because it is iterating over an > element while it is removed > > The _safe macro, which works by taking the "next" pointer in addition to > the "current" one, does not even try to provide the protection at item 2 > above. This is visible e.g. when the "next" element is removed by other > concurrent code. This is what would happen without the added mutex: > > 1. start loop: list_for_each_entry_safe(pos, n, ...) sets: > pos = list_first_entry() = (bridge 1) > n = list_next_entry(pos) = (bridge 2) > 2. enter the loop 1st time, do something with *pos (bridge 1) > 3. in the meanwhile bridge 2 is hot-unplugged > -> another thread removes bridge 2 > -> drm_bridge_detach() > -> list_del() sets (bridge 2)->next = LIST_POISON1 > 4. loop iteration 1 finishes, list_for_each_entry_safe() sets: > pos = n (previously set to bridge 2) > n = (bridge 2)->next = LIST_POISON1 > 5. enter the loop 2nd time, do something with *pos (bridge 2) > 6. loop iteration 2 finishes, list_for_each_entry_safe() sets: > pos = n = LIST_POISON1 ==> bug! > > However, simply adding mutex_[un]lock(&encoder->bridge_chain_mutex) > before/after the list_for_each_entry_safe() seems a simple and good > solution, but it is introducing a possible ABBA deadlock (found by > PROVE_LOCKING). The two code paths involved are: > > * drm_encoder_cleanup(): > - takes the bridge_chain_mutex (A) > - calls drm_bridge_detach -> drm_atomic_private_obj_fini -> > DRM_MODESET_LOCK_ALL_BEGIN() which takes all locks in the > acquisition context (B) > * drm_mode_getconnector() (and other code paths): > - calls drm_helper_probe_single_connector_modes() which: > - takes a drm_modeset_lock in the acquisition context (B) > - calls __drm_helper_update_and_validate -> > drm_bridge_chain_mode_valid -> drm_for_each_bridge_in_chain_from() > which takes the bridge_chain_mutex (A) > > To avoid this potential ABBA deadlock, move all list items to a temporary > list while holding the bridge_chain_mutex, then detach all elements from > the temporary list without the mutex. > > Signed-off-by: Luca Ceresoli Reviewed-by: Louis Chauvet > > --- > > Changes in v5: > - Small commit message improvement > > Changes in v3: > - Prevent ABBA deadlock by using a temporary list > - Improve commit message > > Changes in v2: > - Expanded commit messge with rationale, as discussed > --- > drivers/gpu/drm/drm_encoder.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c > index 3261f142baea..0d5dbed06db4 100644 > --- a/drivers/gpu/drm/drm_encoder.c > +++ b/drivers/gpu/drm/drm_encoder.c > @@ -189,14 +189,26 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) > { > struct drm_device *dev = encoder->dev; > struct drm_bridge *bridge, *next; > + LIST_HEAD(tmplist); > > /* Note that the encoder_list is considered to be static; should we > * remove the drm_encoder at runtime we would have to decrement all > * the indices on the drm_encoder after us in the encoder_list. > */ > > - list_for_each_entry_safe(bridge, next, &encoder->bridge_chain, > - chain_node) > + /* > + * 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); > + list_cut_before(&tmplist, &encoder->bridge_chain, &encoder->bridge_chain); > + mutex_unlock(&encoder->bridge_chain_mutex); > + > + list_for_each_entry_safe(bridge, next, &tmplist, chain_node) > drm_bridge_detach(bridge); > > drm_mode_object_unregister(dev, &encoder->base); >