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

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

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

-- 
2.47.0.338.g60cca15819-goog

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

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

confused,

greg k-h

On Fri, 06 Dec 2024, Greg KH wrote:

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

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

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

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

-- 
Lee Jones [李琼斯]

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

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

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

greg k-h

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

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

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

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

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

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

Alice

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

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

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

thanks,

greg k-h

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

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


Alice

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

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

thanks,

greg k-h