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

[Philipp Stanner <pstanner@redhat.com>]

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
>