Message-Id: <1551F811-1574-4F62-9A18-781A8A2E2D9E@collabora.com>
Date: Sun, 20 Jul 2025 12:45:58 -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,


[…]

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


Maybe this is the piece of information that I missed, i.e.: that scope was
being used in the usual sense in a programming language, and that the term
wasn't being borrowed to describe some other sort of device behavior. This
might look obvious now, but wasn't when I saw it the first time.

Anyway, this is starting to look like I was the only one to get confused, so
scratch what I said :)

Also, looking at this patch again, it looks like you already explain this well
enough, for example in the docs for Device<Core>, where "reference's scope" is
mentioned:

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

[…]

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

I wonder if it would be helpful to specifically state this in the docs somehow?
Perhaps as an extra "Note that ...". It doesn't hurt to repeat ourselves a bit
here, IMHO.

— Daniel

Powered by blists - more mailing lists