From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-10631.protonmail.ch (mail-10631.protonmail.ch [79.135.106.31]) (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 5184E25FA25; Thu, 13 Mar 2025 10:44:53 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=79.135.106.31 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741862695; cv=none; b=WXIUpNYk0gsmvrZNO3rnUm28x7gajpKQaQUZmJ4YYQ/TLUeWFALeQug9T4h4WcBHydgQ0sX+5mtNc7Au4GDB8lLpOxgJYvXJ3VcUElwlhGp95wDcobnUpQSVEu84kJnYPuaqWg28pchxLaVA9tq58hRQQ9lni4Sl2dBT7WdUlxA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741862695; c=relaxed/simple; bh=buE1atiYPYQ607nxgunbzSf/tfSUHJlGFsw5PTuvC/k=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=QjO6Qssgqg0T4gmEdh8Z7ZeBm9BM8zVlS+PgpNrHVWdKwLGhgbsi4VoR3BvD76/RnHeyuLL/sFJqvCXugHENMxpeSC+MbF8/N1QWqbP0W7aKjHc0tq3akjWTPWD8XiA94aWazXCjqOhsCg6gD9LCq6QThZZWjffWQTiVvbTL6+k= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=proton.me; spf=pass smtp.mailfrom=proton.me; dkim=pass (2048-bit key) header.d=proton.me header.i=@proton.me header.b=B92emiyE; arc=none smtp.client-ip=79.135.106.31 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=proton.me Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=proton.me Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=proton.me header.i=@proton.me header.b="B92emiyE" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=protonmail; t=1741862685; x=1742121885; bh=cEp0z9a1YFho7EAFWzdcM9E6nSuKjT0pUbaWukOFjiE=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post; b=B92emiyE4n0MDgfkevUR+bMzJG4iQTRtouqNmwaNhuRkELHDxE2SoIBB2mhg+2qAv OVgWcCVhHTP3dns3k5g5CoNhUpmhcP6YD2F+WUYRN21ft52tsrehwEqAg//x8ShXgO yf4MmXVrNqkQmjekJIpjkUlhf68ejUEccqc+aEP9Vq9PiZufTwnCC+82dwQvcJQwa2 DikqHHoRbk5pHfG+MAUiQgp0a74WucxZZvewBQa1mhLMapVUwn2yKePeT1jXLULumO yqP8BvVH/GZlzPtmpby4qGS5fVBOvIu046BVv6kLDaWiUFOGcjpU5nCvT8S1hpv3Z6 kr3cTSHrMg0Lw== Date: Thu, 13 Mar 2025 10:44:38 +0000 To: Danilo Krummrich , gregkh@linuxfoundation.org, rafael@kernel.org, bhelgaas@google.com, ojeda@kernel.org, alex.gaynor@gmail.com, boqun.feng@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com, a.hindborg@kernel.org, aliceryhl@google.com, tmgross@umich.edu From: Benno Lossin Cc: linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, rust-for-linux@vger.kernel.org Subject: Re: [PATCH 3/4] rust: pci: fix unrestricted &mut pci::Device Message-ID: In-Reply-To: <20250313021550.133041-4-dakr@kernel.org> References: <20250313021550.133041-1-dakr@kernel.org> <20250313021550.133041-4-dakr@kernel.org> Feedback-ID: 71780778:user:proton X-Pm-Message-ID: 89a882be85537588231892a321560284113bea8d Precedence: bulk X-Mailing-List: linux-pci@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote: > As by now, pci::Device is implemented as: > > =09#[derive(Clone)] > =09pub struct Device(ARef); > > This may be convenient, but has the implication that drivers can call > device methods that require a mutable reference concurrently at any > point of time. Which methods take mutable references? The `set_master` method you mentioned also took a shared reference before this patch. > Instead define pci::Device as > > =09pub struct Device( > =09=09Opaque, > =09=09PhantomData, > =09); > > and manually implement the AlwaysRefCounted trait. > > With this we can implement methods that should only be called from > bus callbacks (such as probe()) for pci::Device. Consequently, we > make this type accessible in bus callbacks only. > > Arbitrary references taken by the driver are still of type > ARef and hence don't provide access to methods that are > reserved for bus callbacks. > > Fixes: 1bd8b6b2c5d3 ("rust: pci: add basic PCI device / driver abstractio= ns") > Signed-off-by: Danilo Krummrich Two small nits below, but it already looks good: Reviewed-by: Benno Lossin > --- > drivers/gpu/nova-core/driver.rs | 4 +- > rust/kernel/pci.rs | 126 ++++++++++++++++++++------------ > samples/rust/rust_driver_pci.rs | 8 +- > 3 files changed, 85 insertions(+), 53 deletions(-) > > @@ -351,20 +361,8 @@ fn deref(&self) -> &Self::Target { > } > =20 > impl Device { One alternative to implementing `Deref` below would be to change this to `impl Device`. But then one would lose the ability to just do `&pdev` to get a `Device` from a `Device`... So I think the deref way is better. Just wanted to mention this in case someone re-uses this pattern. > - /// Create a PCI Device instance from an existing `device::Device`. > - /// > - /// # Safety > - /// > - /// `dev` must be an `ARef` whose underlying `bindin= gs::device` is a member of > - /// a `bindings::pci_dev`. > - pub unsafe fn from_dev(dev: ARef) -> Self { > - Self(dev) > - } > - > fn as_raw(&self) -> *mut bindings::pci_dev { > - // SAFETY: By the type invariant `self.0.as_raw` is a pointer to= the `struct device` > - // embedded in `struct pci_dev`. > - unsafe { container_of!(self.0.as_raw(), bindings::pci_dev, dev) = as _ } > + self.0.get() > } > =20 > /// Returns the PCI vendor ID. > impl AsRef for Device { > fn as_ref(&self) -> &device::Device { > - &self.0 > + // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a= pointer to a valid > + // `struct pci_dev`. > + let dev =3D unsafe { addr_of_mut!((*self.as_raw()).dev) }; > + > + // SAFETY: `dev` points to a valid `struct device`. > + unsafe { device::Device::as_ref(dev) } Why not use `&**self` instead (ie go through the `Deref` impl)? > @@ -77,7 +77,7 @@ fn probe(pdev: &mut pci::Device, info: &Self::IdInfo) -= > Result>> > =20 > let drvdata =3D KBox::new( > Self { > - pdev: pdev.clone(), > + pdev: (&**pdev).into(), It might be possible to do: impl From<&pci::Device> for ARef { ... } Then this line could become `pdev: pdev.into()`. --- Cheers, Benno > bar, > }, > GFP_KERNEL,