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?
+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().