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

[Greg KH <gregkh@linuxfoundation.org>]

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