Message-ID: <2024120615-concert-oven-66f1@gregkh>
Date: Fri, 6 Dec 2024 09:44:16 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Lee Jones <lee@...nel.org>, linux-kernel@...r.kernel.org, arnd@...db.de,
	ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com,
	gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	a.hindborg@...nel.org, tmgross@...ch.edu,
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3 0/5] rust: miscdevice: Provide sample driver using the
 new MiscDevice bindings

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@...uxfoundation.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

Powered by blists - more mailing lists