[PATCH 0/8] [RFC] Rust device / driver abstractions

[PATCH 0/8] [RFC] Rust device / driver abstractions

[Danilo Krummrich]

Hi,

This patch series provides some initial Rust abstractions around the device /
driver model, including an abstraction for device private data.

This patch series is sent in the context of [1] and is also available at [2].

- Danilo

[1] https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u
[2] https://github.com/Rust-for-Linux/linux/tree/staging/rust-device

Danilo Krummrich (1):
  arch: x86: tools: increase symbol name size

Wedson Almeida Filho (7):
  rust: device: Add a minimal RawDevice trait
  rust: device: Add a stub abstraction for devices
  rust: add driver abstraction
  rust: add rcu abstraction
  rust: add revocable mutex
  rust: add revocable objects
  rust: add device::Data

 arch/x86/tools/insn_decoder_test.c |   2 +-
 rust/bindings/bindings_helper.h    |   1 +
 rust/helpers.c                     |  28 ++
 rust/kernel/device.rs              | 215 +++++++++++++
 rust/kernel/driver.rs              | 493 +++++++++++++++++++++++++++++
 rust/kernel/lib.rs                 |   6 +-
 rust/kernel/revocable.rs           | 438 +++++++++++++++++++++++++
 rust/kernel/sync.rs                |   3 +
 rust/kernel/sync/rcu.rs            |  52 +++
 rust/kernel/sync/revocable.rs      |  98 ++++++
 rust/macros/module.rs              |   2 +-
 samples/rust/rust_minimal.rs       |   2 +-
 samples/rust/rust_print.rs         |   2 +-
 13 files changed, 1337 insertions(+), 5 deletions(-)
 create mode 100644 rust/kernel/device.rs
 create mode 100644 rust/kernel/driver.rs
 create mode 100644 rust/kernel/revocable.rs
 create mode 100644 rust/kernel/sync/rcu.rs
 create mode 100644 rust/kernel/sync/revocable.rs


base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
-- 
2.44.0

[PATCH 1/8] arch: x86: tools: increase symbol name size

[Danilo Krummrich]

Increase the symbol name size as the Rust compiler seems to generate
symbol names exceeding the previous buffer size of 256.

arch/x86/tools/insn_decoder_test: error: malformed line 2486512:
77620
make[2]: *** [arch/x86/tools/Makefile:26: posttest] Error 3

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 arch/x86/tools/insn_decoder_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/tools/insn_decoder_test.c b/arch/x86/tools/insn_decoder_test.c
index 472540aeabc2..18601b3c5037 100644
--- a/arch/x86/tools/insn_decoder_test.c
+++ b/arch/x86/tools/insn_decoder_test.c
@@ -106,7 +106,7 @@ static void parse_args(int argc, char **argv)
 	}
 }
 
-#define BUFSIZE 256
+#define BUFSIZE 1024
 
 int main(int argc, char **argv)
 {
-- 
2.44.0

Re: [PATCH 1/8] arch: x86: tools: increase symbol name size

[Miguel Ojeda]

On Mon, Mar 25, 2024 at 6:49 PM Danilo Krummrich <dakr@redhat.com> wrote:
>
> Increase the symbol name size as the Rust compiler seems to generate
> symbol names exceeding the previous buffer size of 256.
>
> arch/x86/tools/insn_decoder_test: error: malformed line 2486512:
> 77620
> make[2]: *** [arch/x86/tools/Makefile:26: posttest] Error 3
>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>

For reference, this is
https://lore.kernel.org/rust-for-linux/320c4dba-9919-404b-8a26-a8af16be1845@app.fastmail.com/

(I repeat the comment in the resend here too so that it is clearer)

Cheers,
Miguel

Re: [PATCH 1/8] arch: x86: tools: increase symbol name size

[Danilo Krummrich]

On Mon, Mar 25, 2024 at 06:53:40PM +0100, Miguel Ojeda wrote:
> On Mon, Mar 25, 2024 at 6:49 PM Danilo Krummrich <dakr@redhat.com> wrote:
> >
> > Increase the symbol name size as the Rust compiler seems to generate
> > symbol names exceeding the previous buffer size of 256.
> >
> > arch/x86/tools/insn_decoder_test: error: malformed line 2486512:
> > 77620
> > make[2]: *** [arch/x86/tools/Makefile:26: posttest] Error 3
> >
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> 
> For reference, this is
> https://lore.kernel.org/rust-for-linux/320c4dba-9919-404b-8a26-a8af16be1845@app.fastmail.com/

Huh! The patches, especially [1], seem to be around for quite a while. Which
also means people were hitting this beforehand. Any reason a fix did not land
yet?

[1] https://lore.kernel.org/rust-for-linux/20231119180145.157455-1-sergio.collado@gmail.com/

> 
> (I repeat the comment in the resend here too so that it is clearer)
> 
> Cheers,
> Miguel
> 

Re: [PATCH 1/8] arch: x86: tools: increase symbol name size

[Miguel Ojeda]

On Mon, Mar 25, 2024 at 7:01 PM Danilo Krummrich <dakr@redhat.com> wrote:
>
> Huh! The patches, especially [1], seem to be around for quite a while. Which
> also means people were hitting this beforehand. Any reason a fix did not land
> yet?

To be clear, the patch I linked is older than the one you linked (that
is why I linked that one).

I don't know if there is a particular reason -- I pinged recently in
the thread I linked above [1] and mentioned Sergio's patches too. I
guess they may be overloaded, but it would be definitely nice to at
least have the `256 + KSYM_NAME_LEN` fix/approach, even if it is not
the cleanest.

[1] https://lore.kernel.org/rust-for-linux/CANiq72meap5C9bkj5rNDE=0U5gb5OKwDrrTSrVRwJbkAPWgUsg@mail.gmail.com/

Cheers,
Miguel

[PATCH 2/8] rust: device: Add a minimal RawDevice trait

[Danilo Krummrich]

From: Wedson Almeida Filho <wedsonaf@gmail.com>

Add a RawDevice trait which can be implemented by any type representing
a device class (such as a PlatformDevice). This is the minimum amount of
Device support code required to unblock abstractions that need to take
device pointers.

Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/bindings/bindings_helper.h |  1 +
 rust/kernel/device.rs           | 23 +++++++++++++++++++++++
 rust/kernel/lib.rs              |  1 +
 3 files changed, 25 insertions(+)
 create mode 100644 rust/kernel/device.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index c0cb4b05b918..cb88b429da3d 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -7,6 +7,7 @@
  */
 
 #include <kunit/test.h>
+#include <linux/device.h>
 #include <linux/errname.h>
 #include <linux/ethtool.h>
 #include <linux/mdio.h>
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
new file mode 100644
index 000000000000..9be021e393ca
--- /dev/null
+++ b/rust/kernel/device.rs
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Generic devices that are part of the kernel's driver model.
+//!
+//! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
+
+use crate::bindings;
+
+/// A raw device.
+///
+/// # Safety
+///
+/// Implementers must ensure that the `*mut device` returned by [`RawDevice::raw_device`] is
+/// related to `self`, that is, actions on it will affect `self`. For example, if one calls
+/// `get_device`, then the refcount on the device represented by `self` will be incremented.
+///
+/// Additionally, implementers must ensure that the device is never renamed. Commit a5462516aa99
+/// ("driver-core: document restrictions on device_rename()") has details on why `device_rename`
+/// should not be used.
+pub unsafe trait RawDevice {
+    /// Returns the raw `struct device` related to `self`.
+    fn raw_device(&self) -> *mut bindings::device;
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 7ac39874aeac..155c4b32f5c9 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -34,6 +34,7 @@
 #[cfg(not(testlib))]
 mod allocator;
 mod build_assert;
+pub mod device;
 pub mod error;
 pub mod init;
 pub mod ioctl;
-- 
2.44.0

Re: [PATCH 2/8] rust: device: Add a minimal RawDevice trait

[Greg KH]

On Mon, Mar 25, 2024 at 06:49:06PM +0100, Danilo Krummrich wrote:
> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> 
> Add a RawDevice trait which can be implemented by any type representing
> a device class (such as a PlatformDevice). This is the minimum amount of
> Device support code required to unblock abstractions that need to take
> device pointers.
> 
> Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  rust/bindings/bindings_helper.h |  1 +
>  rust/kernel/device.rs           | 23 +++++++++++++++++++++++

As fun as it is to bury .rs files down in rust/ where no one notices
them, I'm going to ask for this to live in drivers/base/ please so that
we notice it and it shows up as a first-class interface here.  Otherwise
it's going to be impossible to keep in sync over time.

Also, who is going to maintain this?  Didn't checkpatch warn you when
you ran it against this patch?  You did use checkpatch, right?  :)

thanks,

greg k-h

Re: [PATCH 2/8] rust: device: Add a minimal RawDevice trait

[Miguel Ojeda]

On Mon, Mar 25, 2024 at 7:14 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> As fun as it is to bury .rs files down in rust/ where no one notices
> them, I'm going to ask for this to live in drivers/base/ please so that
> we notice it and it shows up as a first-class interface here.  Otherwise
> it's going to be impossible to keep in sync over time.

Agreed -- it is a restriction of the current build system, I am
working on the new one to allow things like this.

Cheers,
Miguel

Re: [PATCH 2/8] rust: device: Add a minimal RawDevice trait

[Danilo Krummrich]

On Mon, Mar 25, 2024 at 07:14:17PM +0100, Greg KH wrote:
> On Mon, Mar 25, 2024 at 06:49:06PM +0100, Danilo Krummrich wrote:
> > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > 
> > Add a RawDevice trait which can be implemented by any type representing
> > a device class (such as a PlatformDevice). This is the minimum amount of
> > Device support code required to unblock abstractions that need to take
> > device pointers.
> > 
> > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > ---
> >  rust/bindings/bindings_helper.h |  1 +
> >  rust/kernel/device.rs           | 23 +++++++++++++++++++++++
> 
> As fun as it is to bury .rs files down in rust/ where no one notices
> them, I'm going to ask for this to live in drivers/base/ please so that
> we notice it and it shows up as a first-class interface here.  Otherwise
> it's going to be impossible to keep in sync over time.
> 
> Also, who is going to maintain this?  Didn't checkpatch warn you when
> you ran it against this patch?  You did use checkpatch, right?  :)

Sure, but I left this open to discuss this intentionally.

As you might have noticed, I'm not the original author of those patches. I
gathered them from various places (mostly the original R4L "rust" branch).

I basically figured out the dependencies, structured things in corresponding
staging branches, fixed some issues and polished the commits; the details are
in [1].

Of course, since I've taken the effort to upstream them, I will also improve
them based on the feedback I get.

Having that said, the best candidates to maintain this are probably the current
maintainers and / or the original authors. However, I'm happy to volunteer as
well.

[1] https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH 2/8] rust: device: Add a minimal RawDevice trait

[Greg KH]

On Tue, Mar 26, 2024 at 11:38:45PM +0100, Danilo Krummrich wrote:
> On Mon, Mar 25, 2024 at 07:14:17PM +0100, Greg KH wrote:
> > On Mon, Mar 25, 2024 at 06:49:06PM +0100, Danilo Krummrich wrote:
> > > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > 
> > > Add a RawDevice trait which can be implemented by any type representing
> > > a device class (such as a PlatformDevice). This is the minimum amount of
> > > Device support code required to unblock abstractions that need to take
> > > device pointers.
> > > 
> > > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > > ---
> > >  rust/bindings/bindings_helper.h |  1 +
> > >  rust/kernel/device.rs           | 23 +++++++++++++++++++++++
> > 
> > As fun as it is to bury .rs files down in rust/ where no one notices
> > them, I'm going to ask for this to live in drivers/base/ please so that
> > we notice it and it shows up as a first-class interface here.  Otherwise
> > it's going to be impossible to keep in sync over time.
> > 
> > Also, who is going to maintain this?  Didn't checkpatch warn you when
> > you ran it against this patch?  You did use checkpatch, right?  :)
> 
> Sure, but I left this open to discuss this intentionally.
> 
> As you might have noticed, I'm not the original author of those patches. I
> gathered them from various places (mostly the original R4L "rust" branch).
> 
> I basically figured out the dependencies, structured things in corresponding
> staging branches, fixed some issues and polished the commits; the details are
> in [1].

And it looks like you ignored the review comments from a year ago when
these were last submitted, as Wedson pointed out :(

Please, when taking code from others, be sure you are not sending stuff
that has already been rejected, that just wastes all of our time...

greg k-h

Re: [PATCH 2/8] rust: device: Add a minimal RawDevice trait

[Danilo Krummrich]

On Wed, Mar 27, 2024 at 06:25:53AM +0100, Greg KH wrote:
> On Tue, Mar 26, 2024 at 11:38:45PM +0100, Danilo Krummrich wrote:
> > On Mon, Mar 25, 2024 at 07:14:17PM +0100, Greg KH wrote:
> > > On Mon, Mar 25, 2024 at 06:49:06PM +0100, Danilo Krummrich wrote:
> > > > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > > 
> > > > Add a RawDevice trait which can be implemented by any type representing
> > > > a device class (such as a PlatformDevice). This is the minimum amount of
> > > > Device support code required to unblock abstractions that need to take
> > > > device pointers.
> > > > 
> > > > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > > > ---
> > > >  rust/bindings/bindings_helper.h |  1 +
> > > >  rust/kernel/device.rs           | 23 +++++++++++++++++++++++
> > > 
> > > As fun as it is to bury .rs files down in rust/ where no one notices
> > > them, I'm going to ask for this to live in drivers/base/ please so that
> > > we notice it and it shows up as a first-class interface here.  Otherwise
> > > it's going to be impossible to keep in sync over time.
> > > 
> > > Also, who is going to maintain this?  Didn't checkpatch warn you when
> > > you ran it against this patch?  You did use checkpatch, right?  :)
> > 
> > Sure, but I left this open to discuss this intentionally.
> > 
> > As you might have noticed, I'm not the original author of those patches. I
> > gathered them from various places (mostly the original R4L "rust" branch).
> > 
> > I basically figured out the dependencies, structured things in corresponding
> > staging branches, fixed some issues and polished the commits; the details are
> > in [1].
> 
> And it looks like you ignored the review comments from a year ago when
> these were last submitted, as Wedson pointed out :(

Please see the other mail thread for explanation.

> 
> Please, when taking code from others, be sure you are not sending stuff
> that has already been rejected, that just wastes all of our time...

I don't see that anything has been rejected from the previous mail thread.

> 
> greg k-h
> 

[PATCH 3/8] rust: device: Add a stub abstraction for devices

[Danilo Krummrich]

From: Wedson Almeida Filho <wedsonaf@gmail.com>

Add a Device type which represents an owned reference to a generic
struct device. This minimal implementation just handles reference
counting and allows the user to get the device name.

Also, implement the rust_helper_dev_get_drvdata helper.

Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Co-developed-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/helpers.c        | 13 ++++++++
 rust/kernel/device.rs | 76 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/rust/helpers.c b/rust/helpers.c
index 70e59efd92bc..1e40661a33d1 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -23,6 +23,7 @@
 #include <kunit/test-bug.h>
 #include <linux/bug.h>
 #include <linux/build_bug.h>
+#include <linux/device.h>
 #include <linux/err.h>
 #include <linux/errname.h>
 #include <linux/mutex.h>
@@ -157,6 +158,18 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
 }
 EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key);
 
+void *rust_helper_dev_get_drvdata(struct device *dev)
+{
+	return dev_get_drvdata(dev);
+}
+EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata);
+
+const char *rust_helper_dev_name(const struct device *dev)
+{
+	return dev_name(dev);
+}
+EXPORT_SYMBOL_GPL(rust_helper_dev_name);
+
 /*
  * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
  * use it in contexts where Rust expects a `usize` like slice (array) indices.
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 9be021e393ca..7309a236f512 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -4,7 +4,7 @@
 //!
 //! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
 
-use crate::bindings;
+use crate::{bindings, str::CStr};
 
 /// A raw device.
 ///
@@ -20,4 +20,78 @@
 pub unsafe trait RawDevice {
     /// Returns the raw `struct device` related to `self`.
     fn raw_device(&self) -> *mut bindings::device;
+
+    /// Returns the name of the device.
+    fn name(&self) -> &CStr {
+        let ptr = self.raw_device();
+
+        // SAFETY: `ptr` is valid because `self` keeps it alive.
+        let name = unsafe { bindings::dev_name(ptr) };
+
+        // SAFETY: The name of the device remains valid while it is alive (because the device is
+        // never renamed, per the safety requirement of this trait). This is guaranteed to be the
+        // case because the reference to `self` outlives the one of the returned `CStr` (enforced
+        // by the compiler because of their lifetimes).
+        unsafe { CStr::from_char_ptr(name) }
+    }
+}
+
+/// A ref-counted device.
+///
+/// # Invariants
+///
+/// `ptr` is valid, non-null, and has a non-zero reference count. One of the references is owned by
+/// `self`, and will be decremented when `self` is dropped.
+pub struct Device {
+    pub(crate) ptr: *mut bindings::device,
+}
+
+// SAFETY: `Device` only holds a pointer to a C device, which is safe to be used from any thread.
+unsafe impl Send for Device {}
+
+// SAFETY: `Device` only holds a pointer to a C device, references to which are safe to be used
+// from any thread.
+unsafe impl Sync for Device {}
+
+impl Device {
+    /// Creates a new device instance.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.
+    pub unsafe fn new(ptr: *mut bindings::device) -> Self {
+        // SAFETY: By the safety requirements, ptr is valid and its refcounted will be incremented.
+        unsafe { bindings::get_device(ptr) };
+        // INVARIANT: The safety requirements satisfy all but one invariant, which is that `self`
+        // owns a reference. This is satisfied by the call to `get_device` above.
+        Self { ptr }
+    }
+
+    /// Creates a new device instance from an existing [`RawDevice`] instance.
+    pub fn from_dev(dev: &dyn RawDevice) -> Self {
+        // SAFETY: The requirements are satisfied by the existence of `RawDevice` and its safety
+        // requirements.
+        unsafe { Self::new(dev.raw_device()) }
+    }
+}
+
+// SAFETY: The device returned by `raw_device` is the one for which we hold a reference.
+unsafe impl RawDevice for Device {
+    fn raw_device(&self) -> *mut bindings::device {
+        self.ptr
+    }
+}
+
+impl Drop for Device {
+    fn drop(&mut self) {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // relinquish it now.
+        unsafe { bindings::put_device(self.ptr) };
+    }
+}
+
+impl Clone for Device {
+    fn clone(&self) -> Self {
+        Self::from_dev(self)
+    }
 }
-- 
2.44.0

Re: [PATCH 3/8] rust: device: Add a stub abstraction for devices

[Boqun Feng]

On Mon, Mar 25, 2024 at 06:49:07PM +0100, Danilo Krummrich wrote:
> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> 
> Add a Device type which represents an owned reference to a generic
> struct device. This minimal implementation just handles reference
> counting and allows the user to get the device name.
> 
> Also, implement the rust_helper_dev_get_drvdata helper.
> 
> Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> Co-developed-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  rust/helpers.c        | 13 ++++++++
>  rust/kernel/device.rs | 76 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 88 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 70e59efd92bc..1e40661a33d1 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -23,6 +23,7 @@
>  #include <kunit/test-bug.h>
>  #include <linux/bug.h>
>  #include <linux/build_bug.h>
> +#include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/errname.h>
>  #include <linux/mutex.h>
> @@ -157,6 +158,18 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key);
>  
> +void *rust_helper_dev_get_drvdata(struct device *dev)
> +{
> +	return dev_get_drvdata(dev);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata);
> +
> +const char *rust_helper_dev_name(const struct device *dev)
> +{
> +	return dev_name(dev);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_dev_name);
> +
>  /*
>   * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
>   * use it in contexts where Rust expects a `usize` like slice (array) indices.
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 9be021e393ca..7309a236f512 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -4,7 +4,7 @@
>  //!
>  //! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
>  
> -use crate::bindings;
> +use crate::{bindings, str::CStr};
>  
>  /// A raw device.
>  ///
> @@ -20,4 +20,78 @@
>  pub unsafe trait RawDevice {
>      /// Returns the raw `struct device` related to `self`.
>      fn raw_device(&self) -> *mut bindings::device;
> +
> +    /// Returns the name of the device.
> +    fn name(&self) -> &CStr {
> +        let ptr = self.raw_device();
> +
> +        // SAFETY: `ptr` is valid because `self` keeps it alive.
> +        let name = unsafe { bindings::dev_name(ptr) };
> +
> +        // SAFETY: The name of the device remains valid while it is alive (because the device is
> +        // never renamed, per the safety requirement of this trait). This is guaranteed to be the
> +        // case because the reference to `self` outlives the one of the returned `CStr` (enforced
> +        // by the compiler because of their lifetimes).
> +        unsafe { CStr::from_char_ptr(name) }
> +    }
> +}
> +
> +/// A ref-counted device.
> +///
> +/// # Invariants
> +///
> +/// `ptr` is valid, non-null, and has a non-zero reference count. One of the references is owned by
> +/// `self`, and will be decremented when `self` is dropped.
> +pub struct Device {
> +    pub(crate) ptr: *mut bindings::device,
> +}
> +

At least for this part of abstraction, please do:

	#[repr(transparent)]
	pub struct Device(Opaque<bindings::device>);

and impl AlwaysRefCounted for `Device`, use `ARef<Device>` as a wrapper
of `*mut bindings::device`.

Regards,
Boqun

> +// SAFETY: `Device` only holds a pointer to a C device, which is safe to be used from any thread.
> +unsafe impl Send for Device {}
> +
> +// SAFETY: `Device` only holds a pointer to a C device, references to which are safe to be used
> +// from any thread.
> +unsafe impl Sync for Device {}
> +
> +impl Device {
> +    /// Creates a new device instance.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.
> +    pub unsafe fn new(ptr: *mut bindings::device) -> Self {
> +        // SAFETY: By the safety requirements, ptr is valid and its refcounted will be incremented.
> +        unsafe { bindings::get_device(ptr) };
> +        // INVARIANT: The safety requirements satisfy all but one invariant, which is that `self`
> +        // owns a reference. This is satisfied by the call to `get_device` above.
> +        Self { ptr }
> +    }
> +
> +    /// Creates a new device instance from an existing [`RawDevice`] instance.
> +    pub fn from_dev(dev: &dyn RawDevice) -> Self {
> +        // SAFETY: The requirements are satisfied by the existence of `RawDevice` and its safety
> +        // requirements.
> +        unsafe { Self::new(dev.raw_device()) }
> +    }
> +}
> +
> +// SAFETY: The device returned by `raw_device` is the one for which we hold a reference.
> +unsafe impl RawDevice for Device {
> +    fn raw_device(&self) -> *mut bindings::device {
> +        self.ptr
> +    }
> +}
> +
> +impl Drop for Device {
> +    fn drop(&mut self) {
> +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> +        // relinquish it now.
> +        unsafe { bindings::put_device(self.ptr) };
> +    }
> +}
> +
> +impl Clone for Device {
> +    fn clone(&self) -> Self {
> +        Self::from_dev(self)
> +    }
>  }
> -- 
> 2.44.0
> 

Re: [PATCH 3/8] rust: device: Add a stub abstraction for devices

[Danilo Krummrich]

On Mon, Mar 25, 2024 at 10:58:38AM -0700, Boqun Feng wrote:
> On Mon, Mar 25, 2024 at 06:49:07PM +0100, Danilo Krummrich wrote:
> > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > 
> > Add a Device type which represents an owned reference to a generic
> > struct device. This minimal implementation just handles reference
> > counting and allows the user to get the device name.
> > 
> > Also, implement the rust_helper_dev_get_drvdata helper.
> > 
> > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > Co-developed-by: Asahi Lina <lina@asahilina.net>
> > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > ---
> >  rust/helpers.c        | 13 ++++++++
> >  rust/kernel/device.rs | 76 ++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 88 insertions(+), 1 deletion(-)
> > 
> > diff --git a/rust/helpers.c b/rust/helpers.c
> > index 70e59efd92bc..1e40661a33d1 100644
> > --- a/rust/helpers.c
> > +++ b/rust/helpers.c
> > @@ -23,6 +23,7 @@
> >  #include <kunit/test-bug.h>
> >  #include <linux/bug.h>
> >  #include <linux/build_bug.h>
> > +#include <linux/device.h>
> >  #include <linux/err.h>
> >  #include <linux/errname.h>
> >  #include <linux/mutex.h>
> > @@ -157,6 +158,18 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
> >  }
> >  EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key);
> >  
> > +void *rust_helper_dev_get_drvdata(struct device *dev)
> > +{
> > +	return dev_get_drvdata(dev);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata);
> > +
> > +const char *rust_helper_dev_name(const struct device *dev)
> > +{
> > +	return dev_name(dev);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_dev_name);
> > +
> >  /*
> >   * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
> >   * use it in contexts where Rust expects a `usize` like slice (array) indices.
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 9be021e393ca..7309a236f512 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -4,7 +4,7 @@
> >  //!
> >  //! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
> >  
> > -use crate::bindings;
> > +use crate::{bindings, str::CStr};
> >  
> >  /// A raw device.
> >  ///
> > @@ -20,4 +20,78 @@
> >  pub unsafe trait RawDevice {
> >      /// Returns the raw `struct device` related to `self`.
> >      fn raw_device(&self) -> *mut bindings::device;
> > +
> > +    /// Returns the name of the device.
> > +    fn name(&self) -> &CStr {
> > +        let ptr = self.raw_device();
> > +
> > +        // SAFETY: `ptr` is valid because `self` keeps it alive.
> > +        let name = unsafe { bindings::dev_name(ptr) };
> > +
> > +        // SAFETY: The name of the device remains valid while it is alive (because the device is
> > +        // never renamed, per the safety requirement of this trait). This is guaranteed to be the
> > +        // case because the reference to `self` outlives the one of the returned `CStr` (enforced
> > +        // by the compiler because of their lifetimes).
> > +        unsafe { CStr::from_char_ptr(name) }
> > +    }
> > +}
> > +
> > +/// A ref-counted device.
> > +///
> > +/// # Invariants
> > +///
> > +/// `ptr` is valid, non-null, and has a non-zero reference count. One of the references is owned by
> > +/// `self`, and will be decremented when `self` is dropped.
> > +pub struct Device {
> > +    pub(crate) ptr: *mut bindings::device,
> > +}
> > +
> 
> At least for this part of abstraction, please do:
> 
> 	#[repr(transparent)]
> 	pub struct Device(Opaque<bindings::device>);
> 
> and impl AlwaysRefCounted for `Device`, use `ARef<Device>` as a wrapper
> of `*mut bindings::device`.

Thanks for pointing this out again, will do.

- Danilo

> 
> Regards,
> Boqun
> 
> > +// SAFETY: `Device` only holds a pointer to a C device, which is safe to be used from any thread.
> > +unsafe impl Send for Device {}
> > +
> > +// SAFETY: `Device` only holds a pointer to a C device, references to which are safe to be used
> > +// from any thread.
> > +unsafe impl Sync for Device {}
> > +
> > +impl Device {
> > +    /// Creates a new device instance.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.
> > +    pub unsafe fn new(ptr: *mut bindings::device) -> Self {
> > +        // SAFETY: By the safety requirements, ptr is valid and its refcounted will be incremented.
> > +        unsafe { bindings::get_device(ptr) };
> > +        // INVARIANT: The safety requirements satisfy all but one invariant, which is that `self`
> > +        // owns a reference. This is satisfied by the call to `get_device` above.
> > +        Self { ptr }
> > +    }
> > +
> > +    /// Creates a new device instance from an existing [`RawDevice`] instance.
> > +    pub fn from_dev(dev: &dyn RawDevice) -> Self {
> > +        // SAFETY: The requirements are satisfied by the existence of `RawDevice` and its safety
> > +        // requirements.
> > +        unsafe { Self::new(dev.raw_device()) }
> > +    }
> > +}
> > +
> > +// SAFETY: The device returned by `raw_device` is the one for which we hold a reference.
> > +unsafe impl RawDevice for Device {
> > +    fn raw_device(&self) -> *mut bindings::device {
> > +        self.ptr
> > +    }
> > +}
> > +
> > +impl Drop for Device {
> > +    fn drop(&mut self) {
> > +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> > +        // relinquish it now.
> > +        unsafe { bindings::put_device(self.ptr) };
> > +    }
> > +}
> > +
> > +impl Clone for Device {
> > +    fn clone(&self) -> Self {
> > +        Self::from_dev(self)
> > +    }
> >  }
> > -- 
> > 2.44.0
> > 
> 

Re: [PATCH 3/8] rust: device: Add a stub abstraction for devices

[Greg KH]

On Mon, Mar 25, 2024 at 06:49:07PM +0100, Danilo Krummrich wrote:
> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> 
> Add a Device type which represents an owned reference to a generic
> struct device. This minimal implementation just handles reference
> counting and allows the user to get the device name.
> 
> Also, implement the rust_helper_dev_get_drvdata helper.

When a changelog says "also", that's a huge hint it needs to be a
separate commit.  Please do so.

thanks,

greg k-h

Re: [PATCH 3/8] rust: device: Add a stub abstraction for devices

[Greg KH]

On Mon, Mar 25, 2024 at 06:49:07PM +0100, Danilo Krummrich wrote:
> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> 
> Add a Device type which represents an owned reference to a generic
> struct device. This minimal implementation just handles reference
> counting and allows the user to get the device name.
> 
> Also, implement the rust_helper_dev_get_drvdata helper.
> 
> Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> Co-developed-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  rust/helpers.c        | 13 ++++++++
>  rust/kernel/device.rs | 76 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 88 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 70e59efd92bc..1e40661a33d1 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -23,6 +23,7 @@
>  #include <kunit/test-bug.h>
>  #include <linux/bug.h>
>  #include <linux/build_bug.h>
> +#include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/errname.h>
>  #include <linux/mutex.h>
> @@ -157,6 +158,18 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key);
>  
> +void *rust_helper_dev_get_drvdata(struct device *dev)
> +{
> +	return dev_get_drvdata(dev);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata);
> +
> +const char *rust_helper_dev_name(const struct device *dev)
> +{
> +	return dev_name(dev);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_dev_name);
> +
>  /*
>   * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
>   * use it in contexts where Rust expects a `usize` like slice (array) indices.
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 9be021e393ca..7309a236f512 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -4,7 +4,7 @@
>  //!
>  //! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
>  
> -use crate::bindings;
> +use crate::{bindings, str::CStr};
>  
>  /// A raw device.
>  ///
> @@ -20,4 +20,78 @@
>  pub unsafe trait RawDevice {
>      /// Returns the raw `struct device` related to `self`.
>      fn raw_device(&self) -> *mut bindings::device;
> +
> +    /// Returns the name of the device.
> +    fn name(&self) -> &CStr {
> +        let ptr = self.raw_device();
> +
> +        // SAFETY: `ptr` is valid because `self` keeps it alive.

How can self keep it alive?

> +        let name = unsafe { bindings::dev_name(ptr) };
> +
> +        // SAFETY: The name of the device remains valid while it is alive (because the device is
> +        // never renamed, per the safety requirement of this trait). This is guaranteed to be the
> +        // case because the reference to `self` outlives the one of the returned `CStr` (enforced
> +        // by the compiler because of their lifetimes).

devices are renamed all the time, I don't understand how this can be
true here.


> +        unsafe { CStr::from_char_ptr(name) }
> +    }
> +}
> +
> +/// A ref-counted device.
> +///
> +/// # Invariants
> +///
> +/// `ptr` is valid, non-null, and has a non-zero reference count. One of the references is owned by
> +/// `self`, and will be decremented when `self` is dropped.
> +pub struct Device {
> +    pub(crate) ptr: *mut bindings::device,
> +}
> +
> +// SAFETY: `Device` only holds a pointer to a C device, which is safe to be used from any thread.
> +unsafe impl Send for Device {}

It's safe if you have a reference count on the pointer.  Do you have
that?

> +
> +// SAFETY: `Device` only holds a pointer to a C device, references to which are safe to be used
> +// from any thread.
> +unsafe impl Sync for Device {}

Same as above.


> +
> +impl Device {
> +    /// Creates a new device instance.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.

device pointers are NULL all the time, you better be able to handle this
otherwise it's not going to go well :(

> +    pub unsafe fn new(ptr: *mut bindings::device) -> Self {
> +        // SAFETY: By the safety requirements, ptr is valid and its refcounted will be incremented.
> +        unsafe { bindings::get_device(ptr) };
> +        // INVARIANT: The safety requirements satisfy all but one invariant, which is that `self`
> +        // owns a reference. This is satisfied by the call to `get_device` above.
> +        Self { ptr }
> +    }
> +
> +    /// Creates a new device instance from an existing [`RawDevice`] instance.
> +    pub fn from_dev(dev: &dyn RawDevice) -> Self {
> +        // SAFETY: The requirements are satisfied by the existence of `RawDevice` and its safety
> +        // requirements.
> +        unsafe { Self::new(dev.raw_device()) }
> +    }
> +}
> +
> +// SAFETY: The device returned by `raw_device` is the one for which we hold a reference.
> +unsafe impl RawDevice for Device {
> +    fn raw_device(&self) -> *mut bindings::device {
> +        self.ptr
> +    }
> +}
> +
> +impl Drop for Device {
> +    fn drop(&mut self) {
> +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> +        // relinquish it now.
> +        unsafe { bindings::put_device(self.ptr) };
> +    }
> +}
> +
> +impl Clone for Device {
> +    fn clone(&self) -> Self {
> +        Self::from_dev(self)

Does this increment the reference count?

thanks,

greg k-h

Re: [PATCH 3/8] rust: device: Add a stub abstraction for devices

[Danilo Krummrich]

On Mon, Mar 25, 2024 at 07:17:46PM +0100, Greg KH wrote:
> On Mon, Mar 25, 2024 at 06:49:07PM +0100, Danilo Krummrich wrote:
> > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > 
> > Add a Device type which represents an owned reference to a generic
> > struct device. This minimal implementation just handles reference
> > counting and allows the user to get the device name.
> > 
> > Also, implement the rust_helper_dev_get_drvdata helper.
> > 
> > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > Co-developed-by: Asahi Lina <lina@asahilina.net>
> > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > ---
> >  rust/helpers.c        | 13 ++++++++
> >  rust/kernel/device.rs | 76 ++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 88 insertions(+), 1 deletion(-)
> > 
> > diff --git a/rust/helpers.c b/rust/helpers.c
> > index 70e59efd92bc..1e40661a33d1 100644
> > --- a/rust/helpers.c
> > +++ b/rust/helpers.c
> > @@ -23,6 +23,7 @@
> >  #include <kunit/test-bug.h>
> >  #include <linux/bug.h>
> >  #include <linux/build_bug.h>
> > +#include <linux/device.h>
> >  #include <linux/err.h>
> >  #include <linux/errname.h>
> >  #include <linux/mutex.h>
> > @@ -157,6 +158,18 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
> >  }
> >  EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key);
> >  
> > +void *rust_helper_dev_get_drvdata(struct device *dev)
> > +{
> > +	return dev_get_drvdata(dev);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata);
> > +
> > +const char *rust_helper_dev_name(const struct device *dev)
> > +{
> > +	return dev_name(dev);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_dev_name);
> > +
> >  /*
> >   * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
> >   * use it in contexts where Rust expects a `usize` like slice (array) indices.
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 9be021e393ca..7309a236f512 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -4,7 +4,7 @@
> >  //!
> >  //! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
> >  
> > -use crate::bindings;
> > +use crate::{bindings, str::CStr};
> >  
> >  /// A raw device.
> >  ///
> > @@ -20,4 +20,78 @@
> >  pub unsafe trait RawDevice {
> >      /// Returns the raw `struct device` related to `self`.
> >      fn raw_device(&self) -> *mut bindings::device;
> > +
> > +    /// Returns the name of the device.
> > +    fn name(&self) -> &CStr {
> > +        let ptr = self.raw_device();
> > +
> > +        // SAFETY: `ptr` is valid because `self` keeps it alive.
> 
> How can self keep it alive?

Device::new() calls get_device(), while the drop trait calls put_device(), hence
the expectation is that as long as self exists, we still hold a reference to the
raw device.

> 
> > +        let name = unsafe { bindings::dev_name(ptr) };
> > +
> > +        // SAFETY: The name of the device remains valid while it is alive (because the device is
> > +        // never renamed, per the safety requirement of this trait). This is guaranteed to be the
> > +        // case because the reference to `self` outlives the one of the returned `CStr` (enforced
> > +        // by the compiler because of their lifetimes).
> 
> devices are renamed all the time, I don't understand how this can be
> true here.

As the comment says, it's the safety requirement of this trait, which is
documented in the DOC comment of trait RawDevice. Admittedly, it's not obvious,
since the comment isn't part of this diff and was introduced in the previous
commit "rust: device: Add a minimal RawDevice trait". Maybe we should move it to
this commit then.

It says: "Additionally, implementers must ensure that the device is never
renamed. Commit a5462516aa99 ("driver-core: document restrictions on
device_rename()") has details on why `device_rename` should not be used."

> 
> 
> > +        unsafe { CStr::from_char_ptr(name) }
> > +    }
> > +}
> > +
> > +/// A ref-counted device.
> > +///
> > +/// # Invariants
> > +///
> > +/// `ptr` is valid, non-null, and has a non-zero reference count. One of the references is owned by
> > +/// `self`, and will be decremented when `self` is dropped.
> > +pub struct Device {
> > +    pub(crate) ptr: *mut bindings::device,
> > +}
> > +
> > +// SAFETY: `Device` only holds a pointer to a C device, which is safe to be used from any thread.
> > +unsafe impl Send for Device {}
> 
> It's safe if you have a reference count on the pointer.  Do you have
> that?

Yes, Device::new() calls get_device().

> 
> > +
> > +// SAFETY: `Device` only holds a pointer to a C device, references to which are safe to be used
> > +// from any thread.
> > +unsafe impl Sync for Device {}
> 
> Same as above.
> 
> 
> > +
> > +impl Device {
> > +    /// Creates a new device instance.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.
> 
> device pointers are NULL all the time, you better be able to handle this
> otherwise it's not going to go well :(

You mean as in check for NULL and return an error in case? I wouldn't see much
value in that.

I think the only places where we call this are the ones where the C side already
guarantees that the raw device pointer is valid, e.g. probe() or remove()
callbacks.

> 
> > +    pub unsafe fn new(ptr: *mut bindings::device) -> Self {
> > +        // SAFETY: By the safety requirements, ptr is valid and its refcounted will be incremented.
> > +        unsafe { bindings::get_device(ptr) };
> > +        // INVARIANT: The safety requirements satisfy all but one invariant, which is that `self`
> > +        // owns a reference. This is satisfied by the call to `get_device` above.
> > +        Self { ptr }
> > +    }
> > +
> > +    /// Creates a new device instance from an existing [`RawDevice`] instance.
> > +    pub fn from_dev(dev: &dyn RawDevice) -> Self {
> > +        // SAFETY: The requirements are satisfied by the existence of `RawDevice` and its safety
> > +        // requirements.
> > +        unsafe { Self::new(dev.raw_device()) }
> > +    }
> > +}
> > +
> > +// SAFETY: The device returned by `raw_device` is the one for which we hold a reference.
> > +unsafe impl RawDevice for Device {
> > +    fn raw_device(&self) -> *mut bindings::device {
> > +        self.ptr
> > +    }
> > +}
> > +
> > +impl Drop for Device {
> > +    fn drop(&mut self) {
> > +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> > +        // relinquish it now.
> > +        unsafe { bindings::put_device(self.ptr) };
> > +    }
> > +}
> > +
> > +impl Clone for Device {
> > +    fn clone(&self) -> Self {
> > +        Self::from_dev(self)
> 
> Does this increment the reference count?

Yes, it does. from_dev() calls new() and new() calls get_device() again.

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH 3/8] rust: device: Add a stub abstraction for devices

[Greg KH]

On Tue, Mar 26, 2024 at 05:01:53PM +0100, Danilo Krummrich wrote:
> On Mon, Mar 25, 2024 at 07:17:46PM +0100, Greg KH wrote:
> > On Mon, Mar 25, 2024 at 06:49:07PM +0100, Danilo Krummrich wrote:
> > > +        let name = unsafe { bindings::dev_name(ptr) };
> > > +
> > > +        // SAFETY: The name of the device remains valid while it is alive (because the device is
> > > +        // never renamed, per the safety requirement of this trait). This is guaranteed to be the
> > > +        // case because the reference to `self` outlives the one of the returned `CStr` (enforced
> > > +        // by the compiler because of their lifetimes).
> > 
> > devices are renamed all the time, I don't understand how this can be
> > true here.
> 
> As the comment says, it's the safety requirement of this trait, which is
> documented in the DOC comment of trait RawDevice. Admittedly, it's not obvious,
> since the comment isn't part of this diff and was introduced in the previous
> commit "rust: device: Add a minimal RawDevice trait". Maybe we should move it to
> this commit then.
> 
> It says: "Additionally, implementers must ensure that the device is never
> renamed. Commit a5462516aa99 ("driver-core: document restrictions on
> device_rename()") has details on why `device_rename` should not be used."

Yes, I agree it should not be used, but that's different from you saying
"this is safe as the device will never be renamed" which is not true
unless you somehow prevent that from ever happening on a pointer that
the rust code gets here.  How can you do that?

And why the issue if the device is renamed (other than the documented
ones), why is the rust code here special in this way?

> > > +
> > > +impl Device {
> > > +    /// Creates a new device instance.
> > > +    ///
> > > +    /// # Safety
> > > +    ///
> > > +    /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.
> > 
> > device pointers are NULL all the time, you better be able to handle this
> > otherwise it's not going to go well :(
> 
> You mean as in check for NULL and return an error in case? I wouldn't see much
> value in that.
> 
> I think the only places where we call this are the ones where the C side already
> guarantees that the raw device pointer is valid, e.g. probe() or remove()
> callbacks.

If that's true, then that's fine, but say that, and perhaps test for it
if you are going to guarantee it.  Otherwise we have many places in the
kernel where device pointers are NULL and code needs to handle it in
general when dealing with device pointers for other types of usages.

> > > +    pub unsafe fn new(ptr: *mut bindings::device) -> Self {
> > > +        // SAFETY: By the safety requirements, ptr is valid and its refcounted will be incremented.
> > > +        unsafe { bindings::get_device(ptr) };
> > > +        // INVARIANT: The safety requirements satisfy all but one invariant, which is that `self`
> > > +        // owns a reference. This is satisfied by the call to `get_device` above.
> > > +        Self { ptr }
> > > +    }
> > > +
> > > +    /// Creates a new device instance from an existing [`RawDevice`] instance.
> > > +    pub fn from_dev(dev: &dyn RawDevice) -> Self {
> > > +        // SAFETY: The requirements are satisfied by the existence of `RawDevice` and its safety
> > > +        // requirements.
> > > +        unsafe { Self::new(dev.raw_device()) }
> > > +    }
> > > +}
> > > +
> > > +// SAFETY: The device returned by `raw_device` is the one for which we hold a reference.
> > > +unsafe impl RawDevice for Device {
> > > +    fn raw_device(&self) -> *mut bindings::device {
> > > +        self.ptr
> > > +    }
> > > +}
> > > +
> > > +impl Drop for Device {
> > > +    fn drop(&mut self) {
> > > +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> > > +        // relinquish it now.
> > > +        unsafe { bindings::put_device(self.ptr) };
> > > +    }
> > > +}
> > > +
> > > +impl Clone for Device {
> > > +    fn clone(&self) -> Self {
> > > +        Self::from_dev(self)
> > 
> > Does this increment the reference count?
> 
> Yes, it does. from_dev() calls new() and new() calls get_device() again.

Ok, but why would you ever allow a device structure to be cloned?
That's not something that should ever happen, once it is created, it is
the ONLY instance of that structure around, and it can't be changed, as
you passed it off to be handled by the driver core, and the driver core
is the one that handles the lifetime of the object, NOT the bus or the
driver that happens to bind to it.

I've been worried about how the interaction between the driver core and
rust code is going to handle structure lifetime rules.  You need to be
very careful here with what you do.  Rust code can create these
structures, but once they are passed to the driver core, you lose the
ability to control them and have to allow the driver core to manage
them, as that's its job, not the rust code.

thanks,

greg k-h

Re: [PATCH 3/8] rust: device: Add a stub abstraction for devices

[Boqun Feng]

On Tue, Mar 26, 2024 at 07:03:47PM +0100, Greg KH wrote:
[...]
> > > > +impl Clone for Device {
> > > > +    fn clone(&self) -> Self {
> > > > +        Self::from_dev(self)
> > > 
> > > Does this increment the reference count?
> > 
> > Yes, it does. from_dev() calls new() and new() calls get_device() again.
> 
> Ok, but why would you ever allow a device structure to be cloned?

This function actually doesn't clone the device structure, it rather
just clones the "smart pointer" to the device. But it's confusing since
it should use an extra layer of abstraction, as I mentioned:

	https://lore.kernel.org/rust-for-linux/ZgG7TlybSa00cuoy@boqun-archlinux/

Right now `Device` is mapped to `struct device *`, which mixes the
abstraction of a device struct and a pointer to a device struct. We
should do the abstraciton in a different way:

	// `Device` maps to `struct device`.
	#[repr(transparent)]
	struct Devcie(Opaque<bindings::device>);

and then having a `&Device` means you have a reference to the `Device`,
you can use it for any device related options except increase/descrease
the refcount of a device. For example:

	unsafe impl RawDevice for Device {
	    fn raw_device(&self) -> *mut bindings::device {
	        self.0.get()
	    }
	}

Driver code that only requires the existence of the device should only
use `&Device`.

And for the code that want to manage the lifetime of a device, since
`struct device` has a refcount in it, we can claim that `Device` is a
`AlwaysRefCounted` type:

	impl AlwaysRefCounted for Device {
	    fn inc_ref(&self) {
	        get_device(self.0.get());
	    }

	    unsafe fn dec_ref(obj: NonNull<Self>) {
	        put_device(obj.as_mut_ptr());
	    }
	}

with that, we have a type `ARef<Device>`, which is similar to `&Device`
in that it's a pointer type, but you can clone (inc refcount) and drop
(dec refcount) on it. We can still define a `Device::new` but the return
type is `ARef<Device>`, meaning we hold a reference to the device.

	impl Device {
	    pub unsafe fn new(ptr: NonNull<bindingds::device>) -> ARef<Device> {
	        unsafe { ARef::from_raw(ptr) }.clone()
	    }
	}

In this way, if Rust code is not the owner of the device (probably it's
the common case right now), Rust code can use a reference to make sure
it doesn't get freed by the owner. And of course, if it's a driver
callback where the driver guarantees the liveness of the device, the
Rust code should take a `&Device` and don't worry about the ownership
anymore.

Does this make sense?

Regards,
Boqun

> That's not something that should ever happen, once it is created, it is
> the ONLY instance of that structure around, and it can't be changed, as
> you passed it off to be handled by the driver core, and the driver core
> is the one that handles the lifetime of the object, NOT the bus or the
> driver that happens to bind to it.
> 
> I've been worried about how the interaction between the driver core and
> rust code is going to handle structure lifetime rules.  You need to be
> very careful here with what you do.  Rust code can create these
> structures, but once they are passed to the driver core, you lose the
> ability to control them and have to allow the driver core to manage
> them, as that's its job, not the rust code.
> 
> thanks,
> 
> greg k-h

Re: [PATCH 3/8] rust: device: Add a stub abstraction for devices

[Danilo Krummrich]

On Tue, Mar 26, 2024 at 07:03:47PM +0100, Greg KH wrote:
> On Tue, Mar 26, 2024 at 05:01:53PM +0100, Danilo Krummrich wrote:
> > On Mon, Mar 25, 2024 at 07:17:46PM +0100, Greg KH wrote:
> > > On Mon, Mar 25, 2024 at 06:49:07PM +0100, Danilo Krummrich wrote:
> > > > +        let name = unsafe { bindings::dev_name(ptr) };
> > > > +
> > > > +        // SAFETY: The name of the device remains valid while it is alive (because the device is
> > > > +        // never renamed, per the safety requirement of this trait). This is guaranteed to be the
> > > > +        // case because the reference to `self` outlives the one of the returned `CStr` (enforced
> > > > +        // by the compiler because of their lifetimes).
> > > 
> > > devices are renamed all the time, I don't understand how this can be
> > > true here.
> > 
> > As the comment says, it's the safety requirement of this trait, which is
> > documented in the DOC comment of trait RawDevice. Admittedly, it's not obvious,
> > since the comment isn't part of this diff and was introduced in the previous
> > commit "rust: device: Add a minimal RawDevice trait". Maybe we should move it to
> > this commit then.
> > 
> > It says: "Additionally, implementers must ensure that the device is never
> > renamed. Commit a5462516aa99 ("driver-core: document restrictions on
> > device_rename()") has details on why `device_rename` should not be used."
> 
> Yes, I agree it should not be used, but that's different from you saying
> "this is safe as the device will never be renamed" which is not true

You omit the restriction "implementers must ensure that the device is never
renamed".

> unless you somehow prevent that from ever happening on a pointer that
> the rust code gets here.  How can you do that?

By setting the above requirement for the subsystem implementing the RawDevice
trait.

Do we really need to expect the device name to change at any (random) point of
time? Isn't the subsystem in control of that?

The alternative would be to always return a copy of the device name when
RawDevice::name() is called.

Besides all that, aren't we in trouble on the C side as well?

devm_request_threaded_irq(), for instance, just picks up the pointer returned by
dev_name(dev), which is stored in struct irqaction and e.g. subsequently
accessed by the irq_handler_entry trace event.

Another example would be ehci_platform_probe() where in a subsequent
usb_create_hcd() call kobj->name (or dev->init_name) is assigned to
hcd->self.bus_name. In contrast to the example above, I guess the USB subsystem
takes care to never rename the device. Isn't this assumption identical to the
requirement of trait RawDevice?

> 
> And why the issue if the device is renamed (other than the documented
> ones), why is the rust code here special in this way?
> 

I think Rust isn't special here. Isn't the old name freed in kobject_rename()?
That'd be a problem for the current Rust code and it'd be a problem for the
examples I gave above, right?

Re: [PATCH 3/8] rust: device: Add a stub abstraction for devices

[Wedson Almeida Filho]

On Mon, 25 Mar 2024 at 15:17, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Mar 25, 2024 at 06:49:07PM +0100, Danilo Krummrich wrote:
> > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> >
> > Add a Device type which represents an owned reference to a generic
> > struct device. This minimal implementation just handles reference
> > counting and allows the user to get the device name.
> >
> > Also, implement the rust_helper_dev_get_drvdata helper.
> >
> > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > Co-developed-by: Asahi Lina <lina@asahilina.net>
> > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > ---
> >  rust/helpers.c        | 13 ++++++++
> >  rust/kernel/device.rs | 76 ++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 88 insertions(+), 1 deletion(-)
> >
> > diff --git a/rust/helpers.c b/rust/helpers.c
> > index 70e59efd92bc..1e40661a33d1 100644
> > --- a/rust/helpers.c
> > +++ b/rust/helpers.c
> > @@ -23,6 +23,7 @@
> >  #include <kunit/test-bug.h>
> >  #include <linux/bug.h>
> >  #include <linux/build_bug.h>
> > +#include <linux/device.h>
> >  #include <linux/err.h>
> >  #include <linux/errname.h>
> >  #include <linux/mutex.h>
> > @@ -157,6 +158,18 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
> >  }
> >  EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key);
> >
> > +void *rust_helper_dev_get_drvdata(struct device *dev)
> > +{
> > +     return dev_get_drvdata(dev);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata);
> > +
> > +const char *rust_helper_dev_name(const struct device *dev)
> > +{
> > +     return dev_name(dev);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_dev_name);
> > +
> >  /*
> >   * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
> >   * use it in contexts where Rust expects a `usize` like slice (array) indices.
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 9be021e393ca..7309a236f512 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -4,7 +4,7 @@
> >  //!
> >  //! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
> >
> > -use crate::bindings;
> > +use crate::{bindings, str::CStr};
> >
> >  /// A raw device.
> >  ///
> > @@ -20,4 +20,78 @@
> >  pub unsafe trait RawDevice {
> >      /// Returns the raw `struct device` related to `self`.
> >      fn raw_device(&self) -> *mut bindings::device;
> > +
> > +    /// Returns the name of the device.
> > +    fn name(&self) -> &CStr {
> > +        let ptr = self.raw_device();
> > +
> > +        // SAFETY: `ptr` is valid because `self` keeps it alive.
>
> How can self keep it alive?
>
> > +        let name = unsafe { bindings::dev_name(ptr) };
> > +
> > +        // SAFETY: The name of the device remains valid while it is alive (because the device is
> > +        // never renamed, per the safety requirement of this trait). This is guaranteed to be the
> > +        // case because the reference to `self` outlives the one of the returned `CStr` (enforced
> > +        // by the compiler because of their lifetimes).
>
> devices are renamed all the time, I don't understand how this can be
> true here.

This was discussed before:
https://lore.kernel.org/lkml/ZAnB%2FDozWsir1cIE@kroah.com/

I even sent a patch (that Greg applied) that fixes the C comment that
lead to the safety comment above:
https://lore.kernel.org/lkml/20230406045435.19452-1-wedsonaf@gmail.com/

The decision then was to remove the `name` method until some driver
actually needed it. (We had no plans to upstream the one that used it
in the rust branch, pl061 gpio.)

>
> > +        unsafe { CStr::from_char_ptr(name) }
> > +    }
> > +}
> > +
> > +/// A ref-counted device.
> > +///
> > +/// # Invariants
> > +///
> > +/// `ptr` is valid, non-null, and has a non-zero reference count. One of the references is owned by
> > +/// `self`, and will be decremented when `self` is dropped.
> > +pub struct Device {
> > +    pub(crate) ptr: *mut bindings::device,
> > +}
> > +
> > +// SAFETY: `Device` only holds a pointer to a C device, which is safe to be used from any thread.
> > +unsafe impl Send for Device {}
>
> It's safe if you have a reference count on the pointer.  Do you have
> that?
>
> > +
> > +// SAFETY: `Device` only holds a pointer to a C device, references to which are safe to be used
> > +// from any thread.
> > +unsafe impl Sync for Device {}
>
> Same as above.
>
>
> > +
> > +impl Device {
> > +    /// Creates a new device instance.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.
>
> device pointers are NULL all the time, you better be able to handle this
> otherwise it's not going to go well :(
>
> > +    pub unsafe fn new(ptr: *mut bindings::device) -> Self {
> > +        // SAFETY: By the safety requirements, ptr is valid and its refcounted will be incremented.
> > +        unsafe { bindings::get_device(ptr) };
> > +        // INVARIANT: The safety requirements satisfy all but one invariant, which is that `self`
> > +        // owns a reference. This is satisfied by the call to `get_device` above.
> > +        Self { ptr }
> > +    }
> > +
> > +    /// Creates a new device instance from an existing [`RawDevice`] instance.
> > +    pub fn from_dev(dev: &dyn RawDevice) -> Self {
> > +        // SAFETY: The requirements are satisfied by the existence of `RawDevice` and its safety
> > +        // requirements.
> > +        unsafe { Self::new(dev.raw_device()) }
> > +    }
> > +}
> > +
> > +// SAFETY: The device returned by `raw_device` is the one for which we hold a reference.
> > +unsafe impl RawDevice for Device {
> > +    fn raw_device(&self) -> *mut bindings::device {
> > +        self.ptr
> > +    }
> > +}
> > +
> > +impl Drop for Device {
> > +    fn drop(&mut self) {
> > +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> > +        // relinquish it now.
> > +        unsafe { bindings::put_device(self.ptr) };
> > +    }
> > +}
> > +
> > +impl Clone for Device {
> > +    fn clone(&self) -> Self {
> > +        Self::from_dev(self)
>
> Does this increment the reference count?
>
> thanks,
>
> greg k-h

Re: [PATCH 3/8] rust: device: Add a stub abstraction for devices

[Greg KH]

On Tue, Mar 26, 2024 at 10:38:49PM -0300, Wedson Almeida Filho wrote:
> On Mon, 25 Mar 2024 at 15:17, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Mar 25, 2024 at 06:49:07PM +0100, Danilo Krummrich wrote:
> > > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > >
> > > Add a Device type which represents an owned reference to a generic
> > > struct device. This minimal implementation just handles reference
> > > counting and allows the user to get the device name.
> > >
> > > Also, implement the rust_helper_dev_get_drvdata helper.
> > >
> > > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > Co-developed-by: Asahi Lina <lina@asahilina.net>
> > > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > > ---
> > >  rust/helpers.c        | 13 ++++++++
> > >  rust/kernel/device.rs | 76 ++++++++++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 88 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/rust/helpers.c b/rust/helpers.c
> > > index 70e59efd92bc..1e40661a33d1 100644
> > > --- a/rust/helpers.c
> > > +++ b/rust/helpers.c
> > > @@ -23,6 +23,7 @@
> > >  #include <kunit/test-bug.h>
> > >  #include <linux/bug.h>
> > >  #include <linux/build_bug.h>
> > > +#include <linux/device.h>
> > >  #include <linux/err.h>
> > >  #include <linux/errname.h>
> > >  #include <linux/mutex.h>
> > > @@ -157,6 +158,18 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
> > >  }
> > >  EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key);
> > >
> > > +void *rust_helper_dev_get_drvdata(struct device *dev)
> > > +{
> > > +     return dev_get_drvdata(dev);
> > > +}
> > > +EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata);
> > > +
> > > +const char *rust_helper_dev_name(const struct device *dev)
> > > +{
> > > +     return dev_name(dev);
> > > +}
> > > +EXPORT_SYMBOL_GPL(rust_helper_dev_name);
> > > +
> > >  /*
> > >   * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
> > >   * use it in contexts where Rust expects a `usize` like slice (array) indices.
> > > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > > index 9be021e393ca..7309a236f512 100644
> > > --- a/rust/kernel/device.rs
> > > +++ b/rust/kernel/device.rs
> > > @@ -4,7 +4,7 @@
> > >  //!
> > >  //! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
> > >
> > > -use crate::bindings;
> > > +use crate::{bindings, str::CStr};
> > >
> > >  /// A raw device.
> > >  ///
> > > @@ -20,4 +20,78 @@
> > >  pub unsafe trait RawDevice {
> > >      /// Returns the raw `struct device` related to `self`.
> > >      fn raw_device(&self) -> *mut bindings::device;
> > > +
> > > +    /// Returns the name of the device.
> > > +    fn name(&self) -> &CStr {
> > > +        let ptr = self.raw_device();
> > > +
> > > +        // SAFETY: `ptr` is valid because `self` keeps it alive.
> >
> > How can self keep it alive?
> >
> > > +        let name = unsafe { bindings::dev_name(ptr) };
> > > +
> > > +        // SAFETY: The name of the device remains valid while it is alive (because the device is
> > > +        // never renamed, per the safety requirement of this trait). This is guaranteed to be the
> > > +        // case because the reference to `self` outlives the one of the returned `CStr` (enforced
> > > +        // by the compiler because of their lifetimes).
> >
> > devices are renamed all the time, I don't understand how this can be
> > true here.
> 
> This was discussed before:
> https://lore.kernel.org/lkml/ZAnB%2FDozWsir1cIE@kroah.com/
> 
> I even sent a patch (that Greg applied) that fixes the C comment that
> lead to the safety comment above:
> https://lore.kernel.org/lkml/20230406045435.19452-1-wedsonaf@gmail.com/
> 
> The decision then was to remove the `name` method until some driver
> actually needed it. (We had no plans to upstream the one that used it
> in the rust branch, pl061 gpio.)

Ah, I thought I had reviewed all of this before, thanks for pointing
this out.  And sad that nothing really changed since then, I'll just
ignore all of this thread for now someone figures out how to act on
review comments that are made.

Ignoring them for a year and resending the lot just wastes everyone's
time :(

greg k-h

Re: [PATCH 3/8] rust: device: Add a stub abstraction for devices

[Philipp Stanner]

On Wed, 2024-03-27 at 06:22 +0100, Greg KH wrote:
> On Tue, Mar 26, 2024 at 10:38:49PM -0300, Wedson Almeida Filho wrote:
> > On Mon, 25 Mar 2024 at 15:17, Greg KH <gregkh@linuxfoundation.org>
> > wrote:
> > > 
> > > On Mon, Mar 25, 2024 at 06:49:07PM +0100, Danilo Krummrich wrote:
> > > > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > > 
> > > > Add a Device type which represents an owned reference to a
> > > > generic
> > > > struct device. This minimal implementation just handles
> > > > reference
> > > > counting and allows the user to get the device name.
> > > > 
> > > > Also, implement the rust_helper_dev_get_drvdata helper.
> > > > 
> > > > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > > Co-developed-by: Asahi Lina <lina@asahilina.net>
> > > > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > > > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > > > ---
> > > >  rust/helpers.c        | 13 ++++++++
> > > >  rust/kernel/device.rs | 76
> > > > ++++++++++++++++++++++++++++++++++++++++++-
> > > >  2 files changed, 88 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/rust/helpers.c b/rust/helpers.c
> > > > index 70e59efd92bc..1e40661a33d1 100644
> > > > --- a/rust/helpers.c
> > > > +++ b/rust/helpers.c
> > > > @@ -23,6 +23,7 @@
> > > >  #include <kunit/test-bug.h>
> > > >  #include <linux/bug.h>
> > > >  #include <linux/build_bug.h>
> > > > +#include <linux/device.h>
> > > >  #include <linux/err.h>
> > > >  #include <linux/errname.h>
> > > >  #include <linux/mutex.h>
> > > > @@ -157,6 +158,18 @@ void rust_helper_init_work_with_key(struct
> > > > work_struct *work, work_func_t func,
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key);
> > > > 
> > > > +void *rust_helper_dev_get_drvdata(struct device *dev)
> > > > +{
> > > > +     return dev_get_drvdata(dev);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata);
> > > > +
> > > > +const char *rust_helper_dev_name(const struct device *dev)
> > > > +{
> > > > +     return dev_name(dev);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(rust_helper_dev_name);
> > > > +
> > > >  /*
> > > >   * `bindgen` binds the C `size_t` type as the Rust `usize`
> > > > type, so we can
> > > >   * use it in contexts where Rust expects a `usize` like slice
> > > > (array) indices.
> > > > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > > > index 9be021e393ca..7309a236f512 100644
> > > > --- a/rust/kernel/device.rs
> > > > +++ b/rust/kernel/device.rs
> > > > @@ -4,7 +4,7 @@
> > > >  //!
> > > >  //! C header:
> > > > [`include/linux/device.h`](../../../../include/linux/device.h)
> > > > 
> > > > -use crate::bindings;
> > > > +use crate::{bindings, str::CStr};
> > > > 
> > > >  /// A raw device.
> > > >  ///
> > > > @@ -20,4 +20,78 @@
> > > >  pub unsafe trait RawDevice {
> > > >      /// Returns the raw `struct device` related to `self`.
> > > >      fn raw_device(&self) -> *mut bindings::device;
> > > > +
> > > > +    /// Returns the name of the device.
> > > > +    fn name(&self) -> &CStr {
> > > > +        let ptr = self.raw_device();
> > > > +
> > > > +        // SAFETY: `ptr` is valid because `self` keeps it
> > > > alive.
> > > 
> > > How can self keep it alive?
> > > 
> > > > +        let name = unsafe { bindings::dev_name(ptr) };
> > > > +
> > > > +        // SAFETY: The name of the device remains valid while
> > > > it is alive (because the device is
> > > > +        // never renamed, per the safety requirement of this
> > > > trait). This is guaranteed to be the
> > > > +        // case because the reference to `self` outlives the
> > > > one of the returned `CStr` (enforced
> > > > +        // by the compiler because of their lifetimes).
> > > 
> > > devices are renamed all the time, I don't understand how this can
> > > be
> > > true here.
> > 
> > This was discussed before:
> > https://lore.kernel.org/lkml/ZAnB%2FDozWsir1cIE@kroah.com/
> > 
> > I even sent a patch (that Greg applied) that fixes the C comment
> > that
> > lead to the safety comment above:
> > https://lore.kernel.org/lkml/20230406045435.19452-1-wedsonaf@gmail.com/
> > 
> > The decision then was to remove the `name` method until some driver
> > actually needed it. (We had no plans to upstream the one that used
> > it
> > in the rust branch, pl061 gpio.)
> 
> Ah, I thought I had reviewed all of this before, thanks for pointing
> this out.  And sad that nothing really changed since then, I'll just
> ignore all of this thread for now someone figures out how to act on
> review comments that are made.
> 
> Ignoring them for a year and resending the lot just wastes everyone's
> time :(

Hi,

I'd ask for some generosity here – as you might have noticed, there's
now a "new team" dedicating their time to these efforts, in addition to
those who have been working on it for longer.

Danilo spent a very great amount of time just collecting commits,
contacting the authors and coordinating with them, gather all the
downstream branches, make sure they build warning-free and work with a
test driver, try to understand the existing code base etc. etc. pp.

It's really a lot of work and some things might be looked over. It's
not as if anyone "ignored [the feedback] for a year", it's more that
it's several parties scattered over several companies, with some of the
parties having dropped out by now.

With that being said, your help and feedback is extremely valuable, so
keeping that process going would be neat :)

Regarding the feedback from the thread from a year ago: Assuming we
implement as much of that as possible, could we get back at you with
follow up questions and discussions here in this thread?

Thanks,
P.

> 
> greg k-h
> 

Re: [PATCH 3/8] rust: device: Add a stub abstraction for devices

[Greg KH]

On Wed, Mar 27, 2024 at 10:05:51AM +0100, Philipp Stanner wrote:
> Regarding the feedback from the thread from a year ago: Assuming we
> implement as much of that as possible, could we get back at you with
> follow up questions and discussions here in this thread?

I'm always willing to review patches, just not patches where previous
review comments are ignored, for obvious reasons.

thanks,

greg k-h

Re: [PATCH 3/8] rust: device: Add a stub abstraction for devices

[Danilo Krummrich]

On Wed, Mar 27, 2024 at 06:22:36AM +0100, Greg KH wrote:
> On Tue, Mar 26, 2024 at 10:38:49PM -0300, Wedson Almeida Filho wrote:
> > On Mon, 25 Mar 2024 at 15:17, Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Mar 25, 2024 at 06:49:07PM +0100, Danilo Krummrich wrote:
> > > > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > >
> > > > Add a Device type which represents an owned reference to a generic
> > > > struct device. This minimal implementation just handles reference
> > > > counting and allows the user to get the device name.
> > > >
> > > > Also, implement the rust_helper_dev_get_drvdata helper.
> > > >
> > > > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > > Co-developed-by: Asahi Lina <lina@asahilina.net>
> > > > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > > > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > > > ---
> > > >  rust/helpers.c        | 13 ++++++++
> > > >  rust/kernel/device.rs | 76 ++++++++++++++++++++++++++++++++++++++++++-
> > > >  2 files changed, 88 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/rust/helpers.c b/rust/helpers.c
> > > > index 70e59efd92bc..1e40661a33d1 100644
> > > > --- a/rust/helpers.c
> > > > +++ b/rust/helpers.c
> > > > @@ -23,6 +23,7 @@
> > > >  #include <kunit/test-bug.h>
> > > >  #include <linux/bug.h>
> > > >  #include <linux/build_bug.h>
> > > > +#include <linux/device.h>
> > > >  #include <linux/err.h>
> > > >  #include <linux/errname.h>
> > > >  #include <linux/mutex.h>
> > > > @@ -157,6 +158,18 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key);
> > > >
> > > > +void *rust_helper_dev_get_drvdata(struct device *dev)
> > > > +{
> > > > +     return dev_get_drvdata(dev);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata);
> > > > +
> > > > +const char *rust_helper_dev_name(const struct device *dev)
> > > > +{
> > > > +     return dev_name(dev);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(rust_helper_dev_name);
> > > > +
> > > >  /*
> > > >   * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
> > > >   * use it in contexts where Rust expects a `usize` like slice (array) indices.
> > > > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > > > index 9be021e393ca..7309a236f512 100644
> > > > --- a/rust/kernel/device.rs
> > > > +++ b/rust/kernel/device.rs
> > > > @@ -4,7 +4,7 @@
> > > >  //!
> > > >  //! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
> > > >
> > > > -use crate::bindings;
> > > > +use crate::{bindings, str::CStr};
> > > >
> > > >  /// A raw device.
> > > >  ///
> > > > @@ -20,4 +20,78 @@
> > > >  pub unsafe trait RawDevice {
> > > >      /// Returns the raw `struct device` related to `self`.
> > > >      fn raw_device(&self) -> *mut bindings::device;
> > > > +
> > > > +    /// Returns the name of the device.
> > > > +    fn name(&self) -> &CStr {
> > > > +        let ptr = self.raw_device();
> > > > +
> > > > +        // SAFETY: `ptr` is valid because `self` keeps it alive.
> > >
> > > How can self keep it alive?
> > >
> > > > +        let name = unsafe { bindings::dev_name(ptr) };
> > > > +
> > > > +        // SAFETY: The name of the device remains valid while it is alive (because the device is
> > > > +        // never renamed, per the safety requirement of this trait). This is guaranteed to be the
> > > > +        // case because the reference to `self` outlives the one of the returned `CStr` (enforced
> > > > +        // by the compiler because of their lifetimes).
> > >
> > > devices are renamed all the time, I don't understand how this can be
> > > true here.
> > 
> > This was discussed before:
> > https://lore.kernel.org/lkml/ZAnB%2FDozWsir1cIE@kroah.com/
> > 
> > I even sent a patch (that Greg applied) that fixes the C comment that
> > lead to the safety comment above:
> > https://lore.kernel.org/lkml/20230406045435.19452-1-wedsonaf@gmail.com/
> > 
> > The decision then was to remove the `name` method until some driver
> > actually needed it. (We had no plans to upstream the one that used it
> > in the rust branch, pl061 gpio.)
> 
> Ah, I thought I had reviewed all of this before, thanks for pointing
> this out.  And sad that nothing really changed since then, I'll just
> ignore all of this thread for now someone figures out how to act on
> review comments that are made.

I was aware of the previous discussions, but I read them quite a while before I
sent this series and hence I think I forgot to mention that this is, partially,
a follow-up. Sorry for that.

However, the only relevant outcome for this series seems to be that you agreed
on dropping RawDevice::name(). I was considering to use the PCI device name in
Nova in the context of debugfs entries for the GSP firmware, which is why I did
not drop this function. Again sorry, should have made this transparent.

> 
> Ignoring them for a year and resending the lot just wastes everyone's
> time :(

Except for the above all the other discussions we currently have doesn't seem to
be repetitive and I think we were making good progress. I think it would be
unfortunate to start over on them in a v2. Can we please continue?

Besides that, I also think that even the dev_name() discussion we have is worth
to follow up in general, even if we just drop RawDevice::name() for now, which
I'm fine with.

- Danilo

> 
> greg k-h
> 

[PATCH 4/8] rust: add driver abstraction

[Danilo Krummrich]

From: Wedson Almeida Filho <wedsonaf@gmail.com>

This defines general functionality related to registering drivers with
their respective subsystems, and registering modules that implement
drivers.

Originally, RawDeviceId was implemented as a const trait. However, this
unstable feature is broken/gone in 1.73. To work around this without
breaking the API, turn IdArray::new() into a macro so that it can use
concrete types (which can still have const associated functions) instead
of a trait.

Co-developed-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Asahi Lina <lina@asahilina.net>
Co-developed-by: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/driver.rs        | 493 +++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs           |   4 +-
 rust/macros/module.rs        |   2 +-
 samples/rust/rust_minimal.rs |   2 +-
 samples/rust/rust_print.rs   |   2 +-
 5 files changed, 499 insertions(+), 4 deletions(-)
 create mode 100644 rust/kernel/driver.rs

diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
new file mode 100644
index 000000000000..6cba3b750be2
--- /dev/null
+++ b/rust/kernel/driver.rs
@@ -0,0 +1,493 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Generic support for drivers of different buses (e.g., PCI, Platform, Amba, etc.).
+//!
+//! Each bus/subsystem is expected to implement [`DriverOps`], which allows drivers to register
+//! using the [`Registration`] class.
+
+use crate::{error::code::*, error::Result, str::CStr, sync::Arc, ThisModule};
+use alloc::boxed::Box;
+use core::{cell::UnsafeCell, marker::PhantomData, ops::Deref, pin::Pin};
+
+/// A subsystem (e.g., PCI, Platform, Amba, etc.) that allows drivers to be written for it.
+pub trait DriverOps {
+    /// The type that holds information about the registration. This is typically a struct defined
+    /// by the C portion of the kernel.
+    type RegType: Default;
+
+    /// Registers a driver.
+    ///
+    /// # Safety
+    ///
+    /// `reg` must point to valid, initialised, and writable memory. It may be modified by this
+    /// function to hold registration state.
+    ///
+    /// On success, `reg` must remain pinned and valid until the matching call to
+    /// [`DriverOps::unregister`].
+    unsafe fn register(
+        reg: *mut Self::RegType,
+        name: &'static CStr,
+        module: &'static ThisModule,
+    ) -> Result;
+
+    /// Unregisters a driver previously registered with [`DriverOps::register`].
+    ///
+    /// # Safety
+    ///
+    /// `reg` must point to valid writable memory, initialised by a previous successful call to
+    /// [`DriverOps::register`].
+    unsafe fn unregister(reg: *mut Self::RegType);
+}
+
+/// The registration of a driver.
+pub struct Registration<T: DriverOps> {
+    is_registered: bool,
+    concrete_reg: UnsafeCell<T::RegType>,
+}
+
+// SAFETY: `Registration` has no fields or methods accessible via `&Registration`, so it is safe to
+// share references to it with multiple threads as nothing can be done.
+unsafe impl<T: DriverOps> Sync for Registration<T> {}
+
+impl<T: DriverOps> Registration<T> {
+    /// Creates a new instance of the registration object.
+    pub fn new() -> Self {
+        Self {
+            is_registered: false,
+            concrete_reg: UnsafeCell::new(T::RegType::default()),
+        }
+    }
+
+    /// Allocates a pinned registration object and registers it.
+    ///
+    /// Returns a pinned heap-allocated representation of the registration.
+    pub fn new_pinned(name: &'static CStr, module: &'static ThisModule) -> Result<Pin<Box<Self>>> {
+        let mut reg = Pin::from(Box::try_new(Self::new())?);
+        reg.as_mut().register(name, module)?;
+        Ok(reg)
+    }
+
+    /// Registers a driver with its subsystem.
+    ///
+    /// It must be pinned because the memory block that represents the registration is potentially
+    /// self-referential.
+    pub fn register(
+        self: Pin<&mut Self>,
+        name: &'static CStr,
+        module: &'static ThisModule,
+    ) -> Result {
+        // SAFETY: We never move out of `this`.
+        let this = unsafe { self.get_unchecked_mut() };
+        if this.is_registered {
+            // Already registered.
+            return Err(EINVAL);
+        }
+
+        // SAFETY: `concrete_reg` was initialised via its default constructor. It is only freed
+        // after `Self::drop` is called, which first calls `T::unregister`.
+        unsafe { T::register(this.concrete_reg.get(), name, module) }?;
+
+        this.is_registered = true;
+        Ok(())
+    }
+}
+
+impl<T: DriverOps> Default for Registration<T> {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
+impl<T: DriverOps> Drop for Registration<T> {
+    fn drop(&mut self) {
+        if self.is_registered {
+            // SAFETY: This path only runs if a previous call to `T::register` completed
+            // successfully.
+            unsafe { T::unregister(self.concrete_reg.get()) };
+        }
+    }
+}
+
+/// Conversion from a device id to a raw device id.
+///
+/// This is meant to be implemented by buses/subsystems so that they can use [`IdTable`] to
+/// guarantee (at compile-time) zero-termination of device id tables provided by drivers.
+///
+/// # Safety
+///
+/// Implementers must ensure that:
+///   - [`RawDeviceId::ZERO`] is actually a zeroed-out version of the raw device id.
+///   - [`RawDeviceId::to_rawid`] stores `offset` in the context/data field of the raw device id so
+///     that buses can recover the pointer to the data.
+pub unsafe trait RawDeviceId {
+    /// The raw type that holds the device id.
+    ///
+    /// Id tables created from [`Self`] are going to hold this type in its zero-terminated array.
+    type RawType: Copy;
+
+    /// A zeroed-out representation of the raw device id.
+    ///
+    /// Id tables created from [`Self`] use [`Self::ZERO`] as the sentinel to indicate the end of
+    /// the table.
+    const ZERO: Self::RawType;
+}
+
+/// A zero-terminated device id array, followed by context data.
+#[repr(C)]
+pub struct IdArray<T: RawDeviceId, U, const N: usize> {
+    ids: [T::RawType; N],
+    sentinel: T::RawType,
+    id_infos: [Option<U>; N],
+}
+
+impl<T: RawDeviceId, U, const N: usize> IdArray<T, U, N> {
+    const U_NONE: Option<U> = None;
+
+    /// Returns an `IdTable` backed by `self`.
+    ///
+    /// This is used to essentially erase the array size.
+    pub const fn as_table(&self) -> IdTable<'_, T, U> {
+        IdTable {
+            first: &self.ids[0],
+            _p: PhantomData,
+        }
+    }
+
+    /// Returns the number of items in the ID table.
+    pub const fn count(&self) -> usize {
+        self.ids.len()
+    }
+
+    /// Returns the inner IdArray array, without the context data.
+    // pub const fn as_ids(&self) -> IdArrayIds<T, N>
+    // where
+    //     T: RawDeviceId + Copy,
+    // {
+    //     self.ids
+    // }
+
+    /// Creates a new instance of the array.
+    ///
+    /// The contents are derived from the given identifiers and context information.
+    #[doc(hidden)]
+    pub const unsafe fn new(raw_ids: [T::RawType; N], infos: [Option<U>; N]) -> Self
+    where
+        T: RawDeviceId + Copy,
+        T::RawType: Copy + Clone,
+    {
+        Self {
+            ids: raw_ids,
+            sentinel: T::ZERO,
+            id_infos: infos,
+        }
+    }
+
+    #[doc(hidden)]
+    pub const fn get_offset(idx: usize) -> isize
+    where
+        T: RawDeviceId + Copy,
+        T::RawType: Copy + Clone,
+    {
+        // SAFETY: We are only using this dummy value to get offsets.
+        let array = unsafe { Self::new([T::ZERO; N], [Self::U_NONE; N]) };
+        // SAFETY: Both pointers are within `array` (or one byte beyond), consequently they are
+        // derived from the same allocated object. We are using a `u8` pointer, whose size 1,
+        // so the pointers are necessarily 1-byte aligned.
+        let ret = unsafe {
+            (&array.id_infos[idx] as *const _ as *const u8)
+                .offset_from(&array.ids[idx] as *const _ as _)
+        };
+        core::mem::forget(array);
+        ret
+    }
+}
+
+// Creates a new ID array. This is a macro so it can take as a parameter the concrete ID type in order
+// to call to_rawid() on it, and still remain const. This is necessary until a new const_trait_impl
+// implementation lands, since the existing implementation was removed in Rust 1.73.
+#[macro_export]
+#[doc(hidden)]
+macro_rules! _new_id_array {
+    (($($args:tt)*), $id_type:ty) => {{
+        /// Creates a new instance of the array.
+        ///
+        /// The contents are derived from the given identifiers and context information.
+        const fn new< U, const N: usize>(ids: [$id_type; N], infos: [Option<U>; N])
+            -> $crate::driver::IdArray<$id_type, U, N>
+        where
+            $id_type: $crate::driver::RawDeviceId + Copy,
+            <$id_type as $crate::driver::RawDeviceId>::RawType: Copy + Clone,
+        {
+            let mut raw_ids =
+                [<$id_type as $crate::driver::RawDeviceId>::ZERO; N];
+            let mut i = 0usize;
+            while i < N {
+                let offset: isize = $crate::driver::IdArray::<$id_type, U, N>::get_offset(i);
+                raw_ids[i] = ids[i].to_rawid(offset);
+                i += 1;
+            }
+
+            // SAFETY: We are passing valid arguments computed with the correct offsets.
+            unsafe {
+                $crate::driver::IdArray::<$id_type, U, N>::new(raw_ids, infos)
+            }
+       }
+
+        new($($args)*)
+    }}
+}
+
+/// A device id table.
+///
+/// The table is guaranteed to be zero-terminated and to be followed by an array of context data of
+/// type `Option<U>`.
+pub struct IdTable<'a, T: RawDeviceId, U> {
+    first: &'a T::RawType,
+    _p: PhantomData<&'a U>,
+}
+
+impl<T: RawDeviceId, U> AsRef<T::RawType> for IdTable<'_, T, U> {
+    fn as_ref(&self) -> &T::RawType {
+        self.first
+    }
+}
+
+/// Counts the number of parenthesis-delimited, comma-separated items.
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::count_paren_items;
+///
+/// assert_eq!(0, count_paren_items!());
+/// assert_eq!(1, count_paren_items!((A)));
+/// assert_eq!(1, count_paren_items!((A),));
+/// assert_eq!(2, count_paren_items!((A), (B)));
+/// assert_eq!(2, count_paren_items!((A), (B),));
+/// assert_eq!(3, count_paren_items!((A), (B), (C)));
+/// assert_eq!(3, count_paren_items!((A), (B), (C),));
+/// ```
+#[macro_export]
+macro_rules! count_paren_items {
+    (($($item:tt)*), $($remaining:tt)*) => { 1 + $crate::count_paren_items!($($remaining)*) };
+    (($($item:tt)*)) => { 1 };
+    () => { 0 };
+}
+
+/// Converts a comma-separated list of pairs into an array with the first element. That is, it
+/// discards the second element of the pair.
+///
+/// Additionally, it automatically introduces a type if the first element is warpped in curly
+/// braces, for example, if it's `{v: 10}`, it becomes `X { v: 10 }`; this is to avoid repeating
+/// the type.
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::first_item;
+///
+/// #[derive(PartialEq, Debug)]
+/// struct X {
+///     v: u32,
+/// }
+///
+/// assert_eq!([] as [X; 0], first_item!(X, ));
+/// assert_eq!([X { v: 10 }], first_item!(X, ({ v: 10 }, Y)));
+/// assert_eq!([X { v: 10 }], first_item!(X, ({ v: 10 }, Y),));
+/// assert_eq!([X { v: 10 }], first_item!(X, (X { v: 10 }, Y)));
+/// assert_eq!([X { v: 10 }], first_item!(X, (X { v: 10 }, Y),));
+/// assert_eq!([X { v: 10 }, X { v: 20 }], first_item!(X, ({ v: 10 }, Y), ({ v: 20 }, Y)));
+/// assert_eq!([X { v: 10 }, X { v: 20 }], first_item!(X, ({ v: 10 }, Y), ({ v: 20 }, Y),));
+/// assert_eq!([X { v: 10 }, X { v: 20 }], first_item!(X, (X { v: 10 }, Y), (X { v: 20 }, Y)));
+/// assert_eq!([X { v: 10 }, X { v: 20 }], first_item!(X, (X { v: 10 }, Y), (X { v: 20 }, Y),));
+/// assert_eq!([X { v: 10 }, X { v: 20 }, X { v: 30 }],
+///            first_item!(X, ({ v: 10 }, Y), ({ v: 20 }, Y), ({v: 30}, Y)));
+/// assert_eq!([X { v: 10 }, X { v: 20 }, X { v: 30 }],
+///            first_item!(X, ({ v: 10 }, Y), ({ v: 20 }, Y), ({v: 30}, Y),));
+/// assert_eq!([X { v: 10 }, X { v: 20 }, X { v: 30 }],
+///            first_item!(X, (X { v: 10 }, Y), (X { v: 20 }, Y), (X {v: 30}, Y)));
+/// assert_eq!([X { v: 10 }, X { v: 20 }, X { v: 30 }],
+///            first_item!(X, (X { v: 10 }, Y), (X { v: 20 }, Y), (X {v: 30}, Y),));
+/// ```
+#[macro_export]
+macro_rules! first_item {
+    ($id_type:ty, $(({$($first:tt)*}, $second:expr)),* $(,)?) => {
+        {
+            type IdType = $id_type;
+            [$(IdType{$($first)*},)*]
+        }
+    };
+    ($id_type:ty, $(($first:expr, $second:expr)),* $(,)?) => { [$($first,)*] };
+}
+
+/// Converts a comma-separated list of pairs into an array with the second element. That is, it
+/// discards the first element of the pair.
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::second_item;
+///
+/// assert_eq!([] as [u32; 0], second_item!());
+/// assert_eq!([10u32], second_item!((X, 10u32)));
+/// assert_eq!([10u32], second_item!((X, 10u32),));
+/// assert_eq!([10u32], second_item!(({ X }, 10u32)));
+/// assert_eq!([10u32], second_item!(({ X }, 10u32),));
+/// assert_eq!([10u32, 20], second_item!((X, 10u32), (X, 20)));
+/// assert_eq!([10u32, 20], second_item!((X, 10u32), (X, 20),));
+/// assert_eq!([10u32, 20], second_item!(({ X }, 10u32), ({ X }, 20)));
+/// assert_eq!([10u32, 20], second_item!(({ X }, 10u32), ({ X }, 20),));
+/// assert_eq!([10u32, 20, 30], second_item!((X, 10u32), (X, 20), (X, 30)));
+/// assert_eq!([10u32, 20, 30], second_item!((X, 10u32), (X, 20), (X, 30),));
+/// assert_eq!([10u32, 20, 30], second_item!(({ X }, 10u32), ({ X }, 20), ({ X }, 30)));
+/// assert_eq!([10u32, 20, 30], second_item!(({ X }, 10u32), ({ X }, 20), ({ X }, 30),));
+/// ```
+#[macro_export]
+macro_rules! second_item {
+    ($(({$($first:tt)*}, $second:expr)),* $(,)?) => { [$($second,)*] };
+    ($(($first:expr, $second:expr)),* $(,)?) => { [$($second,)*] };
+}
+
+/// Defines a new constant [`IdArray`] with a concise syntax.
+///
+/// It is meant to be used by buses and subsystems to create a similar macro with their device id
+/// type already specified, i.e., with fewer parameters to the end user.
+///
+/// # Examples
+///
+// TODO: Exported but not usable by kernel modules (requires `const_trait_impl`).
+/// ```ignore
+/// #![feature(const_trait_impl)]
+/// # use kernel::{define_id_array, driver::RawDeviceId};
+///
+/// #[derive(Copy, Clone)]
+/// struct Id(u32);
+///
+/// // SAFETY: `ZERO` is all zeroes and `to_rawid` stores `offset` as the second element of the raw
+/// // device id pair.
+/// unsafe impl const RawDeviceId for Id {
+///     type RawType = (u64, isize);
+///     const ZERO: Self::RawType = (0, 0);
+///     fn to_rawid(&self, offset: isize) -> Self::RawType {
+///         (self.0 as u64 + 1, offset)
+///     }
+/// }
+///
+/// define_id_array!(A1, Id, (), []);
+/// define_id_array!(A2, Id, &'static [u8], [(Id(10), None)]);
+/// define_id_array!(A3, Id, &'static [u8], [(Id(10), Some(b"id1")), ]);
+/// define_id_array!(A4, Id, &'static [u8], [(Id(10), Some(b"id1")), (Id(20), Some(b"id2"))]);
+/// define_id_array!(A5, Id, &'static [u8], [(Id(10), Some(b"id1")), (Id(20), Some(b"id2")), ]);
+/// define_id_array!(A6, Id, &'static [u8], [(Id(10), None), (Id(20), Some(b"id2")), ]);
+/// define_id_array!(A7, Id, &'static [u8], [(Id(10), Some(b"id1")), (Id(20), None), ]);
+/// define_id_array!(A8, Id, &'static [u8], [(Id(10), None), (Id(20), None), ]);
+/// ```
+#[macro_export]
+macro_rules! define_id_array {
+    ($table_name:ident, $id_type:ty, $data_type:ty, [ $($t:tt)* ]) => {
+        const $table_name:
+            $crate::driver::IdArray<$id_type, $data_type, { $crate::count_paren_items!($($t)*) }> =
+                $crate::_new_id_array!((
+                    $crate::first_item!($id_type, $($t)*), $crate::second_item!($($t)*)), $id_type);
+    };
+}
+
+/// Defines a new constant [`IdTable`] with a concise syntax.
+///
+/// It is meant to be used by buses and subsystems to create a similar macro with their device id
+/// type already specified, i.e., with fewer parameters to the end user.
+///
+/// # Examples
+///
+// TODO: Exported but not usable by kernel modules (requires `const_trait_impl`).
+/// ```ignore
+/// #![feature(const_trait_impl)]
+/// # use kernel::{define_id_table, driver::RawDeviceId};
+///
+/// #[derive(Copy, Clone)]
+/// struct Id(u32);
+///
+/// // SAFETY: `ZERO` is all zeroes and `to_rawid` stores `offset` as the second element of the raw
+/// // device id pair.
+/// unsafe impl const RawDeviceId for Id {
+///     type RawType = (u64, isize);
+///     const ZERO: Self::RawType = (0, 0);
+///     fn to_rawid(&self, offset: isize) -> Self::RawType {
+///         (self.0 as u64 + 1, offset)
+///     }
+/// }
+///
+/// define_id_table!(T1, Id, &'static [u8], [(Id(10), None)]);
+/// define_id_table!(T2, Id, &'static [u8], [(Id(10), Some(b"id1")), ]);
+/// define_id_table!(T3, Id, &'static [u8], [(Id(10), Some(b"id1")), (Id(20), Some(b"id2"))]);
+/// define_id_table!(T4, Id, &'static [u8], [(Id(10), Some(b"id1")), (Id(20), Some(b"id2")), ]);
+/// define_id_table!(T5, Id, &'static [u8], [(Id(10), None), (Id(20), Some(b"id2")), ]);
+/// define_id_table!(T6, Id, &'static [u8], [(Id(10), Some(b"id1")), (Id(20), None), ]);
+/// define_id_table!(T7, Id, &'static [u8], [(Id(10), None), (Id(20), None), ]);
+/// ```
+#[macro_export]
+macro_rules! define_id_table {
+    ($table_name:ident, $id_type:ty, $data_type:ty, [ $($t:tt)* ]) => {
+        const $table_name: Option<$crate::driver::IdTable<'static, $id_type, $data_type>> = {
+            $crate::define_id_array!(ARRAY, $id_type, $data_type, [ $($t)* ]);
+            Some(ARRAY.as_table())
+        };
+    };
+}
+
+/// Custom code within device removal.
+pub trait DeviceRemoval {
+    /// Cleans resources up when the device is removed.
+    ///
+    /// This is called when a device is removed and offers implementers the chance to run some code
+    /// that cleans state up.
+    fn device_remove(&self);
+}
+
+impl DeviceRemoval for () {
+    fn device_remove(&self) {}
+}
+
+impl<T: DeviceRemoval> DeviceRemoval for Arc<T> {
+    fn device_remove(&self) {
+        self.deref().device_remove();
+    }
+}
+
+impl<T: DeviceRemoval> DeviceRemoval for Box<T> {
+    fn device_remove(&self) {
+        self.deref().device_remove();
+    }
+}
+
+/// A kernel module that only registers the given driver on init.
+///
+/// This is a helper struct to make it easier to define single-functionality modules, in this case,
+/// modules that offer a single driver.
+pub struct Module<T: DriverOps> {
+    _driver: Pin<Box<Registration<T>>>,
+}
+
+impl<T: DriverOps> crate::Module for Module<T> {
+    fn init(name: &'static CStr, module: &'static ThisModule) -> Result<Self> {
+        Ok(Self {
+            _driver: Registration::new_pinned(name, module)?,
+        })
+    }
+}
+
+/// Declares a kernel module that exposes a single driver.
+///
+/// It is meant to be used as a helper by other subsystems so they can more easily expose their own
+/// macros.
+#[macro_export]
+macro_rules! module_driver {
+    (<$gen_type:ident>, $driver_ops:ty, { type: $type:ty, $($f:tt)* }) => {
+        type Ops<$gen_type> = $driver_ops;
+        type ModuleType = $crate::driver::Module<Ops<$type>>;
+        $crate::prelude::module! {
+            type: ModuleType,
+            $($f)*
+        }
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 155c4b32f5c9..e5a0a97acaf0 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -15,6 +15,7 @@
 #![feature(allocator_api)]
 #![feature(coerce_unsized)]
 #![feature(const_maybe_uninit_zeroed)]
+#![feature(const_refs_to_cell)]
 #![feature(dispatch_from_dyn)]
 #![feature(new_uninit)]
 #![feature(offset_of)]
@@ -35,6 +36,7 @@
 mod allocator;
 mod build_assert;
 pub mod device;
+pub mod driver;
 pub mod error;
 pub mod init;
 pub mod ioctl;
@@ -74,7 +76,7 @@ pub trait Module: Sized + Sync {
     /// should do.
     ///
     /// Equivalent to the `module_init` macro in the C API.
-    fn init(module: &'static ThisModule) -> error::Result<Self>;
+    fn init(name: &'static str::CStr, module: &'static ThisModule) -> error::Result<Self>;
 }
 
 /// Equivalent to `THIS_MODULE` in the C API.
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index d62d8710d77a..9c0fe4e48b6e 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -270,7 +270,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
             }}
 
             fn __init() -> core::ffi::c_int {{
-                match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
+                match <{type_} as kernel::Module>::init(kernel::c_str!(\"{name}\"), &THIS_MODULE) {{
                     Ok(m) => {{
                         unsafe {{
                             __MOD = Some(m);
diff --git a/samples/rust/rust_minimal.rs b/samples/rust/rust_minimal.rs
index dc05f4bbe27e..54de32b78cec 100644
--- a/samples/rust/rust_minimal.rs
+++ b/samples/rust/rust_minimal.rs
@@ -17,7 +17,7 @@ struct RustMinimal {
 }
 
 impl kernel::Module for RustMinimal {
-    fn init(_module: &'static ThisModule) -> Result<Self> {
+    fn init(_name: &'static CStr, _module: &'static ThisModule) -> Result<Self> {
         pr_info!("Rust minimal sample (init)\n");
         pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
 
diff --git a/samples/rust/rust_print.rs b/samples/rust/rust_print.rs
index 67ed8ebf8e8e..3524f64ce7a0 100644
--- a/samples/rust/rust_print.rs
+++ b/samples/rust/rust_print.rs
@@ -40,7 +40,7 @@ fn arc_print() -> Result {
 }
 
 impl kernel::Module for RustPrint {
-    fn init(_module: &'static ThisModule) -> Result<Self> {
+    fn init(_name: &'static CStr, _module: &'static ThisModule) -> Result<Self> {
         pr_info!("Rust printing macros sample (init)\n");
 
         pr_emerg!("Emergency message (level 0) without args\n");
-- 
2.44.0

Re: [PATCH 4/8] rust: add driver abstraction

[Greg KH]

On Mon, Mar 25, 2024 at 06:49:08PM +0100, Danilo Krummrich wrote:
> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> 
> This defines general functionality related to registering drivers with
> their respective subsystems, and registering modules that implement
> drivers.
> 
> Originally, RawDeviceId was implemented as a const trait. However, this
> unstable feature is broken/gone in 1.73. To work around this without
> breaking the API, turn IdArray::new() into a macro so that it can use
> concrete types (which can still have const associated functions) instead
> of a trait.

Why is this paragraph in the changelog comment?  What API?  This code
isn't in the tree so what would have broken?

confused,

greg k-h

Re: [PATCH 4/8] rust: add driver abstraction

[Greg KH]

On Mon, Mar 25, 2024 at 06:49:08PM +0100, Danilo Krummrich wrote:
> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> 
> This defines general functionality related to registering drivers with
> their respective subsystems, and registering modules that implement
> drivers.

drivers are independent from modules, and modules can contain multiple
drivers (and a single driver can be in multiple modules) so why is
anything being tied together here?

> Originally, RawDeviceId was implemented as a const trait. However, this
> unstable feature is broken/gone in 1.73. To work around this without
> breaking the API, turn IdArray::new() into a macro so that it can use
> concrete types (which can still have const associated functions) instead
> of a trait.

Again, confused by this.

> 
> Co-developed-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> Co-developed-by: Andreas Hindborg <a.hindborg@samsung.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  rust/kernel/driver.rs        | 493 +++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs           |   4 +-
>  rust/macros/module.rs        |   2 +-
>  samples/rust/rust_minimal.rs |   2 +-
>  samples/rust/rust_print.rs   |   2 +-
>  5 files changed, 499 insertions(+), 4 deletions(-)
>  create mode 100644 rust/kernel/driver.rs
> 
> diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
> new file mode 100644
> index 000000000000..6cba3b750be2
> --- /dev/null
> +++ b/rust/kernel/driver.rs
> @@ -0,0 +1,493 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic support for drivers of different buses (e.g., PCI, Platform, Amba, etc.).
> +//!
> +//! Each bus/subsystem is expected to implement [`DriverOps`], which allows drivers to register
> +//! using the [`Registration`] class.
> +
> +use crate::{error::code::*, error::Result, str::CStr, sync::Arc, ThisModule};
> +use alloc::boxed::Box;
> +use core::{cell::UnsafeCell, marker::PhantomData, ops::Deref, pin::Pin};
> +
> +/// A subsystem (e.g., PCI, Platform, Amba, etc.) that allows drivers to be written for it.
> +pub trait DriverOps {
> +    /// The type that holds information about the registration. This is typically a struct defined
> +    /// by the C portion of the kernel.
> +    type RegType: Default;
> +
> +    /// Registers a driver.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `reg` must point to valid, initialised, and writable memory. It may be modified by this
> +    /// function to hold registration state.
> +    ///
> +    /// On success, `reg` must remain pinned and valid until the matching call to
> +    /// [`DriverOps::unregister`].
> +    unsafe fn register(
> +        reg: *mut Self::RegType,
> +        name: &'static CStr,
> +        module: &'static ThisModule,
> +    ) -> Result;
> +
> +    /// Unregisters a driver previously registered with [`DriverOps::register`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// `reg` must point to valid writable memory, initialised by a previous successful call to
> +    /// [`DriverOps::register`].
> +    unsafe fn unregister(reg: *mut Self::RegType);
> +}
> +
> +/// The registration of a driver.
> +pub struct Registration<T: DriverOps> {
> +    is_registered: bool,

This should never be needed, if it is needed, something went wrong.  If
it is needed, why do no .c drivers need it?

> +    concrete_reg: UnsafeCell<T::RegType>,
> +}
> +
> +// SAFETY: `Registration` has no fields or methods accessible via `&Registration`, so it is safe to
> +// share references to it with multiple threads as nothing can be done.
> +unsafe impl<T: DriverOps> Sync for Registration<T> {}
> +
> +impl<T: DriverOps> Registration<T> {
> +    /// Creates a new instance of the registration object.
> +    pub fn new() -> Self {
> +        Self {
> +            is_registered: false,

Again, not needed, who would care?  Who is going to attempt to register
a driver multiple times?  And what's wrong if that were to happen?
Doesn't the driver core of the kernel already catch this properly so why
implement it in rust code only?


> +            concrete_reg: UnsafeCell::new(T::RegType::default()),
> +        }
> +    }
> +
> +    /// Allocates a pinned registration object and registers it.
> +    ///
> +    /// Returns a pinned heap-allocated representation of the registration.
> +    pub fn new_pinned(name: &'static CStr, module: &'static ThisModule) -> Result<Pin<Box<Self>>> {
> +        let mut reg = Pin::from(Box::try_new(Self::new())?);
> +        reg.as_mut().register(name, module)?;

Again, modules are not tied to drivers, please don't do that if at all
possible.  You can pass in the module reference to the driver core, but
that should be all that is needed, right?


> +        Ok(reg)
> +    }
> +
> +    /// Registers a driver with its subsystem.
> +    ///
> +    /// It must be pinned because the memory block that represents the registration is potentially
> +    /// self-referential.
> +    pub fn register(
> +        self: Pin<&mut Self>,
> +        name: &'static CStr,
> +        module: &'static ThisModule,
> +    ) -> Result {
> +        // SAFETY: We never move out of `this`.
> +        let this = unsafe { self.get_unchecked_mut() };
> +        if this.is_registered {
> +            // Already registered.
> +            return Err(EINVAL);
> +        }
> +
> +        // SAFETY: `concrete_reg` was initialised via its default constructor. It is only freed
> +        // after `Self::drop` is called, which first calls `T::unregister`.
> +        unsafe { T::register(this.concrete_reg.get(), name, module) }?;
> +
> +        this.is_registered = true;
> +        Ok(())
> +    }
> +}
> +
> +impl<T: DriverOps> Default for Registration<T> {
> +    fn default() -> Self {
> +        Self::new()
> +    }
> +}
> +
> +impl<T: DriverOps> Drop for Registration<T> {
> +    fn drop(&mut self) {
> +        if self.is_registered {
> +            // SAFETY: This path only runs if a previous call to `T::register` completed
> +            // successfully.
> +            unsafe { T::unregister(self.concrete_reg.get()) };
> +        }
> +    }
> +}
> +
> +/// Conversion from a device id to a raw device id.
> +///
> +/// This is meant to be implemented by buses/subsystems so that they can use [`IdTable`] to
> +/// guarantee (at compile-time) zero-termination of device id tables provided by drivers.
> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that:
> +///   - [`RawDeviceId::ZERO`] is actually a zeroed-out version of the raw device id.
> +///   - [`RawDeviceId::to_rawid`] stores `offset` in the context/data field of the raw device id so
> +///     that buses can recover the pointer to the data.
> +pub unsafe trait RawDeviceId {
> +    /// The raw type that holds the device id.
> +    ///
> +    /// Id tables created from [`Self`] are going to hold this type in its zero-terminated array.
> +    type RawType: Copy;
> +
> +    /// A zeroed-out representation of the raw device id.
> +    ///
> +    /// Id tables created from [`Self`] use [`Self::ZERO`] as the sentinel to indicate the end of
> +    /// the table.
> +    const ZERO: Self::RawType;

Don't we already have something like this as the PHY drivers needed it,
right?  Why create it again?


> +}
> +
> +/// A zero-terminated device id array, followed by context data.
> +#[repr(C)]
> +pub struct IdArray<T: RawDeviceId, U, const N: usize> {
> +    ids: [T::RawType; N],
> +    sentinel: T::RawType,
> +    id_infos: [Option<U>; N],
> +}
> +
> +impl<T: RawDeviceId, U, const N: usize> IdArray<T, U, N> {
> +    const U_NONE: Option<U> = None;
> +
> +    /// Returns an `IdTable` backed by `self`.
> +    ///
> +    /// This is used to essentially erase the array size.
> +    pub const fn as_table(&self) -> IdTable<'_, T, U> {
> +        IdTable {
> +            first: &self.ids[0],
> +            _p: PhantomData,
> +        }
> +    }
> +
> +    /// Returns the number of items in the ID table.
> +    pub const fn count(&self) -> usize {
> +        self.ids.len()

Why do you need to ever know the number of ids?

> +    }
> +
> +    /// Returns the inner IdArray array, without the context data.
> +    // pub const fn as_ids(&self) -> IdArrayIds<T, N>
> +    // where
> +    //     T: RawDeviceId + Copy,
> +    // {
> +    //     self.ids
> +    // }
> +
> +    /// Creates a new instance of the array.

Why would you create a new list of ids?  These should be static in
read-only memory and never copied or modified at any time.  Why the
special class for them?

> +    ///
> +    /// The contents are derived from the given identifiers and context information.
> +    #[doc(hidden)]
> +    pub const unsafe fn new(raw_ids: [T::RawType; N], infos: [Option<U>; N]) -> Self
> +    where
> +        T: RawDeviceId + Copy,
> +        T::RawType: Copy + Clone,
> +    {
> +        Self {
> +            ids: raw_ids,
> +            sentinel: T::ZERO,
> +            id_infos: infos,
> +        }
> +    }
> +
> +    #[doc(hidden)]
> +    pub const fn get_offset(idx: usize) -> isize
> +    where
> +        T: RawDeviceId + Copy,
> +        T::RawType: Copy + Clone,
> +    {
> +        // SAFETY: We are only using this dummy value to get offsets.
> +        let array = unsafe { Self::new([T::ZERO; N], [Self::U_NONE; N]) };
> +        // SAFETY: Both pointers are within `array` (or one byte beyond), consequently they are
> +        // derived from the same allocated object. We are using a `u8` pointer, whose size 1,
> +        // so the pointers are necessarily 1-byte aligned.

Why do you need offsets?

confused as to what this is all for, don't you have a simple way to read
a C structure array?  You'll have to emulate that in rust drivers to get
the auto-module-loading logic to work properly.

But again, I thought the phy code already does all of this, why
implement it again (and if it is needed, why isn't the phy code being
moved to this new implementation.)

I stopped here.  Please build on existing code in the kernel, and also,
a real user of these structures is needed to see if/how any of this
actually works.  Why not implement something that uses them that we can
actually review?

thanks,

greg k-h

Re: [PATCH 4/8] rust: add driver abstraction

[David Airlie]

> I stopped here.  Please build on existing code in the kernel, and also,
> a real user of these structures is needed to see if/how any of this
> actually works.  Why not implement something that uses them that we can
> actually review?

Seems like you missed where the cover letter explained that this was
in the context of:
https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u

Can we stop feeding the chicken/egg problem? :-) This posting attempts
to focus people on *device* abstraction problem, by posting small
patch series directly affecting things in an effort to get them
reviewed, or we post the full patch series touching every subsystem
and then nobody reviews it because it's 50 patches and it isn't
focused on the one thing they care about.

Dave.

Re: [PATCH 4/8] rust: add driver abstraction

[Greg KH]

On Tue, Mar 26, 2024 at 05:36:14AM +1000, David Airlie wrote:
> > I stopped here.  Please build on existing code in the kernel, and also,
> > a real user of these structures is needed to see if/how any of this
> > actually works.  Why not implement something that uses them that we can
> > actually review?
> 
> Seems like you missed where the cover letter explained that this was
> in the context of:
> https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u

Yes I did.

> Can we stop feeding the chicken/egg problem? :-) This posting attempts
> to focus people on *device* abstraction problem, by posting small
> patch series directly affecting things in an effort to get them
> reviewed, or we post the full patch series touching every subsystem
> and then nobody reviews it because it's 50 patches and it isn't
> focused on the one thing they care about.

Would you want to review new api additions without ever seeing how they
are used at all?  I can't agree to that, and neither would you, that's
not how any of this works, sorry.

I can provide general "this looks really odd" comments, but I can never
give a "acked-by:" for something that doesn't have a user, that's just
impossible.

I understand your prediciment, so work at it in tiny steps like everyone
does when adding new apis, this isn't new.

thanks,

greg k-h

Re: [PATCH 4/8] rust: add driver abstraction

[David Airlie]

On Tue, Mar 26, 2024 at 3:37 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Mar 26, 2024 at 05:36:14AM +1000, David Airlie wrote:
> > > I stopped here.  Please build on existing code in the kernel, and also,
> > > a real user of these structures is needed to see if/how any of this
> > > actually works.  Why not implement something that uses them that we can
> > > actually review?
> >
> > Seems like you missed where the cover letter explained that this was
> > in the context of:
> > https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u
>
> Yes I did.
>
> > Can we stop feeding the chicken/egg problem? :-) This posting attempts
> > to focus people on *device* abstraction problem, by posting small
> > patch series directly affecting things in an effort to get them
> > reviewed, or we post the full patch series touching every subsystem
> > and then nobody reviews it because it's 50 patches and it isn't
> > focused on the one thing they care about.
>
> Would you want to review new api additions without ever seeing how they
> are used at all?  I can't agree to that, and neither would you, that's
> not how any of this works, sorry.
>
> I can provide general "this looks really odd" comments, but I can never
> give a "acked-by:" for something that doesn't have a user, that's just
> impossible.
>
> I understand your prediciment, so work at it in tiny steps like everyone
> does when adding new apis, this isn't new.
>

These are the tiny steps, unfortunately to even create a driver with
enough "driver" that you have something to review requires a bunch of
abstractions to a bunch of subsystems.

In order to do a stub drm driver, we need at least the device, pci and
drm device abstractions, they all build on each other, and have
different sets of expert opinions to gather from different people.

We just end up losing the reviewers in the noise, because there are 50
patches to review and nobody wants to look at it. Maybe you would
prefer things that way, and we can certainly optimise for you, but I
think there'll be a bunch of other less rust friendly maintainers that
might be more painful.

Thanks for the review so far, and I don't think we are at the
expecting acks stage anyways, just discussion around lifetimes and how
it integrates with the C side are probably way more valuable.

Maybe for next sending, Danilo can try the all the patches approach
and we can see if your expectations of kernel maintainers align with
mine :-)

Dave.

Re: [PATCH 4/8] rust: add driver abstraction

[Greg KH]

On Tue, Mar 26, 2024 at 04:02:04PM +1000, David Airlie wrote:
> On Tue, Mar 26, 2024 at 3:37 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Mar 26, 2024 at 05:36:14AM +1000, David Airlie wrote:
> > > > I stopped here.  Please build on existing code in the kernel, and also,
> > > > a real user of these structures is needed to see if/how any of this
> > > > actually works.  Why not implement something that uses them that we can
> > > > actually review?
> > >
> > > Seems like you missed where the cover letter explained that this was
> > > in the context of:
> > > https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u
> >
> > Yes I did.
> >
> > > Can we stop feeding the chicken/egg problem? :-) This posting attempts
> > > to focus people on *device* abstraction problem, by posting small
> > > patch series directly affecting things in an effort to get them
> > > reviewed, or we post the full patch series touching every subsystem
> > > and then nobody reviews it because it's 50 patches and it isn't
> > > focused on the one thing they care about.
> >
> > Would you want to review new api additions without ever seeing how they
> > are used at all?  I can't agree to that, and neither would you, that's
> > not how any of this works, sorry.
> >
> > I can provide general "this looks really odd" comments, but I can never
> > give a "acked-by:" for something that doesn't have a user, that's just
> > impossible.
> >
> > I understand your prediciment, so work at it in tiny steps like everyone
> > does when adding new apis, this isn't new.
> >
> 
> These are the tiny steps, unfortunately to even create a driver with
> enough "driver" that you have something to review requires a bunch of
> abstractions to a bunch of subsystems.
> 
> In order to do a stub drm driver, we need at least the device, pci and
> drm device abstractions, they all build on each other, and have
> different sets of expert opinions to gather from different people.
> 
> We just end up losing the reviewers in the noise, because there are 50
> patches to review and nobody wants to look at it. Maybe you would
> prefer things that way, and we can certainly optimise for you, but I
> think there'll be a bunch of other less rust friendly maintainers that
> might be more painful.
> 
> Thanks for the review so far, and I don't think we are at the
> expecting acks stage anyways, just discussion around lifetimes and how
> it integrates with the C side are probably way more valuable.
> 
> Maybe for next sending, Danilo can try the all the patches approach
> and we can see if your expectations of kernel maintainers align with
> mine :-)

I don't mind 50 patch long series, those are fine as we can see how
things are actually used.

But for complex beasts like DRM drivers, perhaps people should stop and
implement the tiny "easy" things first, with working users, to verify
that it is all ok first before biting off the big ones.

Like implement a "simple" bus, which means that you have to have the
driver core bindings all working properly and correctly.  Once that is
done and merged then move to the next thing (i.e. a more complex bus
like PCI or USB or platform).

Drivers consume from almost all of the kernel at once, attempting to
write a driver in rust requires bindings from all of those things at
once, a very daunting and difficult task as people are quickly finding
out.  Break it down into smaller things, like the phy driver did, and
move forward that way if people wish to see rust succeed.  Don't attempt
to go and do the biggest beast first, that's just going to make people
frustrated quickly as you have to interact with too many different parts
of the kernel all at once.

Heck, we don't even have working atomics in rust yet, the basic building
blocks of almost all drivers that have complex data paths...

good luck!

greg k-h

Re: [PATCH 4/8] rust: add driver abstraction

[David Airlie]

> I don't mind 50 patch long series, those are fine as we can see how
> things are actually used.
>
> But for complex beasts like DRM drivers, perhaps people should stop and
> implement the tiny "easy" things first, with working users, to verify
> that it is all ok first before biting off the big ones.
>
> Like implement a "simple" bus, which means that you have to have the
> driver core bindings all working properly and correctly.  Once that is
> done and merged then move to the next thing (i.e. a more complex bus
> like PCI or USB or platform).

This is just reintroducing the chicken/egg problem. Implementing a
"simple" bus usually means implementing a "simple" driver for that
bus. Implementing a simple driver for that bus often means
reimplementing an existing driver that is already in the tree. The
"simple" bus driver gets pushed back on because it's duplicating
functionality that is already in the tree and nobody reviews anything
and we re-enter the deadlock cycle. We are actively trying to break
the cycle and get stuff upstream so this can be properly explored
rather than 20 independent trees carrying out of date patchsets to get
anywhere.

> Drivers consume from almost all of the kernel at once, attempting to
> write a driver in rust requires bindings from all of those things at
> once, a very daunting and difficult task as people are quickly finding
> out.  Break it down into smaller things, like the phy driver did, and
> move forward that way if people wish to see rust succeed.  Don't attempt
> to go and do the biggest beast first, that's just going to make people
> frustrated quickly as you have to interact with too many different parts
> of the kernel all at once.

This is all of that, in the drm community we have an out-of-tree
driver done the hard to upstream way, we've decided a small simple drm
driver is the simplest place to write a driver from scratch that has a
friendly enough maintainer that it won't be refused on grounds of
duplication or not implementing a completely functional driver. I
don't think it's unreasonable to start this with a drm driver, we've
at least got developers and maintainers who understand lifetimes, it's
just how we get over the initial device/pci/drm bindings without
blinding intermediate developers with the patch noise.

Next time I think Danilo can just send the monster patchset and we can
see how people feel about things then as well.

Dave.

Re: [PATCH 4/8] rust: add driver abstraction

[Fabien Parent]

[re-sending as plain text instead of HTML]

On Mon, Mar 25, 2024 at 11:35 PM David Airlie <airlied@redhat.com> wrote:
> > I don't mind 50 patch long series, those are fine as we can see how
> > things are actually used.
> >
> > But for complex beasts like DRM drivers, perhaps people should stop and
> > implement the tiny "easy" things first, with working users, to verify
> > that it is all ok first before biting off the big ones.
> >
> > Like implement a "simple" bus, which means that you have to have the
> > driver core bindings all working properly and correctly.  Once that is
> > done and merged then move to the next thing (i.e. a more complex bus
> > like PCI or USB or platform).
>
> This is just reintroducing the chicken/egg problem. Implementing a
> "simple" bus usually means implementing a "simple" driver for that
> bus. Implementing a simple driver for that bus often means
> reimplementing an existing driver that is already in the tree. The
> "simple" bus driver gets pushed back on because it's duplicating
> functionality that is already in the tree and nobody reviews anything
> and we re-enter the deadlock cycle. We are actively trying to break
> the cycle and get stuff upstream so this can be properly explored
> rather than 20 independent trees carrying out of date patchsets to get
> anywhere.

 I've been working on a new driver [0] (not a reimplementation of an
existing one) that uses I2C. I2C is a "simple bus" and the amount of
commits is quite reasonable, maybe this would fit better as a consumer
of the device driver abstractions for upstreaming purposes?
The work is happening here [1] and is fully functional. Right now the
branch is not based on this series but based on the device
abstractions from the old `rust` branch + a few others picked-up from
the `asahi` tree and the mailing list.

[0] https://github.com/Fabo/linux/commit/64a35401316aa9d77490eadb7c4bb731c7901f69
[1] https://github.com/Fabo/linux/commits/fparent/rust-ncv6336/

Fabien

Re: [PATCH 4/8] rust: add driver abstraction

[Danilo Krummrich]

Hi Fabien,

On Sun, Mar 31, 2024 at 12:17:15PM -0700, Fabien Parent wrote:
> [re-sending as plain text instead of HTML]
> 
> On Mon, Mar 25, 2024 at 11:35 PM David Airlie <airlied@redhat.com> wrote:
> > > I don't mind 50 patch long series, those are fine as we can see how
> > > things are actually used.
> > >
> > > But for complex beasts like DRM drivers, perhaps people should stop and
> > > implement the tiny "easy" things first, with working users, to verify
> > > that it is all ok first before biting off the big ones.
> > >
> > > Like implement a "simple" bus, which means that you have to have the
> > > driver core bindings all working properly and correctly.  Once that is
> > > done and merged then move to the next thing (i.e. a more complex bus
> > > like PCI or USB or platform).
> >
> > This is just reintroducing the chicken/egg problem. Implementing a
> > "simple" bus usually means implementing a "simple" driver for that
> > bus. Implementing a simple driver for that bus often means
> > reimplementing an existing driver that is already in the tree. The
> > "simple" bus driver gets pushed back on because it's duplicating
> > functionality that is already in the tree and nobody reviews anything
> > and we re-enter the deadlock cycle. We are actively trying to break
> > the cycle and get stuff upstream so this can be properly explored
> > rather than 20 independent trees carrying out of date patchsets to get
> > anywhere.
> 
>  I've been working on a new driver [0] (not a reimplementation of an
> existing one) that uses I2C. I2C is a "simple bus" and the amount of
> commits is quite reasonable, maybe this would fit better as a consumer
> of the device driver abstractions for upstreaming purposes?

I don't know if it fits better, but I think it works just as well.

The whole reason Dave agreed to take a stub Nova driver [1] is to keep things
simple enough to get the corresponding *basic* device / driver, PCI and DRM
bindings upstream. I don't think the initial PCI device bindings turn out to be
more complex than any other subsystem device bindings in terms of delivering the
required context to review things.

Nevertheless, the I2C driver might indeed have even less dependencies overall.

Besides that, any additional initial user is desirable to get more evidence of
correctness.

So, maybe we just want to go with both as context for the review process?

> The work is happening here [1] and is fully functional. Right now the
> branch is not based on this series but based on the device
> abstractions from the old `rust` branch + a few others picked-up from
> the `asahi` tree and the mailing list.

That's what staging/rust-device [2] is basically based on as well.

The idea was to come up with topic branches for infrastructure that is in the
process of going upstream or in preparation for entering this process (hence the
"staging" prefix), such that we have a common baseline for drivers that are in
development. Instead of having every driver tree carrying those things in
several different versions missing out on potential improvements a driver tree
adds to them.

You are very welcome to base your project on this branch as well and provide
feedback and / or improvements, if you like to.

- Danilo

[1] https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u
[2] https://github.com/Rust-for-Linux/linux/tree/staging/rust-device

> 
> [0] https://github.com/Fabo/linux/commit/64a35401316aa9d77490eadb7c4bb731c7901f69
> [1] https://github.com/Fabo/linux/commits/fparent/rust-ncv6336/
> 
> Fabien
> 

Re: [PATCH 4/8] rust: add driver abstraction

[Viresh Kumar]

On 25-03-24, 18:49, Danilo Krummrich wrote:
> +/// Declares a kernel module that exposes a single driver.
> +///
> +/// It is meant to be used as a helper by other subsystems so they can more easily expose their own
> +/// macros.
> +#[macro_export]
> +macro_rules! module_driver {
> +    (<$gen_type:ident>, $driver_ops:ty, { type: $type:ty, $($f:tt)* }) => {
> +        type Ops<$gen_type> = $driver_ops;
> +        type ModuleType = $crate::driver::Module<Ops<$type>>;
> +        $crate::prelude::module! {
> +            type: ModuleType,
> +            $($f)*
> +        }
> +    }
> +}

Not a problem with this series but with the internal implementation in
rust/macros/module.rs I guess.

The "name" field for the "module_driver" currently doesn't support the
'-' symbol. That's the right thing to do for function names, init
sections, etc, but this also translates to device names and they are
allowed by the kernel to have a '-'.

-- 
viresh

[PATCH 5/8] rust: add rcu abstraction

[Danilo Krummrich]

From: Wedson Almeida Filho <wedsonaf@gmail.com>

Add a simple abstraction to guard critical code sections with an rcu
read lock.

Co-developed-by: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/helpers.c          | 15 ++++++++++++
 rust/kernel/sync.rs     |  1 +
 rust/kernel/sync/rcu.rs | 52 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+)
 create mode 100644 rust/kernel/sync/rcu.rs

diff --git a/rust/helpers.c b/rust/helpers.c
index 1e40661a33d1..3b9ef2868c7c 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -27,6 +27,7 @@
 #include <linux/err.h>
 #include <linux/errname.h>
 #include <linux/mutex.h>
+#include <linux/rcupdate.h>
 #include <linux/refcount.h>
 #include <linux/sched/signal.h>
 #include <linux/spinlock.h>
@@ -170,6 +171,20 @@ const char *rust_helper_dev_name(const struct device *dev)
 }
 EXPORT_SYMBOL_GPL(rust_helper_dev_name);
 
+/* rcu */
+void rust_helper_rcu_read_lock(void)
+{
+	rcu_read_lock();
+}
+EXPORT_SYMBOL_GPL(rust_helper_rcu_read_lock);
+
+void rust_helper_rcu_read_unlock(void)
+{
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(rust_helper_rcu_read_unlock);
+/* end rcu */
+
 /*
  * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
  * use it in contexts where Rust expects a `usize` like slice (array) indices.
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index d219ee518eff..874a45b0c168 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -11,6 +11,7 @@
 mod condvar;
 pub mod lock;
 mod locked_by;
+pub mod rcu;
 
 pub use arc::{Arc, ArcBorrow, UniqueArc};
 pub use condvar::CondVar;
diff --git a/rust/kernel/sync/rcu.rs b/rust/kernel/sync/rcu.rs
new file mode 100644
index 000000000000..1a1c8ea49359
--- /dev/null
+++ b/rust/kernel/sync/rcu.rs
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! RCU support.
+//!
+//! C header: [`include/linux/rcupdate.h`](../../../../include/linux/rcupdate.h)
+
+use crate::bindings;
+use core::marker::PhantomData;
+
+/// Evidence that the RCU read side lock is held on the current thread/CPU.
+///
+/// The type is explicitly not `Send` because this property is per-thread/CPU.
+///
+/// # Invariants
+///
+/// The RCU read side lock is actually held while instances of this guard exist.
+pub struct Guard {
+    _not_send: PhantomData<*mut ()>,
+}
+
+impl Guard {
+    /// Acquires the RCU read side lock and returns a guard.
+    pub fn new() -> Self {
+        // SAFETY: An FFI call with no additional requirements.
+        unsafe { bindings::rcu_read_lock() };
+        // INVARIANT: The RCU read side lock was just acquired above.
+        Self {
+            _not_send: PhantomData,
+        }
+    }
+
+    /// Explicitly releases the RCU read side lock.
+    pub fn unlock(self) {}
+}
+
+impl Default for Guard {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
+impl Drop for Guard {
+    fn drop(&mut self) {
+        // SAFETY: By the type invariants, the rcu read side is locked, so it is ok to unlock it.
+        unsafe { bindings::rcu_read_unlock() };
+    }
+}
+
+/// Acquires the RCU read side lock.
+pub fn read_lock() -> Guard {
+    Guard::new()
+}
-- 
2.44.0

[PATCH 6/8] rust: add revocable mutex

[Danilo Krummrich]

From: Wedson Almeida Filho <wedsonaf@gmail.com>

This is a mutex where access to its contents can be revoked at runtime.

This is different from `Revocable` in a few ways:
  1. The caller may sleep while holding a guard.
  2. Accessors are all serialised.
  3. Accessing is not as cheap (because it involves acquiring the mutex).
  4. The size of the object is larger (because it involves a mutex +
     flag).

An example of where this a good fit to be used in device state that
holds registrations to other subsystems.

Co-developed-by: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/sync.rs           |  2 +
 rust/kernel/sync/revocable.rs | 98 +++++++++++++++++++++++++++++++++++
 2 files changed, 100 insertions(+)
 create mode 100644 rust/kernel/sync/revocable.rs

diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 874a45b0c168..377446e37961 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -12,11 +12,13 @@
 pub mod lock;
 mod locked_by;
 pub mod rcu;
+mod revocable;
 
 pub use arc::{Arc, ArcBorrow, UniqueArc};
 pub use condvar::CondVar;
 pub use lock::{mutex::Mutex, spinlock::SpinLock};
 pub use locked_by::LockedBy;
+pub use revocable::RevocableMutex;
 
 /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
 #[repr(transparent)]
diff --git a/rust/kernel/sync/revocable.rs b/rust/kernel/sync/revocable.rs
new file mode 100644
index 000000000000..cb987c293669
--- /dev/null
+++ b/rust/kernel/sync/revocable.rs
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Synchronisation primitives where access to their contents can be revoked at runtime.
+
+use macros::pin_data;
+
+use crate::{
+    init::PinInit,
+    pin_init,
+    str::CStr,
+    sync::{lock, lock::Lock, LockClassKey},
+};
+use core::mem::MaybeUninit;
+
+use super::lock::Guard;
+
+/// The state within the revocable synchronisation primitive.
+///
+/// We don't use simply `Option<T>` because we need to drop in-place because the contents are
+/// implicitly pinned.
+///
+/// # Invariants
+///
+/// The `is_available` field determines if `data` is initialised.
+struct Inner<T> {
+    is_available: bool,
+    data: MaybeUninit<T>,
+}
+
+impl<T> Inner<T> {
+    fn new(data: T) -> Self {
+        // INVARIANT: `data` is initialised and `is_available` is `true`, so the state matches.
+        Self {
+            is_available: true,
+            data: MaybeUninit::new(data),
+        }
+    }
+
+    fn drop_in_place(&mut self) {
+        if !self.is_available {
+            // Already dropped.
+            return;
+        }
+
+        // INVARIANT: `data` is being dropped and `is_available` is set to `false`, so the state
+        // matches.
+        self.is_available = false;
+
+        // SAFETY: By the type invariants, `data` is valid because `is_available` was true.
+        unsafe { self.data.assume_init_drop() };
+    }
+}
+
+impl<T> Drop for Inner<T> {
+    fn drop(&mut self) {
+        self.drop_in_place();
+    }
+}
+
+#[pin_data]
+pub struct Revocable<T, B: lock::Backend> {
+    #[pin]
+    inner: Lock<Inner<T>, B>,
+}
+
+/// Safely initialises a [`Revocable`] instance with the given name, generating a new lock class.
+// #[macro_export]
+// macro_rules! revocable_init {
+//     ($mutex:expr, $name:literal) => {
+//         $crate::init_with_lockdep!($mutex, $name)
+//     };
+// }
+
+impl<T, B> Revocable<T, B>
+where
+    B: lock::Backend,
+{
+    /// Creates a new revocable instance of the given lock.
+    pub fn new(data: T, name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> {
+        pin_init!(Self {
+            inner <- Lock::new(Inner::new(data), name, key) ,
+        })
+    }
+
+    /// Revokes access to and drops the wrapped object.
+    ///
+    /// Revocation and dropping happen after ongoing accessors complete.
+    pub fn revoke(&self) {
+        self.lock().drop_in_place();
+    }
+
+    fn lock(&self) -> Guard<'_, Inner<T>, B> {
+        self.inner.lock()
+    }
+}
+
+/// Type alias for a `Revocable` with a `MutexBackend`.
+pub type RevocableMutex<T> = Revocable<T, super::lock::mutex::MutexBackend>;
-- 
2.44.0

Re: [PATCH 6/8] rust: add revocable mutex

[Greg KH]

On Mon, Mar 25, 2024 at 06:49:10PM +0100, Danilo Krummrich wrote:
> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> 
> This is a mutex where access to its contents can be revoked at runtime.
> 
> This is different from `Revocable` in a few ways:
>   1. The caller may sleep while holding a guard.
>   2. Accessors are all serialised.
>   3. Accessing is not as cheap (because it involves acquiring the mutex).
>   4. The size of the object is larger (because it involves a mutex +
>      flag).
> 
> An example of where this a good fit to be used in device state that
> holds registrations to other subsystems.

Why don't we need this in .c files if it is so needed in drivers today?

thanks,

greg k-h

Re: [PATCH 6/8] rust: add revocable mutex

[Danilo Krummrich]

On Mon, Mar 25, 2024 at 07:22:15PM +0100, Greg KH wrote:
> On Mon, Mar 25, 2024 at 06:49:10PM +0100, Danilo Krummrich wrote:
> > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > 
> > This is a mutex where access to its contents can be revoked at runtime.
> > 
> > This is different from `Revocable` in a few ways:
> >   1. The caller may sleep while holding a guard.
> >   2. Accessors are all serialised.
> >   3. Accessing is not as cheap (because it involves acquiring the mutex).
> >   4. The size of the object is larger (because it involves a mutex +
> >      flag).
> > 
> > An example of where this a good fit to be used in device state that
> > holds registrations to other subsystems.
> 
> Why don't we need this in .c files if it is so needed in drivers today?

You mean revoking access to a resource in general? 

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH 6/8] rust: add revocable mutex

[Greg KH]

On Tue, Mar 26, 2024 at 07:13:13PM +0100, Danilo Krummrich wrote:
> On Mon, Mar 25, 2024 at 07:22:15PM +0100, Greg KH wrote:
> > On Mon, Mar 25, 2024 at 06:49:10PM +0100, Danilo Krummrich wrote:
> > > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > 
> > > This is a mutex where access to its contents can be revoked at runtime.
> > > 
> > > This is different from `Revocable` in a few ways:
> > >   1. The caller may sleep while holding a guard.
> > >   2. Accessors are all serialised.
> > >   3. Accessing is not as cheap (because it involves acquiring the mutex).
> > >   4. The size of the object is larger (because it involves a mutex +
> > >      flag).
> > > 
> > > An example of where this a good fit to be used in device state that
> > > holds registrations to other subsystems.
> > 
> > Why don't we need this in .c files if it is so needed in drivers today?
> 
> You mean revoking access to a resource in general? 

I mean "having a mutex that can be revokable" as you describe here.  Why
is a driver written in rust unique in needing this?

thanks,

greg k-h

Re: [PATCH 6/8] rust: add revocable mutex

[Danilo Krummrich]

On Tue, Mar 26, 2024 at 07:17:59PM +0100, Greg KH wrote:
> On Tue, Mar 26, 2024 at 07:13:13PM +0100, Danilo Krummrich wrote:
> > On Mon, Mar 25, 2024 at 07:22:15PM +0100, Greg KH wrote:
> > > On Mon, Mar 25, 2024 at 06:49:10PM +0100, Danilo Krummrich wrote:
> > > > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > > 
> > > > This is a mutex where access to its contents can be revoked at runtime.
> > > > 
> > > > This is different from `Revocable` in a few ways:
> > > >   1. The caller may sleep while holding a guard.
> > > >   2. Accessors are all serialised.
> > > >   3. Accessing is not as cheap (because it involves acquiring the mutex).
> > > >   4. The size of the object is larger (because it involves a mutex +
> > > >      flag).
> > > > 
> > > > An example of where this a good fit to be used in device state that
> > > > holds registrations to other subsystems.
> > > 
> > > Why don't we need this in .c files if it is so needed in drivers today?
> > 
> > You mean revoking access to a resource in general? 
> 
> I mean "having a mutex that can be revokable" as you describe here.  Why
> is a driver written in rust unique in needing this?

Let's go back to the idea of a Revocable<>. As already mentioned the idea is to
make sure a driver can not access a ressource after it has been gone, e.g. a
pointer to an MMIO mapping after it has been unmapped (e.g. by devres). To
synchronize access, Revocable<> uses RCU; we need to wait to revoke the resource
until a potential access by the driver has been finished.

RevocableMutex does the same thing, but uses a Mutex instead of RCU, which leads
to the differences listed above.

As for "why does only Rust need this", I don't think it does. Conceptually,
there are quite similar approaches on the C side. As already mentioned DRM uses
drm_dev_unplug() and drm_dev_{enter,exit}(). There may be other subsystem
specific C implementations that do something similar I don't know about.

I think one difference might be that in C we can sometimes get away with just
knowing that something can't happen and hence isn't protected, whereas in Rust
we want / have to give the safety guarantee no matter what.

> 
> thanks,
> 
> greg k-h
> 

[PATCH 7/8] rust: add revocable objects

[Danilo Krummrich]

From: Wedson Almeida Filho <wedsonaf@gmail.com>

This implements the Revocable and AsyncRevocable types.

Revocable allows access to objects to be safely revoked at run time.

This is useful, for example, for resources allocated during device probe;
when the device is removed, the driver should stop accessing the device
resources even if other state is kept in memory due to existing
references (i.e., device context data is ref-counted and has a non-zero
refcount after removal of the device).

AsyncRevocable allows access to objects to be revoked without having to
wait for existing users to complete. This will be used to drop futures
in tasks when executors are being torn down.

Co-developed-by: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/lib.rs       |   1 +
 rust/kernel/revocable.rs | 438 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 439 insertions(+)
 create mode 100644 rust/kernel/revocable.rs

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e5a0a97acaf0..89b31a46ae92 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -46,6 +46,7 @@
 pub mod net;
 pub mod prelude;
 pub mod print;
+pub mod revocable;
 mod static_assert;
 #[doc(hidden)]
 pub mod std_vendor;
diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
new file mode 100644
index 000000000000..de23b9f38c4e
--- /dev/null
+++ b/rust/kernel/revocable.rs
@@ -0,0 +1,438 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Revocable objects.
+//!
+//! The [`Revocable`] type wraps other types and allows access to them to be revoked. The existence
+//! of a [`RevocableGuard`] ensures that objects remain valid.
+
+use crate::{
+    bindings,
+    init::{self},
+    prelude::*,
+    sync::rcu,
+};
+use core::{
+    cell::UnsafeCell,
+    marker::PhantomData,
+    mem::MaybeUninit,
+    ops::Deref,
+    ptr::drop_in_place,
+    sync::atomic::{fence, AtomicBool, AtomicU32, Ordering},
+};
+
+/// An object that can become inaccessible at runtime.
+///
+/// Once access is revoked and all concurrent users complete (i.e., all existing instances of
+/// [`RevocableGuard`] are dropped), the wrapped object is also dropped.
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::revocable::Revocable;
+///
+/// struct Example {
+///     a: u32,
+///     b: u32,
+/// }
+///
+/// fn add_two(v: &Revocable<Example>) -> Option<u32> {
+///     let guard = v.try_access()?;
+///     Some(guard.a + guard.b)
+/// }
+///
+/// let v = Revocable::new(Example { a: 10, b: 20 });
+/// assert_eq!(add_two(&v), Some(30));
+/// v.revoke();
+/// assert_eq!(add_two(&v), None);
+/// ```
+///
+/// Sample example as above, but explicitly using the rcu read side lock.
+///
+/// ```
+/// # use kernel::revocable::Revocable;
+/// use kernel::sync::rcu;
+///
+/// struct Example {
+///     a: u32,
+///     b: u32,
+/// }
+///
+/// fn add_two(v: &Revocable<Example>) -> Option<u32> {
+///     let guard = rcu::read_lock();
+///     let e = v.try_access_with_guard(&guard)?;
+///     Some(e.a + e.b)
+/// }
+///
+/// let v = Revocable::new(Example { a: 10, b: 20 });
+/// assert_eq!(add_two(&v), Some(30));
+/// v.revoke();
+/// assert_eq!(add_two(&v), None);
+/// ```
+#[pin_data(PinnedDrop)]
+pub struct Revocable<T> {
+    is_available: AtomicBool,
+    #[pin]
+    data: MaybeUninit<UnsafeCell<T>>,
+}
+
+// SAFETY: `Revocable` is `Send` if the wrapped object is also `Send`. This is because while the
+// functionality exposed by `Revocable` can be accessed from any thread/CPU, it is possible that
+// this isn't supported by the wrapped object.
+unsafe impl<T: Send> Send for Revocable<T> {}
+
+// SAFETY: `Revocable` is `Sync` if the wrapped object is both `Send` and `Sync`. We require `Send`
+// from the wrapped object as well because  of `Revocable::revoke`, which can trigger the `Drop`
+// implementation of the wrapped object from an arbitrary thread.
+unsafe impl<T: Sync + Send> Sync for Revocable<T> {}
+
+impl<T> Revocable<T> {
+    /// Creates a new revocable instance of the given data.
+    pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
+        pin_init!(Self {
+            is_available: AtomicBool::new(true),
+            data <- unsafe { init::pin_init_from_closure(move |slot: *mut MaybeUninit<UnsafeCell<T>>| {
+                init::PinInit::<T, core::convert::Infallible>::__pinned_init(data, slot as *mut T)?;
+                Ok::<(), core::convert::Infallible>(())
+            })},
+        })
+    }
+
+    /// Tries to access the \[revocable\] wrapped object.
+    ///
+    /// Returns `None` if the object has been revoked and is therefore no longer accessible.
+    ///
+    /// Returns a guard that gives access to the object otherwise; the object is guaranteed to
+    /// remain accessible while the guard is alive. In such cases, callers are not allowed to sleep
+    /// because another CPU may be waiting to complete the revocation of this object.
+    pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
+        let guard = rcu::read_lock();
+        if self.is_available.load(Ordering::Relaxed) {
+            // SAFETY: Since `self.is_available` is true, data is initialised and has to remain
+            // valid because the RCU read side lock prevents it from being dropped.
+            Some(unsafe { RevocableGuard::new(self.data.assume_init_ref().get(), guard) })
+        } else {
+            None
+        }
+    }
+
+    /// Tries to access the \[revocable\] wrapped object.
+    ///
+    /// Returns `None` if the object has been revoked and is therefore no longer accessible.
+    ///
+    /// Returns a shared reference to the object otherwise; the object is guaranteed to
+    /// remain accessible while the rcu read side guard is alive. In such cases, callers are not
+    /// allowed to sleep because another CPU may be waiting to complete the revocation of this
+    /// object.
+    pub fn try_access_with_guard<'a>(&'a self, _guard: &'a rcu::Guard) -> Option<&'a T> {
+        if self.is_available.load(Ordering::Relaxed) {
+            // SAFETY: Since `self.is_available` is true, data is initialised and has to remain
+            // valid because the RCU read side lock prevents it from being dropped.
+            Some(unsafe { &*self.data.assume_init_ref().get() })
+        } else {
+            None
+        }
+    }
+
+    /// Revokes access to and drops the wrapped object.
+    ///
+    /// Access to the object is revoked immediately to new callers of [`Revocable::try_access`]. If
+    /// there are concurrent users of the object (i.e., ones that called [`Revocable::try_access`]
+    /// beforehand and still haven't dropped the returned guard), this function waits for the
+    /// concurrent access to complete before dropping the wrapped object.
+    pub fn revoke(&self) {
+        if self
+            .is_available
+            .compare_exchange(true, false, Ordering::Relaxed, Ordering::Relaxed)
+            .is_ok()
+        {
+            // SAFETY: Just an FFI call, there are no further requirements.
+            unsafe { bindings::synchronize_rcu() };
+
+            // SAFETY: We know `self.data` is valid because only one CPU can succeed the
+            // `compare_exchange` above that takes `is_available` from `true` to `false`.
+            unsafe { drop_in_place(self.data.assume_init_ref().get()) };
+        }
+    }
+}
+
+#[pinned_drop]
+impl<T> PinnedDrop for Revocable<T> {
+    fn drop(self: Pin<&mut Self>) {
+        // Drop only if the data hasn't been revoked yet (in which case it has already been
+        // dropped).
+        // SAFETY: We are not moving out of `p`, only dropping in place
+        let p = unsafe { self.get_unchecked_mut() };
+        if *p.is_available.get_mut() {
+            // SAFETY: We know `self.data` is valid because no other CPU has changed
+            // `is_available` to `false` yet, and no other CPU can do it anymore because this CPU
+            // holds the only reference (mutable) to `self` now.
+            unsafe { drop_in_place(p.data.assume_init_ref().get()) };
+        }
+    }
+}
+
+/// A guard that allows access to a revocable object and keeps it alive.
+///
+/// CPUs may not sleep while holding on to [`RevocableGuard`] because it's in atomic context
+/// holding the RCU read-side lock.
+///
+/// # Invariants
+///
+/// The RCU read-side lock is held while the guard is alive.
+pub struct RevocableGuard<'a, T> {
+    data_ref: *const T,
+    _rcu_guard: rcu::Guard,
+    _p: PhantomData<&'a ()>,
+}
+
+impl<T> RevocableGuard<'_, T> {
+    fn new(data_ref: *const T, rcu_guard: rcu::Guard) -> Self {
+        Self {
+            data_ref,
+            _rcu_guard: rcu_guard,
+            _p: PhantomData,
+        }
+    }
+}
+
+impl<T> Deref for RevocableGuard<'_, T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: By the type invariants, we hold the rcu read-side lock, so the object is
+        // guaranteed to remain valid.
+        unsafe { &*self.data_ref }
+    }
+}
+
+/// An object that can become inaccessible at runtime.
+///
+/// Once access is revoked and all concurrent users complete (i.e., all existing instances of
+/// [`AsyncRevocableGuard`] are dropped), the wrapped object is also dropped.
+///
+/// Unlike [`Revocable`], [`AsyncRevocable`] does not wait for concurrent users of the wrapped
+/// object to finish before [`AsyncRevocable::revoke`] completes -- thus the async qualifier. This
+/// has the advantage of not requiring RCU locks or waits of any kind.
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::revocable::AsyncRevocable;
+///
+/// struct Example {
+///     a: u32,
+///     b: u32,
+/// }
+///
+/// fn add_two(v: &AsyncRevocable<Example>) -> Option<u32> {
+///     let guard = v.try_access()?;
+///     Some(guard.a + guard.b)
+/// }
+///
+/// let v = AsyncRevocable::new(Example { a: 10, b: 20 });
+/// assert_eq!(add_two(&v), Some(30));
+/// v.revoke();
+/// assert_eq!(add_two(&v), None);
+/// ```
+///
+/// Example where revocation happens while there is a user:
+///
+/// ```
+/// # use kernel::revocable::AsyncRevocable;
+/// use core::sync::atomic::{AtomicBool, Ordering};
+///
+/// struct Example {
+///     a: u32,
+///     b: u32,
+/// }
+///
+/// static DROPPED: AtomicBool = AtomicBool::new(false);
+///
+/// impl Drop for Example {
+///     fn drop(&mut self) {
+///         DROPPED.store(true, Ordering::Relaxed);
+///     }
+/// }
+///
+/// fn add_two(v: &AsyncRevocable<Example>) -> Option<u32> {
+///     let guard = v.try_access()?;
+///     Some(guard.a + guard.b)
+/// }
+///
+/// let v = AsyncRevocable::new(Example { a: 10, b: 20 });
+/// assert_eq!(add_two(&v), Some(30));
+///
+/// let guard = v.try_access().unwrap();
+/// assert!(!v.is_revoked());
+/// assert!(!DROPPED.load(Ordering::Relaxed));
+/// v.revoke();
+/// assert!(!DROPPED.load(Ordering::Relaxed));
+/// assert!(v.is_revoked());
+/// assert!(v.try_access().is_none());
+/// assert_eq!(guard.a + guard.b, 30);
+/// drop(guard);
+/// assert!(DROPPED.load(Ordering::Relaxed));
+/// ```
+pub struct AsyncRevocable<T> {
+    usage_count: AtomicU32,
+    data: MaybeUninit<UnsafeCell<T>>,
+}
+
+// SAFETY: `AsyncRevocable` is `Send` if the wrapped object is also `Send`. This is because while
+// the functionality exposed by `AsyncRevocable` can be accessed from any thread/CPU, it is
+// possible that this isn't supported by the wrapped object.
+unsafe impl<T: Send> Send for AsyncRevocable<T> {}
+
+// SAFETY: `AsyncRevocable` is `Sync` if the wrapped object is both `Send` and `Sync`. We require
+// `Send` from the wrapped object as well because  of `AsyncRevocable::revoke`, which can trigger
+// the `Drop` implementation of the wrapped object from an arbitrary thread.
+unsafe impl<T: Sync + Send> Sync for AsyncRevocable<T> {}
+
+const REVOKED: u32 = 0x80000000;
+const COUNT_MASK: u32 = !REVOKED;
+const SATURATED_COUNT: u32 = REVOKED - 1;
+
+impl<T> AsyncRevocable<T> {
+    /// Creates a new asynchronously revocable instance of the given data.
+    pub fn new(data: T) -> Self {
+        Self {
+            usage_count: AtomicU32::new(0),
+            data: MaybeUninit::new(UnsafeCell::new(data)),
+        }
+    }
+
+    /// Tries to access the \[revocable\] wrapped object.
+    ///
+    /// Returns `None` if the object has been revoked and is therefore no longer accessible.
+    ///
+    /// Returns a guard that gives access to the object otherwise; the object is guaranteed to
+    /// remain accessible while the guard is alive.
+    pub fn try_access(&self) -> Option<AsyncRevocableGuard<'_, T>> {
+        loop {
+            let count = self.usage_count.load(Ordering::Relaxed);
+
+            // Fail attempt to access if the object is already revoked.
+            if count & REVOKED != 0 {
+                return None;
+            }
+
+            // No need to increment if the count is saturated.
+            if count == SATURATED_COUNT
+                || self
+                    .usage_count
+                    .compare_exchange(count, count + 1, Ordering::Relaxed, Ordering::Relaxed)
+                    .is_ok()
+            {
+                return Some(AsyncRevocableGuard { revocable: self });
+            }
+        }
+    }
+
+    /// Revokes access to the protected object.
+    ///
+    /// Returns `true` if access has been revoked, or `false` when the object has already been
+    /// revoked by a previous call to [`AsyncRevocable::revoke`].
+    ///
+    /// This call is non-blocking, that is, no new users of the revocable object will be allowed,
+    /// but potential current users are able to continue to use it and the thread won't wait for
+    /// them to finish. In such cases, the object will be dropped when the last user completes.
+    pub fn revoke(&self) -> bool {
+        // Set the `REVOKED` bit.
+        //
+        // The acquire barrier matches up with the release when decrementing the usage count.
+        let prev = self.usage_count.fetch_or(REVOKED, Ordering::Acquire);
+        if prev & REVOKED != 0 {
+            // Another thread already revoked this object.
+            return false;
+        }
+
+        if prev == 0 {
+            // SAFETY: This thread just revoked the object and the usage count is zero, so the
+            // object is valid and there will be no future users.
+            unsafe { drop_in_place(UnsafeCell::raw_get(self.data.as_ptr())) };
+        }
+
+        true
+    }
+
+    /// Returns whether access to the object has been revoked.
+    pub fn is_revoked(&self) -> bool {
+        self.usage_count.load(Ordering::Relaxed) & REVOKED != 0
+    }
+}
+
+impl<T> Drop for AsyncRevocable<T> {
+    fn drop(&mut self) {
+        let count = *self.usage_count.get_mut();
+        if count != REVOKED {
+            // The object hasn't been dropped yet, so we do it now.
+
+            // This matches with the release when decrementing the usage count.
+            fence(Ordering::Acquire);
+
+            // SAFETY: Since `count` is does not indicate a count of 0 and the REVOKED bit set, the
+            // object is still valid.
+            unsafe { drop_in_place(UnsafeCell::raw_get(self.data.as_ptr())) };
+        }
+    }
+}
+
+/// A guard that allows access to a revocable object and keeps it alive.
+///
+/// # Invariants
+///
+/// The owner owns an increment on the usage count (which may have saturated it), which keeps the
+/// revocable object alive.
+pub struct AsyncRevocableGuard<'a, T> {
+    revocable: &'a AsyncRevocable<T>,
+}
+
+impl<T> Deref for AsyncRevocableGuard<'_, T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: The type invariants guarantee that the caller owns an increment.
+        unsafe { &*self.revocable.data.assume_init_ref().get() }
+    }
+}
+
+impl<T> Drop for AsyncRevocableGuard<'_, T> {
+    fn drop(&mut self) {
+        loop {
+            let count = self.revocable.usage_count.load(Ordering::Relaxed);
+            let actual_count = count & COUNT_MASK;
+            if actual_count == SATURATED_COUNT {
+                // The count is saturated, so we won't decrement (nor do we drop the object).
+                return;
+            }
+
+            if actual_count == 0 {
+                // Trying to underflow the count.
+                panic!("actual_count is zero");
+            }
+
+            // On success, we use release ordering, which matches with the acquire in one of the
+            // places where we drop the object, namely: below, in `AsyncRevocable::revoke`, or in
+            // `AsyncRevocable::drop`.
+            if self
+                .revocable
+                .usage_count
+                .compare_exchange(count, count - 1, Ordering::Release, Ordering::Relaxed)
+                .is_ok()
+            {
+                if count == 1 | REVOKED {
+                    // `count`  is now zero and it is revoked, so free it now.
+
+                    // This matches with the release above (which may have happened in other
+                    // threads concurrently).
+                    fence(Ordering::Acquire);
+
+                    // SAFETY: Since `count` was 1, the object is still alive.
+                    unsafe { drop_in_place(UnsafeCell::raw_get(self.revocable.data.as_ptr())) };
+                }
+
+                return;
+            }
+        }
+    }
+}
-- 
2.44.0

Re: [PATCH 7/8] rust: add revocable objects

[Greg KH]

On Mon, Mar 25, 2024 at 06:49:11PM +0100, Danilo Krummrich wrote:
> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> 
> This implements the Revocable and AsyncRevocable types.
> 
> Revocable allows access to objects to be safely revoked at run time.
> 
> This is useful, for example, for resources allocated during device probe;
> when the device is removed, the driver should stop accessing the device
> resources even if other state is kept in memory due to existing
> references (i.e., device context data is ref-counted and has a non-zero
> refcount after removal of the device).

Again, device removal is different from unbinding a driver from a
device.  Please fix this all up.

greg k-h

Re: [PATCH 7/8] rust: add revocable objects

[Danilo Krummrich]

On Mon, Mar 25, 2024 at 07:21:32PM +0100, Greg KH wrote:
> On Mon, Mar 25, 2024 at 06:49:11PM +0100, Danilo Krummrich wrote:
> > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > 
> > This implements the Revocable and AsyncRevocable types.
> > 
> > Revocable allows access to objects to be safely revoked at run time.
> > 
> > This is useful, for example, for resources allocated during device probe;
> > when the device is removed, the driver should stop accessing the device
> > resources even if other state is kept in memory due to existing
> > references (i.e., device context data is ref-counted and has a non-zero
> > refcount after removal of the device).
> 
> Again, device removal is different from unbinding a driver from a
> device.  Please fix this all up.

I think this isn't an "again", "[PATCH 8/8] rust: add device::Data" actually
phrased things differently (which indeed isn't great and needs to be fixed).

This comment indeed implies that device removal actually means unbinding a
driver from a device. I think "device remove" is meant here as the drivers
remove() callback is called.

Your comment seems to imply you have a different understanding of what "device
remove" means?

> 
> greg k-h
> 

Re: [PATCH 7/8] rust: add revocable objects

[Greg KH]

On Tue, Mar 26, 2024 at 06:07:36PM +0100, Danilo Krummrich wrote:
> On Mon, Mar 25, 2024 at 07:21:32PM +0100, Greg KH wrote:
> > On Mon, Mar 25, 2024 at 06:49:11PM +0100, Danilo Krummrich wrote:
> > > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > 
> > > This implements the Revocable and AsyncRevocable types.
> > > 
> > > Revocable allows access to objects to be safely revoked at run time.
> > > 
> > > This is useful, for example, for resources allocated during device probe;
> > > when the device is removed, the driver should stop accessing the device
> > > resources even if other state is kept in memory due to existing
> > > references (i.e., device context data is ref-counted and has a non-zero
> > > refcount after removal of the device).
> > 
> > Again, device removal is different from unbinding a driver from a
> > device.  Please fix this all up.
> 
> I think this isn't an "again", "[PATCH 8/8] rust: add device::Data" actually
> phrased things differently (which indeed isn't great and needs to be fixed).
> 
> This comment indeed implies that device removal actually means unbinding a
> driver from a device. I think "device remove" is meant here as the drivers
> remove() callback is called.
> 
> Your comment seems to imply you have a different understanding of what "device
> remove" means?

Yes, see my other comments.  A device has a lifespan that exceeds that
of a driver, and "remove" does not mean it is gone from the system just
because the remove() callback of a single driver is called.  It still
can, and does, live outside of a driver quite well, and in fact, the
driver is not in control of its lifespan at all!

All a driver can know is that it has a valid reference to a device for
the amount of time from probe() to remove().  Outside of that a driver
can not touch it, BUT the device structure lives on and can do lots of
other things that do not involve the driver at all.

thanks,

greg k-h

Re: [PATCH 7/8] rust: add revocable objects

[Danilo Krummrich]

On Tue, Mar 26, 2024 at 07:16:51PM +0100, Greg KH wrote:
> On Tue, Mar 26, 2024 at 06:07:36PM +0100, Danilo Krummrich wrote:
> > On Mon, Mar 25, 2024 at 07:21:32PM +0100, Greg KH wrote:
> > > On Mon, Mar 25, 2024 at 06:49:11PM +0100, Danilo Krummrich wrote:
> > > > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > > 
> > > > This implements the Revocable and AsyncRevocable types.
> > > > 
> > > > Revocable allows access to objects to be safely revoked at run time.
> > > > 
> > > > This is useful, for example, for resources allocated during device probe;
> > > > when the device is removed, the driver should stop accessing the device
> > > > resources even if other state is kept in memory due to existing
> > > > references (i.e., device context data is ref-counted and has a non-zero
> > > > refcount after removal of the device).
> > > 
> > > Again, device removal is different from unbinding a driver from a
> > > device.  Please fix this all up.
> > 
> > I think this isn't an "again", "[PATCH 8/8] rust: add device::Data" actually
> > phrased things differently (which indeed isn't great and needs to be fixed).
> > 
> > This comment indeed implies that device removal actually means unbinding a
> > driver from a device. I think "device remove" is meant here as the drivers
> > remove() callback is called.
> > 
> > Your comment seems to imply you have a different understanding of what "device
> > remove" means?
> 
> Yes, see my other comments.  A device has a lifespan that exceeds that
> of a driver, and "remove" does not mean it is gone from the system just
> because the remove() callback of a single driver is called.  It still
> can, and does, live outside of a driver quite well, and in fact, the
> driver is not in control of its lifespan at all!

Yes, that's clear. But what does "device remove" mean for you then?

I think this one here mostly seems to be about terminology and I want to make
sure I don't create any other misunderstandings e.g. by using "device remove"
for unbinding a driver from a device.

> 
> All a driver can know is that it has a valid reference to a device for
> the amount of time from probe() to remove().  Outside of that a driver
> can not touch it, BUT the device structure lives on and can do lots of
> other things that do not involve the driver at all.
> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH 7/8] rust: add revocable objects

[Wedson Almeida Filho]

On Mon, 25 Mar 2024 at 15:21, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Mar 25, 2024 at 06:49:11PM +0100, Danilo Krummrich wrote:
> > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> >
> > This implements the Revocable and AsyncRevocable types.
> >
> > Revocable allows access to objects to be safely revoked at run time.
> >
> > This is useful, for example, for resources allocated during device probe;
> > when the device is removed, the driver should stop accessing the device
> > resources even if other state is kept in memory due to existing
> > references (i.e., device context data is ref-counted and has a non-zero
> > refcount after removal of the device).
>
> Again, device removal is different from unbinding a driver from a
> device.  Please fix this all up.

I wrote the paragraph above.

When a driver registers with a bus, it provides (among other
callbacks) probe and *remove*: pci_driver::remove,
platform_driver::remove, amba::remove are examples of such callbacks.
Even the callback used by core is called remove,
device_driver::remove. And even buses that don't provide an explicit
remove callback still implement device_driver::remove (e.g., usb calls
its remove callback usb_unbind_device).

When a device that is bound to a driver is removed from the bus, the
driver's remove callback is called. When they're unbound manually (say
due to a write to the 'unbind' sysfs file), the driver's remove
callback is called.

So it's seems fair to say that from a driver's pov, it _doesn't_
matter why remove is called. Once it completes, drivers should not
touch resources related to that device anymore.

You seem to dislike the term but I don't see a good explanation for that.

Cheers,
-Wedson

[PATCH 8/8] rust: add device::Data

[Danilo Krummrich]

From: Wedson Almeida Filho <wedsonaf@gmail.com>

This allows access to registrations and io resources to be automatically
revoked when devices are removed, even if the ref count to their state
is non-zero.

Co-developed-by: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/device.rs | 120 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 119 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 7309a236f512..707c699c3090 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -4,7 +4,22 @@
 //!
 //! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
 
-use crate::{bindings, str::CStr};
+use macros::pin_data;
+
+use crate::{
+    bindings,
+    error::Result,
+    init::InPlaceInit,
+    init::PinInit,
+    pin_init,
+    revocable::{Revocable, RevocableGuard},
+    str::CStr,
+    sync::{LockClassKey, RevocableMutex, UniqueArc},
+};
+use core::{
+    ops::{Deref, DerefMut},
+    pin::Pin,
+};
 
 /// A raw device.
 ///
@@ -95,3 +110,106 @@ fn clone(&self) -> Self {
         Self::from_dev(self)
     }
 }
+
+/// Device data.
+///
+/// When a device is removed (for whatever reason, for example, because the device was unplugged or
+/// because the user decided to unbind the driver), the driver is given a chance to clean its state
+/// up, and all io resources should ideally not be used anymore.
+///
+/// However, the device data is reference-counted because other subsystems hold pointers to it. So
+/// some device state must be freed and not used anymore, while others must remain accessible.
+///
+/// This struct separates the device data into three categories:
+///   1. Registrations: are destroyed when the device is removed, but before the io resources
+///      become inaccessible.
+///   2. Io resources: are available until the device is removed.
+///   3. General data: remain available as long as the ref count is nonzero.
+///
+/// This struct implements the `DeviceRemoval` trait so that it can clean resources up even if not
+/// explicitly called by the device drivers.
+#[pin_data]
+pub struct Data<T, U, V> {
+    #[pin]
+    registrations: RevocableMutex<T>,
+    #[pin]
+    resources: Revocable<U>,
+    #[pin]
+    general: V,
+}
+
+/// Safely creates an new reference-counted instance of [`Data`].
+#[doc(hidden)]
+#[macro_export]
+macro_rules! new_device_data {
+    ($reg:expr, $res:expr, $gen:expr, $name:literal) => {{
+        static CLASS1: $crate::sync::LockClassKey = $crate::sync::LockClassKey::new();
+        let regs = $reg;
+        let res = $res;
+        let gen = $gen;
+        let name = $crate::c_str!($name);
+        $crate::device::Data::try_new(regs, res, gen, name, &CLASS1)
+    }};
+}
+
+impl<T, U, V> Data<T, U, V> {
+    /// Creates a new instance of `Data`.
+    ///
+    /// It is recommended that the [`new_device_data`] macro be used as it automatically creates
+    /// the lock classes.
+    pub fn try_new(
+        registrations: T,
+        resources: impl PinInit<U>,
+        general: impl PinInit<V>,
+        name: &'static CStr,
+        key1: &'static LockClassKey,
+    ) -> Result<Pin<UniqueArc<Self>>> {
+        let ret = UniqueArc::pin_init(pin_init!(Self {
+            registrations <- RevocableMutex::new(
+                registrations,
+                name,
+                key1,
+            ),
+            resources <- Revocable::new(resources),
+            general <- general,
+        }))?;
+
+        Ok(ret)
+    }
+
+    /// Returns the resources if they're still available.
+    pub fn resources(&self) -> Option<RevocableGuard<'_, U>> {
+        self.resources.try_access()
+    }
+
+    /// Returns the locked registrations if they're still available.
+    #[cfg(disabled)]
+    pub fn registrations(&self) -> Option<RevocableMutexGuard<'_, T>> {
+        self.registrations.try_write()
+    }
+}
+
+impl<T, U, V> crate::driver::DeviceRemoval for Data<T, U, V> {
+    fn device_remove(&self) {
+        // We revoke the registrations first so that resources are still available to them during
+        // unregistration.
+        self.registrations.revoke();
+
+        // Release resources now. General data remains available.
+        self.resources.revoke();
+    }
+}
+
+impl<T, U, V> Deref for Data<T, U, V> {
+    type Target = V;
+
+    fn deref(&self) -> &V {
+        &self.general
+    }
+}
+
+impl<T, U, V> DerefMut for Data<T, U, V> {
+    fn deref_mut(&mut self) -> &mut V {
+        &mut self.general
+    }
+}
-- 
2.44.0

Re: [PATCH 8/8] rust: add device::Data

[Greg KH]

On Mon, Mar 25, 2024 at 06:49:12PM +0100, Danilo Krummrich wrote:
> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> 
> This allows access to registrations and io resources to be automatically
> revoked when devices are removed, even if the ref count to their state
> is non-zero.

So it's re-implementing the devm_*() calls?  Why?

And this will trigger only on remove, but which remove?  The bus remove?
Or the unbinding of the driver to the device (two totally different
things, be specific and very careful here.)

If this is implementing the devm_() calls, why not call them the same
thing?


> 
> Co-developed-by: Andreas Hindborg <a.hindborg@samsung.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  rust/kernel/device.rs | 120 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 119 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 7309a236f512..707c699c3090 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -4,7 +4,22 @@
>  //!
>  //! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
>  
> -use crate::{bindings, str::CStr};
> +use macros::pin_data;
> +
> +use crate::{
> +    bindings,
> +    error::Result,
> +    init::InPlaceInit,
> +    init::PinInit,
> +    pin_init,
> +    revocable::{Revocable, RevocableGuard},
> +    str::CStr,
> +    sync::{LockClassKey, RevocableMutex, UniqueArc},
> +};
> +use core::{
> +    ops::{Deref, DerefMut},
> +    pin::Pin,
> +};
>  
>  /// A raw device.
>  ///
> @@ -95,3 +110,106 @@ fn clone(&self) -> Self {
>          Self::from_dev(self)
>      }
>  }
> +
> +/// Device data.
> +///
> +/// When a device is removed (for whatever reason, for example, because the device was unplugged or
> +/// because the user decided to unbind the driver), the driver is given a chance to clean its state
> +/// up, and all io resources should ideally not be used anymore.

Wait, unplugging a device and unbinding a device from a driver are two
totally different things, do NOT get them mixed up or assume that they
are the same thing at all please.  They have different lifetime rules
and different patterns of what happens.

So this needs to be taken out and rewritten from the beginning please.
If the comments describe something that is incorrect, I can't trust that
the code is correct...

thanks,

greg k-h

Re: [PATCH 8/8] rust: add device::Data

[Danilo Krummrich]

On Mon, Mar 25, 2024 at 07:21:04PM +0100, Greg KH wrote:
> On Mon, Mar 25, 2024 at 06:49:12PM +0100, Danilo Krummrich wrote:
> > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > 
> > This allows access to registrations and io resources to be automatically
> > revoked when devices are removed, even if the ref count to their state
> > is non-zero.
> 
> So it's re-implementing the devm_*() calls?  Why?

It's not. Taking IO resources as an example it's more a generalization of what
e.g. DRM solves with drm_dev_unplug() and drm_dev_{enter,exit}().

While devm_*() ensures to, for instance, iounmap() a mapping when a device is
detached, the Revocable<> resource ensures that a driver can't access the
pointer pointing to the just unmapped memory anymore.

So, this isn't something that replaces devm_*(), but kinda builds on top of it.
For instance, we could call Revocable::revoke() from a devm_*() callback.
However, and that' what this patch currently does for simplicity, we could also
just call it from the corresponding driver's remove() callback.

> 
> And this will trigger only on remove, but which remove?  The bus remove?
> Or the unbinding of the driver to the device (two totally different
> things, be specific and very careful here.)

The above comment says "device remove", maybe say device / driver detach
instead?

> 
> If this is implementing the devm_() calls, why not call them the same
> thing?
> 
> 
> > 
> > Co-developed-by: Andreas Hindborg <a.hindborg@samsung.com>
> > Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > ---
> >  rust/kernel/device.rs | 120 +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 119 insertions(+), 1 deletion(-)
> > 
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 7309a236f512..707c699c3090 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -4,7 +4,22 @@
> >  //!
> >  //! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
> >  
> > -use crate::{bindings, str::CStr};
> > +use macros::pin_data;
> > +
> > +use crate::{
> > +    bindings,
> > +    error::Result,
> > +    init::InPlaceInit,
> > +    init::PinInit,
> > +    pin_init,
> > +    revocable::{Revocable, RevocableGuard},
> > +    str::CStr,
> > +    sync::{LockClassKey, RevocableMutex, UniqueArc},
> > +};
> > +use core::{
> > +    ops::{Deref, DerefMut},
> > +    pin::Pin,
> > +};
> >  
> >  /// A raw device.
> >  ///
> > @@ -95,3 +110,106 @@ fn clone(&self) -> Self {
> >          Self::from_dev(self)
> >      }
> >  }
> > +
> > +/// Device data.
> > +///
> > +/// When a device is removed (for whatever reason, for example, because the device was unplugged or
> > +/// because the user decided to unbind the driver), the driver is given a chance to clean its state
> > +/// up, and all io resources should ideally not be used anymore.
> 
> Wait, unplugging a device and unbinding a device from a driver are two
> totally different things, do NOT get them mixed up or assume that they
> are the same thing at all please.  They have different lifetime rules
> and different patterns of what happens.

I think the comment does not claim that device unplug and device / driver
unbind are the same thing. I rather think in this context the expection is that,
ultimately, both result into the fact that the corresponding device resources
are not available anymore and hence shouldn't be used anymore. Where the latter
is enforced by using Revocable<>.

> 
> So this needs to be taken out and rewritten from the beginning please.
> If the comments describe something that is incorrect, I can't trust that
> the code is correct...

Not sure the comment is actually claiming something incorrect. If, with the
above explanation, you still think so, please let me know how to phrase it
correctly, such that I can improve this patch accordingly.

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH 8/8] rust: add device::Data

[Greg KH]

On Tue, Mar 26, 2024 at 05:54:19PM +0100, Danilo Krummrich wrote:
> On Mon, Mar 25, 2024 at 07:21:04PM +0100, Greg KH wrote:
> > On Mon, Mar 25, 2024 at 06:49:12PM +0100, Danilo Krummrich wrote:
> > > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > 
> > > This allows access to registrations and io resources to be automatically
> > > revoked when devices are removed, even if the ref count to their state
> > > is non-zero.
> > 
> > So it's re-implementing the devm_*() calls?  Why?
> 
> It's not. Taking IO resources as an example it's more a generalization of what
> e.g. DRM solves with drm_dev_unplug() and drm_dev_{enter,exit}().

That is a drm specific way to handle devm_* stuff, right?

If that is to be "baked" into different bus types, great, let's make it
a bus-specific thing and put it in the driver core, but don't make it
something that is tied to rust devices only as that feels odd, right?

Also, work with the patterns we have, don't create new ones as that just
causes confusion for all of us working on this part of the kernel.

> While devm_*() ensures to, for instance, iounmap() a mapping when a device is
> detached, the Revocable<> resource ensures that a driver can't access the
> pointer pointing to the just unmapped memory anymore.

Great, this is something that we have been asking for in a generic way
for all drivers/devices in the kernel (see the many talks over the past
years at conferences about it.)  Please work with the developers who are
doing this in the .c side to be sure that things are aligned properly to
work the same.

> So, this isn't something that replaces devm_*(), but kinda builds on top of it.

Perhaps document it that way?

> For instance, we could call Revocable::revoke() from a devm_*() callback.
> However, and that' what this patch currently does for simplicity, we could also
> just call it from the corresponding driver's remove() callback.

If that's the proper place for that, yes.  It could be the proper place
is in the close() system call as well, right?  But again, document it to
explain what this is please, as it was not obvious at all.

> > And this will trigger only on remove, but which remove?  The bus remove?
> > Or the unbinding of the driver to the device (two totally different
> > things, be specific and very careful here.)
> 
> The above comment says "device remove", maybe say device / driver detach
> instead?

It depends on what you mean.  Those are different things, as explained
in other places.  Where do you want this to happen?  Where should it
happen?  And why in only those places?

> > > +/// Device data.
> > > +///
> > > +/// When a device is removed (for whatever reason, for example, because the device was unplugged or
> > > +/// because the user decided to unbind the driver), the driver is given a chance to clean its state
> > > +/// up, and all io resources should ideally not be used anymore.
> > 
> > Wait, unplugging a device and unbinding a device from a driver are two
> > totally different things, do NOT get them mixed up or assume that they
> > are the same thing at all please.  They have different lifetime rules
> > and different patterns of what happens.
> 
> I think the comment does not claim that device unplug and device / driver
> unbind are the same thing. I rather think in this context the expection is that,
> ultimately, both result into the fact that the corresponding device resources
> are not available anymore and hence shouldn't be used anymore. Where the latter
> is enforced by using Revocable<>.

But those are different things happening, and different things happen to
the objects when those different things happen, so while a driver might
seem to see the same thing happen from its point of view, the lifespan
of the object itself is VERY different here (could be passed back to the
driver, could be freed, could be sent to a different driver, could just
be ignored for a long time, etc.)

So be specific as to what viewpoint you are considering here, a device
object has a lifespan that greatly exceeds that of just the window where
a driver happens to see it :)

> > So this needs to be taken out and rewritten from the beginning please.
> > If the comments describe something that is incorrect, I can't trust that
> > the code is correct...
> 
> Not sure the comment is actually claiming something incorrect. If, with the
> above explanation, you still think so, please let me know how to phrase it
> correctly, such that I can improve this patch accordingly.

Please step back and try to determine what you want here.  How devices
are created, assigned, reassigned, removed, renamed, destroyed, etc.
They have a well known "phase of life" and one that different parts of
the kernel sees in different ways (driver core, busses, classes,
drivers, userspace, etc.)  Be aware of what you are wanting to do here,
and who is acting on the structure where and what they are wanting to do
with it.

thanks,

greg k-h

Re: [PATCH 8/8] rust: add device::Data

[Danilo Krummrich]

On Tue, Mar 26, 2024 at 07:12:29PM +0100, Greg KH wrote:
> On Tue, Mar 26, 2024 at 05:54:19PM +0100, Danilo Krummrich wrote:
> > On Mon, Mar 25, 2024 at 07:21:04PM +0100, Greg KH wrote:
> > > On Mon, Mar 25, 2024 at 06:49:12PM +0100, Danilo Krummrich wrote:
> > > > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > > 
> > > > This allows access to registrations and io resources to be automatically
> > > > revoked when devices are removed, even if the ref count to their state
> > > > is non-zero.
> > > 
> > > So it's re-implementing the devm_*() calls?  Why?
> > 
> > It's not. Taking IO resources as an example it's more a generalization of what
> > e.g. DRM solves with drm_dev_unplug() and drm_dev_{enter,exit}().
> 
> That is a drm specific way to handle devm_* stuff, right?

Depends, if you mean it's a DRM specific way to ensure devm_* managed resources
can't be accessed anymore after the device is "gone" from the perspective of
the driver, then yes. If you confuse it with drmm_* stuff, which ties resources
to the actual lifetime of a struct drm_device (device is freed), then no.

It's basically the DRM way to implement Revocable<>. drm_dev_unplug() marks a
struct drm_device as unplugged (protected by SRCU). drm_dev_{enter,exit}() are
used as conditional entry / exit points of code sections that access resources
that might have been released by devm_* stuff already, such as IO mappings.

> 
> If that is to be "baked" into different bus types, great, let's make it
> a bus-specific thing and put it in the driver core, but don't make it
> something that is tied to rust devices only as that feels odd, right?

You lost me a bit on this one. Can you please clarify?

> 
> Also, work with the patterns we have, don't create new ones as that just
> causes confusion for all of us working on this part of the kernel.
> 
> > While devm_*() ensures to, for instance, iounmap() a mapping when a device is
> > detached, the Revocable<> resource ensures that a driver can't access the
> > pointer pointing to the just unmapped memory anymore.
> 
> Great, this is something that we have been asking for in a generic way
> for all drivers/devices in the kernel (see the many talks over the past
> years at conferences about it.)  Please work with the developers who are
> doing this in the .c side to be sure that things are aligned properly to
> work the same.

Unfortunately, I'm neither aware of those efforts, nor the talks. Mind providing
me some pointers?

> 
> > So, this isn't something that replaces devm_*(), but kinda builds on top of it.
> 
> Perhaps document it that way?

Sure, I can add this example.

> 
> > For instance, we could call Revocable::revoke() from a devm_*() callback.
> > However, and that' what this patch currently does for simplicity, we could also
> > just call it from the corresponding driver's remove() callback.
> 
> If that's the proper place for that, yes.  It could be the proper place
> is in the close() system call as well, right?  But again, document it to
> explain what this is please, as it was not obvious at all.

Correct, it depends on what you use Revocable<> for, device resources is just
one example where this is useful.

> 
> > > And this will trigger only on remove, but which remove?  The bus remove?
> > > Or the unbinding of the driver to the device (two totally different
> > > things, be specific and very careful here.)
> > 
> > The above comment says "device remove", maybe say device / driver detach
> > instead?
> 
> It depends on what you mean.  Those are different things, as explained
> in other places.  Where do you want this to happen?  Where should it
> happen?  And why in only those places?

For device resources, device / driver detach should be the correct place. Agree?

> 
> > > > +/// Device data.
> > > > +///
> > > > +/// When a device is removed (for whatever reason, for example, because the device was unplugged or
> > > > +/// because the user decided to unbind the driver), the driver is given a chance to clean its state
> > > > +/// up, and all io resources should ideally not be used anymore.
> > > 
> > > Wait, unplugging a device and unbinding a device from a driver are two
> > > totally different things, do NOT get them mixed up or assume that they
> > > are the same thing at all please.  They have different lifetime rules
> > > and different patterns of what happens.
> > 
> > I think the comment does not claim that device unplug and device / driver
> > unbind are the same thing. I rather think in this context the expection is that,
> > ultimately, both result into the fact that the corresponding device resources
> > are not available anymore and hence shouldn't be used anymore. Where the latter
> > is enforced by using Revocable<>.
> 
> But those are different things happening, and different things happen to
> the objects when those different things happen, so while a driver might
> seem to see the same thing happen from its point of view, the lifespan
> of the object itself is VERY different here (could be passed back to the
> driver, could be freed, could be sent to a different driver, could just
> be ignored for a long time, etc.)
> 
> So be specific as to what viewpoint you are considering here, a device
> object has a lifespan that greatly exceeds that of just the window where
> a driver happens to see it :)

Fully agree, this is indeed from the viewpoint of the driver. We don't really
care what is happening to the device after detach. We only care about all the
cases above lead to a detach, which is the point where we have to make sure
we're not accessing any device resources anymore.

I will try to make this more clear.

> 
> > > So this needs to be taken out and rewritten from the beginning please.
> > > If the comments describe something that is incorrect, I can't trust that
> > > the code is correct...
> > 
> > Not sure the comment is actually claiming something incorrect. If, with the
> > above explanation, you still think so, please let me know how to phrase it
> > correctly, such that I can improve this patch accordingly.
> 
> Please step back and try to determine what you want here.  How devices
> are created, assigned, reassigned, removed, renamed, destroyed, etc.
> They have a well known "phase of life" and one that different parts of
> the kernel sees in different ways (driver core, busses, classes,
> drivers, userspace, etc.)  Be aware of what you are wanting to do here,
> and who is acting on the structure where and what they are wanting to do
> with it.
> 
> thanks,
> 
> greg k-h
>