Re: [PATCH v5 01/16] rust: pass module name to `Module::init`

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Dec 11, 2024 at 02:31:07PM +0100, Danilo Krummrich wrote:
> On Wed, Dec 11, 2024 at 02:14:37PM +0100, Greg KH wrote:
> > On Wed, Dec 11, 2024 at 01:34:31PM +0100, Danilo Krummrich wrote:
> > > On Wed, Dec 11, 2024 at 01:22:33PM +0100, Danilo Krummrich wrote:
> > > > On Wed, Dec 11, 2024 at 12:05:10PM +0100, Greg KH wrote:
> > > > > On Wed, Dec 11, 2024 at 11:59:54AM +0100, Greg KH wrote:
> > > > > > On Wed, Dec 11, 2024 at 11:48:23AM +0100, Greg KH wrote:
> > > > > > > On Wed, Dec 11, 2024 at 11:45:20AM +0100, Greg KH wrote:
> > > > > > > > On Tue, Dec 10, 2024 at 11:46:28PM +0100, Danilo Krummrich wrote:
> > > > > > > > > In a subsequent patch we introduce the `Registration` abstraction used
> > > > > > > > > to register driver structures. Some subsystems require the module name on
> > > > > > > > > driver registration (e.g. PCI in __pci_register_driver()), hence pass
> > > > > > > > > the module name to `Module::init`.
> > > > > > > > 
> > > > > > > > Nit, we don't need the NAME of the PCI driver (well, we do like it, but
> > > > > > > > that's not the real thing), we want the pointer to the module structure
> > > > > > > > in the register_driver call.
> > > > > > > > 
> > > > > > > > Does this provide for that?  I'm thinking it does, but it's not the
> > > > > > > > "name" that is the issue here.
> > > > > > > 
> > > > > > > Wait, no, you really do want the name, don't you.  You refer to
> > > > > > > "module.0" to get the module structure pointer (if I'm reading the code
> > > > > > > right), but as you have that pointer already, why can't you just use
> > > > > > > module->name there as well as you have a pointer to a valid module
> > > > > > > structure that has the name already embedded in it.
> > > > > > 
> > > > > > In digging further, it's used by the pci code to call into lower layers,
> > > > > > but why it's using a different string other than the module name string
> > > > > > is beyond me.  Looks like this goes way back before git was around, and
> > > > > > odds are it's my fault for something I wrote a long time ago.
> > > > > > 
> > > > > > I'll see if I can just change the driver core to not need a name at all,
> > > > > > and pull it from the module which would make all of this go away in the
> > > > > > end.  Odds are something will break but who knows...
> > > > > 
> > > > > Nope, things break, the "name" is there to handle built-in modules (as
> > > > > the module pointer will be NULL.)
> > > > > 
> > > > > So what you really want is not the module->name (as I don't think that
> > > > > will be set), but you want KBUILD_MODNAME which the build system sets.
> > > > 
> > > > That's correct, and the reason why I pass through this name argument.
> > > > 
> > > > Sorry I wasn't able to reply earlier to save you some time.
> > > > 
> > > > > You shouldn't need to pass the name through all of the subsystems here,
> > > > > just rely on the build system instead.
> > > > > 
> > > > > Or does the Rust side not have KBUILD_MODNAME?
> > > > 
> > > > AFAIK, it doesn't (or didn't have at the time I wrote the patch).
> > > > 
> > > > @Miguel: Can we access KBUILD_MODNAME conveniently?
> > > 
> > > Actually, I now remember there was another reason why I pass it through in
> > > `Module::init`.
> > > 
> > > Even if we had env!(KBUILD_MODNAME) already, I'd want to use it from the bus
> > > abstraction code, e.g. rust/kernel/pci.rs. But since this is generic code, it
> > > won't get the KBUILD_MODNAME from the module that is using the bus abstraction.
> > 
> > Rust can't do that in a macro somehow that all pci rust drivers can pull
> > from?
> 
> The problem is that register / unregister is encapsulated within methods of the
> abstraction types. So the C macro trick (while generally possible) isn't
> applicable.

Really?  You can't have something in a required "register()" type function?
Something for when the driver "instance" is created as part of
pci::Driver?  You do that today in your sample driver for the id table:
	const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;

Something else called DRIVER_NAME that you could then set:
	const DRIVER_NAME: env!(KBUILD_MODNAME);

Also, I think you will want this for when a single module registers
multiple drivers which I think can happen at times, so you could
manually override the DRIVER_NAME field.

And if DRIVER_NAME doesn't get set, well, you just don't get the module
symlink in sysfs, just like what happens today if you don't provide that
field (but for PCI drivers, the .h file does it automatically for you.)

Anyway, this is a driver issue, NOT a module issue, so having to "plumb"
the module name all the way down through this really isn't the best
abstraction to do here from what I can tell.

thanks,

greg k-h