Re: [PATCH 2/3] device: rust: expand documentation for Device

["Danilo Krummrich" <dakr@kernel.org>]

On Mon Jul 21, 2025 at 1:26 PM CEST, Alice Ryhl wrote:
> On Fri, Jul 18, 2025 at 12:45:38AM +0200, Danilo Krummrich wrote:
>> +/// 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?

In your example there would be a USB device *and* an input device, where the
former is a bus device and the latter a class device.

Any resources from the "real" device on the bus are on the USB device, not the
input device.

Or in other words, class devices do not own resources of a "real" device and
consequently are never bound to or unbound from a "real" device on the bus.

> 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?

Not really, the generic struct device isn't one or the other until it's used by
an actual higher level bus or class device implementation.

There isn't really a difference between the two for a base device.

Regarding the device context, a base device can inherit a device context from
the higher level bus or class device. In case of a class device, it's just that
there's nothing to inherit other than Normal.

>> +/// # 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.

That's a good suggestion, thanks!