Message-Id: <DBF8TZJ2SFLR.1I57R8BX2A9ZN@kernel.org>
Date: Fri, 18 Jul 2025 16:16:50 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.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>,
 <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

On Fri Jul 18, 2025 at 3:09 PM CEST, Daniel Almeida wrote:
> 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”.

Scope really means scope in the sense of programming languages, which is why I
didn't define it more specifically.

So, a reference to a Device<Bound> (i.e. &Device<Bound>) indicates that the
device is guaranteed to be bound for the scope the reference is valid in.

Please also note that the added documentation on Device already says:

"This structure represents the Rust abstraction for a C `struct device`. A
[`Device`] can either  exist as temporary reference (see also
[`Device::from_raw`]), which is only valid within a certain scope or as
[`ARef<Device>`], owning a dedicated reference count."

I think this should clarify it already, or are you looking for something else?

> 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]:

You can't create or have a Device<Bound>, but a &Device<Bound>, which has a
defined lifetime that can't be extended arbitrarily with safe code.

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

That's not possible. Core is specific as in that it's really meant to be the
context of a device when it appears in a bus callback.

But the Bound context may appear whereever we it can be proven that within a
certain scope (e.g. the IRQ callback) the device is guaranteed to be bound.

So the generalization really is to say "scope".

> 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