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

[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

[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 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

[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

[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

[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 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 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 v4 1/4] 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

[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

[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

[PATCH v4 2/4] 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
* Provide open call-back for the aforementioned character device
* Operate on the character device via a simple ioctl()
* Provide close call-back for 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..3837532d259e
--- /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('|' 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(ENOTTY);
+            }
+        }
+
+        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 v4 3/4] 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.

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/ioctl.h>

#define RUST_MISC_DEV_FAIL _IO('|', 0)
#define RUST_MISC_DEV_GET_VALUE _IOR('|', 7, int)
#define RUST_MISC_DEV_SET_VALUE _IOW('|', 8, int)
#define RUST_MISC_DEV_HELLO _IO('|', 9)

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 3837532d259e..02ef8780804e 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>('|' as u32, 7);
+const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('|' as u32, 8);
 const RUST_MISC_DEV_HELLO: u32 = _IO('|' 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(ENOTTY);
             }
-        }
+        };
 
         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 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

[PATCH v4 4/4] 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

[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 v4 2/4] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Danilo Krummrich]

On Fri, Dec 06, 2024 at 09:05:06AM +0000, Lee Jones wrote:
> This sample driver demonstrates the following basic operations:
> 
> * Register a Misc Device
> * Create /dev/rust-misc-device
> * Provide open call-back for the aforementioned character device
> * Operate on the character device via a simple ioctl()
> * Provide close call-back for 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..3837532d259e
> --- /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('|' 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,
> +            )?,
> +        })

Since this v4 was sent just when I was commenting on v3:

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/

> +    }
> +}
> +
> +struct RustMiscDevice;
> +
> +#[vtable]
> +impl MiscDevice for RustMiscDevice {
> +    type Ptr = KBox<Self>;
> +
> +    fn open() -> Result<KBox<Self>> {
> +        pr_info!("Opening Rust Misc Device Sample\n");

This should be `dev_info!`, but I see why you don't have access to the device
structure here...

@Greg: How do miscdev drivers do this in C? I looked at a couple of them, but
all of those seem to use pr_* macros. They can't get the device pointer from
the inode or file pointer.

However, C drivers could refer to the struct miscdevice directly since it's
almost always a static variable in the file scope.

In Rust we do the static allocation part with `InPlaceModule` as mentioned
above. However, this still doesn't let us refer to the underlying struct
miscdevice.

This all would be much cleaner if there'd be a "fake" probe() callback for a
struct miscdevice. This way we could perfectly align the miscdevice abstraction
with the abstractions for all other drivers, such as PCI, platform, etc.

- Danilo

> +
> +        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(ENOTTY);
> +            }
> +        }
> +
> +        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 v4 2/4] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Alice Ryhl]

On Fri, Dec 6, 2024 at 10:29 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Fri, Dec 06, 2024 at 09:05:06AM +0000, Lee Jones wrote:
> > This sample driver demonstrates the following basic operations:
> >
> > * Register a Misc Device
> > * Create /dev/rust-misc-device
> > * Provide open call-back for the aforementioned character device
> > * Operate on the character device via a simple ioctl()
> > * Provide close call-back for 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..3837532d259e
> > --- /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('|' 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,
> > +            )?,
> > +        })
>
> Since this v4 was sent just when I was commenting on v3:
>
> 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/
>
> > +    }
> > +}
> > +
> > +struct RustMiscDevice;
> > +
> > +#[vtable]
> > +impl MiscDevice for RustMiscDevice {
> > +    type Ptr = KBox<Self>;
> > +
> > +    fn open() -> Result<KBox<Self>> {
> > +        pr_info!("Opening Rust Misc Device Sample\n");
>
> This should be `dev_info!`, but I see why you don't have access to the device
> structure here...
>
> @Greg: How do miscdev drivers do this in C? I looked at a couple of them, but
> all of those seem to use pr_* macros. They can't get the device pointer from
> the inode or file pointer.
>
> However, C drivers could refer to the struct miscdevice directly since it's
> almost always a static variable in the file scope.
>
> In Rust we do the static allocation part with `InPlaceModule` as mentioned
> above. However, this still doesn't let us refer to the underlying struct
> miscdevice.
>
> This all would be much cleaner if there'd be a "fake" probe() callback for a
> struct miscdevice. This way we could perfectly align the miscdevice abstraction
> with the abstractions for all other drivers, such as PCI, platform, etc.

It turns out that the file private data is a pointer to the `struct
miscdevice` in fops->open(), so we can access it in open. To access it
in other fops hooks, we need to take a refcount on the device in open
and stash it.

Alice

Re: [PATCH v4 2/4] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Danilo Krummrich]

On Fri, Dec 06, 2024 at 10:40:23AM +0100, Alice Ryhl wrote:
> On Fri, Dec 6, 2024 at 10:29 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Fri, Dec 06, 2024 at 09:05:06AM +0000, Lee Jones wrote:
> > > This sample driver demonstrates the following basic operations:
> > >
> > > * Register a Misc Device
> > > * Create /dev/rust-misc-device
> > > * Provide open call-back for the aforementioned character device
> > > * Operate on the character device via a simple ioctl()
> > > * Provide close call-back for 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..3837532d259e
> > > --- /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('|' 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,
> > > +            )?,
> > > +        })
> >
> > Since this v4 was sent just when I was commenting on v3:
> >
> > 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/
> >
> > > +    }
> > > +}
> > > +
> > > +struct RustMiscDevice;
> > > +
> > > +#[vtable]
> > > +impl MiscDevice for RustMiscDevice {
> > > +    type Ptr = KBox<Self>;
> > > +
> > > +    fn open() -> Result<KBox<Self>> {
> > > +        pr_info!("Opening Rust Misc Device Sample\n");
> >
> > This should be `dev_info!`, but I see why you don't have access to the device
> > structure here...
> >
> > @Greg: How do miscdev drivers do this in C? I looked at a couple of them, but
> > all of those seem to use pr_* macros. They can't get the device pointer from
> > the inode or file pointer.
> >
> > However, C drivers could refer to the struct miscdevice directly since it's
> > almost always a static variable in the file scope.
> >
> > In Rust we do the static allocation part with `InPlaceModule` as mentioned
> > above. However, this still doesn't let us refer to the underlying struct
> > miscdevice.
> >
> > This all would be much cleaner if there'd be a "fake" probe() callback for a
> > struct miscdevice. This way we could perfectly align the miscdevice abstraction
> > with the abstractions for all other drivers, such as PCI, platform, etc.
> 
> It turns out that the file private data is a pointer to the `struct
> miscdevice` in fops->open(), so we can access it in open. To access it
> in other fops hooks, we need to take a refcount on the device in open
> and stash it.

Even better! I suggest to represent it as `misc::Device` and align it with
`platform::Device`, `pci::Device`, etc.

> 
> Alice
> 

Re: [PATCH v4 2/4] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Arnd Bergmann]

On Fri, Dec 6, 2024, at 10:05, Lee Jones wrote:
> This sample driver demonstrates the following basic operations:
>
> * Register a Misc Device
> * Create /dev/rust-misc-device
> * Provide open call-back for the aforementioned character device
> * Operate on the character device via a simple ioctl()
> * Provide close call-back for the character device
>
> Signed-off-by: Lee Jones <lee@kernel.org>

Could you include a compat_ioctl() callback in the example?
I think it would be good to include it as a reminder for
authors of actual drivers that every driver implementing
ioctl should also implement compat_ioctl. In C drivers, this
can usually be done by pointing .compat_ioctl() to the
generic compat_ptr_ioctl() function, which assumes that 'arg'
is a pointer disguised as an 'unsigned long'.

      Arnd

Re: [PATCH v4 2/4] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Alice Ryhl]

On Fri, Dec 6, 2024 at 11:05 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Dec 6, 2024, at 10:05, Lee Jones wrote:
> > This sample driver demonstrates the following basic operations:
> >
> > * Register a Misc Device
> > * Create /dev/rust-misc-device
> > * Provide open call-back for the aforementioned character device
> > * Operate on the character device via a simple ioctl()
> > * Provide close call-back for the character device
> >
> > Signed-off-by: Lee Jones <lee@kernel.org>
>
> Could you include a compat_ioctl() callback in the example?
> I think it would be good to include it as a reminder for
> authors of actual drivers that every driver implementing
> ioctl should also implement compat_ioctl. In C drivers, this
> can usually be done by pointing .compat_ioctl() to the
> generic compat_ptr_ioctl() function, which assumes that 'arg'
> is a pointer disguised as an 'unsigned long'.

The current Rust logic for building the fops table will use
compat_ptr_ioctl() automatically if you specify ioctl() but don't
specify compat_ioctl(), so this already uses compat_ptr_ioctl(). But
maybe that's not what we want?

Alice

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 v4 2/4] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Arnd Bergmann]

On Fri, Dec 6, 2024, at 11:09, Alice Ryhl wrote:
> On Fri, Dec 6, 2024 at 11:05 AM Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> On Fri, Dec 6, 2024, at 10:05, Lee Jones wrote:
>> > This sample driver demonstrates the following basic operations:
>> >
>> > * Register a Misc Device
>> > * Create /dev/rust-misc-device
>> > * Provide open call-back for the aforementioned character device
>> > * Operate on the character device via a simple ioctl()
>> > * Provide close call-back for the character device
>> >
>> > Signed-off-by: Lee Jones <lee@kernel.org>
>>
>> Could you include a compat_ioctl() callback in the example?
>> I think it would be good to include it as a reminder for
>> authors of actual drivers that every driver implementing
>> ioctl should also implement compat_ioctl. In C drivers, this
>> can usually be done by pointing .compat_ioctl() to the
>> generic compat_ptr_ioctl() function, which assumes that 'arg'
>> is a pointer disguised as an 'unsigned long'.
>
> The current Rust logic for building the fops table will use
> compat_ptr_ioctl() automatically if you specify ioctl() but don't
> specify compat_ioctl(), so this already uses compat_ptr_ioctl(). But
> maybe that's not what we want?

Ok, got it. It's usually the right thing to do, but it's easy
to get wrong if there is at least one ioctl command that actually
needs an integer argument instead of a pointer.

Almost all command definitions are for either no argument or
a pointer argument, and compat_ptr_ioctl() works fine there, by
doing a conversion from a 32-bit pointer to a 64-bit pointer
by zero-extending the upper 33 (on s390) or 32 bits (everywhere
else). Integer values need to either a 32-bit sign-extension
or a 32-bit zero-extension depending on how the argument is
interpreted on 32-bit architectures.

I wonder if we should change the prototype of the ioctl
callback to always pass a __user pointer and just not allow
the few commands that pass an integer in rust drivers, and
worry about it only when it's absolutely needed.

     Arnd

Re: [PATCH v4 2/4] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Alice Ryhl]

On Fri, Dec 6, 2024 at 11:31 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Dec 6, 2024, at 11:09, Alice Ryhl wrote:
> > On Fri, Dec 6, 2024 at 11:05 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >>
> >> On Fri, Dec 6, 2024, at 10:05, Lee Jones wrote:
> >> > This sample driver demonstrates the following basic operations:
> >> >
> >> > * Register a Misc Device
> >> > * Create /dev/rust-misc-device
> >> > * Provide open call-back for the aforementioned character device
> >> > * Operate on the character device via a simple ioctl()
> >> > * Provide close call-back for the character device
> >> >
> >> > Signed-off-by: Lee Jones <lee@kernel.org>
> >>
> >> Could you include a compat_ioctl() callback in the example?
> >> I think it would be good to include it as a reminder for
> >> authors of actual drivers that every driver implementing
> >> ioctl should also implement compat_ioctl. In C drivers, this
> >> can usually be done by pointing .compat_ioctl() to the
> >> generic compat_ptr_ioctl() function, which assumes that 'arg'
> >> is a pointer disguised as an 'unsigned long'.
> >
> > The current Rust logic for building the fops table will use
> > compat_ptr_ioctl() automatically if you specify ioctl() but don't
> > specify compat_ioctl(), so this already uses compat_ptr_ioctl(). But
> > maybe that's not what we want?
>
> Ok, got it. It's usually the right thing to do, but it's easy
> to get wrong if there is at least one ioctl command that actually
> needs an integer argument instead of a pointer.
>
> Almost all command definitions are for either no argument or
> a pointer argument, and compat_ptr_ioctl() works fine there, by
> doing a conversion from a 32-bit pointer to a 64-bit pointer
> by zero-extending the upper 33 (on s390) or 32 bits (everywhere
> else). Integer values need to either a 32-bit sign-extension
> or a 32-bit zero-extension depending on how the argument is
> interpreted on 32-bit architectures.
>
> I wonder if we should change the prototype of the ioctl
> callback to always pass a __user pointer and just not allow
> the few commands that pass an integer in rust drivers, and
> worry about it only when it's absolutely needed.

One option is to let the Rust Miscdevice trait have three ioctl methods:

fn ioctl(cmd: u32, arg: UserPtr);
fn ioctl_raw(cmd: u32, arg: usize);
fn compat_ioctl(cmd: u32, arg: usize);

Then when building the fops vtable, we do one of:

1. If `ioctl` is specified, use that implementation with compat_ptr_ioctl().
2. If `ioctl_raw` and `compat_ioctl` are specified, use those two
implementations.
3. If none of the above are specified, use null pointers.
4. All other cases trigger an error at build time.

Thoughts?

Alice

Re: [PATCH v4 2/4] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Arnd Bergmann]

On Fri, Dec 6, 2024, at 11:40, Alice Ryhl wrote:
> On Fri, Dec 6, 2024 at 11:31 AM Arnd Bergmann <arnd@arndb.de> wrote:

>> I wonder if we should change the prototype of the ioctl
>> callback to always pass a __user pointer and just not allow
>> the few commands that pass an integer in rust drivers, and
>> worry about it only when it's absolutely needed.
>
> One option is to let the Rust Miscdevice trait have three ioctl methods:
>
> fn ioctl(cmd: u32, arg: UserPtr);
> fn ioctl_raw(cmd: u32, arg: usize);
> fn compat_ioctl(cmd: u32, arg: usize);
>
> Then when building the fops vtable, we do one of:
>
> 1. If `ioctl` is specified, use that implementation with compat_ptr_ioctl().
> 2. If `ioctl_raw` and `compat_ioctl` are specified, use those two
> implementations.
> 3. If none of the above are specified, use null pointers.
> 4. All other cases trigger an error at build time.
>
> Thoughts?

I think we can combine the latter two into

fn ioctl_raw(cmd: u32, arg: usize, compat: bool);

and only need two cases: either all arguments are pointers to
structures with compatible layout and you can use the simple
ioctl() callback, or there is some special case (incompatible
struct layout or integer arguments) and you use the raw
version. I think this would work just fine.

Or we could take it one step further (going back to the
discussion we had the last time this came up) and make
the default version copy the data as well and pass it
as a kernel pointer. In C code this would look roughly
like

long file_ioctl_wrapper(struct file *f, u32 cmd, unsigned long arg)
{
       void *argp;
       void __user *uarg;
       bool compat = in_compat_syscall());
       size_t size = _IOC_SIZE(cmd);
       int ret = -ENOIOCTLCMD;

       /* Get a pointer argument for both native and compat mode */
       if (compat)
              uarg = compat_ptr(arg);
       else
              uarg = (void __user*)arg;

       /*
        * allow .ioctl_raw to provide a custom version for
        * commands that take an integer argument, have an
        * incompatible compat layout or fail to encode size
        * and/or direction correctly in cmd
        * This can return ENOIOCTLCMD to fall back to the
        * simple handler for other commands.
        */       
       if (f->ops->ioctl_raw)
                ret = f->ops->ioctl_raw(f, cmd, uarg, arg, compat);
       if (ret != -ENOIOCTLCMD)
                return ret;

       /* No data, so skip the allocation */
       if (_IOC_DIR(cmd) == _IOC_NONE || size = 0)
                return f->ops->ioctl(f, cmd, NULL);

       argp = kzalloc(size, GFP_KERNEL);
       if (!argp)
             return -ENOMEM;

       /* _IOW or _IOWR, so copy data into the kernel */
       if (_IOC_DIR(cmd) & _IOC_WRITE) {
             if (copy_from_user(argp, uarg, size))
                    return -EFAULT;
       }

       ret = f->ops->ioctl(f, cmd, arg);

       /* _IOR or _IOWR, copy back even after command failure */
       if (_IOC_DIR(cmd) & _IOC_READ) {
             if (copy_to_user(uarg, arg, size))
                    return -EFAULT;
       }

       return ret;
}

With this, every driver that has a properly designed
set of ioctl commands can just use the simple .ioctl()
callback, and any commands that need some special case
can be put in the .ioctl_raw() callback.

If this works out for Rust, we can actually put that exact
code into vfs_ioctl() and add back a .ioctl() callback
into struct file_operations next to the C .unlocked_ioctl
and .compat_ioctl handlers.

    Arnd

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 [李琼斯]