linux-kernel - Re: [PATCH 2/4] PCI: Add KUnit tests for FIXUP quirks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aMfJCbld_TMHPTbD@google.com>
Date: Mon, 15 Sep 2025 08:06:33 +0000
From: Tzung-Bi Shih <tzungbi@...nel.org>
To: Brian Norris <briannorris@...omium.org>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,
	Luis Chamberlain <mcgrof@...nel.org>,
	Petr Pavlu <petr.pavlu@...e.com>,
	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 2/4] PCI: Add KUnit tests for FIXUP quirks

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