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

[Greg KH <gregkh@linuxfoundation.org>]

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