[PATCH v3 0/5] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[PATCH v3 0/5] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Lee Jones]

It has been suggested that the driver should use dev_info() instead of
pr_info() however there is currently no scaffolding to successfully pull
a 'struct device' out from driver data post register().  This is being
worked on and we will convert this over in due course.

Lee Jones (5):
  rust: miscdevice: Provide accessor to pull out miscdevice::this_device
  Documentation: ioctl-number: Carve out some identifiers for use by
    sample drivers
  samples: rust: Provide example using the new Rust MiscDevice
    abstraction
  sample: rust_misc_device: Demonstrate additional get/set value
    functionality
  MAINTAINERS: Add Rust Misc Sample to MISC entry

 .../userspace-api/ioctl/ioctl-number.rst      |   1 +
 MAINTAINERS                                   |   1 +
 rust/kernel/miscdevice.rs                     |   9 ++
 samples/rust/Kconfig                          |  10 ++
 samples/rust/Makefile                         |   1 +
 samples/rust/rust_misc_device.rs              | 132 ++++++++++++++++++
 6 files changed, 154 insertions(+)
 create mode 100644 samples/rust/rust_misc_device.rs

-- 
2.47.0.338.g60cca15819-goog

[PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device

[Lee Jones]

There are situations where a pointer to a `struct device` will become
necessary (e.g. for calling into dev_*() functions).  This accessor
allows callers to pull this out from the `struct miscdevice`.

Signed-off-by: Lee Jones <lee@kernel.org>
---
 rust/kernel/miscdevice.rs | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index 7e2a79b3ae26..55340f316006 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -10,11 +10,13 @@
 
 use crate::{
     bindings,
+    device::Device,
     error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
     prelude::*,
     str::CStr,
     types::{ForeignOwnable, Opaque},
 };
+
 use core::{
     ffi::{c_int, c_long, c_uint, c_ulong},
     marker::PhantomData,
@@ -84,6 +86,13 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
     pub fn as_raw(&self) -> *mut bindings::miscdevice {
         self.inner.get()
     }
+
+    /// Returns a pointer to the current Device
+    pub fn device(&self) -> &Device {
+        // SAFETY: This is only accessible after a successful register() which always
+        // initialises this_device with a valid device.
+        unsafe { Device::as_ref((*self.as_raw()).this_device) }
+    }
 }
 
 #[pinned_drop]
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device

[Fiona Behrens]

On 5 Dec 2024, at 17:25, Lee Jones wrote:

> There are situations where a pointer to a `struct device` will become
> necessary (e.g. for calling into dev_*() functions).  This accessor
> allows callers to pull this out from the `struct miscdevice`.
>
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
>  rust/kernel/miscdevice.rs | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index 7e2a79b3ae26..55340f316006 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -10,11 +10,13 @@
>
>  use crate::{
>      bindings,
> +    device::Device,
>      error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
>      prelude::*,
>      str::CStr,
>      types::{ForeignOwnable, Opaque},
>  };
> +
>  use core::{
>      ffi::{c_int, c_long, c_uint, c_ulong},
>      marker::PhantomData,
> @@ -84,6 +86,13 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>      pub fn as_raw(&self) -> *mut bindings::miscdevice {
>          self.inner.get()
>      }
> +
> +    /// Returns a pointer to the current Device

I would not call this pointer but rather reference? Pointer is usually associated with *mut or *const, and this is a reference to the Device abstraction


Thanks,
 - Fiona

> +    pub fn device(&self) -> &Device {
> +        // SAFETY: This is only accessible after a successful register() which always
> +        // initialises this_device with a valid device.
> +        unsafe { Device::as_ref((*self.as_raw()).this_device) }
> +    }
>  }
>
>  #[pinned_drop]
> -- 
> 2.47.0.338.g60cca15819-goog

Re: [PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device

[Lee Jones]

On Thu, 05 Dec 2024, Fiona Behrens wrote:

> 
> 
> On 5 Dec 2024, at 17:25, Lee Jones wrote:
> 
> > There are situations where a pointer to a `struct device` will become
> > necessary (e.g. for calling into dev_*() functions).  This accessor
> > allows callers to pull this out from the `struct miscdevice`.
> >
> > Signed-off-by: Lee Jones <lee@kernel.org>
> > ---
> >  rust/kernel/miscdevice.rs | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> > index 7e2a79b3ae26..55340f316006 100644
> > --- a/rust/kernel/miscdevice.rs
> > +++ b/rust/kernel/miscdevice.rs
> > @@ -10,11 +10,13 @@
> >
> >  use crate::{
> >      bindings,
> > +    device::Device,
> >      error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
> >      prelude::*,
> >      str::CStr,
> >      types::{ForeignOwnable, Opaque},
> >  };
> > +
> >  use core::{
> >      ffi::{c_int, c_long, c_uint, c_ulong},
> >      marker::PhantomData,
> > @@ -84,6 +86,13 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> >      pub fn as_raw(&self) -> *mut bindings::miscdevice {
> >          self.inner.get()
> >      }
> > +
> > +    /// Returns a pointer to the current Device
> 
> I would not call this pointer but rather reference? Pointer is usually associated with *mut or *const, and this is a reference to the Device abstraction

No, no, my Rustacean friend.  That's not the point of the comment at
all.  We can all see that it the return value is literally a reference
to Device (&Device), but the thing it's actually passing back is a
`struct device *`:

  % git --no-pager grep -n this_device -- include/
  include/linux/miscdevice.h:85:	struct device *this_device;

I'll change the comment to be a lot more intentional.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device

[Greg KH]

On Thu, Dec 05, 2024 at 04:25:18PM +0000, Lee Jones wrote:
> There are situations where a pointer to a `struct device` will become
> necessary (e.g. for calling into dev_*() functions).  This accessor
> allows callers to pull this out from the `struct miscdevice`.
> 
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
>  rust/kernel/miscdevice.rs | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index 7e2a79b3ae26..55340f316006 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -10,11 +10,13 @@
>  
>  use crate::{
>      bindings,
> +    device::Device,
>      error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
>      prelude::*,
>      str::CStr,
>      types::{ForeignOwnable, Opaque},
>  };
> +
>  use core::{
>      ffi::{c_int, c_long, c_uint, c_ulong},
>      marker::PhantomData,
> @@ -84,6 +86,13 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>      pub fn as_raw(&self) -> *mut bindings::miscdevice {
>          self.inner.get()
>      }
> +
> +    /// Returns a pointer to the current Device
> +    pub fn device(&self) -> &Device {
> +        // SAFETY: This is only accessible after a successful register() which always
> +        // initialises this_device with a valid device.
> +        unsafe { Device::as_ref((*self.as_raw()).this_device) }

A "raw" pointer that you can do something with without incrementing the
reference count of it?  Oh wait, no, it's the rust device structure.
If so, why isn't this calling the get_device() interface instead?  That
way it's properly incremented and decremented when it "leaves the scope"
right?

Or am I missing something here as to why that wouldn't work and this is
the only way to get access to the 'struct device' of this miscdevice?

thanks,

greg k-h

Re: [PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device

[Lee Jones]

On Fri, 06 Dec 2024, Greg KH wrote:

> On Thu, Dec 05, 2024 at 04:25:18PM +0000, Lee Jones wrote:
> > There are situations where a pointer to a `struct device` will become
> > necessary (e.g. for calling into dev_*() functions).  This accessor
> > allows callers to pull this out from the `struct miscdevice`.
> > 
> > Signed-off-by: Lee Jones <lee@kernel.org>
> > ---
> >  rust/kernel/miscdevice.rs | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> > index 7e2a79b3ae26..55340f316006 100644
> > --- a/rust/kernel/miscdevice.rs
> > +++ b/rust/kernel/miscdevice.rs
> > @@ -10,11 +10,13 @@
> >  
> >  use crate::{
> >      bindings,
> > +    device::Device,
> >      error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
> >      prelude::*,
> >      str::CStr,
> >      types::{ForeignOwnable, Opaque},
> >  };
> > +
> >  use core::{
> >      ffi::{c_int, c_long, c_uint, c_ulong},
> >      marker::PhantomData,
> > @@ -84,6 +86,13 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> >      pub fn as_raw(&self) -> *mut bindings::miscdevice {
> >          self.inner.get()
> >      }
> > +
> > +    /// Returns a pointer to the current Device
> > +    pub fn device(&self) -> &Device {
> > +        // SAFETY: This is only accessible after a successful register() which always
> > +        // initialises this_device with a valid device.
> > +        unsafe { Device::as_ref((*self.as_raw()).this_device) }
> 
> A "raw" pointer that you can do something with without incrementing the
> reference count of it?  Oh wait, no, it's the rust device structure.
> If so, why isn't this calling the get_device() interface instead?  That
> way it's properly incremented and decremented when it "leaves the scope"
> right?
> 
> Or am I missing something here as to why that wouldn't work and this is
> the only way to get access to the 'struct device' of this miscdevice?

Fair point.  I'll speak to Alice.

Another reason why using dev_info() is not possible at the moment.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device

[Lee Jones]

On Fri, 06 Dec 2024, Lee Jones wrote:

> On Fri, 06 Dec 2024, Greg KH wrote:
> 
> > On Thu, Dec 05, 2024 at 04:25:18PM +0000, Lee Jones wrote:
> > > There are situations where a pointer to a `struct device` will become
> > > necessary (e.g. for calling into dev_*() functions).  This accessor
> > > allows callers to pull this out from the `struct miscdevice`.
> > > 
> > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > ---
> > >  rust/kernel/miscdevice.rs | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> > > index 7e2a79b3ae26..55340f316006 100644
> > > --- a/rust/kernel/miscdevice.rs
> > > +++ b/rust/kernel/miscdevice.rs
> > > @@ -10,11 +10,13 @@
> > >  
> > >  use crate::{
> > >      bindings,
> > > +    device::Device,
> > >      error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
> > >      prelude::*,
> > >      str::CStr,
> > >      types::{ForeignOwnable, Opaque},
> > >  };
> > > +
> > >  use core::{
> > >      ffi::{c_int, c_long, c_uint, c_ulong},
> > >      marker::PhantomData,
> > > @@ -84,6 +86,13 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> > >      pub fn as_raw(&self) -> *mut bindings::miscdevice {
> > >          self.inner.get()
> > >      }
> > > +
> > > +    /// Returns a pointer to the current Device
> > > +    pub fn device(&self) -> &Device {
> > > +        // SAFETY: This is only accessible after a successful register() which always
> > > +        // initialises this_device with a valid device.
> > > +        unsafe { Device::as_ref((*self.as_raw()).this_device) }
> > 
> > A "raw" pointer that you can do something with without incrementing the
> > reference count of it?  Oh wait, no, it's the rust device structure.
> > If so, why isn't this calling the get_device() interface instead?  That
> > way it's properly incremented and decremented when it "leaves the scope"
> > right?
> > 
> > Or am I missing something here as to why that wouldn't work and this is
> > the only way to get access to the 'struct device' of this miscdevice?
> 
> Fair point.  I'll speak to Alice.

Alice isn't available yet, so I may be talking out of turn at this
point, but I just found this is the Device documentation:

  /// A `Device` instance represents a valid `struct device` created by the C portion of the kernel.
  ///
  /// Instances of this type are always reference-counted, that is, a call to `get_device` ensures
  /// that the allocation remains valid at least until the matching call to `put_device`.

And:

  // SAFETY: Instances of `Device` are always reference-counted.

Ready for some analysis from this beginner?

Since this impl for Device is AlwaysRefCounted, when any references are
taken i.e. in the Device::as_ref line above, inc_ref() is implicitly
called to increase the refcount.  The same will be true of dec_ref()
once it goes out of scope.

  // SAFETY: Instances of `Device` are always reference-counted.
  unsafe impl crate::types::AlwaysRefCounted for Device {
      fn inc_ref(&self) {
          // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
          unsafe { bindings::get_device(self.as_raw()) };
      }

      unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
          // SAFETY: The safety requirements guarantee that the refcount is non-zero.
          unsafe { bindings::put_device(obj.cast().as_ptr()) }
  }

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device

[Greg KH]

On Fri, Dec 06, 2024 at 07:33:09AM +0000, Lee Jones wrote:
> On Fri, 06 Dec 2024, Lee Jones wrote:
> 
> > On Fri, 06 Dec 2024, Greg KH wrote:
> > 
> > > On Thu, Dec 05, 2024 at 04:25:18PM +0000, Lee Jones wrote:
> > > > There are situations where a pointer to a `struct device` will become
> > > > necessary (e.g. for calling into dev_*() functions).  This accessor
> > > > allows callers to pull this out from the `struct miscdevice`.
> > > > 
> > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > > ---
> > > >  rust/kernel/miscdevice.rs | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> > > > index 7e2a79b3ae26..55340f316006 100644
> > > > --- a/rust/kernel/miscdevice.rs
> > > > +++ b/rust/kernel/miscdevice.rs
> > > > @@ -10,11 +10,13 @@
> > > >  
> > > >  use crate::{
> > > >      bindings,
> > > > +    device::Device,
> > > >      error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
> > > >      prelude::*,
> > > >      str::CStr,
> > > >      types::{ForeignOwnable, Opaque},
> > > >  };
> > > > +
> > > >  use core::{
> > > >      ffi::{c_int, c_long, c_uint, c_ulong},
> > > >      marker::PhantomData,
> > > > @@ -84,6 +86,13 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> > > >      pub fn as_raw(&self) -> *mut bindings::miscdevice {
> > > >          self.inner.get()
> > > >      }
> > > > +
> > > > +    /// Returns a pointer to the current Device
> > > > +    pub fn device(&self) -> &Device {
> > > > +        // SAFETY: This is only accessible after a successful register() which always
> > > > +        // initialises this_device with a valid device.
> > > > +        unsafe { Device::as_ref((*self.as_raw()).this_device) }
> > > 
> > > A "raw" pointer that you can do something with without incrementing the
> > > reference count of it?  Oh wait, no, it's the rust device structure.
> > > If so, why isn't this calling the get_device() interface instead?  That
> > > way it's properly incremented and decremented when it "leaves the scope"
> > > right?
> > > 
> > > Or am I missing something here as to why that wouldn't work and this is
> > > the only way to get access to the 'struct device' of this miscdevice?
> > 
> > Fair point.  I'll speak to Alice.
> 
> Alice isn't available yet, so I may be talking out of turn at this
> point, but I just found this is the Device documentation:
> 
>   /// A `Device` instance represents a valid `struct device` created by the C portion of the kernel.
>   ///
>   /// Instances of this type are always reference-counted, that is, a call to `get_device` ensures
>   /// that the allocation remains valid at least until the matching call to `put_device`.
> 
> And:
> 
>   // SAFETY: Instances of `Device` are always reference-counted.
> 
> Ready for some analysis from this beginner?
> 
> Since this impl for Device is AlwaysRefCounted, when any references are
> taken i.e. in the Device::as_ref line above, inc_ref() is implicitly
> called to increase the refcount.  The same will be true of dec_ref()
> once it goes out of scope.
> 
>   // SAFETY: Instances of `Device` are always reference-counted.
>   unsafe impl crate::types::AlwaysRefCounted for Device {
>       fn inc_ref(&self) {
>           // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
>           unsafe { bindings::get_device(self.as_raw()) };
>       }
> 
>       unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>           // SAFETY: The safety requirements guarantee that the refcount is non-zero.
>           unsafe { bindings::put_device(obj.cast().as_ptr()) }
>   }

Ick, really?  So as_ref() implicitly calles inc_ref() and dec_ref()?
Ah, ok, in digging into AlwaysRefCounted I now see that seems to be
true.

So great, this is a reference counted object, so what's preventing it
from now being used in dev_info()?

thanks,

greg k-h

Re: [PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device

[Lee Jones]

On Fri, 06 Dec 2024, Greg KH wrote:

> On Fri, Dec 06, 2024 at 07:33:09AM +0000, Lee Jones wrote:
> > On Fri, 06 Dec 2024, Lee Jones wrote:
> > 
> > > On Fri, 06 Dec 2024, Greg KH wrote:
> > > 
> > > > On Thu, Dec 05, 2024 at 04:25:18PM +0000, Lee Jones wrote:
> > > > > There are situations where a pointer to a `struct device` will become
> > > > > necessary (e.g. for calling into dev_*() functions).  This accessor
> > > > > allows callers to pull this out from the `struct miscdevice`.
> > > > > 
> > > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > > > ---
> > > > >  rust/kernel/miscdevice.rs | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > > 
> > > > > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> > > > > index 7e2a79b3ae26..55340f316006 100644
> > > > > --- a/rust/kernel/miscdevice.rs
> > > > > +++ b/rust/kernel/miscdevice.rs
> > > > > @@ -10,11 +10,13 @@
> > > > >  
> > > > >  use crate::{
> > > > >      bindings,
> > > > > +    device::Device,
> > > > >      error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
> > > > >      prelude::*,
> > > > >      str::CStr,
> > > > >      types::{ForeignOwnable, Opaque},
> > > > >  };
> > > > > +
> > > > >  use core::{
> > > > >      ffi::{c_int, c_long, c_uint, c_ulong},
> > > > >      marker::PhantomData,
> > > > > @@ -84,6 +86,13 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> > > > >      pub fn as_raw(&self) -> *mut bindings::miscdevice {
> > > > >          self.inner.get()
> > > > >      }
> > > > > +
> > > > > +    /// Returns a pointer to the current Device
> > > > > +    pub fn device(&self) -> &Device {
> > > > > +        // SAFETY: This is only accessible after a successful register() which always
> > > > > +        // initialises this_device with a valid device.
> > > > > +        unsafe { Device::as_ref((*self.as_raw()).this_device) }
> > > > 
> > > > A "raw" pointer that you can do something with without incrementing the
> > > > reference count of it?  Oh wait, no, it's the rust device structure.
> > > > If so, why isn't this calling the get_device() interface instead?  That
> > > > way it's properly incremented and decremented when it "leaves the scope"
> > > > right?
> > > > 
> > > > Or am I missing something here as to why that wouldn't work and this is
> > > > the only way to get access to the 'struct device' of this miscdevice?
> > > 
> > > Fair point.  I'll speak to Alice.
> > 
> > Alice isn't available yet, so I may be talking out of turn at this
> > point, but I just found this is the Device documentation:
> > 
> >   /// A `Device` instance represents a valid `struct device` created by the C portion of the kernel.
> >   ///
> >   /// Instances of this type are always reference-counted, that is, a call to `get_device` ensures
> >   /// that the allocation remains valid at least until the matching call to `put_device`.
> > 
> > And:
> > 
> >   // SAFETY: Instances of `Device` are always reference-counted.
> > 
> > Ready for some analysis from this beginner?
> > 
> > Since this impl for Device is AlwaysRefCounted, when any references are
> > taken i.e. in the Device::as_ref line above, inc_ref() is implicitly
> > called to increase the refcount.  The same will be true of dec_ref()
> > once it goes out of scope.
> > 
> >   // SAFETY: Instances of `Device` are always reference-counted.
> >   unsafe impl crate::types::AlwaysRefCounted for Device {
> >       fn inc_ref(&self) {
> >           // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> >           unsafe { bindings::get_device(self.as_raw()) };
> >       }
> > 
> >       unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> >           // SAFETY: The safety requirements guarantee that the refcount is non-zero.
> >           unsafe { bindings::put_device(obj.cast().as_ptr()) }
> >   }
> 
> Ick, really?  So as_ref() implicitly calles inc_ref() and dec_ref()?
> Ah, ok, in digging into AlwaysRefCounted I now see that seems to be
> true.
> 
> So great, this is a reference counted object, so what's preventing it
> from now being used in dev_info()?

We're having this conversation in stereo at this point.

TL;DR, we can't call MiscDeviceRegistration::device() after .init YET.

The longer version of this can be seen in the cover-letter thread.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device

[Alice Ryhl]

On Fri, Dec 6, 2024 at 8:46 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Dec 06, 2024 at 07:33:09AM +0000, Lee Jones wrote:
> > On Fri, 06 Dec 2024, Lee Jones wrote:
> >
> > > On Fri, 06 Dec 2024, Greg KH wrote:
> > >
> > > > On Thu, Dec 05, 2024 at 04:25:18PM +0000, Lee Jones wrote:
> > > > > There are situations where a pointer to a `struct device` will become
> > > > > necessary (e.g. for calling into dev_*() functions).  This accessor
> > > > > allows callers to pull this out from the `struct miscdevice`.
> > > > >
> > > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > > > ---
> > > > >  rust/kernel/miscdevice.rs | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > >
> > > > > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> > > > > index 7e2a79b3ae26..55340f316006 100644
> > > > > --- a/rust/kernel/miscdevice.rs
> > > > > +++ b/rust/kernel/miscdevice.rs
> > > > > @@ -10,11 +10,13 @@
> > > > >
> > > > >  use crate::{
> > > > >      bindings,
> > > > > +    device::Device,
> > > > >      error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
> > > > >      prelude::*,
> > > > >      str::CStr,
> > > > >      types::{ForeignOwnable, Opaque},
> > > > >  };
> > > > > +
> > > > >  use core::{
> > > > >      ffi::{c_int, c_long, c_uint, c_ulong},
> > > > >      marker::PhantomData,
> > > > > @@ -84,6 +86,13 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> > > > >      pub fn as_raw(&self) -> *mut bindings::miscdevice {
> > > > >          self.inner.get()
> > > > >      }
> > > > > +
> > > > > +    /// Returns a pointer to the current Device
> > > > > +    pub fn device(&self) -> &Device {
> > > > > +        // SAFETY: This is only accessible after a successful register() which always
> > > > > +        // initialises this_device with a valid device.
> > > > > +        unsafe { Device::as_ref((*self.as_raw()).this_device) }
> > > >
> > > > A "raw" pointer that you can do something with without incrementing the
> > > > reference count of it?  Oh wait, no, it's the rust device structure.
> > > > If so, why isn't this calling the get_device() interface instead?  That
> > > > way it's properly incremented and decremented when it "leaves the scope"
> > > > right?
> > > >
> > > > Or am I missing something here as to why that wouldn't work and this is
> > > > the only way to get access to the 'struct device' of this miscdevice?
> > >
> > > Fair point.  I'll speak to Alice.
> >
> > Alice isn't available yet, so I may be talking out of turn at this
> > point, but I just found this is the Device documentation:
> >
> >   /// A `Device` instance represents a valid `struct device` created by the C portion of the kernel.
> >   ///
> >   /// Instances of this type are always reference-counted, that is, a call to `get_device` ensures
> >   /// that the allocation remains valid at least until the matching call to `put_device`.
> >
> > And:
> >
> >   // SAFETY: Instances of `Device` are always reference-counted.
> >
> > Ready for some analysis from this beginner?
> >
> > Since this impl for Device is AlwaysRefCounted, when any references are
> > taken i.e. in the Device::as_ref line above, inc_ref() is implicitly
> > called to increase the refcount.  The same will be true of dec_ref()
> > once it goes out of scope.
> >
> >   // SAFETY: Instances of `Device` are always reference-counted.
> >   unsafe impl crate::types::AlwaysRefCounted for Device {
> >       fn inc_ref(&self) {
> >           // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> >           unsafe { bindings::get_device(self.as_raw()) };
> >       }
> >
> >       unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> >           // SAFETY: The safety requirements guarantee that the refcount is non-zero.
> >           unsafe { bindings::put_device(obj.cast().as_ptr()) }
> >   }
>
> Ick, really?  So as_ref() implicitly calles inc_ref() and dec_ref()?
> Ah, ok, in digging into AlwaysRefCounted I now see that seems to be
> true.

It doesn't increment the refcount because it uses the reference type
&_ and not the ARef<_> pointer type. References enforce correctness
using the borrow-checker. For example, consider this attempt to UAF:

#[derive(Debug)]
struct Inner {}

struct Outer {
    inner: Inner,
}

impl Outer {
    fn as_ref(&self) -> &Inner {
        &self.inner
    }
}

fn main() {
    let inner;
    {
        let outer = Outer { inner: Inner {} };
        inner = outer.as_ref();
    }
    println!("{:?}", inner);
}

This fails to compile:

error[E0597]: `outer` does not live long enough
  --> src/main.rs:19:17
   |
18 |         let outer = Outer { inner: Inner {} };
   |             ----- binding `outer` declared here
19 |         inner = outer.as_ref();
   |                 ^^^^^ borrowed value does not live long enough
20 |     }
   |     - `outer` dropped here while still borrowed
21 |     println!("{:?}", inner);
   |                      ----- borrow later used here

The same logic applies to the device() accessor. That is, it ensures
that you can only use the pointer to access the `struct device` when
the `struct miscdevice` is still valid, which should be okay.

To grab a refcount, you need the ARef<_> pointer type. Callers can do

let device = ARef::from(miscdevice.device());

and now device is a value of type ARef<Device> which owns a refcount
and drops it when the destructor runs.

Alice

Re: [PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device

[Boqun Feng]

On Fri, Dec 06, 2024 at 07:33:09AM +0000, Lee Jones wrote:
> On Fri, 06 Dec 2024, Lee Jones wrote:
> 
> > On Fri, 06 Dec 2024, Greg KH wrote:
> > 
> > > On Thu, Dec 05, 2024 at 04:25:18PM +0000, Lee Jones wrote:
> > > > There are situations where a pointer to a `struct device` will become
> > > > necessary (e.g. for calling into dev_*() functions).  This accessor
> > > > allows callers to pull this out from the `struct miscdevice`.
> > > > 
> > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > > ---
> > > >  rust/kernel/miscdevice.rs | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> > > > index 7e2a79b3ae26..55340f316006 100644
> > > > --- a/rust/kernel/miscdevice.rs
> > > > +++ b/rust/kernel/miscdevice.rs
> > > > @@ -10,11 +10,13 @@
> > > >  
> > > >  use crate::{
> > > >      bindings,
> > > > +    device::Device,
> > > >      error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
> > > >      prelude::*,
> > > >      str::CStr,
> > > >      types::{ForeignOwnable, Opaque},
> > > >  };
> > > > +
> > > >  use core::{
> > > >      ffi::{c_int, c_long, c_uint, c_ulong},
> > > >      marker::PhantomData,
> > > > @@ -84,6 +86,13 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> > > >      pub fn as_raw(&self) -> *mut bindings::miscdevice {
> > > >          self.inner.get()
> > > >      }
> > > > +
> > > > +    /// Returns a pointer to the current Device
> > > > +    pub fn device(&self) -> &Device {
> > > > +        // SAFETY: This is only accessible after a successful register() which always
> > > > +        // initialises this_device with a valid device.
> > > > +        unsafe { Device::as_ref((*self.as_raw()).this_device) }
> > > 
> > > A "raw" pointer that you can do something with without incrementing the
> > > reference count of it?  Oh wait, no, it's the rust device structure.
> > > If so, why isn't this calling the get_device() interface instead?  That
> > > way it's properly incremented and decremented when it "leaves the scope"
> > > right?
> > > 
> > > Or am I missing something here as to why that wouldn't work and this is
> > > the only way to get access to the 'struct device' of this miscdevice?
> > 
> > Fair point.  I'll speak to Alice.
> 
> Alice isn't available yet, so I may be talking out of turn at this
> point, but I just found this is the Device documentation:
> 
>   /// A `Device` instance represents a valid `struct device` created by the C portion of the kernel.
>   ///
>   /// Instances of this type are always reference-counted, that is, a call to `get_device` ensures
>   /// that the allocation remains valid at least until the matching call to `put_device`.
> 
> And:
> 
>   // SAFETY: Instances of `Device` are always reference-counted.
> 
> Ready for some analysis from this beginner?
> 
> Since this impl for Device is AlwaysRefCounted, when any references are
> taken i.e. in the Device::as_ref line above, inc_ref() is implicitly
> called to increase the refcount.  The same will be true of dec_ref()

No, inc_ref() is not called implicitly in Device::as_ref().

The thing that might "keep" the original `miscdevice::Device` alive is
the lifetime, since the returned `device::Device` reference has the
same life at the input parameter `miscdevice::Device` reference (i.e.
`&self`), so the returned reference cannot outlive `&self`. That means
if compilers find `&self` go out of scope while the returned reference
be still alive, it will report an error.

Regards,
Boqun

> once it goes out of scope.
> 
>   // SAFETY: Instances of `Device` are always reference-counted.
>   unsafe impl crate::types::AlwaysRefCounted for Device {
>       fn inc_ref(&self) {
>           // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
>           unsafe { bindings::get_device(self.as_raw()) };
>       }
> 
>       unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>           // SAFETY: The safety requirements guarantee that the refcount is non-zero.
>           unsafe { bindings::put_device(obj.cast().as_ptr()) }
>   }
> 
> -- 
> Lee Jones [李琼斯]

Re: [PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device

[Lee Jones]

On Fri, 06 Dec 2024, Boqun Feng wrote:

> On Fri, Dec 06, 2024 at 07:33:09AM +0000, Lee Jones wrote:
> > On Fri, 06 Dec 2024, Lee Jones wrote:
> > 
> > > On Fri, 06 Dec 2024, Greg KH wrote:
> > > 
> > > > On Thu, Dec 05, 2024 at 04:25:18PM +0000, Lee Jones wrote:
> > > > > There are situations where a pointer to a `struct device` will become
> > > > > necessary (e.g. for calling into dev_*() functions).  This accessor
> > > > > allows callers to pull this out from the `struct miscdevice`.
> > > > > 
> > > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > > > ---
> > > > >  rust/kernel/miscdevice.rs | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > > 
> > > > > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> > > > > index 7e2a79b3ae26..55340f316006 100644
> > > > > --- a/rust/kernel/miscdevice.rs
> > > > > +++ b/rust/kernel/miscdevice.rs
> > > > > @@ -10,11 +10,13 @@
> > > > >  
> > > > >  use crate::{
> > > > >      bindings,
> > > > > +    device::Device,
> > > > >      error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
> > > > >      prelude::*,
> > > > >      str::CStr,
> > > > >      types::{ForeignOwnable, Opaque},
> > > > >  };
> > > > > +
> > > > >  use core::{
> > > > >      ffi::{c_int, c_long, c_uint, c_ulong},
> > > > >      marker::PhantomData,
> > > > > @@ -84,6 +86,13 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> > > > >      pub fn as_raw(&self) -> *mut bindings::miscdevice {
> > > > >          self.inner.get()
> > > > >      }
> > > > > +
> > > > > +    /// Returns a pointer to the current Device
> > > > > +    pub fn device(&self) -> &Device {
> > > > > +        // SAFETY: This is only accessible after a successful register() which always
> > > > > +        // initialises this_device with a valid device.
> > > > > +        unsafe { Device::as_ref((*self.as_raw()).this_device) }
> > > > 
> > > > A "raw" pointer that you can do something with without incrementing the
> > > > reference count of it?  Oh wait, no, it's the rust device structure.
> > > > If so, why isn't this calling the get_device() interface instead?  That
> > > > way it's properly incremented and decremented when it "leaves the scope"
> > > > right?
> > > > 
> > > > Or am I missing something here as to why that wouldn't work and this is
> > > > the only way to get access to the 'struct device' of this miscdevice?
> > > 
> > > Fair point.  I'll speak to Alice.
> > 
> > Alice isn't available yet, so I may be talking out of turn at this
> > point, but I just found this is the Device documentation:
> > 
> >   /// A `Device` instance represents a valid `struct device` created by the C portion of the kernel.
> >   ///
> >   /// Instances of this type are always reference-counted, that is, a call to `get_device` ensures
> >   /// that the allocation remains valid at least until the matching call to `put_device`.
> > 
> > And:
> > 
> >   // SAFETY: Instances of `Device` are always reference-counted.
> > 
> > Ready for some analysis from this beginner?
> > 
> > Since this impl for Device is AlwaysRefCounted, when any references are
> > taken i.e. in the Device::as_ref line above, inc_ref() is implicitly
> > called to increase the refcount.  The same will be true of dec_ref()
> 
> No, inc_ref() is not called implicitly in Device::as_ref().
> 
> The thing that might "keep" the original `miscdevice::Device` alive is
> the lifetime, since the returned `device::Device` reference has the
> same life at the input parameter `miscdevice::Device` reference (i.e.
> `&self`), so the returned reference cannot outlive `&self`. That means
> if compilers find `&self` go out of scope while the returned reference
> be still alive, it will report an error.

Okay, so is there something I need to do to ensure we increase the
refcount?  Does inc_ref() need calling manually?

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device

[Boqun Feng]

On Fri, Dec 06, 2024 at 08:07:51AM +0000, Lee Jones wrote:
> On Fri, 06 Dec 2024, Boqun Feng wrote:
> 
> > On Fri, Dec 06, 2024 at 07:33:09AM +0000, Lee Jones wrote:
> > > On Fri, 06 Dec 2024, Lee Jones wrote:
> > > 
> > > > On Fri, 06 Dec 2024, Greg KH wrote:
> > > > 
> > > > > On Thu, Dec 05, 2024 at 04:25:18PM +0000, Lee Jones wrote:
> > > > > > There are situations where a pointer to a `struct device` will become
> > > > > > necessary (e.g. for calling into dev_*() functions).  This accessor
> > > > > > allows callers to pull this out from the `struct miscdevice`.
> > > > > > 
> > > > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > > > > ---
> > > > > >  rust/kernel/miscdevice.rs | 9 +++++++++
> > > > > >  1 file changed, 9 insertions(+)
> > > > > > 
> > > > > > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> > > > > > index 7e2a79b3ae26..55340f316006 100644
> > > > > > --- a/rust/kernel/miscdevice.rs
> > > > > > +++ b/rust/kernel/miscdevice.rs
> > > > > > @@ -10,11 +10,13 @@
> > > > > >  
> > > > > >  use crate::{
> > > > > >      bindings,
> > > > > > +    device::Device,
> > > > > >      error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
> > > > > >      prelude::*,
> > > > > >      str::CStr,
> > > > > >      types::{ForeignOwnable, Opaque},
> > > > > >  };
> > > > > > +
> > > > > >  use core::{
> > > > > >      ffi::{c_int, c_long, c_uint, c_ulong},
> > > > > >      marker::PhantomData,
> > > > > > @@ -84,6 +86,13 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> > > > > >      pub fn as_raw(&self) -> *mut bindings::miscdevice {
> > > > > >          self.inner.get()
> > > > > >      }
> > > > > > +
> > > > > > +    /// Returns a pointer to the current Device
> > > > > > +    pub fn device(&self) -> &Device {
> > > > > > +        // SAFETY: This is only accessible after a successful register() which always
> > > > > > +        // initialises this_device with a valid device.
> > > > > > +        unsafe { Device::as_ref((*self.as_raw()).this_device) }
> > > > > 
> > > > > A "raw" pointer that you can do something with without incrementing the
> > > > > reference count of it?  Oh wait, no, it's the rust device structure.
> > > > > If so, why isn't this calling the get_device() interface instead?  That
> > > > > way it's properly incremented and decremented when it "leaves the scope"
> > > > > right?
> > > > > 
> > > > > Or am I missing something here as to why that wouldn't work and this is
> > > > > the only way to get access to the 'struct device' of this miscdevice?
> > > > 
> > > > Fair point.  I'll speak to Alice.
> > > 
> > > Alice isn't available yet, so I may be talking out of turn at this
> > > point, but I just found this is the Device documentation:
> > > 
> > >   /// A `Device` instance represents a valid `struct device` created by the C portion of the kernel.
> > >   ///
> > >   /// Instances of this type are always reference-counted, that is, a call to `get_device` ensures
> > >   /// that the allocation remains valid at least until the matching call to `put_device`.
> > > 
> > > And:
> > > 
> > >   // SAFETY: Instances of `Device` are always reference-counted.
> > > 
> > > Ready for some analysis from this beginner?
> > > 
> > > Since this impl for Device is AlwaysRefCounted, when any references are
> > > taken i.e. in the Device::as_ref line above, inc_ref() is implicitly
> > > called to increase the refcount.  The same will be true of dec_ref()
> > 
> > No, inc_ref() is not called implicitly in Device::as_ref().
> > 
> > The thing that might "keep" the original `miscdevice::Device` alive is
> > the lifetime, since the returned `device::Device` reference has the
> > same life at the input parameter `miscdevice::Device` reference (i.e.
> > `&self`), so the returned reference cannot outlive `&self`. That means
> > if compilers find `&self` go out of scope while the returned reference
> > be still alive, it will report an error.
> 
> Okay, so is there something I need to do to ensure we increase the
> refcount?  Does inc_ref() need calling manually?
> 

When you convert a `&Device` into a `ARef<Device>`, Device::inc_ref()
will be called. You can do that with:

	ARef::from(Device::as_ref((*self.as_raw()).this_device))

You will also need to change the return type. And when an `ARef<Device>`
goes out of scope, dec_ref() will be called. 


I had an old patch for a bit document on this part:

	https://lore.kernel.org/rust-for-linux/20240710032447.2161189-1-boqun.feng@gmail.com/

maybe I should send a re-spin.

Regards,
Boqun

> -- 
> Lee Jones [李琼斯]

Re: [PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device

[Lee Jones]

On Fri, 06 Dec 2024, Boqun Feng wrote:

> On Fri, Dec 06, 2024 at 08:07:51AM +0000, Lee Jones wrote:
> > On Fri, 06 Dec 2024, Boqun Feng wrote:
> > 
> > > On Fri, Dec 06, 2024 at 07:33:09AM +0000, Lee Jones wrote:
> > > > On Fri, 06 Dec 2024, Lee Jones wrote:
> > > > 
> > > > > On Fri, 06 Dec 2024, Greg KH wrote:
> > > > > 
> > > > > > On Thu, Dec 05, 2024 at 04:25:18PM +0000, Lee Jones wrote:
> > > > > > > There are situations where a pointer to a `struct device` will become
> > > > > > > necessary (e.g. for calling into dev_*() functions).  This accessor
> > > > > > > allows callers to pull this out from the `struct miscdevice`.
> > > > > > > 
> > > > > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > > > > > ---
> > > > > > >  rust/kernel/miscdevice.rs | 9 +++++++++
> > > > > > >  1 file changed, 9 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> > > > > > > index 7e2a79b3ae26..55340f316006 100644
> > > > > > > --- a/rust/kernel/miscdevice.rs
> > > > > > > +++ b/rust/kernel/miscdevice.rs
> > > > > > > @@ -10,11 +10,13 @@
> > > > > > >  
> > > > > > >  use crate::{
> > > > > > >      bindings,
> > > > > > > +    device::Device,
> > > > > > >      error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
> > > > > > >      prelude::*,
> > > > > > >      str::CStr,
> > > > > > >      types::{ForeignOwnable, Opaque},
> > > > > > >  };
> > > > > > > +
> > > > > > >  use core::{
> > > > > > >      ffi::{c_int, c_long, c_uint, c_ulong},
> > > > > > >      marker::PhantomData,
> > > > > > > @@ -84,6 +86,13 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> > > > > > >      pub fn as_raw(&self) -> *mut bindings::miscdevice {
> > > > > > >          self.inner.get()
> > > > > > >      }
> > > > > > > +
> > > > > > > +    /// Returns a pointer to the current Device
> > > > > > > +    pub fn device(&self) -> &Device {
> > > > > > > +        // SAFETY: This is only accessible after a successful register() which always
> > > > > > > +        // initialises this_device with a valid device.
> > > > > > > +        unsafe { Device::as_ref((*self.as_raw()).this_device) }
> > > > > > 
> > > > > > A "raw" pointer that you can do something with without incrementing the
> > > > > > reference count of it?  Oh wait, no, it's the rust device structure.
> > > > > > If so, why isn't this calling the get_device() interface instead?  That
> > > > > > way it's properly incremented and decremented when it "leaves the scope"
> > > > > > right?
> > > > > > 
> > > > > > Or am I missing something here as to why that wouldn't work and this is
> > > > > > the only way to get access to the 'struct device' of this miscdevice?
> > > > > 
> > > > > Fair point.  I'll speak to Alice.
> > > > 
> > > > Alice isn't available yet, so I may be talking out of turn at this
> > > > point, but I just found this is the Device documentation:
> > > > 
> > > >   /// A `Device` instance represents a valid `struct device` created by the C portion of the kernel.
> > > >   ///
> > > >   /// Instances of this type are always reference-counted, that is, a call to `get_device` ensures
> > > >   /// that the allocation remains valid at least until the matching call to `put_device`.
> > > > 
> > > > And:
> > > > 
> > > >   // SAFETY: Instances of `Device` are always reference-counted.
> > > > 
> > > > Ready for some analysis from this beginner?
> > > > 
> > > > Since this impl for Device is AlwaysRefCounted, when any references are
> > > > taken i.e. in the Device::as_ref line above, inc_ref() is implicitly
> > > > called to increase the refcount.  The same will be true of dec_ref()
> > > 
> > > No, inc_ref() is not called implicitly in Device::as_ref().
> > > 
> > > The thing that might "keep" the original `miscdevice::Device` alive is
> > > the lifetime, since the returned `device::Device` reference has the
> > > same life at the input parameter `miscdevice::Device` reference (i.e.
> > > `&self`), so the returned reference cannot outlive `&self`. That means
> > > if compilers find `&self` go out of scope while the returned reference
> > > be still alive, it will report an error.
> > 
> > Okay, so is there something I need to do to ensure we increase the
> > refcount?  Does inc_ref() need calling manually?
> > 
> 
> When you convert a `&Device` into a `ARef<Device>`, Device::inc_ref()
> will be called. You can do that with:
> 
> 	ARef::from(Device::as_ref((*self.as_raw()).this_device))
> 
> You will also need to change the return type. And when an `ARef<Device>`
> goes out of scope, dec_ref() will be called. 

I have been reliably assured by Alice that we don't need to refcount here.

> I had an old patch for a bit document on this part:
> 
> 	https://lore.kernel.org/rust-for-linux/20240710032447.2161189-1-boqun.feng@gmail.com/
> 
> maybe I should send a re-spin.

Very nice!  Yeah, it would be a shame for all that work to go to waste.

-- 
Lee Jones [李琼斯]

[PATCH v3 2/5] Documentation: ioctl-number: Carve out some identifiers for use by sample drivers

[Lee Jones]

32 IDs should be plenty (at yet, not too greedy) since a lot of sample
drivers will use their own subsystem allocations.

Sample drivers predominately reside in <KERNEL_ROOT>/samples, but there
should be no issue with in-place example drivers using them.

Signed-off-by: Lee Jones <lee@kernel.org>
---
 Documentation/userspace-api/ioctl/ioctl-number.rst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 243f1f1b554a..dc4bc0cab69f 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -311,6 +311,7 @@ Code  Seq#    Include File                                           Comments
                                                                      <mailto:oe@port.de>
 'z'   10-4F  drivers/s390/crypto/zcrypt_api.h                        conflict!
 '|'   00-7F  linux/media.h
+'|'   80-9F  samples/                                                Any sample and example drivers
 0x80  00-1F  linux/fb.h
 0x81  00-1F  linux/vduse.h
 0x89  00-06  arch/x86/include/asm/sockios.h
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 2/5] Documentation: ioctl-number: Carve out some identifiers for use by sample drivers

[Greg KH]

On Thu, Dec 05, 2024 at 04:25:19PM +0000, Lee Jones wrote:
> 32 IDs should be plenty (at yet, not too greedy) since a lot of sample
> drivers will use their own subsystem allocations.
> 
> Sample drivers predominately reside in <KERNEL_ROOT>/samples, but there
> should be no issue with in-place example drivers using them.
> 
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
>  Documentation/userspace-api/ioctl/ioctl-number.rst | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 243f1f1b554a..dc4bc0cab69f 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -311,6 +311,7 @@ Code  Seq#    Include File                                           Comments
>                                                                       <mailto:oe@port.de>
>  'z'   10-4F  drivers/s390/crypto/zcrypt_api.h                        conflict!
>  '|'   00-7F  linux/media.h
> +'|'   80-9F  samples/                                                Any sample and example drivers

This is fine, but why does the sample driver use "R" as the key instead?

thanks,

greg k-h

Re: [PATCH v3 2/5] Documentation: ioctl-number: Carve out some identifiers for use by sample drivers

[Lee Jones]

On Fri, 06 Dec 2024, Greg KH wrote:

> On Thu, Dec 05, 2024 at 04:25:19PM +0000, Lee Jones wrote:
> > 32 IDs should be plenty (at yet, not too greedy) since a lot of sample
> > drivers will use their own subsystem allocations.
> > 
> > Sample drivers predominately reside in <KERNEL_ROOT>/samples, but there
> > should be no issue with in-place example drivers using them.
> > 
> > Signed-off-by: Lee Jones <lee@kernel.org>
> > ---
> >  Documentation/userspace-api/ioctl/ioctl-number.rst | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > index 243f1f1b554a..dc4bc0cab69f 100644
> > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > @@ -311,6 +311,7 @@ Code  Seq#    Include File                                           Comments
> >                                                                       <mailto:oe@port.de>
> >  'z'   10-4F  drivers/s390/crypto/zcrypt_api.h                        conflict!
> >  '|'   00-7F  linux/media.h
> > +'|'   80-9F  samples/                                                Any sample and example drivers
> 
> This is fine, but why does the sample driver use "R" as the key instead?

You're right, I need to change that (and the example user app) over
to use this new keyword as well.  Thanks for noticing.

-- 
Lee Jones [李琼斯]

[PATCH v3 3/5] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Lee Jones]

This sample driver demonstrates the following basic operations:

* Register a Misc Device
* Create /dev/rust-misc-device
* Open the aforementioned character device
* Operate on the character device via a simple ioctl()
* Close the character device

Signed-off-by: Lee Jones <lee@kernel.org>
---
 samples/rust/Kconfig             | 10 ++++
 samples/rust/Makefile            |  1 +
 samples/rust/rust_misc_device.rs | 80 ++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+)
 create mode 100644 samples/rust/rust_misc_device.rs

diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index b0f74a81c8f9..df384e679901 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -20,6 +20,16 @@ config SAMPLE_RUST_MINIMAL
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_MISC_DEVICE
+	tristate "Misc device"
+	help
+	  This option builds the Rust misc device.
+
+	  To compile this as a module, choose M here:
+	  the module will be called rust_misc_device.
+
+	  If unsure, say N.
+
 config SAMPLE_RUST_PRINT
 	tristate "Printing macros"
 	help
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index c1a5c1655395..ad4b97a98580 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -2,6 +2,7 @@
 ccflags-y += -I$(src)				# needed for trace events
 
 obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
+obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
 obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
 
 rust_print-y := rust_print_main.o rust_print_events.o
diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
new file mode 100644
index 000000000000..f50925713f1a
--- /dev/null
+++ b/samples/rust/rust_misc_device.rs
@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 Google LLC.
+
+//! Rust misc device sample.
+
+use kernel::{
+    c_str,
+    ioctl::_IO,
+    miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
+    prelude::*,
+};
+
+const RUST_MISC_DEV_HELLO: u32 = _IO('R' as u32, 9);
+
+module! {
+    type: RustMiscDeviceModule,
+    name: "rust_misc_device",
+    author: "Lee Jones",
+    description: "Rust misc device sample",
+    license: "GPL",
+}
+
+struct RustMiscDeviceModule {
+    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
+}
+
+impl kernel::Module for RustMiscDeviceModule {
+    fn init(_module: &'static ThisModule) -> Result<Self> {
+        pr_info!("Initialising Rust Misc Device Sample\n");
+
+        let options = MiscDeviceOptions {
+            name: c_str!("rust-misc-device"),
+        };
+
+        Ok(Self {
+            _miscdev: KBox::pin_init(
+                MiscDeviceRegistration::<RustMiscDevice>::register(options),
+                GFP_KERNEL,
+            )?,
+        })
+    }
+}
+
+struct RustMiscDevice;
+
+#[vtable]
+impl MiscDevice for RustMiscDevice {
+    type Ptr = KBox<Self>;
+
+    fn open() -> Result<KBox<Self>> {
+        pr_info!("Opening Rust Misc Device Sample\n");
+
+        Ok(KBox::new(RustMiscDevice, GFP_KERNEL)?)
+    }
+
+    fn ioctl(
+        _device: <Self::Ptr as kernel::types::ForeignOwnable>::Borrowed<'_>,
+        cmd: u32,
+        _arg: usize,
+    ) -> Result<isize> {
+        pr_info!("IOCTLing Rust Misc Device Sample\n");
+
+        match cmd {
+            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
+            _ => {
+                pr_err!("IOCTL not recognised: {}\n", cmd);
+                return Err(ENOIOCTLCMD);
+            }
+        }
+
+        Ok(0)
+    }
+}
+
+impl Drop for RustMiscDevice {
+    fn drop(&mut self) {
+        pr_info!("Exiting the Rust Misc Device Sample\n");
+    }
+}
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 3/5] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Greg KH]

On Thu, Dec 05, 2024 at 04:25:20PM +0000, Lee Jones wrote:
> This sample driver demonstrates the following basic operations:
> 
> * Register a Misc Device
> * Create /dev/rust-misc-device
> * Open the aforementioned character device
> * Operate on the character device via a simple ioctl()
> * Close the character device

Nit, the driver doesn't demonstrate open/close, it "provides" open/close
functionality if someone wants to use it :)

> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
>  samples/rust/Kconfig             | 10 ++++
>  samples/rust/Makefile            |  1 +
>  samples/rust/rust_misc_device.rs | 80 ++++++++++++++++++++++++++++++++
>  3 files changed, 91 insertions(+)
>  create mode 100644 samples/rust/rust_misc_device.rs
> 
> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> index b0f74a81c8f9..df384e679901 100644
> --- a/samples/rust/Kconfig
> +++ b/samples/rust/Kconfig
> @@ -20,6 +20,16 @@ config SAMPLE_RUST_MINIMAL
>  
>  	  If unsure, say N.
>  
> +config SAMPLE_RUST_MISC_DEVICE
> +	tristate "Misc device"
> +	help
> +	  This option builds the Rust misc device.
> +
> +	  To compile this as a module, choose M here:
> +	  the module will be called rust_misc_device.
> +
> +	  If unsure, say N.
> +
>  config SAMPLE_RUST_PRINT
>  	tristate "Printing macros"
>  	help
> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> index c1a5c1655395..ad4b97a98580 100644
> --- a/samples/rust/Makefile
> +++ b/samples/rust/Makefile
> @@ -2,6 +2,7 @@
>  ccflags-y += -I$(src)				# needed for trace events
>  
>  obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
> +obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
>  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
>  
>  rust_print-y := rust_print_main.o rust_print_events.o
> diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> new file mode 100644
> index 000000000000..f50925713f1a
> --- /dev/null
> +++ b/samples/rust/rust_misc_device.rs
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 Google LLC.
> +
> +//! Rust misc device sample.
> +
> +use kernel::{
> +    c_str,
> +    ioctl::_IO,
> +    miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
> +    prelude::*,
> +};
> +
> +const RUST_MISC_DEV_HELLO: u32 = _IO('R' as u32, 9);
> +
> +module! {
> +    type: RustMiscDeviceModule,
> +    name: "rust_misc_device",
> +    author: "Lee Jones",
> +    description: "Rust misc device sample",
> +    license: "GPL",
> +}
> +
> +struct RustMiscDeviceModule {
> +    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
> +}
> +
> +impl kernel::Module for RustMiscDeviceModule {
> +    fn init(_module: &'static ThisModule) -> Result<Self> {
> +        pr_info!("Initialising Rust Misc Device Sample\n");
> +
> +        let options = MiscDeviceOptions {
> +            name: c_str!("rust-misc-device"),
> +        };
> +
> +        Ok(Self {
> +            _miscdev: KBox::pin_init(
> +                MiscDeviceRegistration::<RustMiscDevice>::register(options),
> +                GFP_KERNEL,
> +            )?,
> +        })
> +    }
> +}
> +
> +struct RustMiscDevice;
> +
> +#[vtable]
> +impl MiscDevice for RustMiscDevice {
> +    type Ptr = KBox<Self>;
> +
> +    fn open() -> Result<KBox<Self>> {
> +        pr_info!("Opening Rust Misc Device Sample\n");

I'd prefer this to be dev_info() to start with please.

thanks,

greg k-h

Re: [PATCH v3 3/5] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Lee Jones]

On Fri, 06 Dec 2024, Greg KH wrote:

> On Thu, Dec 05, 2024 at 04:25:20PM +0000, Lee Jones wrote:
> > This sample driver demonstrates the following basic operations:
> > 
> > * Register a Misc Device
> > * Create /dev/rust-misc-device
> > * Open the aforementioned character device
> > * Operate on the character device via a simple ioctl()
> > * Close the character device
> 
> Nit, the driver doesn't demonstrate open/close, it "provides" open/close
> functionality if someone wants to use it :)

Okay!  :)

> > Signed-off-by: Lee Jones <lee@kernel.org>
> > ---
> >  samples/rust/Kconfig             | 10 ++++
> >  samples/rust/Makefile            |  1 +
> >  samples/rust/rust_misc_device.rs | 80 ++++++++++++++++++++++++++++++++
> >  3 files changed, 91 insertions(+)
> >  create mode 100644 samples/rust/rust_misc_device.rs
> > 
> > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> > index b0f74a81c8f9..df384e679901 100644
> > --- a/samples/rust/Kconfig
> > +++ b/samples/rust/Kconfig
> > @@ -20,6 +20,16 @@ config SAMPLE_RUST_MINIMAL
> >  
> >  	  If unsure, say N.
> >  
> > +config SAMPLE_RUST_MISC_DEVICE
> > +	tristate "Misc device"
> > +	help
> > +	  This option builds the Rust misc device.
> > +
> > +	  To compile this as a module, choose M here:
> > +	  the module will be called rust_misc_device.
> > +
> > +	  If unsure, say N.
> > +
> >  config SAMPLE_RUST_PRINT
> >  	tristate "Printing macros"
> >  	help
> > diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> > index c1a5c1655395..ad4b97a98580 100644
> > --- a/samples/rust/Makefile
> > +++ b/samples/rust/Makefile
> > @@ -2,6 +2,7 @@
> >  ccflags-y += -I$(src)				# needed for trace events
> >  
> >  obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
> > +obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
> >  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
> >  
> >  rust_print-y := rust_print_main.o rust_print_events.o
> > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> > new file mode 100644
> > index 000000000000..f50925713f1a
> > --- /dev/null
> > +++ b/samples/rust/rust_misc_device.rs
> > @@ -0,0 +1,80 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +// Copyright (C) 2024 Google LLC.
> > +
> > +//! Rust misc device sample.
> > +
> > +use kernel::{
> > +    c_str,
> > +    ioctl::_IO,
> > +    miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
> > +    prelude::*,
> > +};
> > +
> > +const RUST_MISC_DEV_HELLO: u32 = _IO('R' as u32, 9);
> > +
> > +module! {
> > +    type: RustMiscDeviceModule,
> > +    name: "rust_misc_device",
> > +    author: "Lee Jones",
> > +    description: "Rust misc device sample",
> > +    license: "GPL",
> > +}
> > +
> > +struct RustMiscDeviceModule {
> > +    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
> > +}
> > +
> > +impl kernel::Module for RustMiscDeviceModule {
> > +    fn init(_module: &'static ThisModule) -> Result<Self> {
> > +        pr_info!("Initialising Rust Misc Device Sample\n");
> > +
> > +        let options = MiscDeviceOptions {
> > +            name: c_str!("rust-misc-device"),
> > +        };
> > +
> > +        Ok(Self {
> > +            _miscdev: KBox::pin_init(
> > +                MiscDeviceRegistration::<RustMiscDevice>::register(options),
> > +                GFP_KERNEL,
> > +            )?,
> > +        })
> > +    }
> > +}
> > +
> > +struct RustMiscDevice;
> > +
> > +#[vtable]
> > +impl MiscDevice for RustMiscDevice {
> > +    type Ptr = KBox<Self>;
> > +
> > +    fn open() -> Result<KBox<Self>> {
> > +        pr_info!("Opening Rust Misc Device Sample\n");
> 
> I'd prefer this to be dev_info() to start with please.

This is not possible at the moment.  Please see the cover-letter.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 3/5] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Greg KH]

On Fri, Dec 06, 2024 at 07:14:30AM +0000, Lee Jones wrote:
> On Fri, 06 Dec 2024, Greg KH wrote:
> > > +    fn open() -> Result<KBox<Self>> {
> > > +        pr_info!("Opening Rust Misc Device Sample\n");
> > 
> > I'd prefer this to be dev_info() to start with please.
> 
> This is not possible at the moment.  Please see the cover-letter.

Then why make the change to miscdevice.rs if that pointer provided there
doesn't work for you here?

confused,

greg k-h

Re: [PATCH v3 3/5] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Lee Jones]

On Fri, 06 Dec 2024, Greg KH wrote:

> On Fri, Dec 06, 2024 at 07:14:30AM +0000, Lee Jones wrote:
> > On Fri, 06 Dec 2024, Greg KH wrote:
> > > > +    fn open() -> Result<KBox<Self>> {
> > > > +        pr_info!("Opening Rust Misc Device Sample\n");
> > > 
> > > I'd prefer this to be dev_info() to start with please.
> > 
> > This is not possible at the moment.  Please see the cover-letter.
> 
> Then why make the change to miscdevice.rs if that pointer provided there
> doesn't work for you here?

It's half the puzzle to get it working.  We're waiting on the other
(more complex) part from Alice before we can make use of it.  Would you
like me to remove it from the set until we have all of the pieces?

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 3/5] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Greg KH]

On Fri, Dec 06, 2024 at 07:35:58AM +0000, Lee Jones wrote:
> On Fri, 06 Dec 2024, Greg KH wrote:
> 
> > On Fri, Dec 06, 2024 at 07:14:30AM +0000, Lee Jones wrote:
> > > On Fri, 06 Dec 2024, Greg KH wrote:
> > > > > +    fn open() -> Result<KBox<Self>> {
> > > > > +        pr_info!("Opening Rust Misc Device Sample\n");
> > > > 
> > > > I'd prefer this to be dev_info() to start with please.
> > > 
> > > This is not possible at the moment.  Please see the cover-letter.
> > 
> > Then why make the change to miscdevice.rs if that pointer provided there
> > doesn't work for you here?
> 
> It's half the puzzle to get it working.  We're waiting on the other
> (more complex) part from Alice before we can make use of it.  Would you
> like me to remove it from the set until we have all of the pieces?

I'm confused, if you have the reference here, what is preventing it from
being used?

And yes, if it doesn't work, it shouldn't be part of this series :)

thanks,

greg k-h

Re: [PATCH v3 3/5] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Lee Jones]

On Fri, 06 Dec 2024, Greg KH wrote:

> On Fri, Dec 06, 2024 at 07:35:58AM +0000, Lee Jones wrote:
> > On Fri, 06 Dec 2024, Greg KH wrote:
> > 
> > > On Fri, Dec 06, 2024 at 07:14:30AM +0000, Lee Jones wrote:
> > > > On Fri, 06 Dec 2024, Greg KH wrote:
> > > > > > +    fn open() -> Result<KBox<Self>> {
> > > > > > +        pr_info!("Opening Rust Misc Device Sample\n");
> > > > > 
> > > > > I'd prefer this to be dev_info() to start with please.
> > > > 
> > > > This is not possible at the moment.  Please see the cover-letter.
> > > 
> > > Then why make the change to miscdevice.rs if that pointer provided there
> > > doesn't work for you here?
> > 
> > It's half the puzzle to get it working.  We're waiting on the other
> > (more complex) part from Alice before we can make use of it.  Would you
> > like me to remove it from the set until we have all of the pieces?
> 
> I'm confused, if you have the reference here, what is preventing it from
> being used?
> 
> And yes, if it doesn't work, it shouldn't be part of this series :)

It works - we can use dev_*() in .init, but not after (i.e. in open(),
ioctl(), etc).

I'll pull it from the series and let Alice re-post it when we have all
of the parts.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 3/5] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Greg KH]

On Thu, Dec 05, 2024 at 04:25:20PM +0000, Lee Jones wrote:
> This sample driver demonstrates the following basic operations:
> 
> * Register a Misc Device
> * Create /dev/rust-misc-device
> * Open the aforementioned character device
> * Operate on the character device via a simple ioctl()
> * Close the character device
> 
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
>  samples/rust/Kconfig             | 10 ++++
>  samples/rust/Makefile            |  1 +
>  samples/rust/rust_misc_device.rs | 80 ++++++++++++++++++++++++++++++++
>  3 files changed, 91 insertions(+)
>  create mode 100644 samples/rust/rust_misc_device.rs
> 
> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> index b0f74a81c8f9..df384e679901 100644
> --- a/samples/rust/Kconfig
> +++ b/samples/rust/Kconfig
> @@ -20,6 +20,16 @@ config SAMPLE_RUST_MINIMAL
>  
>  	  If unsure, say N.
>  
> +config SAMPLE_RUST_MISC_DEVICE
> +	tristate "Misc device"
> +	help
> +	  This option builds the Rust misc device.
> +
> +	  To compile this as a module, choose M here:
> +	  the module will be called rust_misc_device.
> +
> +	  If unsure, say N.
> +
>  config SAMPLE_RUST_PRINT
>  	tristate "Printing macros"
>  	help
> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> index c1a5c1655395..ad4b97a98580 100644
> --- a/samples/rust/Makefile
> +++ b/samples/rust/Makefile
> @@ -2,6 +2,7 @@
>  ccflags-y += -I$(src)				# needed for trace events
>  
>  obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
> +obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
>  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
>  
>  rust_print-y := rust_print_main.o rust_print_events.o
> diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> new file mode 100644
> index 000000000000..f50925713f1a
> --- /dev/null
> +++ b/samples/rust/rust_misc_device.rs
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 Google LLC.
> +
> +//! Rust misc device sample.
> +
> +use kernel::{
> +    c_str,
> +    ioctl::_IO,
> +    miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
> +    prelude::*,
> +};
> +
> +const RUST_MISC_DEV_HELLO: u32 = _IO('R' as u32, 9);
> +
> +module! {
> +    type: RustMiscDeviceModule,
> +    name: "rust_misc_device",
> +    author: "Lee Jones",
> +    description: "Rust misc device sample",
> +    license: "GPL",
> +}
> +
> +struct RustMiscDeviceModule {
> +    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
> +}
> +
> +impl kernel::Module for RustMiscDeviceModule {
> +    fn init(_module: &'static ThisModule) -> Result<Self> {
> +        pr_info!("Initialising Rust Misc Device Sample\n");
> +
> +        let options = MiscDeviceOptions {
> +            name: c_str!("rust-misc-device"),
> +        };
> +
> +        Ok(Self {
> +            _miscdev: KBox::pin_init(
> +                MiscDeviceRegistration::<RustMiscDevice>::register(options),
> +                GFP_KERNEL,
> +            )?,
> +        })
> +    }
> +}
> +
> +struct RustMiscDevice;
> +
> +#[vtable]
> +impl MiscDevice for RustMiscDevice {
> +    type Ptr = KBox<Self>;
> +
> +    fn open() -> Result<KBox<Self>> {
> +        pr_info!("Opening Rust Misc Device Sample\n");
> +
> +        Ok(KBox::new(RustMiscDevice, GFP_KERNEL)?)
> +    }
> +
> +    fn ioctl(
> +        _device: <Self::Ptr as kernel::types::ForeignOwnable>::Borrowed<'_>,
> +        cmd: u32,
> +        _arg: usize,
> +    ) -> Result<isize> {
> +        pr_info!("IOCTLing Rust Misc Device Sample\n");
> +
> +        match cmd {
> +            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
> +            _ => {
> +                pr_err!("IOCTL not recognised: {}\n", cmd);
> +                return Err(ENOIOCTLCMD);

To quote errno.h:
	These should never be seen by user programs

The correct value here is ENOTTY.

Yeah, fun stuff.  Not intuitive at all, sorry.

thanks,

greg k-h

Re: [PATCH v3 3/5] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Arnd Bergmann]

On Fri, Dec 6, 2024, at 07:49, Greg KH wrote:
> On Thu, Dec 05, 2024 at 04:25:20PM +0000, Lee Jones wrote:
>> +
>> +#[vtable]
>> +impl MiscDevice for RustMiscDevice {
>> +    type Ptr = KBox<Self>;
>> +
>> +    fn open() -> Result<KBox<Self>> {
>> +        pr_info!("Opening Rust Misc Device Sample\n");
>> +
>> +        Ok(KBox::new(RustMiscDevice, GFP_KERNEL)?)
>> +    }
>> +
>> +    fn ioctl(
>> +        _device: <Self::Ptr as kernel::types::ForeignOwnable>::Borrowed<'_>,
>> +        cmd: u32,
>> +        _arg: usize,
>> +    ) -> Result<isize> {
>> +        pr_info!("IOCTLing Rust Misc Device Sample\n");
>> +
>> +        match cmd {
>> +            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
>> +            _ => {
>> +                pr_err!("IOCTL not recognised: {}\n", cmd);
>> +                return Err(ENOIOCTLCMD);
>
> To quote errno.h:
> 	These should never be seen by user programs
>
> The correct value here is ENOTTY.
>
> Yeah, fun stuff.  Not intuitive at all, sorry.

That should get handled by vfs_ioctl() converting the
ENOIOCTLCMD to ENOTTY.

       Arnd

Re: [PATCH v3 3/5] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Greg Kroah-Hartman]

On Fri, Dec 06, 2024 at 07:52:46AM +0100, Arnd Bergmann wrote:
> On Fri, Dec 6, 2024, at 07:49, Greg KH wrote:
> > On Thu, Dec 05, 2024 at 04:25:20PM +0000, Lee Jones wrote:
> >> +
> >> +#[vtable]
> >> +impl MiscDevice for RustMiscDevice {
> >> +    type Ptr = KBox<Self>;
> >> +
> >> +    fn open() -> Result<KBox<Self>> {
> >> +        pr_info!("Opening Rust Misc Device Sample\n");
> >> +
> >> +        Ok(KBox::new(RustMiscDevice, GFP_KERNEL)?)
> >> +    }
> >> +
> >> +    fn ioctl(
> >> +        _device: <Self::Ptr as kernel::types::ForeignOwnable>::Borrowed<'_>,
> >> +        cmd: u32,
> >> +        _arg: usize,
> >> +    ) -> Result<isize> {
> >> +        pr_info!("IOCTLing Rust Misc Device Sample\n");
> >> +
> >> +        match cmd {
> >> +            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
> >> +            _ => {
> >> +                pr_err!("IOCTL not recognised: {}\n", cmd);
> >> +                return Err(ENOIOCTLCMD);
> >
> > To quote errno.h:
> > 	These should never be seen by user programs
> >
> > The correct value here is ENOTTY.
> >
> > Yeah, fun stuff.  Not intuitive at all, sorry.
> 
> That should get handled by vfs_ioctl() converting the
> ENOIOCTLCMD to ENOTTY.

Ah, I always miss that, for some reason I thought that didn't happen
there, but happened in subsystems that did ENOIOCTLCMD for their
sub-ioctl handlers.  And yet it's always been that way, nevermind...

Anyway, I always recommend just using ENOTTY to be "safe", the usual way
people get this wrong is using EINVAL.

thanks,

greg k-h

Re: [PATCH v3 3/5] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Lee Jones]

On Fri, 06 Dec 2024, Greg Kroah-Hartman wrote:

> On Fri, Dec 06, 2024 at 07:52:46AM +0100, Arnd Bergmann wrote:
> > On Fri, Dec 6, 2024, at 07:49, Greg KH wrote:
> > > On Thu, Dec 05, 2024 at 04:25:20PM +0000, Lee Jones wrote:
> > >> +
> > >> +#[vtable]
> > >> +impl MiscDevice for RustMiscDevice {
> > >> +    type Ptr = KBox<Self>;
> > >> +
> > >> +    fn open() -> Result<KBox<Self>> {
> > >> +        pr_info!("Opening Rust Misc Device Sample\n");
> > >> +
> > >> +        Ok(KBox::new(RustMiscDevice, GFP_KERNEL)?)
> > >> +    }
> > >> +
> > >> +    fn ioctl(
> > >> +        _device: <Self::Ptr as kernel::types::ForeignOwnable>::Borrowed<'_>,
> > >> +        cmd: u32,
> > >> +        _arg: usize,
> > >> +    ) -> Result<isize> {
> > >> +        pr_info!("IOCTLing Rust Misc Device Sample\n");
> > >> +
> > >> +        match cmd {
> > >> +            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
> > >> +            _ => {
> > >> +                pr_err!("IOCTL not recognised: {}\n", cmd);
> > >> +                return Err(ENOIOCTLCMD);
> > >
> > > To quote errno.h:
> > > 	These should never be seen by user programs
> > >
> > > The correct value here is ENOTTY.
> > >
> > > Yeah, fun stuff.  Not intuitive at all, sorry.
> > 
> > That should get handled by vfs_ioctl() converting the
> > ENOIOCTLCMD to ENOTTY.
> 
> Ah, I always miss that, for some reason I thought that didn't happen
> there, but happened in subsystems that did ENOIOCTLCMD for their
> sub-ioctl handlers.  And yet it's always been that way, nevermind...
> 
> Anyway, I always recommend just using ENOTTY to be "safe", the usual way
> people get this wrong is using EINVAL.

No matter.  I can change it.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 3/5] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Lee Jones]

On Fri, 06 Dec 2024, Greg KH wrote:

> On Thu, Dec 05, 2024 at 04:25:20PM +0000, Lee Jones wrote:
> > This sample driver demonstrates the following basic operations:
> > 
> > * Register a Misc Device
> > * Create /dev/rust-misc-device
> > * Open the aforementioned character device
> > * Operate on the character device via a simple ioctl()
> > * Close the character device
> > 
> > Signed-off-by: Lee Jones <lee@kernel.org>
> > ---
> >  samples/rust/Kconfig             | 10 ++++
> >  samples/rust/Makefile            |  1 +
> >  samples/rust/rust_misc_device.rs | 80 ++++++++++++++++++++++++++++++++
> >  3 files changed, 91 insertions(+)
> >  create mode 100644 samples/rust/rust_misc_device.rs
> > 
> > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> > index b0f74a81c8f9..df384e679901 100644
> > --- a/samples/rust/Kconfig
> > +++ b/samples/rust/Kconfig
> > @@ -20,6 +20,16 @@ config SAMPLE_RUST_MINIMAL
> >  
> >  	  If unsure, say N.
> >  
> > +config SAMPLE_RUST_MISC_DEVICE
> > +	tristate "Misc device"
> > +	help
> > +	  This option builds the Rust misc device.
> > +
> > +	  To compile this as a module, choose M here:
> > +	  the module will be called rust_misc_device.
> > +
> > +	  If unsure, say N.
> > +
> >  config SAMPLE_RUST_PRINT
> >  	tristate "Printing macros"
> >  	help
> > diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> > index c1a5c1655395..ad4b97a98580 100644
> > --- a/samples/rust/Makefile
> > +++ b/samples/rust/Makefile
> > @@ -2,6 +2,7 @@
> >  ccflags-y += -I$(src)				# needed for trace events
> >  
> >  obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
> > +obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
> >  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
> >  
> >  rust_print-y := rust_print_main.o rust_print_events.o
> > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> > new file mode 100644
> > index 000000000000..f50925713f1a
> > --- /dev/null
> > +++ b/samples/rust/rust_misc_device.rs
> > @@ -0,0 +1,80 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +// Copyright (C) 2024 Google LLC.
> > +
> > +//! Rust misc device sample.
> > +
> > +use kernel::{
> > +    c_str,
> > +    ioctl::_IO,
> > +    miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
> > +    prelude::*,
> > +};
> > +
> > +const RUST_MISC_DEV_HELLO: u32 = _IO('R' as u32, 9);
> > +
> > +module! {
> > +    type: RustMiscDeviceModule,
> > +    name: "rust_misc_device",
> > +    author: "Lee Jones",
> > +    description: "Rust misc device sample",
> > +    license: "GPL",
> > +}
> > +
> > +struct RustMiscDeviceModule {
> > +    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
> > +}
> > +
> > +impl kernel::Module for RustMiscDeviceModule {
> > +    fn init(_module: &'static ThisModule) -> Result<Self> {
> > +        pr_info!("Initialising Rust Misc Device Sample\n");
> > +
> > +        let options = MiscDeviceOptions {
> > +            name: c_str!("rust-misc-device"),
> > +        };
> > +
> > +        Ok(Self {
> > +            _miscdev: KBox::pin_init(
> > +                MiscDeviceRegistration::<RustMiscDevice>::register(options),
> > +                GFP_KERNEL,
> > +            )?,
> > +        })
> > +    }
> > +}
> > +
> > +struct RustMiscDevice;
> > +
> > +#[vtable]
> > +impl MiscDevice for RustMiscDevice {
> > +    type Ptr = KBox<Self>;
> > +
> > +    fn open() -> Result<KBox<Self>> {
> > +        pr_info!("Opening Rust Misc Device Sample\n");
> > +
> > +        Ok(KBox::new(RustMiscDevice, GFP_KERNEL)?)
> > +    }
> > +
> > +    fn ioctl(
> > +        _device: <Self::Ptr as kernel::types::ForeignOwnable>::Borrowed<'_>,
> > +        cmd: u32,
> > +        _arg: usize,
> > +    ) -> Result<isize> {
> > +        pr_info!("IOCTLing Rust Misc Device Sample\n");
> > +
> > +        match cmd {
> > +            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
> > +            _ => {
> > +                pr_err!("IOCTL not recognised: {}\n", cmd);
> > +                return Err(ENOIOCTLCMD);
> 
> To quote errno.h:
> 	These should never be seen by user programs
> 
> The correct value here is ENOTTY.
> 
> Yeah, fun stuff.  Not intuitive at all, sorry.

I read the instructions in: Documentation/driver-api/ioctl.rst

"When the ioctl callback is called with an unknown command number, the
 handler returns either -ENOTTY or -ENOIOCTLCMD, which also results in
 -ENOTTY being returned from the system call. Some subsystems return
 -ENOSYS or -EINVAL here for historic reasons, but this is wrong."

This makes it sound like the system call itself converts one to the
other.  Either way, I'll make the change.  Thanks.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 3/5] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Danilo Krummrich]

On Thu, Dec 05, 2024 at 04:25:20PM +0000, Lee Jones wrote:
> This sample driver demonstrates the following basic operations:
> 
> * Register a Misc Device
> * Create /dev/rust-misc-device
> * Open the aforementioned character device
> * Operate on the character device via a simple ioctl()
> * Close the character device
> 
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
>  samples/rust/Kconfig             | 10 ++++
>  samples/rust/Makefile            |  1 +
>  samples/rust/rust_misc_device.rs | 80 ++++++++++++++++++++++++++++++++
>  3 files changed, 91 insertions(+)
>  create mode 100644 samples/rust/rust_misc_device.rs
> 
> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> index b0f74a81c8f9..df384e679901 100644
> --- a/samples/rust/Kconfig
> +++ b/samples/rust/Kconfig
> @@ -20,6 +20,16 @@ config SAMPLE_RUST_MINIMAL
>  
>  	  If unsure, say N.
>  
> +config SAMPLE_RUST_MISC_DEVICE
> +	tristate "Misc device"
> +	help
> +	  This option builds the Rust misc device.
> +
> +	  To compile this as a module, choose M here:
> +	  the module will be called rust_misc_device.
> +
> +	  If unsure, say N.
> +
>  config SAMPLE_RUST_PRINT
>  	tristate "Printing macros"
>  	help
> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> index c1a5c1655395..ad4b97a98580 100644
> --- a/samples/rust/Makefile
> +++ b/samples/rust/Makefile
> @@ -2,6 +2,7 @@
>  ccflags-y += -I$(src)				# needed for trace events
>  
>  obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
> +obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
>  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
>  
>  rust_print-y := rust_print_main.o rust_print_events.o
> diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> new file mode 100644
> index 000000000000..f50925713f1a
> --- /dev/null
> +++ b/samples/rust/rust_misc_device.rs
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 Google LLC.
> +
> +//! Rust misc device sample.
> +
> +use kernel::{
> +    c_str,
> +    ioctl::_IO,
> +    miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
> +    prelude::*,
> +};
> +
> +const RUST_MISC_DEV_HELLO: u32 = _IO('R' as u32, 9);
> +
> +module! {
> +    type: RustMiscDeviceModule,
> +    name: "rust_misc_device",
> +    author: "Lee Jones",
> +    description: "Rust misc device sample",
> +    license: "GPL",
> +}
> +
> +struct RustMiscDeviceModule {
> +    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,

Why do we add examples where we ask people to allocate those structures with
kmalloc()?

`MiscDevice` should switch to the generic `Registration` type in [1] and use
`InPlaceModule`, such that those structures land in the .data section of the
binary.

[1] https://lore.kernel.org/rust-for-linux/20241205141533.111830-3-dakr@kernel.org/

> +}
> +
> +impl kernel::Module for RustMiscDeviceModule {
> +    fn init(_module: &'static ThisModule) -> Result<Self> {
> +        pr_info!("Initialising Rust Misc Device Sample\n");
> +
> +        let options = MiscDeviceOptions {
> +            name: c_str!("rust-misc-device"),
> +        };
> +
> +        Ok(Self {
> +            _miscdev: KBox::pin_init(
> +                MiscDeviceRegistration::<RustMiscDevice>::register(options),
> +                GFP_KERNEL,
> +            )?,
> +        })
> +    }
> +}
> +
> +struct RustMiscDevice;
> +
> +#[vtable]
> +impl MiscDevice for RustMiscDevice {
> +    type Ptr = KBox<Self>;
> +
> +    fn open() -> Result<KBox<Self>> {
> +        pr_info!("Opening Rust Misc Device Sample\n");
> +
> +        Ok(KBox::new(RustMiscDevice, GFP_KERNEL)?)
> +    }
> +
> +    fn ioctl(
> +        _device: <Self::Ptr as kernel::types::ForeignOwnable>::Borrowed<'_>,
> +        cmd: u32,
> +        _arg: usize,
> +    ) -> Result<isize> {
> +        pr_info!("IOCTLing Rust Misc Device Sample\n");
> +
> +        match cmd {
> +            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
> +            _ => {
> +                pr_err!("IOCTL not recognised: {}\n", cmd);
> +                return Err(ENOIOCTLCMD);
> +            }
> +        }
> +
> +        Ok(0)
> +    }
> +}
> +
> +impl Drop for RustMiscDevice {
> +    fn drop(&mut self) {
> +        pr_info!("Exiting the Rust Misc Device Sample\n");
> +    }
> +}
> -- 
> 2.47.0.338.g60cca15819-goog
> 
> 

[PATCH v3 4/5] sample: rust_misc_device: Demonstrate additional get/set value functionality

[Lee Jones]

Expand the complexity of the sample driver by providing the ability to
get and set an integer.  The value is protected by a mutex.

Here is a simple userspace program that fully exercises the sample
driver's capabilities.

int main() {
  int value, new_value;
  int fd, ret;

  // Open the device file
  printf("Opening /dev/rust-misc-device for reading and writing\n");
  fd = open("/dev/rust-misc-device", O_RDWR);
  if (fd < 0) {
    perror("open");
    return errno;
  }

  // Make call into driver to say "hello"
  printf("Calling Hello\n");
  ret = ioctl(fd, RUST_MISC_DEV_HELLO, NULL);
  if (ret < 0) {
    perror("ioctl: Failed to call into Hello");
    close(fd);
    return errno;
  }

  // Get initial value
  printf("Fetching initial value\n");
  ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &value);
  if (ret < 0) {
    perror("ioctl: Failed to fetch the initial value");
    close(fd);
    return errno;
  }

  value++;

  // Set value to something different
  printf("Submitting new value (%d)\n", value);
  ret = ioctl(fd, RUST_MISC_DEV_SET_VALUE, &value);
  if (ret < 0) {
    perror("ioctl: Failed to submit new value");
    close(fd);
    return errno;
  }

  // Ensure new value was applied
  printf("Fetching new value\n");
  ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &new_value);
  if (ret < 0) {
    perror("ioctl: Failed to fetch the new value");
    close(fd);
    return errno;
  }

  if (value != new_value) {
    printf("Failed: Committed and retrieved values are different (%d - %d)\n", value, new_value);
    close(fd);
    return -1;
  }

  // Call the unsuccessful ioctl
  printf("Attempting to call in to an non-existent IOCTL\n");
  ret = ioctl(fd, RUST_MISC_DEV_FAIL, NULL);
  if (ret < 0) {
    perror("ioctl: Succeeded to fail - this was expected");
  } else {
    printf("ioctl: Failed to fail\n");
    close(fd);
    return -1;
  }

  // Close the device file
  printf("Closing /dev/rust-misc-device\n");
  close(fd);

  printf("Success\n");
  return 0;
}

And here is the output (manually spliced together):

USERSPACE: Opening /dev/rust-misc-device for reading and writing
KERNEL: rust_misc_device: Opening Rust Misc Device Sample
USERSPACE: Calling Hello
KERNEL: rust_misc_device: IOCTLing Rust Misc Device Sample
KERNEL: rust_misc_device: -> Hello from the Rust Misc Device
USERSPACE: Fetching initial value
KERNEL: rust_misc_device: IOCTLing Rust Misc Device Sample
KERNEL: rust_misc_device: -> Copying data to userspace (value: 0)
USERSPACE: Submitting new value (1)
KERNEL: rust_misc_device: IOCTLing Rust Misc Device Sample
KERNEL: rust_misc_device: -> Copying data from userspace (value: 1)
USERSPACE: Fetching new value
KERNEL: rust_misc_device: IOCTLing Rust Misc Device Sample
KERNEL: rust_misc_device: -> Copying data to userspace (value: 1)
USERSPACE: Attempting to call in to an non-existent IOCTL
KERNEL: rust_misc_device: IOCTLing Rust Misc Device Sample
KERNEL: rust_misc_device: -> IOCTL not recognised: 20992
USERSPACE: ioctl: Succeeded to fail - this was expected: Inappropriate ioctl for device
USERSPACE: Closing /dev/rust-misc-device
KERNEL: rust_misc_device: Exiting the Rust Misc Device Sample
USERSPACE: Success

Signed-off-by: Lee Jones <lee@kernel.org>
---
 samples/rust/rust_misc_device.rs | 82 ++++++++++++++++++++++++++------
 1 file changed, 67 insertions(+), 15 deletions(-)

diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index f50925713f1a..2d40e2bb7a59 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -4,13 +4,20 @@
 
 //! Rust misc device sample.
 
+use core::pin::Pin;
+
 use kernel::{
     c_str,
-    ioctl::_IO,
+    ioctl::{_IO, _IOC_SIZE, _IOR, _IOW},
     miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
+    new_mutex,
     prelude::*,
+    sync::Mutex,
+    uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
 };
 
+const RUST_MISC_DEV_GET_VALUE: u32 = _IOR::<i32>('R' as u32, 7);
+const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('R' as u32, 8);
 const RUST_MISC_DEV_HELLO: u32 = _IO('R' as u32, 9);
 
 module! {
@@ -42,39 +49,84 @@ fn init(_module: &'static ThisModule) -> Result<Self> {
     }
 }
 
-struct RustMiscDevice;
+struct Inner {
+    value: i32,
+}
+
+#[pin_data(PinnedDrop)]
+struct RustMiscDevice {
+    #[pin]
+    inner: Mutex<Inner>,
+}
 
 #[vtable]
 impl MiscDevice for RustMiscDevice {
-    type Ptr = KBox<Self>;
+    type Ptr = Pin<KBox<Self>>;
 
-    fn open() -> Result<KBox<Self>> {
+    fn open() -> Result<Pin<KBox<Self>>> {
         pr_info!("Opening Rust Misc Device Sample\n");
 
-        Ok(KBox::new(RustMiscDevice, GFP_KERNEL)?)
+        KBox::try_pin_init(
+            try_pin_init! {
+                RustMiscDevice { inner <- new_mutex!( Inner{ value: 0_i32 } )}
+            },
+            GFP_KERNEL,
+        )
     }
 
-    fn ioctl(
-        _device: <Self::Ptr as kernel::types::ForeignOwnable>::Borrowed<'_>,
-        cmd: u32,
-        _arg: usize,
-    ) -> Result<isize> {
+    fn ioctl(device: Pin<&RustMiscDevice>, cmd: u32, arg: usize) -> Result<isize> {
         pr_info!("IOCTLing Rust Misc Device Sample\n");
 
+        let size = _IOC_SIZE(cmd);
+
         match cmd {
-            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
+            RUST_MISC_DEV_GET_VALUE => device.get_value(UserSlice::new(arg, size).writer())?,
+            RUST_MISC_DEV_SET_VALUE => device.set_value(UserSlice::new(arg, size).reader())?,
+            RUST_MISC_DEV_HELLO => device.hello()?,
             _ => {
-                pr_err!("IOCTL not recognised: {}\n", cmd);
+                pr_err!("-> IOCTL not recognised: {}\n", cmd);
                 return Err(ENOIOCTLCMD);
             }
-        }
+        };
 
         Ok(0)
     }
 }
 
-impl Drop for RustMiscDevice {
-    fn drop(&mut self) {
+#[pinned_drop]
+impl PinnedDrop for RustMiscDevice {
+    fn drop(self: Pin<&mut Self>) {
         pr_info!("Exiting the Rust Misc Device Sample\n");
     }
 }
+
+impl RustMiscDevice {
+    fn set_value(&self, mut reader: UserSliceReader) -> Result<isize> {
+        let new_value = reader.read::<i32>()?;
+        let mut guard = self.inner.lock();
+
+        pr_info!("-> Copying data from userspace (value: {})\n", new_value);
+
+        guard.value = new_value;
+        Ok(0)
+    }
+
+    fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
+        let guard = self.inner.lock();
+        let value = guard.value;
+
+        // Refrain from calling write() on a locked resource
+        drop(guard);
+
+        pr_info!("-> Copying data to userspace (value: {})\n", &value);
+
+        writer.write::<i32>(&value)?;
+        Ok(0)
+    }
+
+    fn hello(&self) -> Result<isize> {
+        pr_info!("-> Hello from the Rust Misc Device\n");
+
+        Ok(0)
+    }
+}
-- 
2.47.0.338.g60cca15819-goog

[PATCH v3 5/5] MAINTAINERS: Add Rust Misc Sample to MISC entry

[Lee Jones]

Signed-off-by: Lee Jones <lee@kernel.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 21f855fe468b..ea5f7c628235 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5328,6 +5328,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
 F:	drivers/char/
 F:	drivers/misc/
 F:	include/linux/miscdevice.h
+F:	samples/rust/rust_misc_device.rs
 X:	drivers/char/agp/
 X:	drivers/char/hw_random/
 X:	drivers/char/ipmi/
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 5/5] MAINTAINERS: Add Rust Misc Sample to MISC entry

[Greg KH]

On Thu, Dec 05, 2024 at 04:25:22PM +0000, Lee Jones wrote:
> Signed-off-by: Lee Jones <lee@kernel.org>

You know I can't take changelog entries without any text :(

> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 21f855fe468b..ea5f7c628235 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5328,6 +5328,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
>  F:	drivers/char/
>  F:	drivers/misc/
>  F:	include/linux/miscdevice.h
> +F:	samples/rust/rust_misc_device.rs

Nice, you are signing me up to maintain it for 20+ years?

{sigh}

greg k-h

Re: [PATCH v3 5/5] MAINTAINERS: Add Rust Misc Sample to MISC entry

[Lee Jones]

On Fri, 06 Dec 2024, Greg KH wrote:

> On Thu, Dec 05, 2024 at 04:25:22PM +0000, Lee Jones wrote:
> > Signed-off-by: Lee Jones <lee@kernel.org>
> 
> You know I can't take changelog entries without any text :(

What more is there to say?  Okay, I'll work something out.

> > ---
> >  MAINTAINERS | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 21f855fe468b..ea5f7c628235 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5328,6 +5328,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
> >  F:	drivers/char/
> >  F:	drivers/misc/
> >  F:	include/linux/miscdevice.h
> > +F:	samples/rust/rust_misc_device.rs
> 
> Nice, you are signing me up to maintain it for 20+ years?

It's fine if you don't want it.

So either I can create a new entry with Alice (with permission of
course) and I as maintainers or just leave it as the default catch-call
of samples/rust/ which is already covered by 'RUST'.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 5/5] MAINTAINERS: Add Rust Misc Sample to MISC entry

[Greg KH]

On Fri, Dec 06, 2024 at 07:07:15AM +0000, Lee Jones wrote:
> On Fri, 06 Dec 2024, Greg KH wrote:
> 
> > On Thu, Dec 05, 2024 at 04:25:22PM +0000, Lee Jones wrote:
> > > Signed-off-by: Lee Jones <lee@kernel.org>
> > 
> > You know I can't take changelog entries without any text :(
> 
> What more is there to say?  Okay, I'll work something out.
> 
> > > ---
> > >  MAINTAINERS | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 21f855fe468b..ea5f7c628235 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -5328,6 +5328,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
> > >  F:	drivers/char/
> > >  F:	drivers/misc/
> > >  F:	include/linux/miscdevice.h
> > > +F:	samples/rust/rust_misc_device.rs
> > 
> > Nice, you are signing me up to maintain it for 20+ years?
> 
> It's fine if you don't want it.
> 
> So either I can create a new entry with Alice (with permission of
> course) and I as maintainers or just leave it as the default catch-call
> of samples/rust/ which is already covered by 'RUST'.

A simple "hey, do you want to maintain this" would have at least been
appreciated.  I can maintain it, but to just assume so is, well...

Re: [PATCH v3 5/5] MAINTAINERS: Add Rust Misc Sample to MISC entry

[Lee Jones]

On Fri, 06 Dec 2024, Greg KH wrote:

> On Fri, Dec 06, 2024 at 07:07:15AM +0000, Lee Jones wrote:
> > On Fri, 06 Dec 2024, Greg KH wrote:
> > 
> > > On Thu, Dec 05, 2024 at 04:25:22PM +0000, Lee Jones wrote:
> > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > 
> > > You know I can't take changelog entries without any text :(
> > 
> > What more is there to say?  Okay, I'll work something out.
> > 
> > > > ---
> > > >  MAINTAINERS | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 21f855fe468b..ea5f7c628235 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -5328,6 +5328,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
> > > >  F:	drivers/char/
> > > >  F:	drivers/misc/
> > > >  F:	include/linux/miscdevice.h
> > > > +F:	samples/rust/rust_misc_device.rs
> > > 
> > > Nice, you are signing me up to maintain it for 20+ years?
> > 
> > It's fine if you don't want it.
> > 
> > So either I can create a new entry with Alice (with permission of
> > course) and I as maintainers or just leave it as the default catch-call
> > of samples/rust/ which is already covered by 'RUST'.
> 
> A simple "hey, do you want to maintain this" would have at least been
> appreciated.

This is it - feel free to NACK. :)

> I can maintain it, but to just assume so is, well...

Sorry if it offended you.  That wasn't the intention.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 0/5] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Greg KH]

On Thu, Dec 05, 2024 at 04:25:17PM +0000, Lee Jones wrote:
> It has been suggested that the driver should use dev_info() instead of
> pr_info() however there is currently no scaffolding to successfully pull
> a 'struct device' out from driver data post register().  This is being
> worked on and we will convert this over in due course.

But the miscdevice.rs change provides this to you, right?  Or if not,
why not?

confused,

greg k-h

Re: [PATCH v3 0/5] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Lee Jones]

On Fri, 06 Dec 2024, Greg KH wrote:

> On Thu, Dec 05, 2024 at 04:25:17PM +0000, Lee Jones wrote:
> > It has been suggested that the driver should use dev_info() instead of
> > pr_info() however there is currently no scaffolding to successfully pull
> > a 'struct device' out from driver data post register().  This is being
> > worked on and we will convert this over in due course.
> 
> But the miscdevice.rs change provides this to you, right?  Or if not,
> why not?

This does allow us to pull the 'struct device *` out from `struct
miscdevice`; however, since this resides in MiscDeviceRegistration,
which we lose access to after .init, we have no means to call it.

Alice is going to work on a way to use ThisModule to get the
MiscDeviceRegistration reference back from anywhere in the module. Until
that piece lands, we can't call MiscDeviceRegistration::device() outside
of RustMiscDeviceModule.

One option that I investigated involved global mutable variable that we
could store the &Device into during MiscDeviceRegistration, however this
got messy (actually just bloaty) fast.  Alice's solution will be
cleaner and more universally useful.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 0/5] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Greg KH]

On Fri, Dec 06, 2024 at 07:44:43AM +0000, Lee Jones wrote:
> On Fri, 06 Dec 2024, Greg KH wrote:
> 
> > On Thu, Dec 05, 2024 at 04:25:17PM +0000, Lee Jones wrote:
> > > It has been suggested that the driver should use dev_info() instead of
> > > pr_info() however there is currently no scaffolding to successfully pull
> > > a 'struct device' out from driver data post register().  This is being
> > > worked on and we will convert this over in due course.
> > 
> > But the miscdevice.rs change provides this to you, right?  Or if not,
> > why not?
> 
> This does allow us to pull the 'struct device *` out from `struct
> miscdevice`; however, since this resides in MiscDeviceRegistration,
> which we lose access to after .init, we have no means to call it.
> 
> Alice is going to work on a way to use ThisModule to get the
> MiscDeviceRegistration reference back from anywhere in the module. Until
> that piece lands, we can't call MiscDeviceRegistration::device() outside
> of RustMiscDeviceModule.

That seems crazy, as ThisModule shouldn't be dealing with a static misc
device, what happens for dynamically created ones like all
normal/sane/non-example drivers do?  This should "just" be a dynamic
object that is NOT tied to the module object, or worst case, a "static"
structure that is tied to the module I guess?

Anyway, I'll let you all work it out, good luck!

greg k-h

Re: [PATCH v3 0/5] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Alice Ryhl]

On Fri, Dec 6, 2024 at 9:11 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Dec 06, 2024 at 07:44:43AM +0000, Lee Jones wrote:
> > On Fri, 06 Dec 2024, Greg KH wrote:
> >
> > > On Thu, Dec 05, 2024 at 04:25:17PM +0000, Lee Jones wrote:
> > > > It has been suggested that the driver should use dev_info() instead of
> > > > pr_info() however there is currently no scaffolding to successfully pull
> > > > a 'struct device' out from driver data post register().  This is being
> > > > worked on and we will convert this over in due course.
> > >
> > > But the miscdevice.rs change provides this to you, right?  Or if not,
> > > why not?
> >
> > This does allow us to pull the 'struct device *` out from `struct
> > miscdevice`; however, since this resides in MiscDeviceRegistration,
> > which we lose access to after .init, we have no means to call it.
> >
> > Alice is going to work on a way to use ThisModule to get the
> > MiscDeviceRegistration reference back from anywhere in the module. Until
> > that piece lands, we can't call MiscDeviceRegistration::device() outside
> > of RustMiscDeviceModule.
>
> That seems crazy, as ThisModule shouldn't be dealing with a static misc
> device, what happens for dynamically created ones like all
> normal/sane/non-example drivers do?  This should "just" be a dynamic
> object that is NOT tied to the module object, or worst case, a "static"
> structure that is tied to the module I guess?
>
> Anyway, I'll let you all work it out, good luck!

If you store it somewhere else, you're probably okay. The current
place is just hard to access.

The problem is that the Rust module abstractions generate a global
variable that holds an RustMiscDeviceModule which is initialized in
init_module() and destroyed in cleanup_module(). To have safe access
to this global, we need to ensure that you access it only between
init_module() and cleanup_module(). For loadable modules, the
try_module_get() logic seems perfect, so in Miscdevice::open we have a
file pointer, which implies that the fs infrastructure took a refcount
on fops->owner, which it can only do once init_module() is done.

Unfortunately, this doesn't translate to built-in modules since the
owner pointer is just null, and try_module_get performs no checks at
all.

Also, I'm realizing now that try_module_get() succeeds even if `state
== MODULE_STATE_COMING`. :(

So in conclusion, I don't know of any way to provide safe access to
the global RustMiscDeviceModule value.

Alice

Re: [PATCH v3 0/5] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Greg KH]

On Fri, Dec 06, 2024 at 09:31:28AM +0100, Alice Ryhl wrote:
> On Fri, Dec 6, 2024 at 9:11 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Dec 06, 2024 at 07:44:43AM +0000, Lee Jones wrote:
> > > On Fri, 06 Dec 2024, Greg KH wrote:
> > >
> > > > On Thu, Dec 05, 2024 at 04:25:17PM +0000, Lee Jones wrote:
> > > > > It has been suggested that the driver should use dev_info() instead of
> > > > > pr_info() however there is currently no scaffolding to successfully pull
> > > > > a 'struct device' out from driver data post register().  This is being
> > > > > worked on and we will convert this over in due course.
> > > >
> > > > But the miscdevice.rs change provides this to you, right?  Or if not,
> > > > why not?
> > >
> > > This does allow us to pull the 'struct device *` out from `struct
> > > miscdevice`; however, since this resides in MiscDeviceRegistration,
> > > which we lose access to after .init, we have no means to call it.
> > >
> > > Alice is going to work on a way to use ThisModule to get the
> > > MiscDeviceRegistration reference back from anywhere in the module. Until
> > > that piece lands, we can't call MiscDeviceRegistration::device() outside
> > > of RustMiscDeviceModule.
> >
> > That seems crazy, as ThisModule shouldn't be dealing with a static misc
> > device, what happens for dynamically created ones like all
> > normal/sane/non-example drivers do?  This should "just" be a dynamic
> > object that is NOT tied to the module object, or worst case, a "static"
> > structure that is tied to the module I guess?
> >
> > Anyway, I'll let you all work it out, good luck!
> 
> If you store it somewhere else, you're probably okay. The current
> place is just hard to access.
> 
> The problem is that the Rust module abstractions generate a global
> variable that holds an RustMiscDeviceModule which is initialized in
> init_module() and destroyed in cleanup_module(). To have safe access
> to this global, we need to ensure that you access it only between
> init_module() and cleanup_module(). For loadable modules, the
> try_module_get() logic seems perfect, so in Miscdevice::open we have a
> file pointer, which implies that the fs infrastructure took a refcount
> on fops->owner, which it can only do once init_module() is done.
> 
> Unfortunately, this doesn't translate to built-in modules since the
> owner pointer is just null, and try_module_get performs no checks at
> all.
> 
> Also, I'm realizing now that try_module_get() succeeds even if `state
> == MODULE_STATE_COMING`. :(
> 
> So in conclusion, I don't know of any way to provide safe access to
> the global RustMiscDeviceModule value.

Odd.  How is this any different than what is going to happen for
platform or other drivers of any other type?  Sometimes they want to
only create one single "static" object and register it with the bus they
are assigned to.

Do we need to have a RuscMiscDevice object somewhere instead that
doesn't care about the module logic at all?  And then just use a
"normal" rust module object to create a single instance of that which
the misc binding will handle?

thanks,

greg k-h

Re: [PATCH v3 0/5] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Alice Ryhl]

On Fri, Dec 6, 2024 at 9:44 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Dec 06, 2024 at 09:31:28AM +0100, Alice Ryhl wrote:
> > On Fri, Dec 6, 2024 at 9:11 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Fri, Dec 06, 2024 at 07:44:43AM +0000, Lee Jones wrote:
> > > > On Fri, 06 Dec 2024, Greg KH wrote:
> > > >
> > > > > On Thu, Dec 05, 2024 at 04:25:17PM +0000, Lee Jones wrote:
> > > > > > It has been suggested that the driver should use dev_info() instead of
> > > > > > pr_info() however there is currently no scaffolding to successfully pull
> > > > > > a 'struct device' out from driver data post register().  This is being
> > > > > > worked on and we will convert this over in due course.
> > > > >
> > > > > But the miscdevice.rs change provides this to you, right?  Or if not,
> > > > > why not?
> > > >
> > > > This does allow us to pull the 'struct device *` out from `struct
> > > > miscdevice`; however, since this resides in MiscDeviceRegistration,
> > > > which we lose access to after .init, we have no means to call it.
> > > >
> > > > Alice is going to work on a way to use ThisModule to get the
> > > > MiscDeviceRegistration reference back from anywhere in the module. Until
> > > > that piece lands, we can't call MiscDeviceRegistration::device() outside
> > > > of RustMiscDeviceModule.
> > >
> > > That seems crazy, as ThisModule shouldn't be dealing with a static misc
> > > device, what happens for dynamically created ones like all
> > > normal/sane/non-example drivers do?  This should "just" be a dynamic
> > > object that is NOT tied to the module object, or worst case, a "static"
> > > structure that is tied to the module I guess?
> > >
> > > Anyway, I'll let you all work it out, good luck!
> >
> > If you store it somewhere else, you're probably okay. The current
> > place is just hard to access.
> >
> > The problem is that the Rust module abstractions generate a global
> > variable that holds an RustMiscDeviceModule which is initialized in
> > init_module() and destroyed in cleanup_module(). To have safe access
> > to this global, we need to ensure that you access it only between
> > init_module() and cleanup_module(). For loadable modules, the
> > try_module_get() logic seems perfect, so in Miscdevice::open we have a
> > file pointer, which implies that the fs infrastructure took a refcount
> > on fops->owner, which it can only do once init_module() is done.
> >
> > Unfortunately, this doesn't translate to built-in modules since the
> > owner pointer is just null, and try_module_get performs no checks at
> > all.
> >
> > Also, I'm realizing now that try_module_get() succeeds even if `state
> > == MODULE_STATE_COMING`. :(
> >
> > So in conclusion, I don't know of any way to provide safe access to
> > the global RustMiscDeviceModule value.
>
> Odd.  How is this any different than what is going to happen for
> platform or other drivers of any other type?  Sometimes they want to
> only create one single "static" object and register it with the bus they
> are assigned to.
>
> Do we need to have a RuscMiscDevice object somewhere instead that
> doesn't care about the module logic at all?  And then just use a
> "normal" rust module object to create a single instance of that which
> the misc binding will handle?

Actually, I guess we can access the miscdevice in open via the pointer
that misc_open() stashes into the file private data. We don't have to
go through the global variable.


Alice

Re: [PATCH v3 0/5] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Greg KH]

On Fri, Dec 06, 2024 at 09:51:55AM +0100, Alice Ryhl wrote:
> On Fri, Dec 6, 2024 at 9:44 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Dec 06, 2024 at 09:31:28AM +0100, Alice Ryhl wrote:
> > > On Fri, Dec 6, 2024 at 9:11 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Fri, Dec 06, 2024 at 07:44:43AM +0000, Lee Jones wrote:
> > > > > On Fri, 06 Dec 2024, Greg KH wrote:
> > > > >
> > > > > > On Thu, Dec 05, 2024 at 04:25:17PM +0000, Lee Jones wrote:
> > > > > > > It has been suggested that the driver should use dev_info() instead of
> > > > > > > pr_info() however there is currently no scaffolding to successfully pull
> > > > > > > a 'struct device' out from driver data post register().  This is being
> > > > > > > worked on and we will convert this over in due course.
> > > > > >
> > > > > > But the miscdevice.rs change provides this to you, right?  Or if not,
> > > > > > why not?
> > > > >
> > > > > This does allow us to pull the 'struct device *` out from `struct
> > > > > miscdevice`; however, since this resides in MiscDeviceRegistration,
> > > > > which we lose access to after .init, we have no means to call it.
> > > > >
> > > > > Alice is going to work on a way to use ThisModule to get the
> > > > > MiscDeviceRegistration reference back from anywhere in the module. Until
> > > > > that piece lands, we can't call MiscDeviceRegistration::device() outside
> > > > > of RustMiscDeviceModule.
> > > >
> > > > That seems crazy, as ThisModule shouldn't be dealing with a static misc
> > > > device, what happens for dynamically created ones like all
> > > > normal/sane/non-example drivers do?  This should "just" be a dynamic
> > > > object that is NOT tied to the module object, or worst case, a "static"
> > > > structure that is tied to the module I guess?
> > > >
> > > > Anyway, I'll let you all work it out, good luck!
> > >
> > > If you store it somewhere else, you're probably okay. The current
> > > place is just hard to access.
> > >
> > > The problem is that the Rust module abstractions generate a global
> > > variable that holds an RustMiscDeviceModule which is initialized in
> > > init_module() and destroyed in cleanup_module(). To have safe access
> > > to this global, we need to ensure that you access it only between
> > > init_module() and cleanup_module(). For loadable modules, the
> > > try_module_get() logic seems perfect, so in Miscdevice::open we have a
> > > file pointer, which implies that the fs infrastructure took a refcount
> > > on fops->owner, which it can only do once init_module() is done.
> > >
> > > Unfortunately, this doesn't translate to built-in modules since the
> > > owner pointer is just null, and try_module_get performs no checks at
> > > all.
> > >
> > > Also, I'm realizing now that try_module_get() succeeds even if `state
> > > == MODULE_STATE_COMING`. :(
> > >
> > > So in conclusion, I don't know of any way to provide safe access to
> > > the global RustMiscDeviceModule value.
> >
> > Odd.  How is this any different than what is going to happen for
> > platform or other drivers of any other type?  Sometimes they want to
> > only create one single "static" object and register it with the bus they
> > are assigned to.
> >
> > Do we need to have a RuscMiscDevice object somewhere instead that
> > doesn't care about the module logic at all?  And then just use a
> > "normal" rust module object to create a single instance of that which
> > the misc binding will handle?
> 
> Actually, I guess we can access the miscdevice in open via the pointer
> that misc_open() stashes into the file private data. We don't have to
> go through the global variable.

Ah, nice, that's how a "normal" misc device should be doing this, so
yes, that sounds good!

thanks,

greg k-h

[PATCH v4 0/4] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Lee Jones]

It has been suggested that the driver should use dev_info() instead of
pr_info() however there is currently no scaffolding to successfully pull
a 'struct device' out from driver data post register().  This is being
worked on and we will convert this over in due course.

Lee Jones (4):
  Documentation: ioctl-number: Carve out some identifiers for use by
    sample drivers
  samples: rust: Provide example using the new Rust MiscDevice
    abstraction
  sample: rust_misc_device: Demonstrate additional get/set value
    functionality
  MAINTAINERS: Add Rust Misc Sample to MISC entry

 .../userspace-api/ioctl/ioctl-number.rst      |   1 +
 MAINTAINERS                                   |   1 +
 samples/rust/Kconfig                          |  10 ++
 samples/rust/Makefile                         |   1 +
 samples/rust/rust_misc_device.rs              | 132 ++++++++++++++++++
 5 files changed, 145 insertions(+)
 create mode 100644 samples/rust/rust_misc_device.rs

-- 
2.47.0.338.g60cca15819-goog

[PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device

[Lee Jones]

There are situations where a pointer to a `struct device` will become
necessary (e.g. for calling into dev_*() functions).  This accessor
allows callers to pull this out from the `struct miscdevice`.

Signed-off-by: Lee Jones <lee@kernel.org>
---
 rust/kernel/miscdevice.rs | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index 7e2a79b3ae26..55340f316006 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -10,11 +10,13 @@
 
 use crate::{
     bindings,
+    device::Device,
     error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
     prelude::*,
     str::CStr,
     types::{ForeignOwnable, Opaque},
 };
+
 use core::{
     ffi::{c_int, c_long, c_uint, c_ulong},
     marker::PhantomData,
@@ -84,6 +86,13 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
     pub fn as_raw(&self) -> *mut bindings::miscdevice {
         self.inner.get()
     }
+
+    /// Returns a pointer to the current Device
+    pub fn device(&self) -> &Device {
+        // SAFETY: This is only accessible after a successful register() which always
+        // initialises this_device with a valid device.
+        unsafe { Device::as_ref((*self.as_raw()).this_device) }
+    }
 }
 
 #[pinned_drop]
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device

[Miguel Ojeda]

On Fri, Dec 6, 2024 at 10:05 AM Lee Jones <lee@kernel.org> wrote:
>
> +    /// Returns a pointer to the current Device

Nit: please use intra-doc links wherever possible (if not possible,
please at least format type names as code). We also end sentences with
periods in docs and comments. So e.g.:

    /// Returns a pointer to the current [`Device`].

There was a comment about this line in the previous version, v3, but
there does not seem to be a change. But then again, the title of this
patch is v3 and not v4 -- not sure what happened here.

Thanks!

Cheers,
Miguel

Re: [PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device

[Lee Jones]

On Fri, 06 Dec 2024, Miguel Ojeda wrote:

> On Fri, Dec 6, 2024 at 10:05 AM Lee Jones <lee@kernel.org> wrote:
> >
> > +    /// Returns a pointer to the current Device
> 
> Nit: please use intra-doc links wherever possible (if not possible,
> please at least format type names as code). We also end sentences with
> periods in docs and comments. So e.g.:
> 
>     /// Returns a pointer to the current [`Device`].
> 
> There was a comment about this line in the previous version, v3, but
> there does not seem to be a change. But then again, the title of this
> patch is v3 and not v4 -- not sure what happened here.

This patch should no longer be part of the set after v3.

Looks like v3 was still in the output folder so was sent again with v4
by mistake.  My tooling usually strips out old versions, so I'm not sure
what went wrong specifically.

Thanks for the comment style tips.  I'll make the changes.

-- 
Lee Jones [李琼斯]