linux-kernel - Re: [PATCH v4 08/13] rust: pci: add basic PCI device / driver abstractions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAH5fLgg=fvQOVL-FH72BFtv-5r_e35=esNir9itG_29am_5Sng@mail.gmail.com>
Date: Wed, 11 Dec 2024 14:06:50 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com, 
	ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com, 
	gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me, 
	tmgross@...ch.edu, a.hindborg@...sung.com, airlied@...il.com, 
	fujita.tomonori@...il.com, lina@...hilina.net, pstanner@...hat.com, 
	ajanulgu@...hat.com, lyude@...hat.com, robh@...nel.org, 
	daniel.almeida@...labora.com, saravanak@...gle.com, dirk.behme@...bosch.com, 
	j@...nau.net, fabien.parent@...aro.org, chrisi.schrefl@...il.com, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v4 08/13] rust: pci: add basic PCI device / driver abstractions

On Tue, Dec 10, 2024 at 11:38 PM Danilo Krummrich <dakr@...nel.org> wrote:
>
> On Tue, Dec 10, 2024 at 11:55:33AM +0100, Alice Ryhl wrote:
> > On Mon, Dec 9, 2024 at 11:44 AM Danilo Krummrich <dakr@...nel.org> wrote:
> > >
> > > On Fri, Dec 06, 2024 at 03:01:18PM +0100, Alice Ryhl wrote:
> > > > On Thu, Dec 5, 2024 at 3:16 PM Danilo Krummrich <dakr@...nel.org> wrote:
> > > > >
> > > > > Implement the basic PCI abstractions required to write a basic PCI
> > > > > driver. This includes the following data structures:
> > > > >
> > > > > The `pci::Driver` trait represents the interface to the driver and
> > > > > provides `pci::Driver::probe` for the driver to implement.
> > > > >
> > > > > The `pci::Device` abstraction represents a `struct pci_dev` and provides
> > > > > abstractions for common functions, such as `pci::Device::set_master`.
> > > > >
> > > > > In order to provide the PCI specific parts to a generic
> > > > > `driver::Registration` the `driver::RegistrationOps` trait is implemented
> > > > > by `pci::Adapter`.
> > > > >
> > > > > `pci::DeviceId` implements PCI device IDs based on the generic
> > > > > `device_id::RawDevceId` abstraction.
> > > > >
> > > > > Co-developed-by: FUJITA Tomonori <fujita.tomonori@...il.com>
> > > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
> > > > > Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> > > >
> > > > > +/// The PCI device representation.
> > > > > +///
> > > > > +/// A PCI device is based on an always reference counted `device:Device` instance. Cloning a PCI
> > > > > +/// device, hence, also increments the base device' reference count.
> > > > > +#[derive(Clone)]
> > > > > +pub struct Device(ARef<device::Device>);
> > > >
> > > > It seems more natural for this to be a wrapper around
> > > > `Opaque<bindings::pci_dev>`. Then you can have both &Device and
> > > > ARef<Device> depending on whether you want to hold a refcount or not.
> > >
> > > Yeah, but then every bus device has to re-implement the refcount dance we
> > > already have in `device::Device` for the underlying base `struct device`.
> > >
> > > I forgot to mention this in my previous reply to Boqun, but we even documented
> > > it this way in `device::Device` [1].
> > >
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/device.rs#n28
> >
> > We could perhaps write a derive macro for AlwaysRefCounted that
> > delegates to the inner type? That way, we can have the best of both
> > worlds.
>
> Sounds interesting, how exactly would this work?
>
> (I'll already send out a v5, but let's keep discussing this.)

Well, the derive macro could assume that the refcount is manipulated
in the same way as the inner type does it. I admit that the idea is
not fully formed, but if we can avoid wrapping ARef, that would be
ideal. It sounds like the only reason you don't do that is that it's
more unsafe, which the macro could reduce.


Alice