Message-ID: <aH4juIVmj8euE1CA@google.com>
Date: Mon, 21 Jul 2025 11:26:23 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, ojeda@...nel.org, 
	alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net, 
	bjorn3_gh@...tonmail.com, lossin@...nel.org, a.hindborg@...nel.org, 
	tmgross@...ch.edu, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] device: rust: expand documentation for Device

On Fri, Jul 18, 2025 at 12:45:38AM +0200, Danilo Krummrich wrote:
> The documentation for the generic Device type is outdated and deserves
> much more detail.
> 
> Hence, expand the documentation and cover topics such as device types,
> device contexts, as well as information on how to use the generic device
> infrastructure to implement bus and class specific device types.
> 
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>

Overall I think this series is pretty great. It also clarifies some
things for me, particularly the difference between bus and class
devices.

> +/// # Device Types
>  ///
> +/// A [`Device`] can represent either a bus device or a class device.
>  ///
> +/// ## Bus Devices
> +///
> +/// A bus device is a [`Device`] that is associated with a physical or virtual bus. Examples of
> +/// buses include PCI, USB, I2C, and SPI. Devices attached to a bus are registered with a specific
> +/// bus type, which facilitates matching devices with appropriate drivers based on IDs or other
> +/// identifying information. Bus devices are visible in sysfs under `/sys/bus/<bus-name>/devices/`.
> +///
> +/// ## Class Devices
> +///
> +/// A class device is a [`Device`] that is associated with a logical category of functionality
> +/// rather than a physical bus. Examples of classes include block devices, network interfaces, sound
> +/// cards, and input devices. Class devices are grouped under a common class and exposed to
> +/// userspace via entries in `/sys/class/<class-name>/`.
> +///
> +/// # Device Context
> +///
> +/// [`Device`] references are generic over a [`DeviceContext`], which represents the type state of
> +/// a [`Device`].
> +///
> +/// As the name indicates, this type state represents the context of the scope the [`Device`]
> +/// reference is valid in. For instance, the [`Bound`] context guarantees that the [`Device`] is
> +/// bound to a driver for the entire duration of the existence of a [`Device<Bound>`] reference.
> +///
> +/// Other [`DeviceContext`] types besides [`Bound`] are [`Normal`], [`Core`] and [`CoreInternal`].
> +///
> +/// Unless selected otherwise [`Device`] defaults to the [`Normal`] [`DeviceContext`], which by
> +/// itself has no additional requirements.
> +///
> +/// It is always up to the caller of [`Device::from_raw`] to select the correct [`DeviceContext`]
> +/// type for the corresponding scope the [`Device`] reference is created in.
> +///
> +/// All [`DeviceContext`] types other than [`Normal`] are intended to be used with
> +/// [bus devices](#bus-devices) only.

This raises a few questions for me.

The first one is "why"? On other series I have been told that interrupts
must be registered and deregistered before the device is unbound. Does
the same not apply to interrupts for an input device such as a USB
keyboard?

The second one is why we use the same `Device` type for both cases?
Would it not make more sense to have a BusDevice and ClassDevice type?

> +/// # Implementing Bus Devices
> +///
> +/// This section provides a guideline to implement bus specific devices, such as
> +/// [`pci::Device`](kernel::pci::Device) or [`platform::Device`](kernel::platform::Device).
> +///
> +/// A bus specific device should be defined as follows.
> +///
> +/// ```ignore
> +/// #[repr(transparent)]
> +/// pub struct Device<Ctx: device::DeviceContext = device::Normal>(
> +///     Opaque<bindings::bus_device_type>,
> +///     PhantomData<Ctx>,
> +/// );
> +/// ```
> +///
> +/// Since devices are reference counted, [`AlwaysRefCounted`](kernel::types::AlwaysRefCounted)
> +/// should be implemented for `Device` (i.e. `Device<Normal>`). Note that
> +/// [`AlwaysRefCounted`](kernel::types::AlwaysRefCounted) must not be implemented for any other
> +/// [`DeviceContext`], since all other device context types are only valid in a certain scope.

As a general comment to all three patches, I would suggest separating
out the link locations.

/// Since devices are reference counted, [`AlwaysRefCounted`] should be
/// implemented for `Device` (i.e. `Device<Normal>`). Note that
/// [`AlwaysRefCounted`] must not be implemented for any other
/// [`DeviceContext`], since all other device context types are only
/// valid in a certain scope.

and then at the end:

/// [`AlwaysRefCounted`]: kernel::types::AlwaysRefCounted

I think it's a lot easier to read the markdown version when links are
separated out like this.

Alice

Powered by blists - more mailing lists