Message-Id: <664AA6FF-4EFD-49FD-91A6-4D66B8614529@collabora.com>
Date: Fri, 18 Jul 2025 10:09:43 -0300
From: Daniel Almeida <daniel.almeida@...labora.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,
 aliceryhl@...gle.com,
 tmgross@...ch.edu,
 rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] device: rust: documentation for DeviceContext

Hi Danilo,

> On 17 Jul 2025, at 19:45, Danilo Krummrich <dakr@...nel.org> 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@...nel.org>
> ---
> 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.
> +/// 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`]
> +///
> +/// Bus devices can automatically implement the dereference hierarchy by using
> +/// [`impl_device_context_deref`](kernel::impl_device_context_deref).
> pub trait DeviceContext: private::Sealed {}

Overall this looks good to me. I think that one point you could perhaps
consider is that, to me at least, it wasn't clear that the contexts were only
valid for a given scope. Or what was precisely meant by “scope”.


I.e.: I thought that once you saw Device<Bound>, for example, that would be
valid indefinitely. If we retrieve one of our past conversations at [0]:

> 
> > Fine, but can’t you get a &Device<Bound> from a ARef<drm::Device>, for example?
> > Perhaps a nicer solution would be to offer this capability instead?
> 
> I think you're confusing quite some things here.
> 
>   [...]
> 
>   (2) Owning a reference count of a device (i.e. ARef<Device>) does *not*
>       guarantee that the device is bound. You can own a reference count to the
>       device object way beyond it being bound. Instead, the guarantee comes from
>       the scope.
> 
>       In this case, the scope is the IRQ callback, since the irq::Registration
>       guarantees to call and complete free_irq() before the underlying bus
>       device is unbound.


I see that you mention the word "scope" a few times, but perhaps it would be
more instructional if you say a few more things on this topic.

For example, when you mention probe(), it would be useful to emphasize that the
Core state would only be guaranteed for the _scope of that function_, and that
it wouldn't mean that "the state Core is active from now on", or "I can assume
that we have a Device<Core> from now on in other parts of the driver".

Kind of like you do here:

> +/// The core context indicates that the [`Device<Core>`] reference's scope is limited to the bus
> +/// callback it appears in.

But generalizing to all states if possible.

The difference is very subtle so this can sound a bit confusing. Let me know if
you want me to clarify this further.


[0] https://lore.kernel.org/rust-for-linux/DBB0NXU86D6G.2M3WZMS2NUV10@kernel.org/

— Daniel

Powered by blists - more mailing lists