Message-ID: <p6wq2kakzc53fdf47nmezlp2dccimblhc2ssb5hayll3epe7oe@jlgmjqyffkp4>
Date: Fri, 6 Feb 2026 14:48:10 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, 
	iommu@...ts.linux.dev, Naresh Kamboju <naresh.kamboju@...aro.org>, 
	Pavankumar Kondeti <quic_pkondeti@...cinc.com>, Xingang Wang <wangxingang5@...wei.com>, 
	Marek Szyprowski <m.szyprowski@...sung.com>, Robin Murphy <robin.murphy@....com>, 
	Jason Gunthorpe <jgg@...pe.ca>
Subject: Re: [PATCH v3 1/4] PCI: Enable ACS only after configuring IOMMU for
 OF platforms

On Wed, Feb 04, 2026 at 05:33:30PM -0600, Bjorn Helgaas wrote:
> On Fri, Jan 02, 2026 at 09:04:47PM +0530, Manivannan Sadhasivam wrote:
> > For enabling ACS without the cmdline params, the platform drivers are
> > expected to call pci_request_acs() API which sets a static flag,
> > 'pci_acs_enable' in drivers/pci/pci.c. And this flag is used to enable ACS
> > in pci_enable_acs() helper, which gets called during pci_acs_init(), as per
> > this call stack:
> > 
> > -> pci_device_add()
> > 	-> pci_init_capabilities()
> > 		-> pci_acs_init()
> > 			/* check for pci_acs_enable */
> > 			-> pci_enable_acs()
> > 
> > For the OF platforms, pci_request_acs() is called during
> > of_iommu_configure() during device_add(), as per this call stack:
> > 
> > -> device_add()
> > 	-> iommu_bus_notifier()
> > 		-> iommu_probe_device()
> > 			-> pci_dma_configure()
> > 				-> of_dma_configure()
> > 					-> of_iommu_configure()
> > 						/* set pci_acs_enable */
> > 						-> pci_request_acs()
> > 
> > As seen from both call stacks, pci_enable_acs() is called way before the
> > invocation of pci_request_acs() for the OF platforms. This means,
> > pci_enable_acs() will not enable ACS for the first device that gets
> > enumerated, which is usally the Root Port device. But since the static
> > flag, 'pci_acs_enable' is set *afterwards*, ACS will be enabled for the
> > ACS capable devices enumerated later.
> > 
> > To fix this issue, do not call pci_enable_acs() from pci_acs_init(), but
> > only from pci_dma_configure() after calling of_dma_configure(). This makes
> > sure that pci_enable_acs() only gets called after the IOMMU framework has
> > called pci_request_acs(). The ACS enablement flow now looks like:
> > 
> > -> pci_device_add()
> > 	-> pci_init_capabilities()
> > 		/* Just store the ACS cap */
> > 		-> pci_acs_init()
> > 	-> device_add()
> > 		...
> > 		-> pci_dma_configure()
> > 			-> of_dma_configure()
> > 				-> pci_request_acs()
> > 			-> pci_enable_acs()
> > 
> > For the ACPI platforms, pci_request_acs() is called during ACPI
> > initialization time itself, independent of the IOMMU framework.
> 
> I don't think cmdline params are relevant, since I don't see any that
> set pci_acs_enable or call pci_request_acs().
> 
> I propose a longer but combined call chain in the commit log to show
> the old sequence vs the new one.  I do notice the double call of
> pci_enable_acs() (along with dev->bus->dma_configure()), which all
> looks odd, but maybe it will be rationalized someday:
> 

Yeah, there is no issue with calling pci_enable_acs() twice as of now, but I
don't have a better solution either.

>     PCI: Enable ACS after configuring IOMMU for OF platforms
>     
>     Platform, ACPI, or IOMMU drivers call pci_request_acs(), which sets
>     'pci_acs_enable' to request that ACS be enabled for any devices enumerated
>     in the future.
>     
>     OF platforms called pci_enable_acs() for the first device before
>     of_iommu_configure() called pci_request_acs(), so ACS was never enabled for
>     that device (typically a Root Port).
>     
>     Call pci_enable_acs() later, from pci_dma_configure(), after
>     of_dma_configure() has had a chance to call pci_request_acs().
>     
>     Here's the call path, showing the move of pci_enable_acs() from
>     pci_acs_init() to pci_dma_configure(), where it always happens after
>     pci_request_acs():
>     
>         pci_device_add
>           pci_init_capabilities
>             pci_acs_init
>      -        pci_enable_acs
>      -          if (pci_acs_enable)                <-- previous test
>      -            ...
>           device_add
>             bus_notify(BUS_NOTIFY_ADD_DEVICE)
>               iommu_bus_notifier
>                 iommu_probe_device
>                   iommu_init_device
>                     dev->bus->dma_configure
>                       pci_dma_configure            # pci_bus_type.dma_configure
>                         of_dma_configure
>                           of_iommu_configure
>                             pci_request_acs
>                               pci_acs_enable = 1   <-- set
>      +                  pci_enable_acs
>      +                    if (pci_acs_enable)      <-- new test
>      +                      ...
>             bus_probe_device
>               device_initial_probe
>                 ...
>                   really_probe
>                     dev->bus->dma_configure
>                       pci_dma_configure            # pci_bus_type.dma_configure
>                         ...
>                           pci_enable_acs
>     
>     Note that we will now call pci_enable_acs() twice for every device, first
>     from the iommu_probe_device() path and again from the really_probe() path.
>     Presumably that's not an issue since we also call dev->bus->dma_configure()
>     twice.
>     
>     For the ACPI platforms, pci_request_acs() is called during ACPI
>     initialization time itself, independent of the IOMMU framework.
> 

Thanks! Let me know if I need to respin or you want to ammend it while applying.
(It is not clear to me what is your plan here).

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists