diff mbox series

[v2,2/4] rust: device: implement device context marker

Message ID 20250314160932.100165-3-dakr@kernel.org (mailing list archive)
State Handled Elsewhere
Delegated to: Bjorn Helgaas
Headers show
Series Improve soundness of bus device abstractions | expand

Commit Message

Danilo Krummrich March 14, 2025, 4:09 p.m. UTC
Some bus device functions should only be called from bus callbacks,
such as probe(), remove(), resume(), suspend(), etc.

To ensure this add device context marker structs, that can be used as
generics for bus device implementations.

Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Suggested-by: Benno Lossin <benno.lossin@proton.me>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/device.rs | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Boqun Feng March 14, 2025, 5:21 p.m. UTC | #1
On Fri, Mar 14, 2025 at 05:09:05PM +0100, Danilo Krummrich wrote:
> Some bus device functions should only be called from bus callbacks,
> such as probe(), remove(), resume(), suspend(), etc.
> 
> To ensure this add device context marker structs, that can be used as
> generics for bus device implementations.
> 
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> Suggested-by: Benno Lossin <benno.lossin@proton.me>

Try chronological order for the tags? It was suggested first and then
reviewed.

Regards,
Boqun

> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/device.rs | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index db2d9658ba47..21b343a1dc4d 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -209,6 +209,32 @@ unsafe impl Send for Device {}
>  // synchronization in `struct device`.
>  unsafe impl Sync for Device {}
>  
> +/// Marker trait for the context 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()`.
> +///
> +/// This is the marker trait for structures representing the context of a bus specific device.
> +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.
> +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()`.
> +pub struct Core;
> +
> +mod private {
> +    pub trait Sealed {}
> +
> +    impl Sealed for super::Core {}
> +    impl Sealed for super::Normal {}
> +}
> +
> +impl DeviceContext for Core {}
> +impl DeviceContext for Normal {}
> +
>  #[doc(hidden)]
>  #[macro_export]
>  macro_rules! dev_printk {
> -- 
> 2.48.1
>
Danilo Krummrich March 14, 2025, 5:31 p.m. UTC | #2
On Fri, Mar 14, 2025 at 10:21:58AM -0700, Boqun Feng wrote:
> On Fri, Mar 14, 2025 at 05:09:05PM +0100, Danilo Krummrich wrote:
> > Some bus device functions should only be called from bus callbacks,
> > such as probe(), remove(), resume(), suspend(), etc.
> > 
> > To ensure this add device context marker structs, that can be used as
> > generics for bus device implementations.
> > 
> > Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> > Suggested-by: Benno Lossin <benno.lossin@proton.me>
> 
> Try chronological order for the tags? It was suggested first and then
> reviewed.

Is that a thing? When I apply patches I usully keep ACKs, RBs and SOBs together
at the bottom.
Miguel Ojeda March 14, 2025, 5:43 p.m. UTC | #3
On Fri, Mar 14, 2025 at 6:31 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> Is that a thing? When I apply patches I usully keep ACKs, RBs and SOBs together
> at the bottom.

It depends on the maintainers/subsystem. Some do chronological, some
do groups (even to the point of having a defined order). Chronological
loses less information but "looks worse". Some consider RBs should go
on top, others below.

I think most people respect the SoB boundary though, when applying a
patch from someone else, and that is likely the most important part.

Cheers,
Miguel
Boqun Feng March 14, 2025, 5:48 p.m. UTC | #4
On Fri, Mar 14, 2025 at 06:31:48PM +0100, Danilo Krummrich wrote:
> On Fri, Mar 14, 2025 at 10:21:58AM -0700, Boqun Feng wrote:
> > On Fri, Mar 14, 2025 at 05:09:05PM +0100, Danilo Krummrich wrote:
> > > Some bus device functions should only be called from bus callbacks,
> > > such as probe(), remove(), resume(), suspend(), etc.
> > > 
> > > To ensure this add device context marker structs, that can be used as
> > > generics for bus device implementations.
> > > 
> > > Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> > > Suggested-by: Benno Lossin <benno.lossin@proton.me>
> > 
> > Try chronological order for the tags? It was suggested first and then
> > reviewed.
> 
> Is that a thing? When I apply patches I usully keep ACKs, RBs and SOBs together
> at the bottom.

I don't think it's a hard requirement, but it makes logical sense to
order the tags except your own SoB based on chronological order when
re-submitting a new version IMO. It's in the same spirit of putting SoBs
in chronological when multiple people handle the patches.

But it's your choice, I just feel it's a bit odd in the current order
;-)

Regards,
Boqun
diff mbox series

Patch

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index db2d9658ba47..21b343a1dc4d 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -209,6 +209,32 @@  unsafe impl Send for Device {}
 // synchronization in `struct device`.
 unsafe impl Sync for Device {}
 
+/// Marker trait for the context 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()`.
+///
+/// This is the marker trait for structures representing the context of a bus specific device.
+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.
+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()`.
+pub struct Core;
+
+mod private {
+    pub trait Sealed {}
+
+    impl Sealed for super::Core {}
+    impl Sealed for super::Normal {}
+}
+
+impl DeviceContext for Core {}
+impl DeviceContext for Normal {}
+
 #[doc(hidden)]
 #[macro_export]
 macro_rules! dev_printk {