diff mbox series

[v4,08/13] rust: pci: add basic PCI device / driver abstractions

Message ID 20241205141533.111830-9-dakr@kernel.org (mailing list archive)
State Superseded
Headers show
Series Device / Driver PCI / Platform Rust abstractions | expand

Commit Message

Danilo Krummrich Dec. 5, 2024, 2:14 p.m. UTC
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@gmail.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 MAINTAINERS                     |   1 +
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/pci.c              |  18 ++
 rust/kernel/lib.rs              |   2 +
 rust/kernel/pci.rs              | 284 ++++++++++++++++++++++++++++++++
 6 files changed, 307 insertions(+)
 create mode 100644 rust/helpers/pci.c
 create mode 100644 rust/kernel/pci.rs

Comments

Alice Ryhl Dec. 6, 2024, 2:01 p.m. UTC | #1
On Thu, Dec 5, 2024 at 3:16 PM Danilo Krummrich <dakr@kernel.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@gmail.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.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.

Alice

> +impl Device {
> +    /// Create a PCI Device instance from an existing `device::Device`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `dev` must be an `ARef<device::Device>` whose underlying `bindings::device` is a member of
> +    /// a `bindings::pci_dev`.
> +    pub unsafe fn from_dev(dev: ARef<device::Device>) -> 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 _ }
> +    }
> +
> +    /// Enable memory resources for this device.
> +    pub fn enable_device_mem(&self) -> Result {
> +        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
> +        let ret = unsafe { bindings::pci_enable_device_mem(self.as_raw()) };
> +        if ret != 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }
> +
> +    /// Enable bus-mastering for this device.
> +    pub fn set_master(&self) {
> +        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
> +        unsafe { bindings::pci_set_master(self.as_raw()) };
> +    }
> +}
> +
> +impl AsRef<device::Device> for Device {
> +    fn as_ref(&self) -> &device::Device {
> +        &self.0
> +    }
> +}
> --
> 2.47.0
>
Alice Ryhl Dec. 6, 2024, 3:25 p.m. UTC | #2
On Thu, Dec 5, 2024 at 3:16 PM Danilo Krummrich <dakr@kernel.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@gmail.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> +impl<T: Driver + 'static> Adapter<T> {
> +    extern "C" fn probe_callback(
> +        pdev: *mut bindings::pci_dev,
> +        id: *const bindings::pci_device_id,
> +    ) -> core::ffi::c_int {
> +        // SAFETY: The PCI bus only ever calls the probe callback with a valid pointer to a
> +        // `struct pci_dev`.
> +        let dev = unsafe { device::Device::get_device(&mut (*pdev).dev) };

It shouldn't be necessary to increment the refcount here.

Also, the mutable reference is illegal because it references a C type
without a Opaque wrapper. Please use the addr_of_mut! macro instead of
a mutable reference.

Alice
Danilo Krummrich Dec. 9, 2024, 10:44 a.m. UTC | #3
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@kernel.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@gmail.com>
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > Signed-off-by: Danilo Krummrich <dakr@kernel.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

> 
> Alice
> 
> > +impl Device {
> > +    /// Create a PCI Device instance from an existing `device::Device`.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// `dev` must be an `ARef<device::Device>` whose underlying `bindings::device` is a member of
> > +    /// a `bindings::pci_dev`.
> > +    pub unsafe fn from_dev(dev: ARef<device::Device>) -> 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 _ }
> > +    }
> > +
> > +    /// Enable memory resources for this device.
> > +    pub fn enable_device_mem(&self) -> Result {
> > +        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
> > +        let ret = unsafe { bindings::pci_enable_device_mem(self.as_raw()) };
> > +        if ret != 0 {
> > +            Err(Error::from_errno(ret))
> > +        } else {
> > +            Ok(())
> > +        }
> > +    }
> > +
> > +    /// Enable bus-mastering for this device.
> > +    pub fn set_master(&self) {
> > +        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
> > +        unsafe { bindings::pci_set_master(self.as_raw()) };
> > +    }
> > +}
> > +
> > +impl AsRef<device::Device> for Device {
> > +    fn as_ref(&self) -> &device::Device {
> > +        &self.0
> > +    }
> > +}
> > --
> > 2.47.0
> >
Alice Ryhl Dec. 10, 2024, 10:55 a.m. UTC | #4
On Mon, Dec 9, 2024 at 11:44 AM Danilo Krummrich <dakr@kernel.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@kernel.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@gmail.com>
> > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > > Signed-off-by: Danilo Krummrich <dakr@kernel.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.

Alice
Danilo Krummrich Dec. 10, 2024, 10:38 p.m. UTC | #5
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@kernel.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@kernel.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@gmail.com>
> > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > > > Signed-off-by: Danilo Krummrich <dakr@kernel.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.)

> 
> Alice
Alice Ryhl Dec. 11, 2024, 1:06 p.m. UTC | #6
On Tue, Dec 10, 2024 at 11:38 PM Danilo Krummrich <dakr@kernel.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@kernel.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@kernel.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@gmail.com>
> > > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.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
Danilo Krummrich Dec. 11, 2024, 2:32 p.m. UTC | #7
On Wed, Dec 11, 2024 at 02:06:50PM +0100, Alice Ryhl wrote:
> On Tue, Dec 10, 2024 at 11:38 PM Danilo Krummrich <dakr@kernel.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@kernel.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@kernel.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@gmail.com>
> > > > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.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.

If we can get this to work, I agree it's a good solution.

What do you think about making this a follow up of this series?

> It sounds like the only reason you don't do that is that it's
> more unsafe, which the macro could reduce.

Exactly, yes.

> 
> 
> Alice
Greg Kroah-Hartman Dec. 11, 2024, 2:41 p.m. UTC | #8
On Wed, Dec 11, 2024 at 03:32:21PM +0100, Danilo Krummrich wrote:
> On Wed, Dec 11, 2024 at 02:06:50PM +0100, Alice Ryhl wrote:
> > On Tue, Dec 10, 2024 at 11:38 PM Danilo Krummrich <dakr@kernel.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@kernel.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@kernel.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@gmail.com>
> > > > > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > > > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.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.
> 
> If we can get this to work, I agree it's a good solution.
> 
> What do you think about making this a follow up of this series?

That's fine, if you remove the patch that adds the module name to the
function as ideally that's what we are trying to remove here :)

thanks,

greg k-h
Greg Kroah-Hartman Dec. 11, 2024, 2:42 p.m. UTC | #9
On Wed, Dec 11, 2024 at 03:41:35PM +0100, Greg KH wrote:
> On Wed, Dec 11, 2024 at 03:32:21PM +0100, Danilo Krummrich wrote:
> > On Wed, Dec 11, 2024 at 02:06:50PM +0100, Alice Ryhl wrote:
> > > On Tue, Dec 10, 2024 at 11:38 PM Danilo Krummrich <dakr@kernel.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@kernel.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@kernel.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@gmail.com>
> > > > > > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > > > > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.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.
> > 
> > If we can get this to work, I agree it's a good solution.
> > 
> > What do you think about making this a follow up of this series?
> 
> That's fine, if you remove the patch that adds the module name to the
> function as ideally that's what we are trying to remove here :)

Argh, wrong thread, nevermind me, I'm fine with this...
Alice Ryhl Dec. 11, 2024, 2:44 p.m. UTC | #10
On Wed, Dec 11, 2024 at 3:32 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed, Dec 11, 2024 at 02:06:50PM +0100, Alice Ryhl wrote:
> > On Tue, Dec 10, 2024 at 11:38 PM Danilo Krummrich <dakr@kernel.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@kernel.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@kernel.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@gmail.com>
> > > > > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > > > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.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.
>
> If we can get this to work, I agree it's a good solution.
>
> What do you think about making this a follow up of this series?

I'm fine with it being a follow-up.

Alice
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index a3535297ee6f..041b0c1b476f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18099,6 +18099,7 @@  F:	include/asm-generic/pci*
 F:	include/linux/of_pci.h
 F:	include/linux/pci*
 F:	include/uapi/linux/pci*
+F:	rust/kernel/pci.rs
 
 PCIE BANDWIDTH CONTROLLER
 M:	Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 5c4dfe22f41a..6d7a68e2ecb7 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -20,6 +20,7 @@ 
 #include <linux/jump_label.h>
 #include <linux/mdio.h>
 #include <linux/miscdevice.h>
+#include <linux/pci.h>
 #include <linux/phy.h>
 #include <linux/pid_namespace.h>
 #include <linux/poll.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index a3b52aa021de..3fda33cd42d4 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -20,6 +20,7 @@ 
 #include "kunit.c"
 #include "mutex.c"
 #include "page.c"
+#include "pci.c"
 #include "pid_namespace.c"
 #include "rbtree.c"
 #include "rcu.c"
diff --git a/rust/helpers/pci.c b/rust/helpers/pci.c
new file mode 100644
index 000000000000..8ba22f911459
--- /dev/null
+++ b/rust/helpers/pci.c
@@ -0,0 +1,18 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/pci.h>
+
+void rust_helper_pci_set_drvdata(struct pci_dev *pdev, void *data)
+{
+	pci_set_drvdata(pdev, data);
+}
+
+void *rust_helper_pci_get_drvdata(struct pci_dev *pdev)
+{
+	return pci_get_drvdata(pdev);
+}
+
+resource_size_t rust_helper_pci_resource_len(struct pci_dev *pdev, int bar)
+{
+	return pci_resource_len(pdev, bar);
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 4b94e6072c63..07770add5ee2 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -82,6 +82,8 @@ 
 pub use bindings;
 pub mod io;
 pub use macros;
+#[cfg(all(CONFIG_PCI, CONFIG_PCI_MSI))]
+pub mod pci;
 pub use uapi;
 
 #[doc(hidden)]
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
new file mode 100644
index 000000000000..748912c1c994
--- /dev/null
+++ b/rust/kernel/pci.rs
@@ -0,0 +1,284 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstractions for the PCI bus.
+//!
+//! C header: [`include/linux/pci.h`](srctree/include/linux/pci.h)
+
+use crate::{
+    bindings, container_of, device,
+    device_id::RawDeviceId,
+    driver,
+    error::{to_result, Result},
+    str::CStr,
+    types::{ARef, ForeignOwnable},
+    ThisModule,
+};
+use core::ops::Deref;
+use kernel::prelude::*;
+
+/// An adapter for the registration of PCI drivers.
+pub struct Adapter<T: Driver>(T);
+
+impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
+    type RegType = bindings::pci_driver;
+
+    fn register(
+        pdrv: &mut Self::RegType,
+        name: &'static CStr,
+        module: &'static ThisModule,
+    ) -> Result {
+        pdrv.name = name.as_char_ptr();
+        pdrv.probe = Some(Self::probe_callback);
+        pdrv.remove = Some(Self::remove_callback);
+        pdrv.id_table = T::ID_TABLE.as_ptr();
+
+        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        to_result(unsafe {
+            bindings::__pci_register_driver(pdrv as _, module.0, name.as_char_ptr())
+        })
+    }
+
+    fn unregister(pdrv: &mut Self::RegType) {
+        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        unsafe { bindings::pci_unregister_driver(pdrv) }
+    }
+}
+
+impl<T: Driver + 'static> Adapter<T> {
+    extern "C" fn probe_callback(
+        pdev: *mut bindings::pci_dev,
+        id: *const bindings::pci_device_id,
+    ) -> core::ffi::c_int {
+        // SAFETY: The PCI bus only ever calls the probe callback with a valid pointer to a
+        // `struct pci_dev`.
+        let dev = unsafe { device::Device::get_device(&mut (*pdev).dev) };
+        // SAFETY: `dev` is guaranteed to be embedded in a valid `struct pci_dev` by the call
+        // above.
+        let mut pdev = unsafe { Device::from_dev(dev) };
+
+        // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct pci_device_id` and
+        // does not add additional invariants, so it's safe to transmute.
+        let id = unsafe { &*id.cast::<DeviceId>() };
+        let info = T::ID_TABLE.info(id.index());
+
+        match T::probe(&mut pdev, id, info) {
+            Ok(data) => {
+                // Let the `struct pci_dev` own a reference of the driver's private data.
+                // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
+                // `struct pci_dev`.
+                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
+            }
+            Err(err) => return Error::to_errno(err),
+        }
+
+        0
+    }
+
+    extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) {
+        // SAFETY: The PCI bus only ever calls the remove callback with a valid pointer to a
+        // `struct pci_dev`.
+        let ptr = unsafe { bindings::pci_get_drvdata(pdev) };
+
+        // SAFETY: `remove_callback` is only ever called after a successful call to
+        // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
+        // `KBox<T>` pointer created through `KBox::into_foreign`.
+        let _ = unsafe { KBox::<T>::from_foreign(ptr) };
+    }
+}
+
+/// Declares a kernel module that exposes a single PCI driver.
+///
+/// # Example
+///
+///```ignore
+/// kernel::module_pci_driver! {
+///     type: MyDriver,
+///     name: "Module name",
+///     author: "Author name",
+///     description: "Description",
+///     license: "GPL v2",
+/// }
+///```
+#[macro_export]
+macro_rules! module_pci_driver {
+($($f:tt)*) => {
+    $crate::module_driver!(<T>, $crate::pci::Adapter<T>, { $($f)* });
+};
+}
+
+/// Abstraction for bindings::pci_device_id.
+#[repr(transparent)]
+#[derive(Clone, Copy)]
+pub struct DeviceId(bindings::pci_device_id);
+
+impl DeviceId {
+    const PCI_ANY_ID: u32 = !0;
+
+    /// PCI_DEVICE macro.
+    pub const fn new(vendor: u32, device: u32) -> Self {
+        Self(bindings::pci_device_id {
+            vendor,
+            device,
+            subvendor: DeviceId::PCI_ANY_ID,
+            subdevice: DeviceId::PCI_ANY_ID,
+            class: 0,
+            class_mask: 0,
+            driver_data: 0,
+            override_only: 0,
+        })
+    }
+
+    /// PCI_DEVICE_CLASS macro.
+    pub const fn with_class(class: u32, class_mask: u32) -> Self {
+        Self(bindings::pci_device_id {
+            vendor: DeviceId::PCI_ANY_ID,
+            device: DeviceId::PCI_ANY_ID,
+            subvendor: DeviceId::PCI_ANY_ID,
+            subdevice: DeviceId::PCI_ANY_ID,
+            class,
+            class_mask,
+            driver_data: 0,
+            override_only: 0,
+        })
+    }
+}
+
+// Allow drivers R/O access to the fields of `pci_device_id`; should we prefer accessor functions
+// to void exposing C structure fields?
+impl Deref for DeviceId {
+    type Target = bindings::pci_device_id;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+// SAFETY:
+// * `DeviceId` is a `#[repr(transparent)` wrapper of `pci_device_id` and does not add
+//   additional invariants, so it's safe to transmute to `RawType`.
+// * `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field.
+unsafe impl RawDeviceId for DeviceId {
+    type RawType = bindings::pci_device_id;
+
+    const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::pci_device_id, driver_data);
+
+    fn index(&self) -> usize {
+        self.driver_data as _
+    }
+}
+
+/// IdTable type for PCI
+pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<DeviceId, T>;
+
+/// Create a PCI `IdTable` with its alias for modpost.
+#[macro_export]
+macro_rules! pci_device_table {
+    ($table_name:ident, $module_table_name:ident, $id_info_type: ty, $table_data: expr) => {
+        const $table_name: $crate::device_id::IdArray<
+            $crate::pci::DeviceId,
+            $id_info_type,
+            { $table_data.len() },
+        > = $crate::device_id::IdArray::new($table_data);
+
+        $crate::module_device_table!("pci", $module_table_name, $table_name);
+    };
+}
+
+/// The PCI driver trait.
+///
+/// # Example
+///
+///```
+/// # use kernel::{bindings, pci};
+///
+/// struct MyDriver;
+///
+/// kernel::pci_device_table!(
+///     PCI_TABLE,
+///     MODULE_PCI_TABLE,
+///     <MyDriver as pci::Driver>::IdInfo,
+///     [
+///         (pci::DeviceId::new(bindings::PCI_VENDOR_ID_REDHAT, bindings::PCI_ANY_ID as u32), ())
+///     ]
+/// );
+///
+/// impl pci::Driver for MyDriver {
+///     type IdInfo = ();
+///     const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
+///
+///     fn probe(
+///         _pdev: &mut pci::Device,
+///         _id: &pci::DeviceId,
+///         _id_info: &Self::IdInfo,
+///     ) -> Result<Pin<KBox<Self>>> {
+///         Err(ENODEV)
+///     }
+/// }
+///```
+/// Drivers must implement this trait in order to get a PCI driver registered. Please refer to the
+/// `Adapter` documentation for an example.
+pub trait Driver {
+    /// The type holding information about each device id supported by the driver.
+    ///
+    /// TODO: Use associated_type_defaults once stabilized:
+    ///
+    /// type IdInfo: 'static = ();
+    type IdInfo: 'static;
+
+    /// The table of device ids supported by the driver.
+    const ID_TABLE: IdTable<Self::IdInfo>;
+
+    /// PCI driver probe.
+    ///
+    /// Called when a new platform device is added or discovered.
+    /// Implementers should attempt to initialize the device here.
+    fn probe(dev: &mut Device, id: &DeviceId, id_info: &Self::IdInfo) -> Result<Pin<KBox<Self>>>;
+}
+
+/// 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>);
+
+impl Device {
+    /// Create a PCI Device instance from an existing `device::Device`.
+    ///
+    /// # Safety
+    ///
+    /// `dev` must be an `ARef<device::Device>` whose underlying `bindings::device` is a member of
+    /// a `bindings::pci_dev`.
+    pub unsafe fn from_dev(dev: ARef<device::Device>) -> 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 _ }
+    }
+
+    /// Enable memory resources for this device.
+    pub fn enable_device_mem(&self) -> Result {
+        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
+        let ret = unsafe { bindings::pci_enable_device_mem(self.as_raw()) };
+        if ret != 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(())
+        }
+    }
+
+    /// Enable bus-mastering for this device.
+    pub fn set_master(&self) {
+        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
+        unsafe { bindings::pci_set_master(self.as_raw()) };
+    }
+}
+
+impl AsRef<device::Device> for Device {
+    fn as_ref(&self) -> &device::Device {
+        &self.0
+    }
+}