[5/5] rust: device: Add a stub abstraction for devices

  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.

Lina: Rewrote commit message, dropped the Amba bits, and squashed in
simple changes to the core Device code from latter commits in
rust-for-linux/rust. Also include the rust_helper_dev_get_drvdata
helper which will be needed by consumers later on anyway.

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: Asahi Lina <lina@asahilina.net>
---
 rust/helpers.c        | 13 +++++++++
 rust/kernel/device.rs | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 88 insertions(+), 1 deletion(-)
  

On Fri, Feb 24, 2023 at 07:53:17PM +0900, Asahi Lina 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.

What good is just the device name?  I'm all for proper bindings to hook
up properly to the driver core, but this feels like it's not really
doing much of anything.

Do you have a real user that we can see how this is interacting?

And what does a driver care about the device name anyway?  It should
only be using the dev_*() calls to print that info out to the log, and
never messing around with it in any other format as that's what
userspace expects.

> Lina: Rewrote commit message, dropped the Amba bits, and squashed in
> simple changes to the core Device code from latter commits in
> rust-for-linux/rust. Also include the rust_helper_dev_get_drvdata
> helper which will be needed by consumers later on anyway.
> 
> 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: Asahi Lina <lina@asahilina.net>
> ---
>  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 04b9be46e887..54954fd80c77 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -20,6 +20,7 @@
>  
>  #include <linux/bug.h>
>  #include <linux/build_bug.h>
> +#include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/refcount.h>
>  
> @@ -65,6 +66,18 @@ long rust_helper_PTR_ERR(__force const void *ptr)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR);
>  
> +void *rust_helper_dev_get_drvdata(struct device *dev)
> +{
> +	return dev_get_drvdata(dev);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata);

No matching dev_set_drvdata()?  What good is getting a random void
pointer if you couldn't set it in the first place?  :)

> +const char *rust_helper_dev_name(const struct device *dev)
> +{
> +	return dev_name(dev);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_dev_name);

Again, why?  What is going to use this?

And I don't really understand the rules you are putting on the name
string after calling this, more below:

> +
>  /*
>   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
>   * as the Rust `usize` type, so we can use it in contexts where Rust
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 9be021e393ca..e57da622d817 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 @@ use crate::bindings;
>  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) }

Why can the device never be renamed?  Devices are renamed all the time,
sometimes when you least expect it (i.e. by userspace).  So how is this
considered "safe"? and actually correct?

Again, maybe seeing a real user of this might make more sense, but
as-is, this feels wrong and not needed at all.


> +    }
> +}
> +
> +/// 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) };

You don't check the return value of get_device()?  What if it failed
(hint, it can)?


> +        // 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 {

I am a rust newbie, but I don't understand this "RawDevice" here at all.


> +        // 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
> +    }

What does this raw device do?  What is the relationship to a "real"
device?  Maybe it's just my lack of rust knowledge here showing, so any
hints would be appreciated.

> +}
> +
> +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) };

So it is now ok for this to be freed?

One meta-comment, why is any of this needed at all?  No driver should
ever be dealing with a "raw" struct device pointer at all, right?  They
should be calling into subsystems that give it a pointer to a
bus-specific pointer type (gpio, usb, pci, etc.)

So I'm thinking that adding support for "raw" struct device pointers
feels ripe for abuse in a "the code should not be doing that" type of
thing.

Unless you are writing a new bus/class in rust?

thanks,

greg k-h
  

Hi!

First, I should say that some of this is really an example, and this
abstraction doesn't need to go in as part of this series. I just added
it at the end as an example of how RawDevice could/will be implemented,
but there is more discussion that needs to happen around Devices
(especially around how actual driver implementations and device data
stuff work) I think. The main goal of having the RawDevice trait now is
that we can start reviewing and upstreaming things that aren't part of
the device model but take device references. Otherwise we end up
serializing everything too much and it will be difficult to get
everything upstream in a reasonable timeframe...

Also note that I didn't write any of this code so if you have questions
about *why* it was all designed like this, I think Wedson will have to
answer that ^^

On 2023/02/24 20:19, Greg Kroah-Hartman wrote:
> On Fri, Feb 24, 2023 at 07:53:17PM +0900, Asahi Lina 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.
> 
> What good is just the device name?  I'm all for proper bindings to hook
> up properly to the driver core, but this feels like it's not really
> doing much of anything.

I think we can just drop this, I don't use it. It was just an example of
how a RawDevice method might work that is in the downstream tree, and I
kept it here.

A more practical example would be `of_node()`, which returns the OF node
associated with a device and is how the OF abstraction I wrote hooks
into the device model. I think I can submit that one pretty soon too, if
I'm not mistaken.

>> +void *rust_helper_dev_get_drvdata(struct device *dev)
>> +{
>> +	return dev_get_drvdata(dev);
>> +}
>> +EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata);
> 
> No matching dev_set_drvdata()?  What good is getting a random void
> pointer if you couldn't set it in the first place?  :)

I thought something else used this, but looking again the bus drivers
just use stuff like platform_{set,get}_drvdata. I'll drop it, I'm not
sure why I thought I needed this later...

>> +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) };
> 
> You don't check the return value of get_device()?  What if it failed
> (hint, it can)?

Really? I looked at the implementation and I don't see how it can fail,
as long as the argument is non-NULL and valid... (which is a
precondition of this unsafe function). Did I miss something?

>> +        // 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 {
> 
> I am a rust newbie, but I don't understand this "RawDevice" here at all.

RawDevice is a trait, so this means "a dynamic reference to any type
that implements RawDevice". That could be a reference to a bus device
outright (like a PlatformDevice), or another Device, or a type-erased
dynamic object that is "some device type" but you don't know which (like
a reference taken from a Box<dyn RawDevice>).

What this means is that you can always create a Device from a reference
to anything that is a device (a RawDevice impl). This code path does a
get_device(), so it creates a new reference that becomes logically owned
by the Device object.

Think of it like "clone, and erase the specific device type to a generic
device". This is the operation you would use in a kernel abstraction
that needs to take "some device", and then grab its own reference and
keep it for later, but which does not care about the specific bus type.

In particular, the way the bus abstractions work right now downstream,
the specific bus device types are not cloneable and only represent
borrowed device references that exist for the lifetime of the driver
probe callback. The idea is that you grab references to all the
bus-specific resources you need within (there is revocability logic to
make sure resources can disappear without breaking pointers, failing
gracefully if accessed) and do any bus-specific init you need, and then
you no longer need to directly touch the bus device after that. This is
definitely not going to cover some corner cases, but I haven't had any
need to use platform device ops in my GPU driver after probe, so it
works for me. But the driver does need to pass around device references
and keep them in some inner structures to make things like `dev_warn!()`
macros work, and that is what `Device` and `RawDevice` are for: `Device`
is an owning reference that can be cloned and constructed out of the
original `PlatformDevice` and can outlive it.
>> +        // 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
>> +    }
> 
> What does this raw device do?  What is the relationship to a "real"
> device?  Maybe it's just my lack of rust knowledge here showing, so any
> hints would be appreciated.

This means that a Device (an owned reference to a generic struct device)
can be used anywhere you need a generic device reference (this sounds
tautological but the idea is that specific bus device types can also be
used, which is why this layer of indirection exists).

In this case it's just an accessor for self.ptr since a Device is just a
wrapper around the pointer.

> 
>> +}
>> +
>> +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) };
> 
> So it is now ok for this to be freed?

Yes, because the Rust type system guarantees that Drop only gets called
when the Device no longer has an owner. That means there are no
references of any kind left and the object is ceasing to exist. Rust
also blocks you from manually calling drop() (this is a special case,
without it you could drop something more than once).

In other words, a Device represents a reference to a struct device. Once
the Device gets dropped, there is nobody left to potentially use that
reference through the Device by definition. (Of course you can invalidly
stash away the raw pointer like those C API issues you found... but
that's a different problem).

One way to solve the C abstraction issue would be to embed a Device in
the RtKit/IoPageTableInner structs (the respective Rust abstraction
types), creating it out of the passed RawDevice reference with
Device::from_raw() in the constructor for those. That means you'd
automatically `get_device()` on construction and `put_device()` when the
abstraction objects get dropped, and that would solve all the problems
without any extra cleanup or management code.

> One meta-comment, why is any of this needed at all?  No driver should
> ever be dealing with a "raw" struct device pointer at all, right?  They
> should be calling into subsystems that give it a pointer to a
> bus-specific pointer type (gpio, usb, pci, etc.)

I'm getting the feeling maybe the name is just bad. Maybe it should be
called AbstractDevice or IntoDevice or something like that. It's just
"some device".

Drivers indeed should never be dealing with actual device pointers, the
pointers are for consumption by the kernel crate only. That this is done
using trait functions that can technically be called by the driver code
is just the way Rust models this: you need *some* trait function to get
at the raw pointer (because the kernel crate needs it) and the trait
needs to be public (because it's part of the public interface as the
abstract device type) and there is no such thing as private trait
functions in a public trait in Rust. But the idea is that this doesn't
matter: sure, drivers can *get* a raw device pointer but they can't *do*
anything with it without unsafe.

Technically though, if we wanted to hide this further, we could make it
return a newtype wrapper around the raw pointer. Then as long as the
inner field is not public, drivers wouldn't even be able to get at the
raw pointer but the kernel crate would. I'm not sure if this is worth
it, though. It's not like drivers can't work around that with unsafe
anyway, once you use unsafe you can do all kinds of crazy stuff like raw
memory reinterpretations, copies, and transmutations, which will allow
you to cheat the entire type system if you want... but of course the
whole idea here is that we make things impossible in safe code and then
we look at the unsafe blocks with a magnifying glass during review to
make sure they aren't doing anything actually unsafe or crazy.

~~ Lina
  

On Sat, Feb 25, 2023 at 12:10:47AM +0900, Asahi Lina wrote:
> >> +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) };
> > 
> > You don't check the return value of get_device()?  What if it failed
> > (hint, it can)?
> 
> Really? I looked at the implementation and I don't see how it can fail,
> as long as the argument is non-NULL and valid... (which is a
> precondition of this unsafe function). Did I miss something?

This function has changed a bit over time, but yes, it could fail if
someone else just dropped the last reference right before you tried to
grab a new one (look at the implementation of kobject_get()).

Yes, if you "know" you have a non-zero reference count first, it should
never fail, but how do you know this?  We have the ability to check the
return value of the function, shouldn't that happen?

thanks,

greg k-h
  

On 25/02/2023 00.34, Greg Kroah-Hartman wrote:
> On Sat, Feb 25, 2023 at 12:10:47AM +0900, Asahi Lina wrote:
>>>> +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) };
>>>
>>> You don't check the return value of get_device()?  What if it failed
>>> (hint, it can)?
>>
>> Really? I looked at the implementation and I don't see how it can fail,
>> as long as the argument is non-NULL and valid... (which is a
>> precondition of this unsafe function). Did I miss something?
> 
> This function has changed a bit over time, but yes, it could fail if
> someone else just dropped the last reference right before you tried to
> grab a new one (look at the implementation of kobject_get()).
> 
> Yes, if you "know" you have a non-zero reference count first, it should
> never fail, but how do you know this?  We have the ability to check the
> return value of the function, shouldn't that happen?

Well, we know this because it is part of the invariant. If we don't
uphold invariants, things fall apart... that's why we create safe
abstractions that don't let you break those invariants after all!

In this particular case though, I don't see what we can usefully do
here. `device_get()` is going to be part of Clone impls and things like
that which are non-fallible. At most we can assert!() and rust-panic
(which is a BUG as far as I know) if it fails... would that be preferable?

~~ Lina
  

On Sat, Feb 25, 2023 at 12:51:38AM +0900, Asahi Lina wrote:
> On 25/02/2023 00.34, Greg Kroah-Hartman wrote:
> > On Sat, Feb 25, 2023 at 12:10:47AM +0900, Asahi Lina wrote:
> >>>> +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) };
> >>>
> >>> You don't check the return value of get_device()?  What if it failed
> >>> (hint, it can)?
> >>
> >> Really? I looked at the implementation and I don't see how it can fail,
> >> as long as the argument is non-NULL and valid... (which is a
> >> precondition of this unsafe function). Did I miss something?
> > 
> > This function has changed a bit over time, but yes, it could fail if
> > someone else just dropped the last reference right before you tried to
> > grab a new one (look at the implementation of kobject_get()).
> > 
> > Yes, if you "know" you have a non-zero reference count first, it should
> > never fail, but how do you know this?  We have the ability to check the
> > return value of the function, shouldn't that happen?
> 
> Well, we know this because it is part of the invariant. If we don't
> uphold invariants, things fall apart... that's why we create safe
> abstractions that don't let you break those invariants after all!
> 
> In this particular case though, I don't see what we can usefully do
> here. `device_get()` is going to be part of Clone impls and things like
> that which are non-fallible. At most we can assert!() and rust-panic
> (which is a BUG as far as I know) if it fails... would that be preferable?

That's a good question, I don't know.  In thinking about it some more,
I think we are ok with this as-is for now.

BUT, I want to see any code that is actually using a struct device, and
I really want to review the heck out of the platform device api that you
must have somewhere, as that might show some other issues around this
type of thing that might be lurking.

thanks,

greg k-h
  

On 25/02/2023 01.31, Greg Kroah-Hartman wrote:
> On Sat, Feb 25, 2023 at 12:51:38AM +0900, Asahi Lina wrote:
>> On 25/02/2023 00.34, Greg Kroah-Hartman wrote:
>>> On Sat, Feb 25, 2023 at 12:10:47AM +0900, Asahi Lina wrote:
>>>>>> +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) };
>>>>>
>>>>> You don't check the return value of get_device()?  What if it failed
>>>>> (hint, it can)?
>>>>
>>>> Really? I looked at the implementation and I don't see how it can fail,
>>>> as long as the argument is non-NULL and valid... (which is a
>>>> precondition of this unsafe function). Did I miss something?
>>>
>>> This function has changed a bit over time, but yes, it could fail if
>>> someone else just dropped the last reference right before you tried to
>>> grab a new one (look at the implementation of kobject_get()).
>>>
>>> Yes, if you "know" you have a non-zero reference count first, it should
>>> never fail, but how do you know this?  We have the ability to check the
>>> return value of the function, shouldn't that happen?
>>
>> Well, we know this because it is part of the invariant. If we don't
>> uphold invariants, things fall apart... that's why we create safe
>> abstractions that don't let you break those invariants after all!
>>
>> In this particular case though, I don't see what we can usefully do
>> here. `device_get()` is going to be part of Clone impls and things like
>> that which are non-fallible. At most we can assert!() and rust-panic
>> (which is a BUG as far as I know) if it fails... would that be preferable?
> 
> That's a good question, I don't know.  In thinking about it some more,
> I think we are ok with this as-is for now.
> 
> BUT, I want to see any code that is actually using a struct device, and
> I really want to review the heck out of the platform device api that you
> must have somewhere, as that might show some other issues around this
> type of thing that might be lurking.

The platform abstraction (which I didn't write either) is here [1],
though that's just pulled straight from RfL right now without proper
commit/attribution info and has a few other dependencies still.

But the refcounting is actually really boring on that one... there is
none! A platform::Device instance only exists during probe and cannot be
cloned or stolen, so it only represents a borrowed reference during that
time, and obviously the subsystem is guaranteed to hold a reference
while the driver probe callback runs (right? I mean, things would be
incredibly broken if that weren't the case and probe could randomly lose
the reference halfway through). There is definitely going to come a time
when this isn't enough, but right now with the device resource model
stuff it works, and of course it's really easy to review since it's so
simple!

More to the point though: I think it's probably okay if some things
can't be checked in these initial submissions, but then we check them as
more users (in `kernel` and drivers) come in later? As you mentioned
there's a chicken-and-egg issue, and also I'd say a mismatch between
"what you need to validate certain invariants/design decisions" and
"what is reasonable to submit as a single series". But at the end of the
day we're going to have to refer back to this code when reviewing
further code that uses it inside `kernel` anyway (especially this early
on), so it's probably okay to fix anything that we missed or discover is
broken then? I certainly don't mind adding a bunch of "fix broken thing"
patches at the head of later reviews if we run into issues ^^

[1]
https://github.com/Rust-for-Linux/linux/pull/969/commits/33770890aa71a491d81ec2cb2b7a45d953d1b7dd


~~ Lina
  

Hi Greg,

As Lina points out, I wrote this code, so I'll try to answer your questions.

Lina, apologies for the delay in participating, I've been distracted
with other stuff.

On Fri, 24 Feb 2023 at 08:19, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Feb 24, 2023 at 07:53:17PM +0900, Asahi Lina 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.
>
> What good is just the device name?  I'm all for proper bindings to hook
> up properly to the driver core, but this feels like it's not really
> doing much of anything.
>
> Do you have a real user that we can see how this is interacting?
>
> And what does a driver care about the device name anyway?  It should
> only be using the dev_*() calls to print that info out to the log, and
> never messing around with it in any other format as that's what
> userspace expects.

I added this for the pl061 gpio driver, which initialises the gpio
chip label with the device name:
https://elixir.bootlin.com/linux/v6.2.2/source/drivers/gpio/gpio-pl061.c#L333

Someone suggested this driver as good example in some of these earlier
interminable threads. The source code for it is in our Rust-for-Linux
rust branch.

> > Lina: Rewrote commit message, dropped the Amba bits, and squashed in
> > simple changes to the core Device code from latter commits in
> > rust-for-linux/rust. Also include the rust_helper_dev_get_drvdata
> > helper which will be needed by consumers later on anyway.
> >
> > 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: Asahi Lina <lina@asahilina.net>
> > ---
> >  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 04b9be46e887..54954fd80c77 100644
> > --- a/rust/helpers.c
> > +++ b/rust/helpers.c
> > @@ -20,6 +20,7 @@
> >
> >  #include <linux/bug.h>
> >  #include <linux/build_bug.h>
> > +#include <linux/device.h>
> >  #include <linux/err.h>
> >  #include <linux/refcount.h>
> >
> > @@ -65,6 +66,18 @@ long rust_helper_PTR_ERR(__force const void *ptr)
> >  }
> >  EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR);
> >
> > +void *rust_helper_dev_get_drvdata(struct device *dev)
> > +{
> > +     return dev_get_drvdata(dev);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata);
>
> No matching dev_set_drvdata()?  What good is getting a random void
> pointer if you couldn't set it in the first place?  :)
>
> > +const char *rust_helper_dev_name(const struct device *dev)
> > +{
> > +     return dev_name(dev);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_dev_name);
>
> Again, why?  What is going to use this?
>
> And I don't really understand the rules you are putting on the name
> string after calling this, more below:
>
> > +
> >  /*
> >   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
> >   * as the Rust `usize` type, so we can use it in contexts where Rust
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 9be021e393ca..e57da622d817 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 @@ use crate::bindings;
> >  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) }
>
> Why can the device never be renamed?  Devices are renamed all the time,
> sometimes when you least expect it (i.e. by userspace).  So how is this
> considered "safe"? and actually correct?
>
> Again, maybe seeing a real user of this might make more sense, but
> as-is, this feels wrong and not needed at all.

This requirement is to allow callers to use the string without having
to make a copy of it.

If subsystems/buses are not following what the C documentation says,
as you point out in another thread, we have a several options: (a)
remove access to names altogether, (b) leave things as they are, then
those subsystems wouldn't be able to honour the safety requirements of
this trait therefore they wouldn't implement it, (c) make a copy of
the string, etc.

Since we don't have immediate plans to upstream the pl061 driver, I
think we should just remove the name functionality for now and revisit
it later if the need arises.

>
> > +    }
> > +}
> > +
> > +/// 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) };
>
> You don't check the return value of get_device()?  What if it failed
> (hint, it can)?

I think this was already discussed, but anyway: the caller owns a
reference to this device, so `get_device`  is safe to call and cannot
fail.

Note that `get_device` calls `kobject_get`, which doesn't fail. I
suppose you're hinting at `kobject_get_unless_zero`, which of course
can fail but isn't in use here.

>
> > +        // 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 {
>
> I am a rust newbie, but I don't understand this "RawDevice" here at all.

Different buses will have their own Rust "Device" type, for example,
pci::Device, amba::Device, platform::Device that wrap their C
counterparts pci_dev, amba_device, platform_device.

"RawDevice" is a trait for functionality that is common to all
devices. It exposes the "struct device" of each bus/subsystem so that
functions that work on any "struct device", for example, `clk_get`,
`pr_info`. will automatically work on all subsystems.

For example, as part writing Rust abstractions for a platform devices,
we have a platform::Device type, which is wrapper around `struct
platform_device`. It has a bunch of associated functions that do
things that are specific to the platform bus. But then they also
implement the `RawDevice` trait (by implementing `raw_device` that
returns &pdev->dev), which allows drivers to call `clk_get` and the
printing functions directly.

Let's say `pdev` is a platform device; if we wanted to call `clk_get`
in C, we'd do something like:

clk = clk_get(&pdev->dev, NULL);

In Rust, we'd do:

clk = pdev.clk_get(None);

(Note that we didn't have to know that pdev had a field whose type is
a `struct device` that we could use to call clk_get on; `RawDevice`
encoded this information.)

Does the intent of the abstraction make sense to you now?

> > +        // 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
> > +    }
>
> What does this raw device do?  What is the relationship to a "real"
> device?  Maybe it's just my lack of rust knowledge here showing, so any
> hints would be appreciated.
>

This is the function that returns the `struct device` field embedded
in the different devices for each bus.

> > +}
> > +
> > +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) };
>
> So it is now ok for this to be freed?
>
> One meta-comment, why is any of this needed at all?  No driver should
> ever be dealing with a "raw" struct device pointer at all, right?  They
> should be calling into subsystems that give it a pointer to a
> bus-specific pointer type (gpio, usb, pci, etc.)
>
> So I'm thinking that adding support for "raw" struct device pointers
> feels ripe for abuse in a "the code should not be doing that" type of
> thing.
>
> Unless you are writing a new bus/class in rust?

Drivers will not use this directly. They'll benefit from it as I
described above to be able to call bus-agnostic functions.

Implementers of bus abstractions will use this to say "my device also
has a `struct device` in it, and I want it to benefit from all
bus-agnostic device manipulation functions".

Implementers of abstractions of bus-agnostic device functions will add
their abstractions to `RawDevice` instead of bus-specific types so
that they work with all devices.

Cheers,
-Wedson
  

On Sun, Mar 05, 2023 at 03:39:25AM -0300, Wedson Almeida Filho wrote:
> > > +    /// 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) }
> >
> > Why can the device never be renamed?  Devices are renamed all the time,
> > sometimes when you least expect it (i.e. by userspace).  So how is this
> > considered "safe"? and actually correct?
> >
> > Again, maybe seeing a real user of this might make more sense, but
> > as-is, this feels wrong and not needed at all.
> 
> This requirement is to allow callers to use the string without having
> to make a copy of it.
> 
> If subsystems/buses are not following what the C documentation says,
> as you point out in another thread, we have a several options: (a)
> remove access to names altogether, (b) leave things as they are, then
> those subsystems wouldn't be able to honour the safety requirements of
> this trait therefore they wouldn't implement it, (c) make a copy of
> the string, etc.

How about we fix the documentation in the .c code and also drop this as
you really don't need it now.

Want to send a patch for the driver core code fix?

> > > +        // 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 {
> >
> > I am a rust newbie, but I don't understand this "RawDevice" here at all.
> 
> Different buses will have their own Rust "Device" type, for example,
> pci::Device, amba::Device, platform::Device that wrap their C
> counterparts pci_dev, amba_device, platform_device.
> 
> "RawDevice" is a trait for functionality that is common to all
> devices. It exposes the "struct device" of each bus/subsystem so that
> functions that work on any "struct device", for example, `clk_get`,
> `pr_info`. will automatically work on all subsystems.

Why is this being called "Raw" then?  Why not just "Device" to follow
along with the naming scheme that the kernel already uses?

> For example, as part writing Rust abstractions for a platform devices,
> we have a platform::Device type, which is wrapper around `struct
> platform_device`. It has a bunch of associated functions that do
> things that are specific to the platform bus. But then they also
> implement the `RawDevice` trait (by implementing `raw_device` that
> returns &pdev->dev), which allows drivers to call `clk_get` and the
> printing functions directly.
> 
> Let's say `pdev` is a platform device; if we wanted to call `clk_get`
> in C, we'd do something like:
> 
> clk = clk_get(&pdev->dev, NULL);
> 
> In Rust, we'd do:
> 
> clk = pdev.clk_get(None);
> 
> (Note that we didn't have to know that pdev had a field whose type is
> a `struct device` that we could use to call clk_get on; `RawDevice`
> encoded this information.)
> 
> Does the intent of the abstraction make sense to you now?

A bit more, yes.  But I want to see some real users before agreeing that
it is sane :)

thanks,

greg k-h
  

On Thu, 9 Mar 2023 at 08:24, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, Mar 05, 2023 at 03:39:25AM -0300, Wedson Almeida Filho wrote:
> > > > +    /// 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) }
> > >
> > > Why can the device never be renamed?  Devices are renamed all the time,
> > > sometimes when you least expect it (i.e. by userspace).  So how is this
> > > considered "safe"? and actually correct?
> > >
> > > Again, maybe seeing a real user of this might make more sense, but
> > > as-is, this feels wrong and not needed at all.
> >
> > This requirement is to allow callers to use the string without having
> > to make a copy of it.
> >
> > If subsystems/buses are not following what the C documentation says,
> > as you point out in another thread, we have a several options: (a)
> > remove access to names altogether, (b) leave things as they are, then
> > those subsystems wouldn't be able to honour the safety requirements of
> > this trait therefore they wouldn't implement it, (c) make a copy of
> > the string, etc.
>
> How about we fix the documentation in the .c code and also drop this as
> you really don't need it now.
>
> Want to send a patch for the driver core code fix?

Sure, will do.

> > > > +        // 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 {
> > >
> > > I am a rust newbie, but I don't understand this "RawDevice" here at all.
> >
> > Different buses will have their own Rust "Device" type, for example,
> > pci::Device, amba::Device, platform::Device that wrap their C
> > counterparts pci_dev, amba_device, platform_device.
> >
> > "RawDevice" is a trait for functionality that is common to all
> > devices. It exposes the "struct device" of each bus/subsystem so that
> > functions that work on any "struct device", for example, `clk_get`,
> > `pr_info`. will automatically work on all subsystems.
>
> Why is this being called "Raw" then?  Why not just "Device" to follow
> along with the naming scheme that the kernel already uses?

Because it gives us access to underlying raw `struct device` pointer,
in Rust raw pointers are those unsafe `*mut T` or `*const T`. I'm not
married to the name though, we should probably look for a better one
if this one is confusing.

Just "Device" is already taken. It's a ref-counted `struct device` (it
calls get_device/put_device in the right places automatically,
guarantees no dandling pointers); it is meant to be used by code that
needs to hold on to devices when they don't care about the bus. (It in
fact implements `RawDevice`.)

 How about `IsDevice`?

Then, for example, the platform bus would implement `IsDevice` for
`plaform::Device`.

> > For example, as part writing Rust abstractions for a platform devices,
> > we have a platform::Device type, which is wrapper around `struct
> > platform_device`. It has a bunch of associated functions that do
> > things that are specific to the platform bus. But then they also
> > implement the `RawDevice` trait (by implementing `raw_device` that
> > returns &pdev->dev), which allows drivers to call `clk_get` and the
> > printing functions directly.
> >
> > Let's say `pdev` is a platform device; if we wanted to call `clk_get`
> > in C, we'd do something like:
> >
> > clk = clk_get(&pdev->dev, NULL);
> >
> > In Rust, we'd do:
> >
> > clk = pdev.clk_get(None);
> >
> > (Note that we didn't have to know that pdev had a field whose type is
> > a `struct device` that we could use to call clk_get on; `RawDevice`
> > encoded this information.)
> >
> > Does the intent of the abstraction make sense to you now?
>
> A bit more, yes.  But I want to see some real users before agreeing that
> it is sane :)

Fair enough.

Cheers,
-Wedson
  

On Thu, Mar 09, 2023 at 01:46:39PM -0300, Wedson Almeida Filho wrote:
> On Thu, 9 Mar 2023 at 08:24, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > > > > +        // 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 {
> > > >
> > > > I am a rust newbie, but I don't understand this "RawDevice" here at all.
> > >
> > > Different buses will have their own Rust "Device" type, for example,
> > > pci::Device, amba::Device, platform::Device that wrap their C
> > > counterparts pci_dev, amba_device, platform_device.
> > >
> > > "RawDevice" is a trait for functionality that is common to all
> > > devices. It exposes the "struct device" of each bus/subsystem so that
> > > functions that work on any "struct device", for example, `clk_get`,
> > > `pr_info`. will automatically work on all subsystems.
> >
> > Why is this being called "Raw" then?  Why not just "Device" to follow
> > along with the naming scheme that the kernel already uses?
> 
> Because it gives us access to underlying raw `struct device` pointer,
> in Rust raw pointers are those unsafe `*mut T` or `*const T`. I'm not
> married to the name though, we should probably look for a better one
> if this one is confusing.
> 
> Just "Device" is already taken. It's a ref-counted `struct device` (it
> calls get_device/put_device in the right places automatically,
> guarantees no dandling pointers); it is meant to be used by code that
> needs to hold on to devices when they don't care about the bus. (It in
> fact implements `RawDevice`.)

I don't understand, why do you need both of these?  Why can't one just
do?  Why would you need one without the other?  I would think that
"Device" and "RawDevice" here would be the same thing, that is a way to
refer to a "larger" underlying struct device memory chunk in a way that
can be passed around without knowing, or caring, what the "real" device
type is.

>  How about `IsDevice`?

That sounds like a question, and would return a boolean, not a structure :)

> Then, for example, the platform bus would implement `IsDevice` for
> `plaform::Device`.

I don't really understand that, sorry.

thanks,

greg k-h
  

On Thu, 9 Mar 2023 at 14:11, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Mar 09, 2023 at 01:46:39PM -0300, Wedson Almeida Filho wrote:
> > On Thu, 9 Mar 2023 at 08:24, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > > > > +        // 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 {
> > > > >
> > > > > I am a rust newbie, but I don't understand this "RawDevice" here at all.
> > > >
> > > > Different buses will have their own Rust "Device" type, for example,
> > > > pci::Device, amba::Device, platform::Device that wrap their C
> > > > counterparts pci_dev, amba_device, platform_device.
> > > >
> > > > "RawDevice" is a trait for functionality that is common to all
> > > > devices. It exposes the "struct device" of each bus/subsystem so that
> > > > functions that work on any "struct device", for example, `clk_get`,
> > > > `pr_info`. will automatically work on all subsystems.
> > >
> > > Why is this being called "Raw" then?  Why not just "Device" to follow
> > > along with the naming scheme that the kernel already uses?
> >
> > Because it gives us access to underlying raw `struct device` pointer,
> > in Rust raw pointers are those unsafe `*mut T` or `*const T`. I'm not
> > married to the name though, we should probably look for a better one
> > if this one is confusing.
> >
> > Just "Device" is already taken. It's a ref-counted `struct device` (it
> > calls get_device/put_device in the right places automatically,
> > guarantees no dandling pointers); it is meant to be used by code that
> > needs to hold on to devices when they don't care about the bus. (It in
> > fact implements `RawDevice`.)
>
> I don't understand, why do you need both of these?  Why can't one just
> do?  Why would you need one without the other?  I would think that
> "Device" and "RawDevice" here would be the same thing, that is a way to
> refer to a "larger" underlying struct device memory chunk in a way that
> can be passed around without knowing, or caring, what the "real" device
> type is.

`Device` is a struct, it is the Rust abstraction for C's `struct device`.

Let's use the platform bus as our running example: we have
`platform::Device` as the Rust abstraction for C's `struct
platform_device`.

Let's use `clk_get`as our running example of a function that takes a
`struct device` as argument.

If we have a platform device, we can't just call `clk_get` because the
types don't match. In C, we access the `dev` field of `struct
platform_device` before we call `clk_get` (i.e., we call
clk_get(&pdev->dev, ...)), but in Rust we don't want to make the
fields of `platform::Device` public, especially because they're fields
of a C struct. So as part of `platform::Device` we'd have to implement
something like:

impl platform::Device {
    fn get_device(&self) -> &Device {
    ...
    }
}

Then calling `clk_get` would be something like:

pdev.get_device().clk_get(...)

The problem is that `clk_get` doesn't know that `platform::Device` is
a device, that's why we need this `get_device()` call on each bus
abstraction of a device, plus on each call to bus-agnostic device
functions.

Since we're implementing this "adapter" function anyway, we may as
well put in a _trait_ and improve how people use it. We say: every
struct that is a device should implement the `RawDevice` (or
`IsDevice`, or whatever we decide to call it) trait. Then
`platform::Device` would still have to implement something like:

impl RawDevice for platform::Device {
    fn get_device(&self) -> &Device {
    ...
    }
}

(Note that we went from `impl X` to `impl RawDevice for X`.)

With this trait, users can call `clk_get` on a platform device as follows:

pdev.clk_get(...)

So we improve the experience of driver developers by having bus
abstraction developers add "RawDevice for" to their impl block of the
`get_device` function for their device.

> >  How about `IsDevice`?
>
> That sounds like a question, and would return a boolean, not a structure :)

`RawDevice` is not a struct, it's a trait that is implemented by
bus-specific structs.

> > Then, for example, the platform bus would implement `IsDevice` for
> > `plaform::Device`.
>
> I don't really understand that, sorry.

I tried to explain it above. I think the source of the problem is the
distinction between `struct` and `trait`, the latter of course doesn't
exist in C.

Cheers,
-Wedson
  

On Thu, Mar 9, 2023 at 6:11 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> I don't understand, why do you need both of these?  Why can't one just
> do?  Why would you need one without the other?  I would think that
> "Device" and "RawDevice" here would be the same thing, that is a way to
> refer to a "larger" underlying struct device memory chunk in a way that
> can be passed around without knowing, or caring, what the "real" device
> type is.
>
> That sounds like a question, and would return a boolean, not a structure :)
>
> I don't really understand that, sorry.

To complement what Wedson wrote, perhaps this helps a bit, since there
are a few `*Device` things around (I will prefix things with the
module they are in for clarity):

  - `device::RawDevice` is a trait, not a struct. This means it is
just an interface/property/tag/feature. Then types (like structs) can
say "I support/provide/implement this trait". In this case, it
requires those types implementing `device::RawDevice` to provide a
suitable `raw_device()` method.

  - Then, you have `device::Device`. This is an actual struct,
containing a `struct device *`. It implements `device::RawDevice`, by
returning the pointer.

  - Then there are others like `platform::Device` or `amba::Device`.
They are also structs, containing e.g. `struct platform_device *` or
anything they may need. They implement `device::RawDevice` too if it
makes sense, say, by returning a pointer to the `->dev` in their C
struct.

So you have two different kinds of entities here. One is the trait,
and the others are structs that promise to implement that trait
correctly, regardless of how each struct manages to do it.

The trait can also then provide extra default functionality for those
that have implemented it. For instance, the `clk_get()` method Wedson
mentioned can be defined in the trait, and in its body we can call the
`raw_device()` to do so. `raw_device()` is available because we are
inside the trait, even if each struct implementing the trait provides
a different implementation.

Traits are sometimes named after the "main" method they contain, e.g.
`ToString` may be a trait with a `to_string()` method. Here
`device::RawDevice` has a `raw_device()` method, so it makes sense in
that way.

Wedson was suggesting `device::IsDevice` as an alternative, because
`device::Device` is taken for the ref-counted device. Of course, other
arrangements of names could be possible.

`IsDevice` uses Pascal case, so by convention it would be fairly clear
that it would not be a function returning a boolean. But one may
expect that it is a trait that implements a `is_device()` method,
though. (And if that was the case, which it isn't, one could then
indeed expect that such a method would return the boolean you
expected).

Cheers,
Miguel
  

On Thu, 9 Mar 2023 16:06:06 -0300
Wedson Almeida Filho <wedsonaf@gmail.com> wrote:

> On Thu, 9 Mar 2023 at 14:11, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Mar 09, 2023 at 01:46:39PM -0300, Wedson Almeida Filho wrote:  
> > > On Thu, 9 Mar 2023 at 08:24, Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:  
> > > > > > > +        // 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 {  
> > > > > >
> > > > > > I am a rust newbie, but I don't understand this "RawDevice" here at all.  
> > > > >
> > > > > Different buses will have their own Rust "Device" type, for example,
> > > > > pci::Device, amba::Device, platform::Device that wrap their C
> > > > > counterparts pci_dev, amba_device, platform_device.
> > > > >
> > > > > "RawDevice" is a trait for functionality that is common to all
> > > > > devices. It exposes the "struct device" of each bus/subsystem so that
> > > > > functions that work on any "struct device", for example, `clk_get`,
> > > > > `pr_info`. will automatically work on all subsystems.  
> > > >
> > > > Why is this being called "Raw" then?  Why not just "Device" to follow
> > > > along with the naming scheme that the kernel already uses?  
> > >
> > > Because it gives us access to underlying raw `struct device` pointer,
> > > in Rust raw pointers are those unsafe `*mut T` or `*const T`. I'm not
> > > married to the name though, we should probably look for a better one
> > > if this one is confusing.
> > >
> > > Just "Device" is already taken. It's a ref-counted `struct device` (it
> > > calls get_device/put_device in the right places automatically,
> > > guarantees no dandling pointers); it is meant to be used by code that
> > > needs to hold on to devices when they don't care about the bus. (It in
> > > fact implements `RawDevice`.)  
> >
> > I don't understand, why do you need both of these?  Why can't one just
> > do?  Why would you need one without the other?  I would think that
> > "Device" and "RawDevice" here would be the same thing, that is a way to
> > refer to a "larger" underlying struct device memory chunk in a way that
> > can be passed around without knowing, or caring, what the "real" device
> > type is.  
> 
> `Device` is a struct, it is the Rust abstraction for C's `struct device`.
> 
> Let's use the platform bus as our running example: we have
> `platform::Device` as the Rust abstraction for C's `struct
> platform_device`.
> 
> Let's use `clk_get`as our running example of a function that takes a
> `struct device` as argument.
> 
> If we have a platform device, we can't just call `clk_get` because the
> types don't match. In C, we access the `dev` field of `struct
> platform_device` before we call `clk_get` (i.e., we call
> clk_get(&pdev->dev, ...)), but in Rust we don't want to make the
> fields of `platform::Device` public, especially because they're fields
> of a C struct. So as part of `platform::Device` we'd have to implement
> something like:
> 
> impl platform::Device {
>     fn get_device(&self) -> &Device {
>     ...
>     }
> }
> 
> Then calling `clk_get` would be something like:
> 
> pdev.get_device().clk_get(...)

Just thinking out loud here, would it work if `platform::Device`
implements `Deref<Target = Device>`?

Best,
Gary
  

On Fri, 24 Feb 2023 19:53:17 +0900
Asahi Lina <lina@asahilina.net> 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.
> 
> Lina: Rewrote commit message, dropped the Amba bits, and squashed in
> simple changes to the core Device code from latter commits in
> rust-for-linux/rust. Also include the rust_helper_dev_get_drvdata
> helper which will be needed by consumers later on anyway.
> 
> 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: Asahi Lina <lina@asahilina.net>
> ---
>  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 04b9be46e887..54954fd80c77 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -20,6 +20,7 @@
>  
>  #include <linux/bug.h>
>  #include <linux/build_bug.h>
> +#include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/refcount.h>
>  
> @@ -65,6 +66,18 @@ long rust_helper_PTR_ERR(__force const void *ptr)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR);
>  
> +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);
> +
>  /*
>   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
>   * as the Rust `usize` type, so we can use it in contexts where Rust
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 9be021e393ca..e57da622d817 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 @@ use crate::bindings;
>  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,
> +}
> +

Shouldn't this be

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

?

Best,
Gary
  

On Mon, Mar 13, 2023 at 05:52:02PM +0000, Gary Guo wrote:
[...]
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 9be021e393ca..e57da622d817 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 @@ use crate::bindings;
> >  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,
> > +}
> > +
> 
> Shouldn't this be
> 
> #[repr(transparent)]
> pub struct Device(Opaque<bindings::device>);
> 
> ?

I have the same feeling, for `task_struct`, we have

	#[repr(transparent)]
	pub struct Task(pub(crate) UnsafeCell<bindings::task_struct>);

and
	
	pub struct TaskRef<'a> {
	    task: &'a Task,
	    _not_send: PhantomData<*mut ()>,
	}

I wonder whether we should do the similar for `Device`, meaning `Device`
is just a tranparent representation for `struct device` and another
type (say, `DeviceRef`) is introduced as the reference type, or we can
just use `ARef`.

Regards,
Boqun



> 
> Best,
> Gary