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

[Alice Ryhl]

On Fri, Dec 6, 2024 at 9:44 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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