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

[Leon Romanovsky <leon@kernel.org>]

On Tue, Nov 03, 2026 at 11:11:10AM +0000, sashiko-bot@kernel.org wrote:
> 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?

Yes, I can combine them.

Thanks

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