From: Petr Pavlu <petr.pavlu@suse.com>
To: Brian Norris <briannorris@chromium.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
Luis Chamberlain <mcgrof@kernel.org>,
Daniel Gomez <da.gomez@kernel.org>,
linux-pci@vger.kernel.org, David Gow <davidgow@google.com>,
Rae Moar <rmoar@google.com>,
linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-modules@vger.kernel.org,
Johannes Berg <johannes@sipsolutions.net>,
Sami Tolvanen <samitolvanen@google.com>,
Richard Weinberger <richard@nod.at>, Wei Liu <wei.liu@kernel.org>,
Brendan Higgins <brendan.higgins@linux.dev>,
kunit-dev@googlegroups.com,
Anton Ivanov <anton.ivanov@cambridgegreys.com>,
linux-um@lists.infradead.org
Subject: Re: [PATCH 1/4] PCI: Support FIXUP quirks in modules
Date: Mon, 20 Oct 2025 13:53:49 +0200 [thread overview]
Message-ID: <eca6031d-7f72-4f24-bbd4-95354d5c9ca5@suse.com> (raw)
In-Reply-To: <aORJhL1yAPyV7YAW@google.com>
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
next prev parent reply other threads:[~2025-10-20 11:53 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-12 22:59 [PATCH 0/4] PCI: Add support and tests for FIXUP quirks in modules Brian Norris
2025-09-12 22:59 ` [PATCH 1/4] PCI: Support " Brian Norris
2025-09-15 6:33 ` Johannes Berg
2025-09-15 18:34 ` Brian Norris
2025-09-23 12:55 ` Petr Pavlu
2025-09-23 17:42 ` Brian Norris
2025-09-24 7:48 ` Petr Pavlu
2025-10-06 22:58 ` Brian Norris
2025-10-20 11:53 ` Petr Pavlu [this message]
2025-09-12 22:59 ` [PATCH 2/4] PCI: Add KUnit tests for FIXUP quirks Brian Norris
2025-09-15 8:06 ` Tzung-Bi Shih
2025-09-15 20:25 ` Brian Norris
2025-09-12 22:59 ` [PATCH 3/4] um: Select PCI_DOMAINS_GENERIC Brian Norris
2025-09-12 22:59 ` [PATCH 4/4] kunit: qemu_configs: Add PCI to arm, arm64 Brian Norris
2025-09-15 13:48 ` [PATCH 0/4] PCI: Add support and tests for FIXUP quirks in modules Christoph Hellwig
2025-09-15 18:41 ` Brian Norris
2025-09-22 18:13 ` Christoph Hellwig
2025-09-22 18:48 ` Brian Norris
2025-09-29 8:56 ` Christoph Hellwig
2025-09-23 16:20 ` Manivannan Sadhasivam
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=eca6031d-7f72-4f24-bbd4-95354d5c9ca5@suse.com \
--to=petr.pavlu@suse.com \
--cc=anton.ivanov@cambridgegreys.com \
--cc=bhelgaas@google.com \
--cc=brendan.higgins@linux.dev \
--cc=briannorris@chromium.org \
--cc=da.gomez@kernel.org \
--cc=davidgow@google.com \
--cc=johannes@sipsolutions.net \
--cc=kunit-dev@googlegroups.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-modules@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-um@lists.infradead.org \
--cc=mcgrof@kernel.org \
--cc=richard@nod.at \
--cc=rmoar@google.com \
--cc=samitolvanen@google.com \
--cc=wei.liu@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).