Re: [PATCH 1/3] device: rust: documentation for DeviceContext

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

On Fri Jul 18, 2025 at 2:32 PM CEST, Alexandre Courbot wrote:
> On Fri Jul 18, 2025 at 7:45 AM JST, Danilo Krummrich wrote:
>> Expand the documentation around DeviceContext states and types, in order
>> to provide detailed information about their purpose and relationship
>> with each other.
>>
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>
> Thanks, that's a welcome clarification and I think I finally understand
> the Rust device model after going through this series!
>
> A few minor nits/questions below.
>
>> ---
>>  rust/kernel/device.rs | 63 +++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 52 insertions(+), 11 deletions(-)
>>
>> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
>> index ca82926fd67f..d7ac56628fe5 100644
>> --- a/rust/kernel/device.rs
>> +++ b/rust/kernel/device.rs
>> @@ -311,28 +311,69 @@ unsafe impl Send for Device {}
>>  // synchronization in `struct device`.
>>  unsafe impl Sync for Device {}
>>  
>> -/// Marker trait for the context of a bus specific device.
>> +/// Marker trait for the context or scope of a bus specific device.
>>  ///
>> -/// Some functions of a bus specific device should only be called from a certain context, i.e. bus
>> -/// callbacks, such as `probe()`.
>> +/// [`DeviceContext`] is a marker trait for structures representing the context of a bus specific
>> +/// [`Device`].
>>  ///
>> -/// This is the marker trait for structures representing the context of a bus specific device.
>
> Shall we say `types` instead of `structures`, since these are ZSTs?
> `structures` carries the hint that they will contain data, when they
> don't (but maybe that's only me :)).

I agree, 'types' is better.

>> +/// The specific device context types are: [`CoreInternal`], [`Core`], [`Bound`] and [`Normal`].
>> +///
>> +/// [`DeviceContext`] types are hierarchical, which means that there is a strict hierarchy that
>> +/// defines which [`DeviceContext`] type can be derived from another. For instance, any
>> +/// [`Device<Core>`] can dereference to a [`Device<Bound>`].
>> +///
>> +/// The following enunumeration illustrates the dereference hierarchy of [`DeviceContext`] types.
>> +///
>> +/// - [`CoreInternal`] => [`Core`] => [`Bound`] => [`Normal`]
>> +/// - [`Core`] => [`Bound`] => [`Normal`]
>> +/// - [`Bound`] => [`Normal`]
>> +/// - [`Normal`]
>
> That graph is super helpful. The last 3 lines look redundant though,
> since the graph can be followed from any node.

Yeah, it's indeed unnecessarily redundant.

>> +///
>> +/// Bus devices can automatically implement the dereference hierarchy by using
>> +/// [`impl_device_context_deref`](kernel::impl_device_context_deref).
>>  pub trait DeviceContext: private::Sealed {}
>>  
>> -/// The [`Normal`] context is the context of a bus specific device when it is not an argument of
>> -/// any bus callback.
>> +/// The [`Normal`] context is the default [`DeviceContext`] of any [`Device`].
>> +///
>> +/// The normal context does not indicate any specific scope. Any `Device<Ctx>` is also a valid
>> +/// [`Device<Normal>`]. It is the only [`DeviceContext`] for which it is valid to implement
>> +/// [`AlwaysRefCounted`](kernel::types::AlwaysRefCounted) for.
>>  pub struct Normal;
>
> `Normal` as a name can be interpreted in many different ways, and in the
> case of a device context it is not clear what the "normal" state is. I
> think it would be helpful if we can elaborate a bit more on what this
> implies (i.e. what concretely speaking are the limitations), and if
> possible why this name has been chosen.

It's the context that does not guarantee any specific scope. But that's also
what the documentation says.

I also wouldn't speak of limitations, it's just that it doesn't allow to make
*additional* assumptions compared to other device context types.

Yet, if you have suggestions on what to add specifically, please let me know
(maybe simply my previous sentence?).

Regarding the name, "Normal" seems reasonable for the device context that does
not guarantee any specific scope. We could have also named it just "Default".

I think "Normal" is fine, as in "it's just a normal device reference, no
specific scope guaranteed".