The PCI framework supports device quirks via a series of macros and
linker sections. This support previously did not work when used in
modules. Add a basic set of tests for matching/non-matching devices.
Example run:
$ ./tools/testing/kunit/kunit.py run 'pci_fixup*'
[...]
[15:31:30] ============ pci_fixup_test_cases (2 subtests) =============
[15:31:30] [PASSED] pci_fixup_match_test
[15:31:30] [PASSED] pci_fixup_nomatch_test
[15:31:30] ============== [PASSED] pci_fixup_test_cases ===============
[15:31:30] ============================================================
[15:31:30] Testing complete. Ran 2 tests: passed: 2
[15:31:30] Elapsed time: 11.197s total, 0.001s configuring, 9.870s building, 1.299s running
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
drivers/pci/Kconfig | 11 +++
drivers/pci/Makefile | 1 +
drivers/pci/fixup-test.c | 197 +++++++++++++++++++++++++++++++++++++++
3 files changed, 209 insertions(+)
create mode 100644 drivers/pci/fixup-test.c
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 9a249c65aedc..a4fa9be797e7 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -68,6 +68,17 @@ config PCI_QUIRKS
Disable this only if your target machine is unaffected by PCI
quirks.
+config PCI_FIXUP_KUNIT_TEST
+ tristate "KUnit tests for PCI fixup code" if !KUNIT_ALL_TESTS
+ depends on KUNIT
+ depends on PCI_DOMAINS_GENERIC
+ default KUNIT_ALL_TESTS
+ help
+ This builds unit tests for the PCI quirk/fixup framework. Recommended
+ only for kernel developers.
+
+ When in doubt, say N.
+
config PCI_DEBUG
bool "PCI Debugging"
depends on DEBUG_KERNEL
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 67647f1880fb..ade400250ceb 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -20,6 +20,7 @@ endif
obj-$(CONFIG_OF) += of.o
obj-$(CONFIG_PCI_QUIRKS) += quirks.o
+obj-$(CONFIG_PCI_FIXUP_KUNIT_TEST) += fixup-test.o
obj-$(CONFIG_HOTPLUG_PCI) += hotplug/
obj-$(CONFIG_PCI_ATS) += ats.o
obj-$(CONFIG_PCI_IOV) += iov.o
diff --git a/drivers/pci/fixup-test.c b/drivers/pci/fixup-test.c
new file mode 100644
index 000000000000..54b895fc8f3e
--- /dev/null
+++ b/drivers/pci/fixup-test.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2025 Google, Inc.
+ */
+
+#include <kunit/device.h>
+#include <kunit/test.h>
+#include <linux/cleanup.h>
+#include <linux/pci.h>
+
+#define DEVICE_NAME "pci_fixup_test_device"
+#define TEST_VENDOR_ID 0xdead
+#define TEST_DEVICE_ID 0xbeef
+
+#define TEST_CONF_SIZE 4096
+static u8 *test_conf_space;
+
+#define test_readb(addr) (*(u8 *)(addr))
+#define test_readw(addr) le16_to_cpu(*((__force __le16 *)(addr)))
+#define test_readl(addr) le32_to_cpu(*((__force __le32 *)(addr)))
+#define test_writeb(addr, v) (*(u8 *)(addr) = (v))
+#define test_writew(addr, v) (*((__force __le16 *)(addr)) = cpu_to_le16(v))
+#define test_writel(addr, v) (*((__force __le32 *)(addr)) = cpu_to_le32(v))
+
+static int test_config_read(struct pci_bus *bus, unsigned int devfn, int where,
+ int size, u32 *val)
+{
+ if (PCI_SLOT(devfn) > 0)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+
+ if (where + size > TEST_CONF_SIZE)
+ return PCIBIOS_BUFFER_TOO_SMALL;
+
+ if (size == 1)
+ *val = test_readb(test_conf_space + where);
+ else if (size == 2)
+ *val = test_readw(test_conf_space + where);
+ else if (size == 4)
+ *val = test_readl(test_conf_space + where);
+
+ return PCIBIOS_SUCCESSFUL;
+}
+
+static int test_config_write(struct pci_bus *bus, unsigned int devfn, int where,
+ int size, u32 val)
+{
+ if (PCI_SLOT(devfn) > 0)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+
+ if (where + size > TEST_CONF_SIZE)
+ return PCIBIOS_BUFFER_TOO_SMALL;
+
+ if (size == 1)
+ test_writeb(test_conf_space + where, val);
+ else if (size == 2)
+ test_writew(test_conf_space + where, val);
+ else if (size == 4)
+ test_writel(test_conf_space + where, val);
+
+ return PCIBIOS_SUCCESSFUL;
+}
+
+static struct pci_ops test_ops = {
+ .read = test_config_read,
+ .write = test_config_write,
+};
+
+static struct pci_dev *hook_device_early;
+static struct pci_dev *hook_device_header;
+static struct pci_dev *hook_device_final;
+static struct pci_dev *hook_device_enable;
+
+static void pci_fixup_early_hook(struct pci_dev *pdev)
+{
+ hook_device_early = pdev;
+}
+DECLARE_PCI_FIXUP_EARLY(TEST_VENDOR_ID, TEST_DEVICE_ID, pci_fixup_early_hook);
+
+static void pci_fixup_header_hook(struct pci_dev *pdev)
+{
+ hook_device_header = pdev;
+}
+DECLARE_PCI_FIXUP_HEADER(TEST_VENDOR_ID, TEST_DEVICE_ID, pci_fixup_header_hook);
+
+static void pci_fixup_final_hook(struct pci_dev *pdev)
+{
+ hook_device_final = pdev;
+}
+DECLARE_PCI_FIXUP_FINAL(TEST_VENDOR_ID, TEST_DEVICE_ID, pci_fixup_final_hook);
+
+static void pci_fixup_enable_hook(struct pci_dev *pdev)
+{
+ hook_device_enable = pdev;
+}
+DECLARE_PCI_FIXUP_ENABLE(TEST_VENDOR_ID, TEST_DEVICE_ID, pci_fixup_enable_hook);
+
+static int pci_fixup_test_init(struct kunit *test)
+{
+ hook_device_early = NULL;
+ hook_device_header = NULL;
+ hook_device_final = NULL;
+ hook_device_enable = NULL;
+
+ return 0;
+}
+
+static void pci_fixup_match_test(struct kunit *test)
+{
+ struct device *dev = kunit_device_register(test, DEVICE_NAME);
+
+ KUNIT_ASSERT_PTR_NE(test, NULL, dev);
+
+ test_conf_space = kunit_kzalloc(test, TEST_CONF_SIZE, GFP_KERNEL);
+ KUNIT_ASSERT_PTR_NE(test, NULL, test_conf_space);
+
+ /* Minimal configuration space: a stub vendor and device ID */
+ test_writew(test_conf_space + PCI_VENDOR_ID, TEST_VENDOR_ID);
+ test_writew(test_conf_space + PCI_DEVICE_ID, TEST_DEVICE_ID);
+
+ struct pci_host_bridge *bridge = devm_pci_alloc_host_bridge(dev, 0);
+
+ KUNIT_ASSERT_PTR_NE(test, NULL, bridge);
+ bridge->ops = &test_ops;
+
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_early);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_header);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_final);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_enable);
+
+ KUNIT_EXPECT_EQ(test, 0, pci_host_probe(bridge));
+
+ struct pci_dev *pdev __free(pci_dev_put) =
+ pci_get_slot(bridge->bus, PCI_DEVFN(0, 0));
+ KUNIT_ASSERT_PTR_NE(test, NULL, pdev);
+
+ KUNIT_EXPECT_PTR_EQ(test, pdev, hook_device_early);
+ KUNIT_EXPECT_PTR_EQ(test, pdev, hook_device_header);
+ KUNIT_EXPECT_PTR_EQ(test, pdev, hook_device_final);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_enable);
+
+ KUNIT_EXPECT_EQ(test, 0, pci_enable_device(pdev));
+ KUNIT_EXPECT_PTR_EQ(test, pdev, hook_device_enable);
+}
+
+static void pci_fixup_nomatch_test(struct kunit *test)
+{
+ struct device *dev = kunit_device_register(test, DEVICE_NAME);
+
+ KUNIT_ASSERT_PTR_NE(test, NULL, dev);
+
+ test_conf_space = kunit_kzalloc(test, TEST_CONF_SIZE, GFP_KERNEL);
+ KUNIT_ASSERT_PTR_NE(test, NULL, test_conf_space);
+
+ /* Minimal configuration space: a stub vendor and device ID */
+ test_writew(test_conf_space + PCI_VENDOR_ID, TEST_VENDOR_ID + 1);
+ test_writew(test_conf_space + PCI_DEVICE_ID, TEST_DEVICE_ID);
+
+ struct pci_host_bridge *bridge = devm_pci_alloc_host_bridge(dev, 0);
+
+ KUNIT_ASSERT_PTR_NE(test, NULL, bridge);
+ bridge->ops = &test_ops;
+
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_early);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_header);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_final);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_enable);
+
+ KUNIT_EXPECT_EQ(test, 0, pci_host_probe(bridge));
+
+ struct pci_dev *pdev __free(pci_dev_put) =
+ pci_get_slot(bridge->bus, PCI_DEVFN(0, 0));
+ KUNIT_ASSERT_PTR_NE(test, NULL, pdev);
+
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_early);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_header);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_final);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_enable);
+
+ KUNIT_EXPECT_EQ(test, 0, pci_enable_device(pdev));
+ KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_enable);
+}
+
+static struct kunit_case pci_fixup_test_cases[] = {
+ KUNIT_CASE(pci_fixup_match_test),
+ KUNIT_CASE(pci_fixup_nomatch_test),
+ {}
+};
+
+static struct kunit_suite pci_fixup_test_suite = {
+ .name = "pci_fixup_test_cases",
+ .test_cases = pci_fixup_test_cases,
+ .init = pci_fixup_test_init,
+};
+
+kunit_test_suite(pci_fixup_test_suite);
+MODULE_DESCRIPTION("PCI fixups unit test suite");
+MODULE_LICENSE("GPL");
--
2.51.0.384.g4c02a37b29-googOn Fri, Sep 12, 2025 at 03:59:33PM -0700, Brian Norris wrote:
> +static int test_config_read(struct pci_bus *bus, unsigned int devfn, int where,
> + int size, u32 *val)
> +{
> + if (PCI_SLOT(devfn) > 0)
> + return PCIBIOS_DEVICE_NOT_FOUND;
> +
> + if (where + size > TEST_CONF_SIZE)
> + return PCIBIOS_BUFFER_TOO_SMALL;
> +
> + if (size == 1)
> + *val = test_readb(test_conf_space + where);
> + else if (size == 2)
> + *val = test_readw(test_conf_space + where);
> + else if (size == 4)
> + *val = test_readl(test_conf_space + where);
> +
> + return PCIBIOS_SUCCESSFUL;
To handle cases where size might be a value other than {1, 2, 4}, would a
switch statement with a default case be more robust here?
> +static int test_config_write(struct pci_bus *bus, unsigned int devfn, int where,
> + int size, u32 val)
> +{
> + if (PCI_SLOT(devfn) > 0)
> + return PCIBIOS_DEVICE_NOT_FOUND;
> +
> + if (where + size > TEST_CONF_SIZE)
> + return PCIBIOS_BUFFER_TOO_SMALL;
> +
> + if (size == 1)
> + test_writeb(test_conf_space + where, val);
> + else if (size == 2)
> + test_writew(test_conf_space + where, val);
> + else if (size == 4)
> + test_writel(test_conf_space + where, val);
> +
> + return PCIBIOS_SUCCESSFUL;
Same here.
> +static struct pci_dev *hook_device_early;
> +static struct pci_dev *hook_device_header;
> +static struct pci_dev *hook_device_final;
> +static struct pci_dev *hook_device_enable;
> +
> +static void pci_fixup_early_hook(struct pci_dev *pdev)
> +{
> + hook_device_early = pdev;
> +}
> +DECLARE_PCI_FIXUP_EARLY(TEST_VENDOR_ID, TEST_DEVICE_ID, pci_fixup_early_hook);
> [...]
> +static int pci_fixup_test_init(struct kunit *test)
> +{
> + hook_device_early = NULL;
> + hook_device_header = NULL;
> + hook_device_final = NULL;
> + hook_device_enable = NULL;
> +
> + return 0;
> +}
FWIW: if the probe is synchronous and the thread is the same task_struct,
the module level variables can be eliminated by using:
test->priv = kunit_kzalloc(...);
KUNIT_ASSERT_PTR_NE(...);
And in the hooks, kunit_get_current_test() returns the struct kunit *.
> +static void pci_fixup_match_test(struct kunit *test)
> +{
> + struct device *dev = kunit_device_register(test, DEVICE_NAME);
> +
> + KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> +
> + test_conf_space = kunit_kzalloc(test, TEST_CONF_SIZE, GFP_KERNEL);
> + KUNIT_ASSERT_PTR_NE(test, NULL, test_conf_space);
The common initialization code can be moved to pci_fixup_test_init().
> + struct pci_host_bridge *bridge = devm_pci_alloc_host_bridge(dev, 0);
> +
> + KUNIT_ASSERT_PTR_NE(test, NULL, bridge);
> + bridge->ops = &test_ops;
The `bridge` allocation can be moved to .init() too.
> + KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_early);
> + KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_header);
> + KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_final);
> + KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_enable);
Does it really need to check them? They are just initialized by .init().
Hi,
On Mon, Sep 15, 2025 at 08:06:33AM +0000, Tzung-Bi Shih wrote:
> On Fri, Sep 12, 2025 at 03:59:33PM -0700, Brian Norris wrote:
> > +static int test_config_read(struct pci_bus *bus, unsigned int devfn, int where,
> > + int size, u32 *val)
> > +{
> > + if (PCI_SLOT(devfn) > 0)
> > + return PCIBIOS_DEVICE_NOT_FOUND;
> > +
> > + if (where + size > TEST_CONF_SIZE)
> > + return PCIBIOS_BUFFER_TOO_SMALL;
> > +
> > + if (size == 1)
> > + *val = test_readb(test_conf_space + where);
> > + else if (size == 2)
> > + *val = test_readw(test_conf_space + where);
> > + else if (size == 4)
> > + *val = test_readl(test_conf_space + where);
> > +
> > + return PCIBIOS_SUCCESSFUL;
>
> To handle cases where size might be a value other than {1, 2, 4}, would a
> switch statement with a default case be more robust here?
I was patterning based on pci_generic_config_read() and friends, but I
see that those use an 'else' for the last block, where I used an 'else
if'.
I suppose I could switch to 'else'. I'm not sure 'switch/case' is much
better.
> > +static int test_config_write(struct pci_bus *bus, unsigned int devfn, int where,
> > + int size, u32 val)
> > +{
> > + if (PCI_SLOT(devfn) > 0)
> > + return PCIBIOS_DEVICE_NOT_FOUND;
> > +
> > + if (where + size > TEST_CONF_SIZE)
> > + return PCIBIOS_BUFFER_TOO_SMALL;
> > +
> > + if (size == 1)
> > + test_writeb(test_conf_space + where, val);
> > + else if (size == 2)
> > + test_writew(test_conf_space + where, val);
> > + else if (size == 4)
> > + test_writel(test_conf_space + where, val);
> > +
> > + return PCIBIOS_SUCCESSFUL;
>
> Same here.
>
> > +static struct pci_dev *hook_device_early;
> > +static struct pci_dev *hook_device_header;
> > +static struct pci_dev *hook_device_final;
> > +static struct pci_dev *hook_device_enable;
> > +
> > +static void pci_fixup_early_hook(struct pci_dev *pdev)
> > +{
> > + hook_device_early = pdev;
> > +}
> > +DECLARE_PCI_FIXUP_EARLY(TEST_VENDOR_ID, TEST_DEVICE_ID, pci_fixup_early_hook);
> > [...]
> > +static int pci_fixup_test_init(struct kunit *test)
> > +{
> > + hook_device_early = NULL;
> > + hook_device_header = NULL;
> > + hook_device_final = NULL;
> > + hook_device_enable = NULL;
> > +
> > + return 0;
> > +}
>
> FWIW: if the probe is synchronous and the thread is the same task_struct,
> the module level variables can be eliminated by using:
>
> test->priv = kunit_kzalloc(...);
> KUNIT_ASSERT_PTR_NE(...);
>
> And in the hooks, kunit_get_current_test() returns the struct kunit *.
Ah, good suggestion, will give that a shot.
> > +static void pci_fixup_match_test(struct kunit *test)
> > +{
> > + struct device *dev = kunit_device_register(test, DEVICE_NAME);
> > +
> > + KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> > +
> > + test_conf_space = kunit_kzalloc(test, TEST_CONF_SIZE, GFP_KERNEL);
> > + KUNIT_ASSERT_PTR_NE(test, NULL, test_conf_space);
>
> The common initialization code can be moved to pci_fixup_test_init().
>
> > + struct pci_host_bridge *bridge = devm_pci_alloc_host_bridge(dev, 0);
> > +
> > + KUNIT_ASSERT_PTR_NE(test, NULL, bridge);
> > + bridge->ops = &test_ops;
>
> The `bridge` allocation can be moved to .init() too.
>
> > + KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_early);
> > + KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_header);
> > + KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_final);
> > + KUNIT_EXPECT_PTR_EQ(test, NULL, hook_device_enable);
>
> Does it really need to check them? They are just initialized by .init().
Probably not. I wrote these before there were multiple test cases and an
.init() function, and I didn't reconsider them afterward. And they'll be
especially pointless once these move into a kzalloc'd private structure.
Thanks for the suggestions.
Brian
© 2016 - 2025 Red Hat, Inc.