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

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

[Pavankumar Kondeti]

From: Xingang Wang <wangxingang5@huawei.com>

When booting with devicetree, the pci_request_acs() is called after the
enumeration and initialization of PCI devices, thus the ACS is not
enabled. And ACS should be enabled when IOMMU is detected for the
PCI host bridge, so add check for IOMMU before probe of PCI host and call
pci_request_acs() to make sure ACS will be enabled when enumerating PCI
devices.

Fixes: 6bf6c24720d33 ("iommu/of: Request ACS from the PCI core when configuring IOMMU linkage")
Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
---
Earlier this patch made it to linux-next but got dropped later as it
broke PCI on ARM Juno R1 board. AFAICT, this issue is never root caused,
so resending this patch to get attention again.

https://lore.kernel.org/all/1621566204-37456-1-git-send-email-wangxingang5@huawei.com/

The original problem that is being fixed by this patch still exists. In
my use case, all the PCI VF(s) assigned to a VM are sharing the same
group since these functions are attached under a Multi function PCIe root port 
emulated by the QEMU. This patch fixes that problem.
---
 drivers/iommu/of_iommu.c | 1 -
 drivers/pci/of.c         | 8 +++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e7a6a1611d19..f19db52388f5 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -141,7 +141,6 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
 			.np = master_np,
 		};
 
-		pci_request_acs();
 		err = pci_for_each_dma_alias(to_pci_dev(dev),
 					     of_pci_iommu_init, &info);
 		of_pci_check_device_ats(dev, master_np);
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index dacea3fc5128..dc90f4e45dd3 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -637,9 +637,15 @@ static int pci_parse_request_of_pci_ranges(struct device *dev,
 
 int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge)
 {
-	if (!dev->of_node)
+	struct device_node *node = dev->of_node;
+
+	if (!node)
 		return 0;
 
+	/* Detect IOMMU and make sure ACS will be enabled */
+	if (of_property_read_bool(node, "iommu-map"))
+		pci_request_acs();
+
 	bridge->swizzle_irq = pci_common_swizzle;
 	bridge->map_irq = of_irq_parse_and_map_pci;
 

---
base-commit: 59b723cd2adbac2a34fc8e12c74ae26ae45bf230
change-id: 20241107-pci_acs_fix-2239e0fb1768

Best regards,
-- 
Pavankumar Kondeti <quic_pkondeti@quicinc.com>

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

[Greg Kroah-Hartman]

On Thu, Nov 07, 2024 at 01:29:15PM +0530, Pavankumar Kondeti wrote:
> From: Xingang Wang <wangxingang5@huawei.com>
> 
> When booting with devicetree, the pci_request_acs() is called after the
> enumeration and initialization of PCI devices, thus the ACS is not
> enabled. And ACS should be enabled when IOMMU is detected for the
> PCI host bridge, so add check for IOMMU before probe of PCI host and call
> pci_request_acs() to make sure ACS will be enabled when enumerating PCI
> devices.
> 
> Fixes: 6bf6c24720d33 ("iommu/of: Request ACS from the PCI core when configuring IOMMU linkage")
> Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
> Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
> ---
> Earlier this patch made it to linux-next but got dropped later as it
> broke PCI on ARM Juno R1 board. AFAICT, this issue is never root caused,
> so resending this patch to get attention again.
> 
> https://lore.kernel.org/all/1621566204-37456-1-git-send-email-wangxingang5@huawei.com/

Please don't resend known-broken patches.  Please fix them up before
resending, otherwise we will just ignore this one as well as obviously
we can not take such a thing (nor should you want us to.)

thanks,

greg k-h

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

[Pavan Kondeti]

On Thu, Nov 07, 2024 at 09:17:44AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Nov 07, 2024 at 01:29:15PM +0530, Pavankumar Kondeti wrote:
> > From: Xingang Wang <wangxingang5@huawei.com>
> > 
> > When booting with devicetree, the pci_request_acs() is called after the
> > enumeration and initialization of PCI devices, thus the ACS is not
> > enabled. And ACS should be enabled when IOMMU is detected for the
> > PCI host bridge, so add check for IOMMU before probe of PCI host and call
> > pci_request_acs() to make sure ACS will be enabled when enumerating PCI
> > devices.
> > 
> > Fixes: 6bf6c24720d33 ("iommu/of: Request ACS from the PCI core when configuring IOMMU linkage")
> > Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
> > Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
> > ---
> > Earlier this patch made it to linux-next but got dropped later as it
> > broke PCI on ARM Juno R1 board. AFAICT, this issue is never root caused,
> > so resending this patch to get attention again.
> > 
> > https://lore.kernel.org/all/1621566204-37456-1-git-send-email-wangxingang5@huawei.com/
> 
> Please don't resend known-broken patches.  Please fix them up before
> resending, otherwise we will just ignore this one as well as obviously
> we can not take such a thing (nor should you want us to.)
> 

Thanks Greg for taking a look.

AFAICT, there is nothing wrong with the patch (It was Ackd before). The
root cause of the breakage on Juno ARM board is not identified. I resent
the patch to get the attention from people who reported this problem few
years back. I am *hoping* that they might have fixed the issue on their
side.

Thanks,
Pavan

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

[Bjorn Helgaas]

On Thu, Nov 07, 2024 at 01:29:15PM +0530, Pavankumar Kondeti wrote:
> From: Xingang Wang <wangxingang5@huawei.com>
> 
> When booting with devicetree, the pci_request_acs() is called after the
> enumeration and initialization of PCI devices, thus the ACS is not
> enabled. And ACS should be enabled when IOMMU is detected for the
> PCI host bridge, so add check for IOMMU before probe of PCI host and call
> pci_request_acs() to make sure ACS will be enabled when enumerating PCI
> devices.
> 
> Fixes: 6bf6c24720d33 ("iommu/of: Request ACS from the PCI core when configuring IOMMU linkage")
> Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
> Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
> ---
> Earlier this patch made it to linux-next but got dropped later as it
> broke PCI on ARM Juno R1 board. AFAICT, this issue is never root caused,
> so resending this patch to get attention again.
> 
> https://lore.kernel.org/all/1621566204-37456-1-git-send-email-wangxingang5@huawei.com/
> 
> The original problem that is being fixed by this patch still exists. In
> my use case, all the PCI VF(s) assigned to a VM are sharing the same
> group since these functions are attached under a Multi function PCIe root port 

FWIW, here are the problem reports (which are buried in the thread
above):

  https://lore.kernel.org/all/01314d70-41e6-70f9-e496-84091948701a@samsung.com/ (Marek)
  https://lore.kernel.org/all/CADYN=9JWU3CMLzMEcD5MSQGnaLyDRSKc5SofBFHUax6YuTRaJA@mail.gmail.com/ (Anders)

Given problem reports, the fact that the patch was acked and reviewed
earlier means nothing.  We have to ensure that any issues are resolved
before considering this patch again.

> emulated by the QEMU. This patch fixes that problem.
> ---
>  drivers/iommu/of_iommu.c | 1 -
>  drivers/pci/of.c         | 8 +++++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index e7a6a1611d19..f19db52388f5 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -141,7 +141,6 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
>  			.np = master_np,
>  		};
>  
> -		pci_request_acs();
>  		err = pci_for_each_dma_alias(to_pci_dev(dev),
>  					     of_pci_iommu_init, &info);
>  		of_pci_check_device_ats(dev, master_np);
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index dacea3fc5128..dc90f4e45dd3 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -637,9 +637,15 @@ static int pci_parse_request_of_pci_ranges(struct device *dev,
>  
>  int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge)
>  {
> -	if (!dev->of_node)
> +	struct device_node *node = dev->of_node;
> +
> +	if (!node)
>  		return 0;
>  
> +	/* Detect IOMMU and make sure ACS will be enabled */
> +	if (of_property_read_bool(node, "iommu-map"))
> +		pci_request_acs();
> +
>  	bridge->swizzle_irq = pci_common_swizzle;
>  	bridge->map_irq = of_irq_parse_and_map_pci;
>  
> 
> ---
> base-commit: 59b723cd2adbac2a34fc8e12c74ae26ae45bf230
> change-id: 20241107-pci_acs_fix-2239e0fb1768
> 
> Best regards,
> -- 
> Pavankumar Kondeti <quic_pkondeti@quicinc.com>
>