From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 638231F9ED1 for ; Wed, 8 Jan 2025 12:56:47 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.50 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736341009; cv=none; b=i2t6KmLxGOmI++BcGLRjo++8hF5qAjXjafGyLb0G4hnn6IWdr3yo1peUGaHIong5WSSI6sLJFqsMIy00LnyS6LoqciSn4J5M2P1MOS++pWuhFxM0oeWQFRqg/FFSoouzu2bPvybu4rTeaN5D0KxCbX5Gn0mR5dcPSL5VQ3t2tBE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736341009; c=relaxed/simple; bh=Mt+Y+eAlI83vkVz0/uHM1GOoU0OcM2i3QBoRY9pzfhg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=jyCo6V1sx8jm/upTkkzVQYXnwki9kqGxjYvi3AgMPnCchu+32EAG84SKOiZlEn3C21WTIK/eDRO/HAguFV+bUT8sWxiTsY4ZD1/8iaSXxvhsC7jC30I/DPI8HCrocLLaqOLx4roB5ViHv4BSPvKqLmGIPRNyH0Yr1Q36cn+uJlU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch; spf=none smtp.mailfrom=ffwll.ch; dkim=pass (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b=Klf7ut55; arc=none smtp.client-ip=209.85.128.50 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=ffwll.ch Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="Klf7ut55" Received: by mail-wm1-f50.google.com with SMTP id 5b1f17b1804b1-43618283dedso165790645e9.3 for ; Wed, 08 Jan 2025 04:56:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; t=1736341006; x=1736945806; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:mail-followup-to:message-id:subject:cc:to :from:date:from:to:cc:subject:date:message-id:reply-to; bh=kWo+GlKIigtpa34bkHtmr0MamXMbRLQ/pZ7GQzj+wuI=; b=Klf7ut556MvNBqWRbjyDDKgo/0LoHg7yMyND07WCgI/0qKMQdTs4eLPBcclAoQcmDM MarSHAYVu85iu7xi3340VhNwFiZ3btLWwgAAhu9/+vrYZjix5emaBGawxxDBR/vMBmCf h1YomZSk4m20ZcJxDiS8Z8vQJpNJIpoixuTw4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736341006; x=1736945806; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:mail-followup-to:message-id:subject:cc:to :from:date:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=kWo+GlKIigtpa34bkHtmr0MamXMbRLQ/pZ7GQzj+wuI=; b=pNtlUCdlcmjNISuF21oE014cCK/mQo1+pxkyn2D5ccbofBNfIi1OMNGWNeo9wkNTc/ ZyGOVGCIvZwXairs4ctdBg67FwPN8rRbq1tjiuf1hdUh5exCPJiSvSw8gUpIlo+sc5kE imQEIl2+OxduA6atzikx+CsdDN1QSRz4dD1u2+BHyLECY3/brqk27gJ6P7RmWUZDA4nJ Pxkiz8m9mQI+HPI+gqM13XPsyme5T9KZvX2feSpx0UIMS2r6wrlfN6PNqISCWVr8S8ha UAVbfAp8XbpvmdGD60Fx2u8cPB/w8VwaWTi04uqdWW1dsMT01boZJayyRx+LfCCGhdF6 9TTA== X-Forwarded-Encrypted: i=1; AJvYcCW4Q07bzzllgP4rOkuL9xGmkr2hohfdOydXrgR9TtDn2wxoSil8bhJuK0BE2YAttU/b3xeEzw0iE0IMIXU=@vger.kernel.org X-Gm-Message-State: AOJu0Yw93pY2MMkYOp3BYgJ4Yio5D6UMOWkE66qMzt0EsIY0Ny9j/FnX hEA1XJkXzGDe5HVaKOApt9XTTXNxHMgxbDMvm3IHIvIBur7XLdXjmbyQLdSW7j77rQisBG9ZdQY E X-Gm-Gg: ASbGnct43ADd3fVDmr9l/UQbZlHQw4cYlx+MVCnxU8rSECOyEDsqCO4Vy8uw5eS++T5 b7Fu2Lp2YCgznLC6pgOEp8a1qytDh7exuiJqD4LZdeaItQcpG3a6ewL5AfQzCOuheEzs1zKX1EM u+zMRolWafZk64shnjNQ8e6T9i/njODXJrKMTZk/4q1Go5r5QAlGMUbi3of/sfjgz3ktElXMvOX WbKc8E1eG4e37LWL6GY3eP/lRSf66vkJCcc9u7Yq3QhngDLTmJZVY/f1HV8F18UqX4n X-Google-Smtp-Source: AGHT+IFYUt0wdrIln88qUKqM/5Tl4MtyUA5lucQ+sLPiHIweT2zj1vcz3JMVtOBCX9RnY7ZqRFTTQg== X-Received: by 2002:a05:6000:1a8c:b0:382:4ab4:b428 with SMTP id ffacd0b85a97d-38a872d071cmr2236620f8f.8.1736341005164; Wed, 08 Jan 2025 04:56:45 -0800 (PST) Received: from phenom.ffwll.local ([2a02:168:57f4:0:5485:d4b2:c087:b497]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-38a1c89e150sm52759253f8f.66.2025.01.08.04.56.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 08 Jan 2025 04:56:44 -0800 (PST) Date: Wed, 8 Jan 2025 13:56:42 +0100 From: Simona Vetter To: Danilo Krummrich Cc: Alice Ryhl , gregkh@linuxfoundation.org, rafael@kernel.org, ojeda@kernel.org, alex.gaynor@gmail.com, boqun.feng@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com, benno.lossin@proton.me, a.hindborg@kernel.org, tmgross@umich.edu, linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org Subject: Re: [PATCH 1/2] devres: add devm_remove_action_nowarn() Message-ID: Mail-Followup-To: Danilo Krummrich , Alice Ryhl , gregkh@linuxfoundation.org, rafael@kernel.org, ojeda@kernel.org, alex.gaynor@gmail.com, boqun.feng@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com, benno.lossin@proton.me, a.hindborg@kernel.org, tmgross@umich.edu, linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org References: <20250103164436.96449-1-dakr@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Operating-System: Linux phenom 6.12.3-amd64 On Tue, Jan 07, 2025 at 11:23:23AM +0100, Danilo Krummrich wrote: > On Tue, Jan 07, 2025 at 11:11:20AM +0100, Alice Ryhl wrote: > > On Tue, Jan 7, 2025 at 11:05 AM Danilo Krummrich wrote: > > > > > > On Mon, Jan 06, 2025 at 12:47:52PM +0100, Simona Vetter wrote: > > > > On Fri, Jan 03, 2025 at 05:44:30PM +0100, Danilo Krummrich wrote: > > > > > devm_remove_action() warns if the action to remove does not exist > > > > > (anymore). > > > > > > > > > > The Rust devres abstraction, however, has a use-case to call > > > > > devm_remove_action() at a point where it can't be guaranteed that the > > > > > corresponding action hasn't been released yet. > > > > > > > > > > In particular, an instance of `Devres` may be dropped after the > > > > > action has been released. So far, `Devres` worked around this by > > > > > keeping the inner type alive. > > > > > > > > > > Hence, add devm_remove_action_nowarn(), which returns an error code if > > > > > the action has been removed already. > > > > > > > > > > A subsequent patch uses devm_remove_action_nowarn() to remove the action > > > > > when `Devres` is dropped. > > > > > > > > > > Signed-off-by: Danilo Krummrich > > > > > --- > > > > > drivers/base/devres.c | 17 ++++++++++++----- > > > > > include/linux/device.h | 18 +++++++++++++++++- > > > > > 2 files changed, 29 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > > > > > index 2152eec0c135..d59b8078fc33 100644 > > > > > --- a/drivers/base/devres.c > > > > > +++ b/drivers/base/devres.c > > > > > @@ -750,25 +750,32 @@ int __devm_add_action(struct device *dev, void (*action)(void *), void *data, co > > > > > EXPORT_SYMBOL_GPL(__devm_add_action); > > > > > > > > > > /** > > > > > - * devm_remove_action() - removes previously added custom action > > > > > + * devm_remove_action_nowarn() - removes previously added custom action > > > > > * @dev: Device that owns the action > > > > > * @action: Function implementing the action > > > > > * @data: Pointer to data passed to @action implementation > > > > > * > > > > > * Removes instance of @action previously added by devm_add_action(). > > > > > * Both action and data should match one of the existing entries. > > > > > + * > > > > > + * In contrast to devm_remove_action(), this function does not WARN() if no > > > > > + * entry could have been found. > > > > > > > > I'd put a caution here that most likely, using this is a bad idea. Maybe > > > > something like: > > > > > > > > "This should only be used if the action is contained in an object with > > > > independent lifetime management, like the Devres rust abstraction. > > > > Anywhere is the warning most likely indicates a driver bug." > > > > > > Yes, I thought of something similar too, but wasn't quite sure if it's needed. > > > At least for me, if something has the postfix "nowarn", it already makes me > > > wonder if I should really use it. > > > > > > I'll add a paragraph. > > > > > > > > > > > At least I really can't come up with a reasonable design in a C driver > > > > that would ever need this. > > > > > > I tried, but couldn't either. The only thing I could think of was a revocable > > > thing in C. > > > > Potentially if there are two cleanup paths that could run in parallel, > > they could use this to avoid needing to synchronize which one removes > > it? > > Yeah, I also though if I can make up such a case. But I think the real issue is > that even if we can find one, it's probably an abuse of devres. > > Devres is there to indicate that the driver was unbound from the device, which > causes remove() for the driver to cleanup. So, rather than removing the action > from some async path, we can just wait for remove() to clean up. The only > exception can hence be probe(). Yeah writing a correct ->remove implementation in C is already really hard, even when you're using devres. If you think you can write one that runs concurrently with other stuff in a way you need this _nowarn variant, you're just too dangerous to write driver code in C imo. -Sima -- Simona Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch