Re: [PATCH v2] iommu/of: Fix pci_request_acs() before enumerating PCI devices

[Xingang Wang]

On 2021/5/20 3:14, Rob Herring wrote:
> On Mon, May 17, 2021 at 01:17:05PM +0000, Wang Xingang wrote:
> > From: Xingang Wang <wangxingang5@xxxxxxxxxx>
> >
> > When booting with devicetree, the pci_request_acs() is called after the
> > enumeration and initialization of PCI devices, thus the ACS is not
> > enabled. This patch add check for IOMMU in of_core_init(), and call
> > pci_request_acs() when iommu is detected, making sure that the ACS will
> > be enabled.
> >
> > Fixes: 6bf6c24720d33 ("iommu/of: Request ACS from the PCI core when
> > configuring IOMMU linkage")
> > Signed-off-by: Xingang Wang <wangxingang5@xxxxxxxxxx>
> > ---
> >   drivers/iommu/of_iommu.c | 1 -
> >   drivers/of/base.c        | 9 ++++++++-
> >   2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > Change log:
> > v1->v2:
> >   - remove pci_request_acs() in of_iommu_configure
> >   - check and call pci_request_acs() in of_core_init()
> >
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index a9d2df001149..54a14da242cc 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -205,7 +205,6 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
> >   			.np = master_np,
> >   		};
> >   
> > -		pci_request_acs();
> >   		err = pci_for_each_dma_alias(to_pci_dev(dev),
> >   					     of_pci_iommu_init, &info);
> >   	} else {
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 48e941f99558..95cd8f0e5435 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -24,6 +24,7 @@
> >   #include <linux/of.h>
> >   #include <linux/of_device.h>
> >   #include <linux/of_graph.h>
> > +#include <linux/pci.h>
> >   #include <linux/spinlock.h>
> >   #include <linux/slab.h>
> >   #include <linux/string.h>
> > @@ -166,7 +167,7 @@ void __of_phandle_cache_inv_entry(phandle handle)
> >   void __init of_core_init(void)
> >   {
> >   	struct device_node *np;
> > -
> > +	bool of_iommu_detect = false;
> >   
> >   	/* Create the kset, and register existing nodes */
> >   	mutex_lock(&of_mutex);
> > @@ -180,6 +181,12 @@ void __init of_core_init(void)
> >   		__of_attach_node_sysfs(np);
> >   		if (np->phandle && !phandle_cache[of_phandle_cache_hash(np->phandle)])
> >   			phandle_cache[of_phandle_cache_hash(np->phandle)] = np;
> > +
> > +		/* Detect IOMMU and make sure ACS will be enabled */
> > +		if (!of_iommu_detect && of_get_property(np, "iommu-map", NULL)) {
> > +			of_iommu_detect = true;
> > +			pci_request_acs();
> > +		}
>
> Private DT internal init code doesn't seem like the right place for
> this. If this needs to be ordered WRT PCI device enumeration, then
> somewhere in the PCI host bridge or bus init code seems like the right
> place to me.
>
> Also, shouldn't this be conditional on 'iommu-map' being in the host
> bridge or a parent or ??? rather than just any iommu-map anywhere in the
> DT.

Thanks for your suggestion, I will fix this and move the modification to 
procedure of pci_host_common_probe(), and before pci_host_probe().

> >   	}
> >   	mutex_unlock(&of_mutex);
> >   
> > --
> > 2.19.1
> .