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.
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.
--- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -2702,6 +2702,32 @@ static int find_module_sections(struct module *mod, struct load_info *info) sizeof(*mod->kunit_init_suites), &mod->num_kunit_init_suites); #endif +#ifdef CONFIG_PCI_QUIRKS
- mod->pci_fixup_early = section_objs(info, ".pci_fixup_early",
sizeof(*mod->pci_fixup_early),&mod->pci_fixup_early_size);- mod->pci_fixup_header = section_objs(info, ".pci_fixup_header",
sizeof(*mod->pci_fixup_header),&mod->pci_fixup_header_size);- mod->pci_fixup_final = section_objs(info, ".pci_fixup_final",
sizeof(*mod->pci_fixup_final),&mod->pci_fixup_final_size);- mod->pci_fixup_enable = section_objs(info, ".pci_fixup_enable",
sizeof(*mod->pci_fixup_enable),&mod->pci_fixup_enable_size);- mod->pci_fixup_resume = section_objs(info, ".pci_fixup_resume",
sizeof(*mod->pci_fixup_resume),&mod->pci_fixup_resume_size);- mod->pci_fixup_suspend = section_objs(info, ".pci_fixup_suspend",
sizeof(*mod->pci_fixup_suspend),&mod->pci_fixup_suspend_size);- mod->pci_fixup_resume_early = section_objs(info, ".pci_fixup_resume_early",
sizeof(*mod->pci_fixup_resume_early),&mod->pci_fixup_resume_early_size);- mod->pci_fixup_suspend_late = section_objs(info, ".pci_fixup_suspend_late",
sizeof(*mod->pci_fixup_suspend_late),&mod->pci_fixup_suspend_late_size);+#endif mod->extable = section_objs(info, "__ex_table", sizeof(*mod->extable), &mod->num_exentries);
Nit: I suggest writing the object_size argument passed to section_objs() here directly as "1" instead of using sizeof(*mod->pci_fixup_...) = sizeof(void). This makes the style consistent with the other code in find_module_sections().
Ack.
Thanks, Brian