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

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: <DBHPYX0Y0NN5.2NMGLAY6PWBQU@kernel.org>
Date: Mon, 21 Jul 2025 14:07:56 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Alice Ryhl" <aliceryhl@...gle.com>
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 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!