Re: [PATCH v12 15/19] PCI: Add KUnit coverage for ACS isolation checks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] PCI: duplicated mock device setup in pci_acs_flags_enabled_test()
--

commit cd21c328560f2b4befdb418098279304a429237f
Author: Leon Romanovsky <leon@kernel.org>

PCI: Add KUnit coverage for ACS isolation checks

This commit exposes pci_acs_flags_enabled() when KUnit is enabled and adds
KUnit tests in drivers/pci/pci_acs_test.c using mock config-space operations
to verify ACS isolation checks, missing capabilities, and failed config reads.

> diff --git a/drivers/pci/pci_acs_test.c b/drivers/pci/pci_acs_test.c
> index 18c697226762d..369444d2a04a0 100644
> --- a/drivers/pci/pci_acs_test.c
> +++ b/drivers/pci/pci_acs_test.c
> @@ -103,10 +103,145 @@ static void pci_acs_p2pdma_completion_test(struct kunit *test)

[ ... ]

> +static void pci_acs_flags_enabled_test(struct kunit *test)
> +{
> +	const struct acs_isolation_case *c = test->param_value;
> +	struct acs_ctrl_cfg cfg = {
> +		.devfn = PCI_DEVFN(0, 0),
> +		.cap = ACS_TEST_CAP,
> +		.ctrl = c->ctrl,
> +	};
> +	struct pci_bus *bus = kunit_kzalloc(test, sizeof(*bus), GFP_KERNEL);
> +	struct pci_dev *pdev = kunit_kzalloc(test, sizeof(*pdev), GFP_KERNEL);
> +
> +	KUNIT_ASSERT_NOT_NULL(test, bus);
> +	KUNIT_ASSERT_NOT_NULL(test, pdev);
> +
> +	bus->ops = &acs_ctrl_ops;
> +	bus->sysdata = &cfg;
> +
> +	pdev->bus = bus;
> +	pdev->devfn = cfg.devfn;
> +	pdev->acs_cap = ACS_TEST_CAP;
> +	pdev->acs_capabilities = ACS_ALL_CAPS;
> +
> +	KUNIT_EXPECT_EQ(test, pci_acs_flags_enabled(pdev, c->req), c->expect);
> +}
> +
> +static bool acs_isolated(struct kunit *test, struct acs_ctrl_cfg *cfg,
> +			 u16 acs_cap, u16 acs_flags)
> +{
> +	struct pci_bus *bus = kunit_kzalloc(test, sizeof(*bus), GFP_KERNEL);
> +	struct pci_dev *pdev = kunit_kzalloc(test, sizeof(*pdev), GFP_KERNEL);
> +
> +	KUNIT_ASSERT_NOT_NULL(test, bus);
> +	KUNIT_ASSERT_NOT_NULL(test, pdev);
> +
> +	bus->ops = &acs_ctrl_ops;
> +	bus->sysdata = cfg;
> +
> +	pdev->bus = bus;
> +	pdev->devfn = cfg->devfn;
> +	pdev->acs_cap = acs_cap;
> +	pdev->acs_capabilities = ACS_ALL_CAPS;
> +
> +	return pci_acs_flags_enabled(pdev, acs_flags);
> +}

[Severity: Low]
This isn't a bug, but in drivers/pci/pci_acs_test.c,
pci_acs_flags_enabled_test() open-codes the same mock struct pci_bus and
struct pci_dev allocation and initialization that acs_isolated() performs
immediately below it.

Also, struct acs_ctrl_cfg already carries the cap field, which every caller
initializes to the same value passed as the separate acs_cap argument to
acs_isolated().

Could acs_isolated() set pdev->acs_cap from cfg->cap directly, and could
pci_acs_flags_enabled_test() call acs_isolated() to avoid duplicating the
setup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-fix-p2p-acs-v4-0-v12-0-912101f93517@nvidia.com?part=15