From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 5DF79664C6; Tue, 3 Jun 2025 10:09:05 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1748945345; cv=none; b=pToPqpMKiC2M/ujCIWFscoev+iYUSbKn8dIs7nx5IlfTcs7OTDVInnD9HcPLmjo57BBTdchf9XlfjdC0Q1eY/vEgwyUTDnWX3WmxVcB2Wwqtf0vbtUnxunhn7+bvPovVxDmMCko/wSdT+EkKamUaVDrxgMgAm7zQQNyErOsrs7A= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1748945345; c=relaxed/simple; bh=UwI4XUZPbK9PisF4gurC498orwO4ATvOHiOSueoRh7M=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=kjOaVmcBKV4c2p9LTBVxZSUWFrF/K0PXjYFiHVm9EcTdXXnq4IflnsQb38w36uVJyO2eh9GOhY49o1s8rLCIRK9m5KB5ZECVQBWppiePnikU3w2CP/5crfaNGm6V2O1ZOa/jAQOwck0YiL7mK7nUSss8BysZwWCGoIpaAgR2cHE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=LPqQCrVB; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="LPqQCrVB" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2E772C4CEED; Tue, 3 Jun 2025 10:09:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1748945345; bh=UwI4XUZPbK9PisF4gurC498orwO4ATvOHiOSueoRh7M=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=LPqQCrVBVIOulCDtCmHVq5P7G3rxEeCe06XDoOMpLf8Cy50ZjUDFVB2ouI8NHaU3b xKS2p8GGzhqUx7PO3NFHyclntCfVc/dMBzG+eeGsLm4FUIlgrMLr3q5c3Ee+3imp8p epgPMz5KCJjt9Dl96QlXbvyrRH4qfaSY5mCpVVFX+3/+6PAK2hgAFiv8RoLh/kC06f 7XmEthyo3Z12D5FEMcEoyyTGLS+Lx/9obRo+U/rk7YhBI8+AjrRt+CD9EaxRG4GV7D JF5UzdmfFj8IDY2EuVPBa58D1r/IFKUyq+NhtPkUVSacqPl8jJYLJY603Ru3w+TZRd j18VRQEV76Tsg== Date: Tue, 3 Jun 2025 12:08:59 +0200 From: Danilo Krummrich To: Alice Ryhl Cc: Daniel Almeida , Miguel Ojeda , Alex Gaynor , Boqun Feng , Gary Guo , =?iso-8859-1?Q?Bj=F6rn?= Roy Baron , Benno Lossin , Andreas Hindborg , Trevor Gross , Greg Kroah-Hartman , "Rafael J. Wysocki" , Thomas Gleixner , linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org Subject: Re: [PATCH v3 1/2] rust: irq: add support for request_irq() Message-ID: References: Precedence: bulk X-Mailing-List: rust-for-linux@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: On Tue, Jun 03, 2025 at 11:57:22AM +0200, Alice Ryhl wrote: > On Tue, Jun 3, 2025 at 11:43 AM Danilo Krummrich wrote: > > > > On Tue, Jun 03, 2025 at 11:18:40AM +0200, Alice Ryhl wrote: > > > I don't think that helps. If Devres::drop gets to swap is_available > > > before the devm callback performs the swap, then the devm callback is > > > just a no-op and the device still doesn't wait for free_irq() to > > > finish running. > > > > True, this will indeed always be racy. The rule from the C API has always been > > that devm_{remove,release}_action() must not be called if a concurrent unbind > > can't be ruled out. Consequently, the same is true for Revocable::revoke() in > > this case. > > > > I think Devres::drop() shouldn't do anything then and instead we should provide > > Devres::release() and Devres::remove(), which require the &Device > > reference the Devres object was created with, in order to prove that there > > can't be a concurrent unbind, just like Devres::access(). > > What I suggested with the mutex would work if you remove the devm > callback *after* calling free_irq. > > // drop Registration > mutex_lock(); > free_irq(); > mutex_unlock(); > devm_remove_callback(); I think it would need to be if (!devm_remove_callback()) { mutex_lock(); free_irq(); mutex_unlock(); } > // devm callback > mutex_lock(); > free_irq(); > mutex_unlock(); Yes, we could solve this with a lock as well, but it would be an additional lock, just to maintain the current drop() semantics, which I don't see much value in. The common case is that the object wrapped in a Devres is meant to live for the entire duration the device is bound to the driver. > Another simpler option is to just not support unregistering the irq > callback except through devm. Then you don't have a registration at > all. Creating the callback can take an irq number and a ForeignOwnable > to put in the void pointer. The devm callback calls free_irq and drops > the ForeignOwnable. That's basically what Devres::new_foreign_owned() already does.