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

[Danilo Krummrich <dakr@redhat.com>]

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
>