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

[Danilo Krummrich <dakr@redhat.com>]

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
>