Documentation for Device / Driver infrastructure
Subsystems (2)
Danilo Krummrich
maintainercover Documentation for Device / Driver infrastructure
This patch series expands the documentation for the generic device and driver infrastructure and provides a guideline on how to implement bus and class specific devices as well as bus specific driver infrastructure, i.e. bus abstractions. It also expands on the existing documentation of device context types.
Danilo Krummrich (3):
device: rust: documentation for DeviceContext
device: rust: expand documentation for Device
driver: rust: expand documentation for driver infrastructure
rust/kernel/device.rs | 198 +++++++++++++++++++++++++++++++++++++----- rust/kernel/driver.rs | 82 ++++++++++++++++- 2 files changed, 254 insertions(+), 26 deletions(-)
base-commit: 9a8682f0875b8cedad42bdfe601e6ab204fad06d
Signature · 1 line
2.50.0
Danilo Krummrich
maintainerPATCH 1/3 device: rust: documentation for DeviceContext
Expand the documentation around DeviceContext states and types, in order to provide detailed information about their purpose and relationship with each other.
--- rust/kernel/device.rs | 63 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 52 insertions(+), 11 deletions(-)
rust/kernel/device.rs +52 -11
@@ -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 {} -/// 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 ; -/// The [`Core`] context is the context of a bus specific device when it is supplied as argument of -/// any of the bus callbacks, such as `probe()`. +/// The [`Core`] context is the context of a bus specific device when it appears as argument of +/// any bus specific callback, such as `probe()`. +/// +/// The core context indicates that the [`Device<Core>`] reference's scope is limited to the bus +/// callback it appears in. It is intended to be used for synchronization purposes. Bus device +/// implementations can implement methods for [`Device<Core>`], such that they can only be called +/// from bus callbacks. pub struct Core ; -/// Semantically the same as [`Core`] but reserved for internal usage of the corresponding bus +/// Semantically the same as [`Core`], but reserved for internal usage of the corresponding bus /// abstraction. +/// +/// The internal core context is intended to be used in exactly the same way as the [Core] context, +/// with the difference that this [`DeviceContext`] is internal to the corresponding bus +/// abstraction. +/// +/// This context mainly exists to share generic [`Device`] infrastructure that should only be called +/// from bus callbacks with bus abstractions, but without making them accessible for drivers. pub struct CoreInternal ; -/// The [`Bound`] context is the context of a bus specific device reference when it is guaranteed to -/// be bound for the duration of its lifetime. +/// The [`Bound`] context is the [`DeviceContext`] of a bus specific device when it is guaranteed to +/// be bound to a driver. +/// +/// The bound context indicates that for the entire duration of the lifetime of a [`Device<Bound>`] +/// reference, the [`Device`] is guaranteed to be bound to a driver. +/// +/// Some APIs, such as [`dma::CoherentAllocation`](kernel::dma::CoherentAllocation) or +/// [`Devres`](kernel::devres::Devres) rely on the [`Device`] to be bound, which can be proven with +/// the [`Bound`] device context. +/// +/// Any abstraction that can guarantee a scope where the corresponding bus device is bound, should +/// provide a [`Device<Bound>`] reference to its users for this scope. This allows users to benefit +/// from optimizations for accessing device resources, see also +/// [Devres::access](kernel::devres::Devres::access). pub struct Bound ;mod private {
Signature · 1 line
2.50.0
Alexandre Courbot
reviewerRe: [PATCH 1/3] device: rust: documentation for DeviceContext
Quoted 5 lines · Danilo Krummrich
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 <[email protected]>
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.
Quoted 11 lines
--- rust/kernel/device.rs | 63 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 52 insertions(+), 11 deletions(-)rust/kernel/device.rs +0
-0@@ -311,28 +311,69 @@ unsafe impl Send for Device {}// synchronization in `struct device`. unsafe impl Sync for Device {}
rust/kernel/device.rs unsafe impl Send for Device {} · line 319
Quoted 10 lines
rust/kernel/device.rs +3
-4-/// 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 :)).
rust/kernel/device.rs unsafe impl Send for Device {} · line 330
Quoted 12 lines
rust/kernel/device.rs +12
-0+/// 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.
rust/kernel/device.rs unsafe impl Send for Device {} · line 341
Quoted 13 lines
rust/kernel/device.rs +8
-2+/// +/// 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.
Danilo Krummrich
maintainerRe: [PATCH 1/3] device: rust: documentation for DeviceContext
Quoted 24 lines · Alexandre Courbot
Quoted 5 lines · Danilo Krummrich
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 <[email protected]>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.
Quoted 12 lines
--- rust/kernel/device.rs | 63 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 52 insertions(+), 11 deletions(-)rust/kernel/device.rs +0
-0@@ -311,28 +311,69 @@ unsafe impl Send for Device {}// synchronization in `struct device`. unsafe impl Sync for Device {}
Quoted 13 lines
rust/kernel/device.rs unsafe impl Send for Device {} · line 319
Quoted 9 lines
rust/kernel/device.rs +3
-4-/// 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
typesinstead ofstructures, since these are ZSTs?structurescarries the hint that they will contain data, when they don't (but maybe that's only me :)).
I agree, 'types' is better.
Quoted 7 lines
Quoted 7 lines
rust/kernel/device.rs +7
-0+/// 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.
Quoted 8 lines
rust/kernel/device.rs unsafe impl Send for Device {} · line 330
Quoted 5 lines
rust/kernel/device.rs +5
-0+/// +/// - [`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.
Quoted 5 lines
Quoted 5 lines
rust/kernel/device.rs +3
-0+/// +/// Bus devices can automatically implement the dereference hierarchy by using +/// [`impl_device_context_deref`](kernel::impl_device_context_deref). pub trait DeviceContext : private::Sealed {}
Quoted 14 lines
rust/kernel/device.rs unsafe impl Send for Device {} · line 341
Quoted 8 lines
rust/kernel/device.rs +5
-2-/// 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 ;
Normalas 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".
Benno Lossin
reviewerRe: [PATCH 1/3] device: rust: documentation for DeviceContext
Quoted 28 lines · Danilo Krummrich
Quoted 6 lines · Alexandre Courbot
Quoted 5 lines · Danilo Krummrich
rust/kernel/device.rs +3
-0+/// +/// Bus devices can automatically implement the dereference hierarchy by using +/// [`impl_device_context_deref`](kernel::impl_device_context_deref). pub trait DeviceContext : private::Sealed {}Quoted 14 lines
rust/kernel/device.rs unsafe impl Send for Device {} · line 341
Quoted 8 lines
rust/kernel/device.rs +5
-2-/// 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 ;
Normalas 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.
Quoted 8 lines
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".
Not sure if this helps, but Plain might carry a slightly different meaning from Normal that is better in this case?
(but you probably can't infer much from the name anyways)
---
Cheers,
Benno
Daniel Almeida
reviewerRe: [PATCH 1/3] device: rust: documentation for DeviceContext
In reply to Danilo KrummrichHi Danilo,
Quoted 34 lines · Danilo Krummrich
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 <[email protected]>---
rust/kernel/device.rs| 63 +++++++++++++++++++++++++++++++++++--------1 file changed, 52 insertions(+), 11 deletions(-)rust/kernel/device.rs +9
-4@@ -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>`]. +///
rust/kernel/device.rs unsafe impl Send for Device {} · line 334
Quoted 10 lines
rust/kernel/device.rs +9
-0+/// 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]:
Quoted 8 lines
Quoted 3 lines
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.
[...]
Quoted 8 lines
(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::Registrationguarantees to call and completefree_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:
rust/kernel/device.rs
Quoted 2 lines
rust/kernel/device.rs +2
-0+/// 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/[email protected]/
— Daniel
Danilo Krummrich
maintainerRe: [PATCH 1/3] device: rust: documentation for DeviceContext
Quoted 42 lines · Daniel Almeida
Hi Danilo,
Quoted 40 lines · Danilo Krummrich
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 <[email protected]>---
rust/kernel/device.rs| 63 +++++++++++++++++++++++++++++++++++--------1 file changed, 52 insertions(+), 11 deletions(-)rust/kernel/device.rs +15
-4@@ -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`]
Quoted 8 lines
rust/kernel/device.rs unsafe impl Send for Device {} · line 334
Quoted 4 lines
rust/kernel/device.rs +3
-0+/// +/// 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?
Quoted 2 lines
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.
Quoted 26 lines
Quoted 8 lines
Quoted 3 lines
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.
[...]
Quoted 8 lines
(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::Registrationguarantees to call and completefree_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_scopeof thatfunction_, 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".
Quoted 6 lines
Kind of like you do here:
rust/kernel/device.rs
Quoted 2 lines
rust/kernel/device.rs +2
-0+/// 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".
Quoted 7 lines
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/[email protected]/
— Daniel
Daniel Almeida
reviewerRe: [PATCH 1/3] device: rust: documentation for DeviceContext
Hi Danilo,
[…]
Quoted 32 lines
Quoted 23 lines
Quoted 23 lines
unsafe impl Sync for Device {}
rust/kernel/device.rs +1
-1-/// Marker trait for the context of a bus specific device. +/// Marker trait for the context or scope of a bus specific device. ///
rust/kernel/device.rs +2
-2-/// 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`]. ///
rust/kernel/device.rs +12
-1-/// 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`] Quoted 8 lines
Quoted 4 lines
rust/kernel/device.rs +3
-0+/// +/// 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”.
Quoted 5 lines
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.
[…]
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
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.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
[...]
(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
Danilo Krummrich
maintainerPATCH 2/3 device: rust: expand documentation for Device
In reply to Danilo KrummrichThe documentation for the generic Device type is outdated and deserves much more detail.
Hence, expand the documentation and cover topics such as device types, device contexts, as well as information on how to use the generic device infrastructure to implement bus and class specific device types.
--- rust/kernel/device.rs | 135 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 122 insertions(+), 13 deletions(-)
rust/kernel/device.rs +122 -13
@@ -15,23 +15,132 @@pub mod property; -/// A reference-counted device. +/// The core representation of a device in the kernel's driver model. /// -/// This structure represents the Rust abstraction for a C `struct device`. This implementation -/// abstracts the usage of an already existing C `struct device` within Rust code that we get -/// passed from the C side. +/// 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. /// -/// An instance of this abstraction can be obtained temporarily or permanent. +/// # Device Types /// -/// A temporary one is bound to the lifetime of the C `struct device` pointer used for creation. -/// A permanent instance is always reference-counted and hence not restricted by any lifetime -/// boundaries. +/// A [`Device`] can represent either a bus device or a class device. /// -/// For subsystems it is recommended to create a permanent instance to wrap into a subsystem -/// specific device structure (e.g. `pci::Device`). This is useful for passing it to drivers in -/// `T::probe()`, such that a driver can store the `ARef<Device>` (equivalent to storing a -/// `struct device` pointer in a C driver) for arbitrary purposes, e.g. allocating DMA coherent -/// memory. +/// ## Bus Devices +/// +/// A bus device is a [`Device`] that is associated with a physical or virtual bus. Examples of +/// buses include PCI, USB, I2C, and SPI. Devices attached to a bus are registered with a specific +/// bus type, which facilitates matching devices with appropriate drivers based on IDs or other +/// identifying information. Bus devices are visible in sysfs under `/sys/bus/<bus-name>/devices/`. +/// +/// ## Class Devices +/// +/// A class device is a [`Device`] that is associated with a logical category of functionality +/// rather than a physical bus. Examples of classes include block devices, network interfaces, sound +/// cards, and input devices. Class devices are grouped under a common class and exposed to +/// userspace via entries in `/sys/class/<class-name>/`. +/// +/// # Device Context +/// +/// [`Device`] references are generic over a [`DeviceContext`], which represents the type state of +/// a [`Device`]. +/// +/// As the name indicates, this type state represents the context of the scope the [`Device`] +/// reference is valid in. For instance, the [`Bound`] context guarantees that the [`Device`] is +/// bound to a driver for the entire duration of the existence of a [`Device<Bound>`] reference. +/// +/// Other [`DeviceContext`] types besides [`Bound`] are [`Normal`], [`Core`] and [`CoreInternal`]. +/// +/// Unless selected otherwise [`Device`] defaults to the [`Normal`] [`DeviceContext`], which by +/// itself has no additional requirements. +/// +/// It is always up to the caller of [`Device::from_raw`] to select the correct [`DeviceContext`] +/// type for the corresponding scope the [`Device`] reference is created in. +/// +/// All [`DeviceContext`] types other than [`Normal`] are intended to be used with +/// [bus devices](#bus-devices) only. +/// +/// # 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. +/// +/// In order to be able to implement the [`DeviceContext`] dereference hierarchy, bus device +/// implementations should call the [`impl_device_context_deref`](kernel::impl_device_context_deref) +/// macro as shown below. +/// +/// ```ignore +/// // SAFETY: `Device` is a transparent wrapper of a type that doesn't depend on `Device`'s +/// // generic argument. +/// kernel::impl_device_context_deref!(unsafe { Device }); +/// ``` +/// In order to convert from a any [`Device<Ctx>`] to [`ARef<Device>`], bus devices can implement +/// the following macro call. +/// +/// ```ignore +/// kernel::impl_device_context_into_aref!(Device); +/// ``` +/// Bus devices should also implement the following [`AsRef`] implementation, such that users can +/// easily derive a generic [`Device`] reference. +/// +/// ```ignore +/// impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> { +/// fn as_ref(&self) -> &device::Device<Ctx> { +/// ... +/// } +/// } +/// ``` +/// +/// # Implementing Class Devices +/// +/// Class device implementations require less infrastructure and depend slightly more on the +/// specific subsystem. +/// +/// An example implementation for a class device could look like this. +/// +/// ```ignore +/// #[repr(C)] +/// #[pin_data] +/// pub struct Device<T: class::Driver> { +/// dev: Opaque<bindings::class_device_type>, +/// #[pin] +/// data: T::Data, +/// } +/// ``` +/// +/// This class device uses the sub-classing pattern to embed the driver's private data within the +/// allocation of the class device. For this to be possible the class device is generic over the +/// class specific `Driver` trait implementation. +/// +/// Just like any device, class devices are reference counted and should hence implement +/// [`AlwaysRefCounted`](kernel::types::AlwaysRefCounted) for `Device`. +/// +/// Class devices should also implement the following [`AsRef`] implementation, such that users can +/// easily derive a generic [`Device`] reference. +/// +/// ```ignore +/// impl<T: class::Driver> AsRef<device::Device> for Device<T> { +/// fn as_ref(&self) -> &device::Device { +/// ... +/// } +/// } +/// ``` +/// +/// An example for a class device implementation is [`drm::Device`](kernel::drm::Device). /// /// # Invariants ///
Signature · 1 line
2.50.0
Re: [PATCH 2/3] device: rust: expand documentation for Device
This looks good to me!
Quoted 10 lines · Danilo Krummrich
The documentation for the generic Device type is outdated and deserves much more detail.
Hence, expand the documentation and cover topics such as device types, device contexts, as well as information on how to use the generic device infrastructure to implement bus and class specific device types.
Signed-off-by Danilo Krummrich <[email protected]>
Alice Ryhl
reviewerRe: [PATCH 2/3] device: rust: expand documentation for Device
In reply to Danilo KrummrichQuoted 8 lines · Danilo Krummrich
The documentation for the generic Device type is outdated and deserves much more detail.
Hence, expand the documentation and cover topics such as device types, device contexts, as well as information on how to use the generic device infrastructure to implement bus and class specific device types.
Signed-off-by Danilo Krummrich <[email protected]>
Overall I think this series is pretty great. It also clarifies some things for me, particularly the difference between bus and class devices.
Quoted 27 lines
rust/kernel/device.rs +25
-0+/// # Device Types /// +/// A [`Device`] can represent either a bus device or a class device. /// +/// ## Bus Devices +/// +/// A bus device is a [`Device`] that is associated with a physical or virtual bus. Examples of +/// buses include PCI, USB, I2C, and SPI. Devices attached to a bus are registered with a specific +/// bus type, which facilitates matching devices with appropriate drivers based on IDs or other +/// identifying information. Bus devices are visible in sysfs under `/sys/bus/<bus-name>/devices/`. +/// +/// ## Class Devices +/// +/// A class device is a [`Device`] that is associated with a logical category of functionality +/// rather than a physical bus. Examples of classes include block devices, network interfaces, sound +/// cards, and input devices. Class devices are grouped under a common class and exposed to +/// userspace via entries in `/sys/class/<class-name>/`. +/// +/// # Device Context +/// +/// [`Device`] references are generic over a [`DeviceContext`], which represents the type state of +/// a [`Device`]. +/// +/// As the name indicates, this type state represents the context of the scope the [`Device`] +/// reference is valid in. For instance, the [`Bound`] context guarantees that the [`Device`] is +/// bound to a driver for the entire duration of the existence of a [`Device<Bound>`] reference. +///
rust/kernel/device.rs line 60
Quoted 10 lines
rust/kernel/device.rs +10
-0+/// Other [`DeviceContext`] types besides [`Bound`] are [`Normal`], [`Core`] and [`CoreInternal`]. +/// +/// Unless selected otherwise [`Device`] defaults to the [`Normal`] [`DeviceContext`], which by +/// itself has no additional requirements. +/// +/// It is always up to the caller of [`Device::from_raw`] to select the correct [`DeviceContext`] +/// type for the corresponding scope the [`Device`] reference is created in. +/// +/// 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?
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?
Quoted 9 lines
rust/kernel/device.rs +9
-0+/// # 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)]
rust/kernel/device.rs line 80
Quoted 10 lines
rust/kernel/device.rs +10
-0+/// 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.
Alice
Greg KH
maintainerRe: [PATCH 2/3] device: rust: expand documentation for Device
Quoted 54 lines · Alice Ryhl
Quoted 8 lines · Danilo Krummrich
The documentation for the generic Device type is outdated and deserves much more detail.
Hence, expand the documentation and cover topics such as device types, device contexts, as well as information on how to use the generic device infrastructure to implement bus and class specific device types.
Signed-off-by Danilo Krummrich <[email protected]>Overall I think this series is pretty great. It also clarifies some things for me, particularly the difference between bus and class devices.
Quoted 27 lines
rust/kernel/device.rs +25
-0+/// # Device Types /// +/// A [`Device`] can represent either a bus device or a class device. /// +/// ## Bus Devices +/// +/// A bus device is a [`Device`] that is associated with a physical or virtual bus. Examples of +/// buses include PCI, USB, I2C, and SPI. Devices attached to a bus are registered with a specific +/// bus type, which facilitates matching devices with appropriate drivers based on IDs or other +/// identifying information. Bus devices are visible in sysfs under `/sys/bus/<bus-name>/devices/`. +/// +/// ## Class Devices +/// +/// A class device is a [`Device`] that is associated with a logical category of functionality +/// rather than a physical bus. Examples of classes include block devices, network interfaces, sound +/// cards, and input devices. Class devices are grouped under a common class and exposed to +/// userspace via entries in `/sys/class/<class-name>/`. +/// +/// # Device Context +/// +/// [`Device`] references are generic over a [`DeviceContext`], which represents the type state of +/// a [`Device`]. +/// +/// As the name indicates, this type state represents the context of the scope the [`Device`] +/// reference is valid in. For instance, the [`Bound`] context guarantees that the [`Device`] is +/// bound to a driver for the entire duration of the existence of a [`Device<Bound>`] reference. +/// rust/kernel/device.rs line 60
Quoted 10 lines
rust/kernel/device.rs +10
-0+/// Other [`DeviceContext`] types besides [`Bound`] are [`Normal`], [`Core`] and [`CoreInternal`]. +/// +/// Unless selected otherwise [`Device`] defaults to the [`Normal`] [`DeviceContext`], which by +/// itself has no additional requirements. +/// +/// It is always up to the caller of [`Device::from_raw`] to select the correct [`DeviceContext`] +/// type for the corresponding scope the [`Device`] reference is created in. +/// +/// All [`DeviceContext`] types other than [`Normal`] are intended to be used with +/// [bus devices](#bus-devices) only. This raises a few questions for me.
Quoted 4 lines
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?
USB drivers don't have interrupts :)
Seriously, generally, yes, you should always unregister your irq handler BEFORE you unbind the driver from the device, otherwise the callback logic normally assumes that the driver is bound to the device and the pointers are almost always "stale" at that time.
But I don't understand what interrupts have to do with the above documentation, what's the connection?
Quoted 2 lines
The second one is why we use the same
Devicetype for both cases? Would it not make more sense to have a BusDevice and ClassDevice type?
We used to have both types, years/decades ago, and in the end, just gave up and merged them together as it really didn't make much sense as the structure really was the same type of thing everywhere. So let's not go back to that mess if we can prevent it please.
We also used to have a out-of-tree patchset that just merged class and bus together, as they are almost the same thing as well, but that just caused too much confusion so we never merged it.
Hope this helps,
greg k-h
Alice Ryhl
reviewerRe: [PATCH 2/3] device: rust: expand documentation for Device
Quoted 55 lines · Greg KH
Quoted 54 lines · Alice Ryhl
Quoted 8 lines · Danilo Krummrich
The documentation for the generic Device type is outdated and deserves much more detail.
Hence, expand the documentation and cover topics such as device types, device contexts, as well as information on how to use the generic device infrastructure to implement bus and class specific device types.
Signed-off-by Danilo Krummrich <[email protected]>Overall I think this series is pretty great. It also clarifies some things for me, particularly the difference between bus and class devices.
Quoted 27 lines
rust/kernel/device.rs +25
-0+/// # Device Types /// +/// A [`Device`] can represent either a bus device or a class device. /// +/// ## Bus Devices +/// +/// A bus device is a [`Device`] that is associated with a physical or virtual bus. Examples of +/// buses include PCI, USB, I2C, and SPI. Devices attached to a bus are registered with a specific +/// bus type, which facilitates matching devices with appropriate drivers based on IDs or other +/// identifying information. Bus devices are visible in sysfs under `/sys/bus/<bus-name>/devices/`. +/// +/// ## Class Devices +/// +/// A class device is a [`Device`] that is associated with a logical category of functionality +/// rather than a physical bus. Examples of classes include block devices, network interfaces, sound +/// cards, and input devices. Class devices are grouped under a common class and exposed to +/// userspace via entries in `/sys/class/<class-name>/`. +/// +/// # Device Context +/// +/// [`Device`] references are generic over a [`DeviceContext`], which represents the type state of +/// a [`Device`]. +/// +/// As the name indicates, this type state represents the context of the scope the [`Device`] +/// reference is valid in. For instance, the [`Bound`] context guarantees that the [`Device`] is +/// bound to a driver for the entire duration of the existence of a [`Device<Bound>`] reference. +/// rust/kernel/device.rs line 60
Quoted 10 lines
rust/kernel/device.rs +10
-0+/// Other [`DeviceContext`] types besides [`Bound`] are [`Normal`], [`Core`] and [`CoreInternal`]. +/// +/// Unless selected otherwise [`Device`] defaults to the [`Normal`] [`DeviceContext`], which by +/// itself has no additional requirements. +/// +/// It is always up to the caller of [`Device::from_raw`] to select the correct [`DeviceContext`] +/// type for the corresponding scope the [`Device`] reference is created in. +/// +/// All [`DeviceContext`] types other than [`Normal`] are intended to be used with +/// [bus devices](#bus-devices) only. This raises a few questions for me.
Quoted 6 lines
Quoted 4 lines
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?
USB drivers don't have interrupts :)
I guess that if class drivers never have interrupts, then that is an answer to my question.
Quoted 7 lines
Seriously, generally, yes, you should always unregister your irq handler BEFORE you unbind the driver from the device, otherwise the callback logic normally assumes that the driver is bound to the device and the pointers are almost always "stale" at that time.
But I don't understand what interrupts have to do with the above documentation, what's the connection?
The connection is that to request an irq you must have a &Device<Bound>, so if you can only obtain a &Device<Bound> to a bus device, then that means that you can never request an irq for a class device.
Quoted 11 lines
Quoted 2 lines
The second one is why we use the same
Devicetype for both cases? Would it not make more sense to have a BusDevice and ClassDevice type?We used to have both types, years/decades ago, and in the end, just gave up and merged them together as it really didn't make much sense as the structure really was the same type of thing everywhere. So let's not go back to that mess if we can prevent it please.
We also used to have a out-of-tree patchset that just merged class and bus together, as they are almost the same thing as well, but that just caused too much confusion so we never merged it.
Okay, thanks!
Alice
Danilo Krummrich
maintainerRe: [PATCH 2/3] device: rust: expand documentation for Device
Quoted 3 lines · Alice Ryhl
The connection is that to request an irq you must have a &Device<Bound>, so if you can only obtain a &Device<Bound> to a bus device, then that means that you can never request an irq for a class device.
As mentioned in my other reply, a class device never owns resources of a "real" device such as an IRQ.
A USB device, which represents a real device on a bus, is a bus device, in your example the class device is the input device.
Greg KH
maintainerRe: [PATCH 2/3] device: rust: expand documentation for Device
Quoted 11 lines · Danilo Krummrich
Quoted 3 lines · Alice Ryhl
The connection is that to request an irq you must have a &Device<Bound>, so if you can only obtain a &Device<Bound> to a bus device, then that means that you can never request an irq for a class device.
As mentioned in my other reply, a class device never owns resources of a "real" device such as an IRQ.
A USB device, which represents a real device on a bus, is a bus device, in your example the class device is the input device.
Just to confuse things a bit more (sorry), there are also "USB class devices" that represent USB devices that use the "generic" USB interface to userspace api. You can find these by searching for usb_register_dev().
Note, this is different from the drivers/usb/class/ drivers, which represent various "USB class protocol" that the USB.org group defines, and those talk to userspace through the various common class apis depending on the specific device type (input, network, etc.) and are USB bus drivers.
Naming is hard, wait until you learn about "usb gadget" a word we had to invent to describe the thing :)
thanks,
greg k-h
Danilo Krummrich
maintainerRe: [PATCH 2/3] device: rust: expand documentation for Device
Quoted 9 lines · Greg KH
Quoted 8 lines · Danilo Krummrich
Quoted 3 lines · Alice Ryhl
The connection is that to request an irq you must have a &Device<Bound>, so if you can only obtain a &Device<Bound> to a bus device, then that means that you can never request an irq for a class device.
As mentioned in my other reply, a class device never owns resources of a "real" device such as an IRQ.
Quoted 8 lines
Quoted 3 lines
A USB device, which represents a real device on a bus, is a bus device, in your example the class device is the input device.
Just to confuse things a bit more (sorry), there are also "USB class devices" that represent USB devices that use the "generic" USB interface to userspace api. You can find these by searching for
usb_register_dev().
In this case the struct usb_interface represents a bus device, that can become associated with e.g. a class device of the usbmisc class. :)
So, yeah, there are multiple USB devices, some of them are bus devices, some of them are class devices.
I'm not sure about the exact topology, but AFAIK USB has different bus device types representing the hierarchy, i.e. a device structure for the entire "real" USB device and another one for its interfaces.
Quoted 12 lines
Note, this is different from the
drivers/usb/class/drivers, which represent various "USB class protocol" that the USB.org group defines, and those talk to userspace through the various common class apis depending on the specific device type (input, network, etc.) and are USB bus drivers.Naming is hard, wait until you learn about "usb gadget" a word we had to invent to describe the thing :)
thanks,
greg k-h
Danilo Krummrich
maintainerRe: [PATCH 2/3] device: rust: expand documentation for Device
In reply to Alice RyhlQuoted 10 lines · Alice Ryhl
rust/kernel/device.rs line 60
Quoted 2 lines · Danilo Krummrich
rust/kernel/device.rs +2
-0+/// 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.
Quoted 2 lines
The second one is why we use the same
Devicetype 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.
Quoted 29 lines
Quoted 9 lines
rust/kernel/device.rs +9
-0+/// # 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)] rust/kernel/device.rs line 80
Quoted 10 lines
rust/kernel/device.rs +10
-0+/// 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 forDevice(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.
Quoted 6 lines
and then at the end:
/// [
AlwaysRefCounted]:kernel::types::AlwaysRefCountedI think it's a lot easier to read the markdown version when links are separated out like this.
That's a good suggestion, thanks!
Danilo Krummrich
maintainerPATCH 3/3 driver: rust: expand documentation for driver infrastructure
In reply to Danilo KrummrichAdd documentation about generic driver infrastructure, representing a guideline on how the generic driver infrastructure is intended to be used to implement bus specific driver APIs.
This covers aspects such as the bus specific driver trait, adapter implementation, driver registration and custom device ID types.
--- rust/kernel/driver.rs | 82 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 80 insertions(+), 2 deletions(-)
rust/kernel/driver.rs +80 -2
@@ -2,8 +2,86 @@//! Generic support for drivers of different buses (e.g., PCI, Platform, Amba, etc.). //! -//! Each bus / subsystem is expected to implement [`RegistrationOps`], which allows drivers to -//! register using the [`Registration`] class. +//! This documentation describes how to implement a bus specific driver API and how to align it with +//! the design of (bus specific) devices. +//! +//! Note: Readers are expected to know the content of the documentation of +//! [`Device`](device::Device) and [`device::DeviceContext`]. +//! +//! # Driver Trait +//! +//! The main driver interface is defined by a bus specific driver trait. For instance: +//! +//! ```ignore +//! pub trait Driver: Send { +//! /// The type holding information about each device ID supported by the driver. +//! type IdInfo: 'static; +//! +//! /// The table of OF device ids supported by the driver. +//! const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = None; +//! +//! /// The table of ACPI device ids supported by the driver. +//! const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = None; +//! +//! /// Driver probe. +//! fn probe(dev: &Device<device::Core>, id_info: &Self::IdInfo) -> Result<Pin<KBox<Self>>>; +//! +//! /// Driver unbind (optional). +//! fn unbind(dev: &Device<device::Core>, this: Pin<&Self>) { +//! let _ = (dev, this); +//! } +//! } +//! ``` +//! +//! For specific examples see [`pci::Driver`](kernel::pci::Driver), +//! [`platform::Driver`](kernel::platform::Driver) and +//! [`auxiliary::Driver`](kernel::auxiliary::Driver). +//! +//! The `probe()` callback should return a `Result<Pin<KBox<Self>>>`, i.e. the driver's private +//! data. The bus abstraction should store the pointer in the corresponding bus device. The generic +//! [`Device`](device::Device) infrastructure provides common helpers for this purpose on its +//! [`Device<CoreInternal>`](device::Device<device::CoreInternal>) implementation. +//! +//! All driver callbacks should provide a reference to the driver's private data. Once the driver +//! is unbound from the device, the bus abstraction should take back the ownership of the driver's +//! private data from the corresponding [`Device`](device::Device) and [`drop`] it. +//! +//! All driver callbacks should provide a [`Device<Core>`](device::Device<device::Core>) reference +//! (see also [`device::Core`]). +//! +//! # Adapter +//! +//! The adapter implementation of a bus represents the abstraction layer between the C bus +//! callbacks and the Rust bus callbacks. It therefore has to be generic over an implementation of +//! the [driver trait](#driver-trait). +//! +//! ```ignore +//! pub struct Adapter<T: Driver>; +//! ``` +//! +//! There's a common [`Adapter`] trait that can be implemented to inherit common driver +//! infrastructure, such as finding the ID info from an [`of::IdTable`] or [`acpi::IdTable`]. +//! +//! # Driver Registration +//! +//! In order to register C driver types (such as `struct platform_driver`) the [adapter](#adapter) +//! should implement the [`RegistrationOps`] trait. +//! +//! This trait implementation can be used to create the actual registration with the common +//! [`Registration`] type. +//! +//! Typically, bus abstractions want to provide a bus specific `module_bus_driver!` macro, which +//! creates a kernel module with exactly one [`Registration`] for the bus specific adapter. +//! +//! The generic driver infrastructure provides a helper for this with the +//! [`module_driver`](kernel::module_driver) macro. +//! +//! # Device IDs +//! +//! Besides the common device ID types, such as [`of::DeviceId`] and [`acpi::DeviceId`], most buses +//! may need to implement their own device ID types. +//! +//! For this purpose the generic infrastructure in [`device_id`](kernel::device_id) should be used. use crate ::error::{Error ,Result };use crate ::{acpi, device, of, str::CStr , try_pin_init, types::Opaque ,ThisModule };
Signature · 1 line
2.50.0
Re: [PATCH 3/3] driver: rust: expand documentation for driver infrastructure
This also looks good to me.
Quoted 10 lines · Danilo Krummrich
Add documentation about generic driver infrastructure, representing a guideline on how the generic driver infrastructure is intended to be used to implement bus specific driver APIs.
This covers aspects such as the bus specific driver trait, adapter implementation, driver registration and custom device ID types.
Signed-off-by Danilo Krummrich <[email protected]>
— Daniel
Quoted 5 lines · Danilo Krummrich
This patch series expands the documentation for the generic device and driver infrastructure and provides a guideline on how to implement bus and class specific devices as well as bus specific driver infrastructure, i.e. bus abstractions. It also expands on the existing documentation of device context types.
Looks great to me, thanks for writing this all up: