| Message ID | 1591762694-9131-1-git-send-email-zhangfei.gao@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [RFC] PCI: Remove End-End TLP as PASID dependency | expand |
On Wed, Jun 10, 2020 at 12:18:14PM +0800, Zhangfei Gao wrote: > Some platform devices appear as PCI and have PCI cfg space, > but are actually on the AMBA bus. > They can support PASID via smmu stall feature, but does not > support tlp since they are not real pci devices. > So remove tlp as a PASID dependency. > > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> > --- > drivers/pci/ats.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c > index 390e92f..8e31278 100644 > --- a/drivers/pci/ats.c > +++ b/drivers/pci/ats.c > @@ -344,9 +344,6 @@ int pci_enable_pasid(struct pci_dev *pdev, int features) > if (WARN_ON(pdev->pasid_enabled)) > return -EBUSY; > > - if (!pdev->eetlp_prefix_path) > - return -EINVAL; > - This check is useful, and follows the PCI specification (4.0r1.0 2.2.10.2 End-End TLP Prefix Processing: "Software should ensure that TLPs containing End-End TLP Prefixes are not sent to components that do not support them.") Why not set the eetlp_prefix_path bit from a PCI quirk? Unlike the stall problem from the other thread, this one looks like a simple design mistake that can be fixed easily in future iterations of the platform: just set the "End-End TLP Prefix Supported" bit in the Device Capability 2 Register of all bridges. Thanks, Jean
On 2020/6/10 下午3:46, Jean-Philippe Brucker wrote: > On Wed, Jun 10, 2020 at 12:18:14PM +0800, Zhangfei Gao wrote: >> Some platform devices appear as PCI and have PCI cfg space, >> but are actually on the AMBA bus. >> They can support PASID via smmu stall feature, but does not >> support tlp since they are not real pci devices. >> So remove tlp as a PASID dependency. >> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >> --- >> drivers/pci/ats.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c >> index 390e92f..8e31278 100644 >> --- a/drivers/pci/ats.c >> +++ b/drivers/pci/ats.c >> @@ -344,9 +344,6 @@ int pci_enable_pasid(struct pci_dev *pdev, int features) >> if (WARN_ON(pdev->pasid_enabled)) >> return -EBUSY; >> >> - if (!pdev->eetlp_prefix_path) >> - return -EINVAL; >> - > This check is useful, and follows the PCI specification (4.0r1.0 > 2.2.10.2 End-End TLP Prefix Processing: "Software should ensure that TLPs > containing End-End TLP Prefixes are not sent to components that do not > support them.") Thanks Jean, > > Why not set the eetlp_prefix_path bit from a PCI quirk? Unlike the stall > problem from the other thread, this one looks like a simple design mistake > that can be fixed easily in future iterations of the platform: just set > the "End-End TLP Prefix Supported" bit in the Device Capability 2 Register > of all bridges. Yes, we can still set eetlp_prefix_path bit from a PCI quirk. And we also have considered adding this bit in Device Capability 2 Register in future silicon. But we hesitated that it does reflect the real function: from register, it can support tlp, but in fact, it does not. Thanks
On 6/10/2020 4:00 AM, Zhangfei Gao wrote: >> Why not set the eetlp_prefix_path bit from a PCI quirk? Unlike the stall >> problem from the other thread, this one looks like a simple design >> mistake >> that can be fixed easily in future iterations of the platform: just set >> the "End-End TLP Prefix Supported" bit in the Device Capability 2 >> Register >> of all bridges. > Yes, we can still set eetlp_prefix_path bit from a PCI quirk. I agree. Vendor specific quirk should be the way to go here if it is not compliant with the spec.
[+cc Sinan] On Wed, Jun 10, 2020 at 12:18:14PM +0800, Zhangfei Gao wrote: > Some platform devices appear as PCI and have PCI cfg space, > but are actually on the AMBA bus. > They can support PASID via smmu stall feature, but does not > support tlp since they are not real pci devices. > So remove tlp as a PASID dependency. When you iterate on this, pay attention to things like: - Wrap paragraphs to 75 columns or so, so they fill the whole line but don't overflow when "git show" adds 4 spaces. - Leave a blank line between paragraphs. - Capitalize consistently: "SMMU", "PCI", "TLP". - Provide references to relevant spec sections, e.g., for the SMMU stall feature. > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> > --- > drivers/pci/ats.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c > index 390e92f..8e31278 100644 > --- a/drivers/pci/ats.c > +++ b/drivers/pci/ats.c > @@ -344,9 +344,6 @@ int pci_enable_pasid(struct pci_dev *pdev, int features) > if (WARN_ON(pdev->pasid_enabled)) > return -EBUSY; > > - if (!pdev->eetlp_prefix_path) > - return -EINVAL; No. This would mean we might enable PASID on actual PCIe devices when it is not safe to do so, as Jean-Philippe pointed out. You cannot break actual PCIe devices just to make your non-PCIe device work. These devices do not support PASID as defined in the PCIe spec. They might support something *like* PASID, and you might be able to make parts of the PCI core work with them, but you're going to have to deal with the parts that don't follow the PCIe spec on your own. That might be quirks, it might be some sort of AMBA adaptation shim, I don't know. But it's not the responsibility of the PCI core to adapt to them. > if (!pasid) > return -EINVAL; > > -- > 2.7.4 >
On 2020/6/12 上午1:41, Bjorn Helgaas wrote: > [+cc Sinan] > > On Wed, Jun 10, 2020 at 12:18:14PM +0800, Zhangfei Gao wrote: >> Some platform devices appear as PCI and have PCI cfg space, >> but are actually on the AMBA bus. >> They can support PASID via smmu stall feature, but does not >> support tlp since they are not real pci devices. >> So remove tlp as a PASID dependency. > When you iterate on this, pay attention to things like: > > - Wrap paragraphs to 75 columns or so, so they fill the whole line > but don't overflow when "git show" adds 4 spaces. > > - Leave a blank line between paragraphs. > > - Capitalize consistently: "SMMU", "PCI", "TLP". > > - Provide references to relevant spec sections, e.g., for the SMMU > stall feature. OK, Thanks Bjorn > >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >> --- >> drivers/pci/ats.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c >> index 390e92f..8e31278 100644 >> --- a/drivers/pci/ats.c >> +++ b/drivers/pci/ats.c >> @@ -344,9 +344,6 @@ int pci_enable_pasid(struct pci_dev *pdev, int features) >> if (WARN_ON(pdev->pasid_enabled)) >> return -EBUSY; >> >> - if (!pdev->eetlp_prefix_path) >> - return -EINVAL; > No. This would mean we might enable PASID on actual PCIe devices when > it is not safe to do so, as Jean-Philippe pointed out. > > You cannot break actual PCIe devices just to make your non-PCIe device > work. > > These devices do not support PASID as defined in the PCIe spec. They > might support something *like* PASID, and you might be able to make > parts of the PCI core work with them, but you're going to have to deal > with the parts that don't follow the PCIe spec on your own. That > might be quirks, it might be some sort of AMBA adaptation shim, I > don't know. But it's not the responsibility of the PCI core to adapt > to them. Understand now. Will continue use quirk for this. Thanks
diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c index 390e92f..8e31278 100644 --- a/drivers/pci/ats.c +++ b/drivers/pci/ats.c @@ -344,9 +344,6 @@ int pci_enable_pasid(struct pci_dev *pdev, int features) if (WARN_ON(pdev->pasid_enabled)) return -EBUSY; - if (!pdev->eetlp_prefix_path) - return -EINVAL; - if (!pasid) return -EINVAL;
Some platform devices appear as PCI and have PCI cfg space, but are actually on the AMBA bus. They can support PASID via smmu stall feature, but does not support tlp since they are not real pci devices. So remove tlp as a PASID dependency. Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> --- drivers/pci/ats.c | 3 --- 1 file changed, 3 deletions(-)