| Message ID | 20241210224947.23804-2-dakr@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Device / Driver PCI / Platform Rust abstractions | expand |
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. thanks, greg k-h
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. still confused, greg k-h
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... thanks, greg k-h
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. 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? thanks, greg k-h
On Wed, Dec 11, 2024 at 12:05 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > Or does the Rust side not have KBUILD_MODNAME? We can definitely give access to it at compile-time with e.g. `env!`: pr_info!("{}\n", env!("KBUILD_MODNAME")); Cheers, Miguel
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? > > thanks, > > greg k-h
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. > > > > > thanks, > > > > greg k-h
On Wed, Dec 11, 2024 at 12:41 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Wed, Dec 11, 2024 at 12:05 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > Or does the Rust side not have KBUILD_MODNAME? > > We can definitely give access to it at compile-time with e.g. `env!`: > > pr_info!("{}\n", env!("KBUILD_MODNAME")); I guess the challenge is that we need that to get evaluated in the driver, rather than inside the rust/kernel/ crate. Perhaps we could have the module! macro emit something that attaches the name to the module type, or something like that. Alice
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? thanks, greg k-h
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. I think we could avoid having an additional `name` parameter in `Module::init`, but it would still need to be the driver resolving `env!(KBUILD_MODNAME)` passing it into the bus abstraction. However, similar to what Alice suggested in another thread, we could include this step in the `module_*_driver!` macros. Modules that don't use this convenience macro would need to do it by hand though. But that's probably not that big a deal. > > thanks, > > greg k-h
On Wed, Dec 11, 2024 at 2:31 PM Danilo Krummrich <dakr@kernel.org> 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. > > I think we could avoid having an additional `name` parameter in `Module::init`, > but it would still need to be the driver resolving `env!(KBUILD_MODNAME)` > passing it into the bus abstraction. > > However, similar to what Alice suggested in another thread, we could include > this step in the `module_*_driver!` macros. > > Modules that don't use this convenience macro would need to do it by hand > though. But that's probably not that big a deal. I think we can do it in the core `module!` macro that everyone has to use. Alice
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
On Wed, Dec 11, 2024 at 02:45:14PM +0100, Greg KH wrote: > 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); Sure, that's possible. But that means that the driver has to set it explicitly -- even when e.g. module_pci_driver! is used. In C you don't have that, because there it's implicit within the pci_register_driver() macro. (Regardless of whether it's a single module for a single driver or multiple drivers per module.) Anyways, like I mentioned, given that we have `env!(KBUILD_MODNAME)` (which we still need to add), there are other options to make it work similarly, e.g. add a parameter to `pci::Adapter` and bake this into `module_pci_driver!`. For this particular option, it would mean that for modules registering multiple drivers a corresponding name would need to be passed explicitly. > > 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. My proposal above would provide this option, but do we care? In C no one ever changes the name. There is zero users of __pci_register_driver(), everyone uses pci_register_driver() where the name is just KBUILD_MODNAME. Same for __platform_driver_register(). > > 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
On Wed, Dec 11, 2024 at 02:34:54PM +0100, Alice Ryhl wrote: > On Wed, Dec 11, 2024 at 2:31 PM Danilo Krummrich <dakr@kernel.org> 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. > > > > I think we could avoid having an additional `name` parameter in `Module::init`, > > but it would still need to be the driver resolving `env!(KBUILD_MODNAME)` > > passing it into the bus abstraction. > > > > However, similar to what Alice suggested in another thread, we could include > > this step in the `module_*_driver!` macros. > > > > Modules that don't use this convenience macro would need to do it by hand > > though. But that's probably not that big a deal. > > I think we can do it in the core `module!` macro that everyone has to use. How? The `module!` macro does not know about the registration instances within the module structure. > > Alice
On Wed, Dec 11, 2024 at 03:21:53PM +0100, Danilo Krummrich wrote: > > 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); > > Sure, that's possible. But that means that the driver has to set it explicitly > -- even when e.g. module_pci_driver! is used. > > In C you don't have that, because there it's implicit within the > pci_register_driver() macro. (Regardless of whether it's a single module for a > single driver or multiple drivers per module.) > > Anyways, like I mentioned, given that we have `env!(KBUILD_MODNAME)` (which we > still need to add), there are other options to make it work similarly, e.g. add > a parameter to `pci::Adapter` and bake this into `module_pci_driver!`. Ok, I'm all for that, just don't modify the module rust functions for it :) > For this particular option, it would mean that for modules registering multiple > drivers a corresponding name would need to be passed explicitly. True. > > 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. > > My proposal above would provide this option, but do we care? In C no one ever > changes the name. There is zero users of __pci_register_driver(), everyone uses > pci_register_driver() where the name is just KBUILD_MODNAME. Same for > __platform_driver_register(). You are right. But I see other drivers messing with that field, oh no, wait, that's just the EDAC layer doing really odd things (a known issue as that layer does very many odd things.) So nevermind, multiple drivers per module isn't going to be an issue, if you all can move it to a macro (best yet, like what Alice pointed out), that would be fine with me. thanks, greg k-h
On Wed, Dec 11, 2024 at 3:29 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On Wed, Dec 11, 2024 at 02:34:54PM +0100, Alice Ryhl wrote: > > On Wed, Dec 11, 2024 at 2:31 PM Danilo Krummrich <dakr@kernel.org> 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. > > > > > > I think we could avoid having an additional `name` parameter in `Module::init`, > > > but it would still need to be the driver resolving `env!(KBUILD_MODNAME)` > > > passing it into the bus abstraction. > > > > > > However, similar to what Alice suggested in another thread, we could include > > > this step in the `module_*_driver!` macros. > > > > > > Modules that don't use this convenience macro would need to do it by hand > > > though. But that's probably not that big a deal. > > > > I think we can do it in the core `module!` macro that everyone has to use. > > How? The `module!` macro does not know about the registration instances within > the module structure. You could have the module! macro emit something along these lines: impl ModuleName for {type_} { const NAME: &'static CStr = c_str!(env!("KBUILD_MODNAME")); } Then you can do `<Self as ModuleName>::NAME` to obtain the name elsewhere. Alice
On Wed, Dec 11, 2024 at 03:45:53PM +0100, Alice Ryhl wrote: > On Wed, Dec 11, 2024 at 3:29 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > On Wed, Dec 11, 2024 at 02:34:54PM +0100, Alice Ryhl wrote: > > > On Wed, Dec 11, 2024 at 2:31 PM Danilo Krummrich <dakr@kernel.org> 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. > > > > > > > > I think we could avoid having an additional `name` parameter in `Module::init`, > > > > but it would still need to be the driver resolving `env!(KBUILD_MODNAME)` > > > > passing it into the bus abstraction. > > > > > > > > However, similar to what Alice suggested in another thread, we could include > > > > this step in the `module_*_driver!` macros. > > > > > > > > Modules that don't use this convenience macro would need to do it by hand > > > > though. But that's probably not that big a deal. > > > > > > I think we can do it in the core `module!` macro that everyone has to use. > > > > How? The `module!` macro does not know about the registration instances within > > the module structure. > > You could have the module! macro emit something along these lines: > > impl ModuleName for {type_} { > const NAME: &'static CStr = c_str!(env!("KBUILD_MODNAME")); > } > > Then you can do `<Self as ModuleName>::NAME` to obtain the name elsewhere. Where {type_} would need to be the driver's `Driver` structure? We'd then need to define the bus adapter as: `pub struct Adapter<T: Driver + ModuleName>(T)` But the question stands I guess, how would the module macro know {type_}? > > Alice
On Wed, Dec 11, 2024 at 3:52 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On Wed, Dec 11, 2024 at 03:45:53PM +0100, Alice Ryhl wrote: > > On Wed, Dec 11, 2024 at 3:29 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > > > On Wed, Dec 11, 2024 at 02:34:54PM +0100, Alice Ryhl wrote: > > > > On Wed, Dec 11, 2024 at 2:31 PM Danilo Krummrich <dakr@kernel.org> 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. > > > > > > > > > > I think we could avoid having an additional `name` parameter in `Module::init`, > > > > > but it would still need to be the driver resolving `env!(KBUILD_MODNAME)` > > > > > passing it into the bus abstraction. > > > > > > > > > > However, similar to what Alice suggested in another thread, we could include > > > > > this step in the `module_*_driver!` macros. > > > > > > > > > > Modules that don't use this convenience macro would need to do it by hand > > > > > though. But that's probably not that big a deal. > > > > > > > > I think we can do it in the core `module!` macro that everyone has to use. > > > > > > How? The `module!` macro does not know about the registration instances within > > > the module structure. > > > > You could have the module! macro emit something along these lines: > > > > impl ModuleName for {type_} { > > const NAME: &'static CStr = c_str!(env!("KBUILD_MODNAME")); > > } > > > > Then you can do `<Self as ModuleName>::NAME` to obtain the name elsewhere. > > Where {type_} would need to be the driver's `Driver` structure? > > We'd then need to define the bus adapter as: > > `pub struct Adapter<T: Driver + ModuleName>(T)` > > But the question stands I guess, how would the module macro know {type_}? If you look at the macro implementation in rust/macros/module.rs you will find many uses of {type_} throughout the expansion. It's whatever is passed to the macro using the `type:` argument. Alice
On Wed, Dec 11, 2024 at 03:55:47PM +0100, Alice Ryhl wrote: > On Wed, Dec 11, 2024 at 3:52 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > On Wed, Dec 11, 2024 at 03:45:53PM +0100, Alice Ryhl wrote: > > > On Wed, Dec 11, 2024 at 3:29 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > > > > > On Wed, Dec 11, 2024 at 02:34:54PM +0100, Alice Ryhl wrote: > > > > > On Wed, Dec 11, 2024 at 2:31 PM Danilo Krummrich <dakr@kernel.org> 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. > > > > > > > > > > > > I think we could avoid having an additional `name` parameter in `Module::init`, > > > > > > but it would still need to be the driver resolving `env!(KBUILD_MODNAME)` > > > > > > passing it into the bus abstraction. > > > > > > > > > > > > However, similar to what Alice suggested in another thread, we could include > > > > > > this step in the `module_*_driver!` macros. > > > > > > > > > > > > Modules that don't use this convenience macro would need to do it by hand > > > > > > though. But that's probably not that big a deal. > > > > > > > > > > I think we can do it in the core `module!` macro that everyone has to use. > > > > > > > > How? The `module!` macro does not know about the registration instances within > > > > the module structure. > > > > > > You could have the module! macro emit something along these lines: > > > > > > impl ModuleName for {type_} { > > > const NAME: &'static CStr = c_str!(env!("KBUILD_MODNAME")); > > > } > > > > > > Then you can do `<Self as ModuleName>::NAME` to obtain the name elsewhere. > > > > Where {type_} would need to be the driver's `Driver` structure? > > > > We'd then need to define the bus adapter as: > > > > `pub struct Adapter<T: Driver + ModuleName>(T)` > > > > But the question stands I guess, how would the module macro know {type_}? > > If you look at the macro implementation in rust/macros/module.rs you > will find many uses of {type_} throughout the expansion. It's whatever > is passed to the macro using the `type:` argument. Oh, I see. So, this means that module / driver author would still need to create the "connection" by listing the correspong driver types in the module! macro, right? If so, I think it'd be better to do it in the `module_*_driver!` macro and let people implement the trait by hand for modules with multiple drivers (which should be pretty rare). The reason is that I think that otherwise we're probably encoding too much semantics into the `module!` macro that isn't obvious and people need to understand. > > > Alice
On Wed, Dec 11, 2024 at 4:03 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On Wed, Dec 11, 2024 at 03:55:47PM +0100, Alice Ryhl wrote: > > On Wed, Dec 11, 2024 at 3:52 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > > > On Wed, Dec 11, 2024 at 03:45:53PM +0100, Alice Ryhl wrote: > > > > On Wed, Dec 11, 2024 at 3:29 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > > > > > > > On Wed, Dec 11, 2024 at 02:34:54PM +0100, Alice Ryhl wrote: > > > > > > On Wed, Dec 11, 2024 at 2:31 PM Danilo Krummrich <dakr@kernel.org> 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. > > > > > > > > > > > > > > I think we could avoid having an additional `name` parameter in `Module::init`, > > > > > > > but it would still need to be the driver resolving `env!(KBUILD_MODNAME)` > > > > > > > passing it into the bus abstraction. > > > > > > > > > > > > > > However, similar to what Alice suggested in another thread, we could include > > > > > > > this step in the `module_*_driver!` macros. > > > > > > > > > > > > > > Modules that don't use this convenience macro would need to do it by hand > > > > > > > though. But that's probably not that big a deal. > > > > > > > > > > > > I think we can do it in the core `module!` macro that everyone has to use. > > > > > > > > > > How? The `module!` macro does not know about the registration instances within > > > > > the module structure. > > > > > > > > You could have the module! macro emit something along these lines: > > > > > > > > impl ModuleName for {type_} { > > > > const NAME: &'static CStr = c_str!(env!("KBUILD_MODNAME")); > > > > } > > > > > > > > Then you can do `<Self as ModuleName>::NAME` to obtain the name elsewhere. > > > > > > Where {type_} would need to be the driver's `Driver` structure? > > > > > > We'd then need to define the bus adapter as: > > > > > > `pub struct Adapter<T: Driver + ModuleName>(T)` > > > > > > But the question stands I guess, how would the module macro know {type_}? > > > > If you look at the macro implementation in rust/macros/module.rs you > > will find many uses of {type_} throughout the expansion. It's whatever > > is passed to the macro using the `type:` argument. > > Oh, I see. So, this means that module / driver author would still need to create > the "connection" by listing the correspong driver types in the module! macro, > right? I'm not sure what you mean. I'm *not* suggesting any changes to the interface of module! or module_*_driver!. > If so, I think it'd be better to do it in the `module_*_driver!` macro and let > people implement the trait by hand for modules with multiple drivers (which > should be pretty rare). > > The reason is that I think that otherwise we're probably encoding too much > semantics into the `module!` macro that isn't obvious and people need to > understand. > > > > > > > Alice
On Wed, Dec 11, 2024 at 04:15:19PM +0100, Alice Ryhl wrote: > On Wed, Dec 11, 2024 at 4:03 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > On Wed, Dec 11, 2024 at 03:55:47PM +0100, Alice Ryhl wrote: > > > On Wed, Dec 11, 2024 at 3:52 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > > > > > On Wed, Dec 11, 2024 at 03:45:53PM +0100, Alice Ryhl wrote: > > > > > On Wed, Dec 11, 2024 at 3:29 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > > > > > > > > > On Wed, Dec 11, 2024 at 02:34:54PM +0100, Alice Ryhl wrote: > > > > > > > On Wed, Dec 11, 2024 at 2:31 PM Danilo Krummrich <dakr@kernel.org> 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. > > > > > > > > > > > > > > > > I think we could avoid having an additional `name` parameter in `Module::init`, > > > > > > > > but it would still need to be the driver resolving `env!(KBUILD_MODNAME)` > > > > > > > > passing it into the bus abstraction. > > > > > > > > > > > > > > > > However, similar to what Alice suggested in another thread, we could include > > > > > > > > this step in the `module_*_driver!` macros. > > > > > > > > > > > > > > > > Modules that don't use this convenience macro would need to do it by hand > > > > > > > > though. But that's probably not that big a deal. > > > > > > > > > > > > > > I think we can do it in the core `module!` macro that everyone has to use. > > > > > > > > > > > > How? The `module!` macro does not know about the registration instances within > > > > > > the module structure. > > > > > > > > > > You could have the module! macro emit something along these lines: > > > > > > > > > > impl ModuleName for {type_} { > > > > > const NAME: &'static CStr = c_str!(env!("KBUILD_MODNAME")); > > > > > } > > > > > > > > > > Then you can do `<Self as ModuleName>::NAME` to obtain the name elsewhere. > > > > > > > > Where {type_} would need to be the driver's `Driver` structure? > > > > > > > > We'd then need to define the bus adapter as: > > > > > > > > `pub struct Adapter<T: Driver + ModuleName>(T)` > > > > > > > > But the question stands I guess, how would the module macro know {type_}? > > > > > > If you look at the macro implementation in rust/macros/module.rs you > > > will find many uses of {type_} throughout the expansion. It's whatever > > > is passed to the macro using the `type:` argument. > > > > Oh, I see. So, this means that module / driver author would still need to create > > the "connection" by listing the correspong driver types in the module! macro, > > right? > > I'm not sure what you mean. I'm *not* suggesting any changes to the > interface of module! or module_*_driver!. Huh! Seems like we're talking past each other than. Maybe we can briefly discuss it in today's call? > > > If so, I think it'd be better to do it in the `module_*_driver!` macro and let > > people implement the trait by hand for modules with multiple drivers (which > > should be pretty rare). > > > > The reason is that I think that otherwise we're probably encoding too much > > semantics into the `module!` macro that isn't obvious and people need to > > understand. > > > > > > > > > > > Alice
On Wed, Dec 11, 2024 at 04:36:35PM +0100, Danilo Krummrich wrote: > On Wed, Dec 11, 2024 at 04:15:19PM +0100, Alice Ryhl wrote: > > On Wed, Dec 11, 2024 at 4:03 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > > > On Wed, Dec 11, 2024 at 03:55:47PM +0100, Alice Ryhl wrote: > > > > On Wed, Dec 11, 2024 at 3:52 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > > > > > > > On Wed, Dec 11, 2024 at 03:45:53PM +0100, Alice Ryhl wrote: > > > > > > On Wed, Dec 11, 2024 at 3:29 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > > > > > > > > > > > On Wed, Dec 11, 2024 at 02:34:54PM +0100, Alice Ryhl wrote: > > > > > > > > On Wed, Dec 11, 2024 at 2:31 PM Danilo Krummrich <dakr@kernel.org> 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. > > > > > > > > > > > > > > > > > > I think we could avoid having an additional `name` parameter in `Module::init`, > > > > > > > > > but it would still need to be the driver resolving `env!(KBUILD_MODNAME)` > > > > > > > > > passing it into the bus abstraction. > > > > > > > > > > > > > > > > > > However, similar to what Alice suggested in another thread, we could include > > > > > > > > > this step in the `module_*_driver!` macros. > > > > > > > > > > > > > > > > > > Modules that don't use this convenience macro would need to do it by hand > > > > > > > > > though. But that's probably not that big a deal. > > > > > > > > > > > > > > > > I think we can do it in the core `module!` macro that everyone has to use. > > > > > > > > > > > > > > How? The `module!` macro does not know about the registration instances within > > > > > > > the module structure. > > > > > > > > > > > > You could have the module! macro emit something along these lines: > > > > > > > > > > > > impl ModuleName for {type_} { > > > > > > const NAME: &'static CStr = c_str!(env!("KBUILD_MODNAME")); > > > > > > } > > > > > > > > > > > > Then you can do `<Self as ModuleName>::NAME` to obtain the name elsewhere. > > > > > > > > > > Where {type_} would need to be the driver's `Driver` structure? > > > > > > > > > > We'd then need to define the bus adapter as: > > > > > > > > > > `pub struct Adapter<T: Driver + ModuleName>(T)` > > > > > > > > > > But the question stands I guess, how would the module macro know {type_}? > > > > > > > > If you look at the macro implementation in rust/macros/module.rs you > > > > will find many uses of {type_} throughout the expansion. It's whatever > > > > is passed to the macro using the `type:` argument. > > > > > > Oh, I see. So, this means that module / driver author would still need to create > > > the "connection" by listing the correspong driver types in the module! macro, > > > right? > > > > I'm not sure what you mean. I'm *not* suggesting any changes to the > > interface of module! or module_*_driver!. > > Huh! Seems like we're talking past each other than. Maybe we can briefly > discuss it in today's call? I think I figured out the confusion. The {type_} in the module macro is the thing that implements the `Module` or `InPlaceModule` trait. However, that's *not* the driver type that is embedded in the bus adapter. For instance, what we have is: ``` struct MyDriver; struct MyModule { _driver: Registration<pci::Adapter<MyDriver>, }; impl pci::Driver for MyDriver { ... } impl InPlaceModule for MyModule { ... } module! { type: MyModule, ... } ``` This means `module!` would generate: `impl ModuleName for MyModule { ... }` But this doesn't help, because `pci::Adapter` doesn't know about `MyModule`, but only about `MyDriver`. Do I miss anything? > > > > > > If so, I think it'd be better to do it in the `module_*_driver!` macro and let > > > people implement the trait by hand for modules with multiple drivers (which > > > should be pretty rare). > > > > > > The reason is that I think that otherwise we're probably encoding too much > > > semantics into the `module!` macro that isn't obvious and people need to > > > understand. > > > > > > > > > > > > > > > Alice
diff --git a/drivers/block/rnull.rs b/drivers/block/rnull.rs index 9cca05dcf772..8e859dda70c3 100644 --- a/drivers/block/rnull.rs +++ b/drivers/block/rnull.rs @@ -37,7 +37,7 @@ struct NullBlkModule { } impl kernel::Module for NullBlkModule { - fn init(_module: &'static ThisModule) -> Result<Self> { + fn init(_name: &'static CStr, _module: &'static ThisModule) -> Result<Self> { pr_info!("Rust null_blk loaded\n"); let tagset = Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?; diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index e1065a7551a3..686db6aa3323 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -90,7 +90,7 @@ pub trait Module: Sized + Sync + Send { /// should do. /// /// Equivalent to the `module_init` macro in the C API. - fn init(module: &'static ThisModule) -> error::Result<Self>; + fn init(name: &'static str::CStr, module: &'static ThisModule) -> error::Result<Self>; } /// A module that is pinned and initialised in-place. @@ -98,13 +98,19 @@ pub trait InPlaceModule: Sync + Send { /// Creates an initialiser for the module. /// /// It is called when the module is loaded. - fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error>; + fn init( + name: &'static str::CStr, + module: &'static ThisModule, + ) -> impl init::PinInit<Self, error::Error>; } impl<T: Module> InPlaceModule for T { - fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error> { + fn init( + name: &'static str::CStr, + module: &'static ThisModule, + ) -> impl init::PinInit<Self, error::Error> { let initer = move |slot: *mut Self| { - let m = <Self as Module>::init(module)?; + let m = <Self as Module>::init(name, module)?; // SAFETY: `slot` is valid for write per the contract with `pin_init_from_closure`. unsafe { slot.write(m) }; diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs index b89c681d97c0..0f41df2e6b96 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -903,7 +903,7 @@ struct Module { [$($crate::net::phy::create_phy_driver::<$driver>()),+]; impl $crate::Module for Module { - fn init(module: &'static ThisModule) -> Result<Self> { + fn init(_name: &'static CStr, module: &'static ThisModule) -> Result<Self> { // SAFETY: The anonymous constant guarantees that nobody else can access // the `DRIVERS` static. The array is used only in the C side. let drivers = unsafe { &mut DRIVERS }; diff --git a/rust/kernel/sync/lock/global.rs b/rust/kernel/sync/lock/global.rs index 480ee724e3cc..feff4638e20b 100644 --- a/rust/kernel/sync/lock/global.rs +++ b/rust/kernel/sync/lock/global.rs @@ -187,6 +187,7 @@ pub fn get_mut(&mut self) -> &mut T { /// ``` /// # mod ex { /// # use kernel::prelude::*; +/// # use kernel::str; /// kernel::sync::global_lock! { /// // SAFETY: Initialized in module initializer before first use. /// unsafe(uninit) static MY_COUNTER: Mutex<u32> = 0; @@ -199,7 +200,7 @@ pub fn get_mut(&mut self) -> &mut T { /// } /// /// impl kernel::Module for MyModule { -/// fn init(_module: &'static ThisModule) -> Result<Self> { +/// fn init(_name: &'static str::CStr, _module: &'static ThisModule) -> Result<Self> { /// // SAFETY: Called exactly once. /// unsafe { MY_COUNTER.init() }; /// @@ -216,6 +217,7 @@ pub fn get_mut(&mut self) -> &mut T { /// # mod ex { /// # use kernel::prelude::*; /// use kernel::sync::{GlobalGuard, GlobalLockedBy}; +/// # use kernel::str; /// /// kernel::sync::global_lock! { /// // SAFETY: Initialized in module initializer before first use. @@ -239,7 +241,7 @@ pub fn get_mut(&mut self) -> &mut T { /// } /// /// impl kernel::Module for MyModule { -/// fn init(_module: &'static ThisModule) -> Result<Self> { +/// fn init(_name: &'static str::CStr, _module: &'static ThisModule) -> Result<Self> { /// // SAFETY: Called exactly once. /// unsafe { MY_MUTEX.init() }; /// diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index 4ab94e44adfe..d669f9cd1726 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -32,6 +32,7 @@ /// /// ``` /// use kernel::prelude::*; +/// use kernel::str; /// /// module!{ /// type: MyModule, @@ -45,7 +46,7 @@ /// struct MyModule(i32); /// /// impl kernel::Module for MyModule { -/// fn init(_module: &'static ThisModule) -> Result<Self> { +/// fn init(_name: &'static str::CStr, _module: &'static ThisModule) -> Result<Self> { /// let foo: i32 = 42; /// pr_info!("I contain: {}\n", foo); /// Ok(Self(foo)) @@ -65,6 +66,7 @@ /// /// ``` /// use kernel::prelude::*; +/// use kernel::str; /// /// module!{ /// type: MyDeviceDriverModule, @@ -78,7 +80,7 @@ /// struct MyDeviceDriverModule; /// /// impl kernel::Module for MyDeviceDriverModule { -/// fn init(_module: &'static ThisModule) -> Result<Self> { +/// fn init(_name: &'static str::CStr, _module: &'static ThisModule) -> Result<Self> { /// Ok(Self) /// } /// } diff --git a/rust/macros/module.rs b/rust/macros/module.rs index 2587f41b0d39..1f14ef55341a 100644 --- a/rust/macros/module.rs +++ b/rust/macros/module.rs @@ -333,8 +333,11 @@ mod __module_init {{ /// /// This function must only be called once. unsafe fn __init() -> kernel::ffi::c_int {{ - let initer = - <{type_} as kernel::InPlaceModule>::init(&super::super::THIS_MODULE); + let initer = <{type_} as kernel::InPlaceModule>::init( + kernel::c_str!(\"{name}\"), + &super::super::THIS_MODULE + ); + // SAFETY: No data race, since `__MOD` can only be accessed by this module // and there only `__init` and `__exit` access it. These functions are only // called once and `__exit` cannot be called before or during `__init`. diff --git a/samples/rust/rust_minimal.rs b/samples/rust/rust_minimal.rs index 4aaf117bf8e3..1577dc34e563 100644 --- a/samples/rust/rust_minimal.rs +++ b/samples/rust/rust_minimal.rs @@ -17,7 +17,7 @@ struct RustMinimal { } impl kernel::Module for RustMinimal { - fn init(_module: &'static ThisModule) -> Result<Self> { + fn init(_name: &'static CStr, _module: &'static ThisModule) -> Result<Self> { pr_info!("Rust minimal sample (init)\n"); pr_info!("Am I built-in? {}\n", !cfg!(MODULE)); diff --git a/samples/rust/rust_print_main.rs b/samples/rust/rust_print_main.rs index aed90a6feecf..0853d767439b 100644 --- a/samples/rust/rust_print_main.rs +++ b/samples/rust/rust_print_main.rs @@ -41,7 +41,7 @@ fn arc_print() -> Result { } impl kernel::Module for RustPrint { - fn init(_module: &'static ThisModule) -> Result<Self> { + fn init(_name: &'static CStr, _module: &'static ThisModule) -> Result<Self> { pr_info!("Rust printing macros sample (init)\n"); pr_emerg!("Emergency message (level 0) without args\n");
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`. Signed-off-by: Danilo Krummrich <dakr@kernel.org> --- drivers/block/rnull.rs | 2 +- rust/kernel/lib.rs | 14 ++++++++++---- rust/kernel/net/phy.rs | 2 +- rust/kernel/sync/lock/global.rs | 6 ++++-- rust/macros/lib.rs | 6 ++++-- rust/macros/module.rs | 7 +++++-- samples/rust/rust_minimal.rs | 2 +- samples/rust/rust_print_main.rs | 2 +- 8 files changed, 27 insertions(+), 14 deletions(-)