Message-ID: <eca6031d-7f72-4f24-bbd4-95354d5c9ca5@suse.com>
Date: Mon, 20 Oct 2025 13:53:49 +0200
From: Petr Pavlu <petr.pavlu@...e.com>
To: Brian Norris <briannorris@...omium.org>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, Luis Chamberlain
 <mcgrof@...nel.org>, Daniel Gomez <da.gomez@...nel.org>,
 linux-pci@...r.kernel.org, David Gow <davidgow@...gle.com>,
 Rae Moar <rmoar@...gle.com>, linux-kselftest@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-modules@...r.kernel.org,
 Johannes Berg <johannes@...solutions.net>,
 Sami Tolvanen <samitolvanen@...gle.com>, Richard Weinberger
 <richard@....at>, Wei Liu <wei.liu@...nel.org>,
 Brendan Higgins <brendan.higgins@...ux.dev>, kunit-dev@...glegroups.com,
 Anton Ivanov <anton.ivanov@...bridgegreys.com>, linux-um@...ts.infradead.org
Subject: Re: [PATCH 1/4] PCI: Support FIXUP quirks in modules

On 10/7/25 12:58 AM, Brian Norris wrote:
> 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.

We could introduce a new module state and have the module notifier fire
with this state at the required point. This seems to align best with how
other code is hooked into the module loader, although I dislike the idea
of introducing a new module state.

It might also be possible to insert a quirk registration function into
.init_array so it is called by do_mod_ctors().

> 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 don't think you want to modify module_{init,exit}(), but you could
introduce something like module_pci_controller(), along with
pci_register_quirks() and pci_unregister_quirks(), which would handle
quirk registration, somewhat similar to how module_pci_driver() works.

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

If it turns out there is no better way than register_module_notifier(),
then as mentioned above, it is possible to add a new module state to
properly support this case.

-- 
Thanks,
Petr

Powered by blists - more mailing lists