Hi Petr,
On Wed, Sep 24, 2025 at 09:48:47AM +0200, Petr Pavlu wrote:
On 9/23/25 7:42 PM, Brian Norris wrote:
Hi Petr,
On Tue, Sep 23, 2025 at 02:55:34PM +0200, Petr Pavlu wrote:
On 9/13/25 12:59 AM, Brian Norris wrote:
@@ -259,6 +315,12 @@ void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev) return; } pci_do_fixups(dev, start, end);
- struct pci_fixup_arg arg = {
.dev = dev,.pass = pass,- };
- module_for_each_mod(pci_module_fixup, &arg);
The function module_for_each_mod() walks not only modules that are LIVE, but also those in the COMING and GOING states. This means that this code can potentially execute a PCI fixup from a module before its init function is invoked, and similarly, a fixup can be executed after the exit function has already run. Is this intentional?
Thanks for the callout. I didn't really give this part much thought previously.
Per the comments, COMING means "Full formed, running module_init". I believe that is a good thing, actually; specifically for controller drivers, module_init() might be probing the controller and enumerating child PCI devices to which we should apply these FIXUPs. That is a key case to support.
GOING is not clearly defined in the header comments, but it seems like it's a relatively narrow window between determining there are no module refcounts (and transition to GOING) and starting to really tear it down (transitioning to UNFORMED before any significant teardown). module_exit() runs in the GOING phase.
I think it does not make sense to execute FIXUPs on a GOING module; I'll make that change.
Note that when walking the modules list using module_for_each_mod(), the delete_module() operation can concurrently transition a module to MODULE_STATE_GOING. If you are thinking about simply having pci_module_fixup() check that mod->state isn't MODULE_STATE_GOING, I believe this won't quite work.
Good point. I think this at least suggests that this should hook into some blocking point in the module-load sequence, such as the notifiers or even module_init() as you suggest below.
Re-quoting one piece:
This means that this code can potentially execute a PCI fixup from a module before its init function is invoked,
IIUC, this part is not true? A module is put into COMING state before its init function is invoked.
When loading a module, the load_module() function calls complete_formation(), which puts the module into the COMING state. At this point, the new code in pci_fixup_device() can see the new module and potentially attempt to invoke its PCI fixups. However, such a module has still a bit of way to go before its init function is called from do_init_module(). The module hasn't yet had its arguments parsed, is not linked in sysfs, isn't fully registered with codetag support, and hasn't invoked its constructors (needed for gcov/kasan support).
It seems unlikely that sysfs, codetag, or arguments should matter much. gcov and kasan might be nice to have though.
I don't know enough about PCI fixups and what is allowable in them, but I suspect it would be better to ensure that no fixup can be invoked from the module during this period.
I don't know of general rules, but they generally do pretty minimal work to adjust various fields in and around 'struct pci_dev', to account for broken IDs. Sometimes they need to read a few PCI registers. They may even tweak PM-related features. It varies based on what kind of "quriky" devices need to be handled, but it's usually pretty straightforward and well-contained -- not relying on any kind of global state, or even all that much specific to the module in question besides constant IDs.
(You can peruse drivers/pci/quirks.c or the various other files that use DECLARE_PCI_FIXUP_*() macros, if you're curious.)
If the above makes sense, I think using module_for_each_mod() might not be the right approach. Alternative options include registering a module notifier or having modules explicitly register their PCI fixups in their init function.
I agree module_for_each_mod() is probably not the right choice, but I'm not sure what the right choice is.
register_module_notifier() + keying off MODULE_STATE_COMING before pulling in the '.pci_fixup*' list seems attractive, but it still comes before gcov/kasan.
It seems like "first thing in module_init()" would be the right choice, but I don't know of a great way to do that. I could insert PCI-related calls directly into do_init_module() / delete_module(), but that doesn't seem very elegant. I could also mess with the module_{init,exit}() macros, but that seems a bit strange too.
I'm open to suggestions. Or else maybe I'll just go with register_module_notifier(), and accept that there may some small downsides still.
Thanks, Brian