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
