Re: [PATCH] PCI: Add a quirk to enable SVA for HiSilicon chip

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

On Wed, Jan 13, 2021 at 08:05:11PM +0800, Zhangfei Gao wrote:
> > > +	/* Device-tree can set the stall property */
> > > +	if (!pdev->dev.of_node &&
> > > +	    device_add_properties(&pdev->dev, properties))
> > Does this mean "dma-can-stall" *can* be set via DT, and if it is, this
> > quirk is not needed?  So is this quirk basically a workaround for an
> > old or broken DT?
> The quirk is still needed for uefi case, since uefi can not describe the
> endpoints (peripheral devices).

Yes, this comment isn't very clear. How about
	/*
	 * Set the dma-can-stall property on ACPI platforms. Device tree
	 * can set it directly.
	 */ 

> > 
> > > +		pci_warn(pdev, "could not add stall property");
> > > +}
> > > +
> > Remove this blank line to follow the style of the rest of the file.
> > 
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa250, quirk_huawei_pcie_sva);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa251, quirk_huawei_pcie_sva);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa255, quirk_huawei_pcie_sva);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa256, quirk_huawei_pcie_sva);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa258, quirk_huawei_pcie_sva);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa259, quirk_huawei_pcie_sva);
> > > +
> > >   /*
> > >    * It's possible for the MSI to get corrupted if SHPC and ACPI are used
> > >    * together on certain PXH-based systems.
> 
> How about changes like this
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 68f53f7..886ea26 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2466,6 +2466,9 @@ static int arm_smmu_enable_pasid(struct
> arm_smmu_master *master)
>      if (num_pasids <= 0)
>          return num_pasids;
> 
> +    if (master->stall_enabled)
> +        pdev->pasid_no_tlp = 1;
> +

From the SMMU perspective there is no relation between stall and pasid, so
I don't think this makes a lot of sense. Could we instead set pasid_no_tlp
for the list of device IDs above?

I agree with splitting the patches. PASID support for SMMUv3 is upstream,
but the introduction of dma-can-stall, which this depends on, is still
pending on the list.

Thanks,
Jean