Re: [PATCH V3] PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30

Re: [PATCH V3] PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30

[Bjorn Helgaas]

On Thu, Jul 04, 2019 at 08:34:28PM +0530, Vidya Sagar wrote:
> Currently Relaxed Ordering bit in the configuration space is enabled for
> all PCIe devices as the quirk uses PCI_ANY_ID for both Vendor-ID and
> Device-ID, but, as per the Technical Reference Manual of Tegra20 which is
> available at https://developer.nvidia.com/embedded/downloads#?search=tegra%202
> in Sec 34.1, it is mentioned that Relaxed Ordering bit needs to be enabled in
> its root ports to avoid deadlock in hardware. The same is applicable for
> Tegra30 as well though it is not explicitly mentioned in Tegra30 TRM document,
> but the same must not be extended to root ports of other Tegra SoCs or
> other hosts as the same issue doesn't exist there.

While researching another thread about RO [1], I got concerned about
setting RO for root ports.

Setting RO for *endpoints* makes sense: that allows (but does not
require) the endpoint to issue writes that don't require strong
ordering.

Setting RO for *root ports* seems more problematic.  It allows the
root port to issue PCIe writes that don't require strong ordering.
These would be CPU MMIO writes to devices.  But Linux currently does
not have a way for drivers to indicate that some MMIO writes need to
be ordered while others do not, and I think drivers assume that all
MMIO writes are performed in order. 

We merged this patch as 7be142caabc4 ("PCI: tegra: Enable Relaxed
Ordering only for Tegra20 & Tegra30") [2], so Tegra20 and Tegra30 root
ports are allowed (but again, not required) to set the RO bit for MMIO
writes initiated by a CPU.

Because I think drivers *rely* on MMIO writes being strongly ordered,
this is a potential problem.  I think we should probably consider
explicitly *disabling* RO in all root ports (with exceptions for
quirks like this) in case it's set by any firmware.

So the question is, how do Tegra20 and Tegra30 actually work?  Do they
ever actually set the RO bit for these MMIO writes?  If so, I think
drivers are really at risk, and we probably should log some kind of
warning.

But if Tegra20 and Tegra30 just need "Enable Relaxed Ordering" set as
a bug workaround and they never actually initiate PCIe writes with the
RO bit set, maybe we should add a comment to that effect, but there
should be no actual problem.

[1] https://lore.kernel.org/r/20210830123704.221494-2-verdre@v0yd.nl
[2] https://git.kernel.org/linus/7be142caabc4

> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V3:
> * Modified commit message to make it more precise and explicit
> 
> V2:
> * Modified commit message to include reference to Tegra20 TRM document.
> 
>  drivers/pci/controller/pci-tegra.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index 9cc03a2549c0..241760aa15bd 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -787,12 +787,15 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0bf1, tegra_pcie_fixup_class);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0e1c, tegra_pcie_fixup_class);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0e1d, tegra_pcie_fixup_class);
>  
> -/* Tegra PCIE requires relaxed ordering */
> +/* Tegra20 and Tegra30 PCIE requires relaxed ordering */
>  static void tegra_pcie_relax_enable(struct pci_dev *dev)
>  {
>  	pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN);
>  }
> -DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, tegra_pcie_relax_enable);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0bf0, tegra_pcie_relax_enable);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0bf1, tegra_pcie_relax_enable);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0e1c, tegra_pcie_relax_enable);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0e1d, tegra_pcie_relax_enable);
>  
>  static int tegra_pcie_request_resources(struct tegra_pcie *pcie)
>  {
> -- 
> 2.17.1
> 

Re: [PATCH V3] PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30

[Bjorn Helgaas]

[+cc Keith]

On Wed, Sep 01, 2021 at 03:40:47PM -0500, Bjorn Helgaas wrote:
> On Thu, Jul 04, 2019 at 08:34:28PM +0530, Vidya Sagar wrote:
> > Currently Relaxed Ordering bit in the configuration space is enabled for
> > all PCIe devices as the quirk uses PCI_ANY_ID for both Vendor-ID and
> > Device-ID, but, as per the Technical Reference Manual of Tegra20 which is
> > available at https://developer.nvidia.com/embedded/downloads#?search=tegra%202
> > in Sec 34.1, it is mentioned that Relaxed Ordering bit needs to be enabled in
> > its root ports to avoid deadlock in hardware. The same is applicable for
> > Tegra30 as well though it is not explicitly mentioned in Tegra30 TRM document,
> > but the same must not be extended to root ports of other Tegra SoCs or
> > other hosts as the same issue doesn't exist there.
> 
> While researching another thread about RO [1], I got concerned about
> setting RO for root ports.
> 
> Setting RO for *endpoints* makes sense: that allows (but does not
> require) the endpoint to issue writes that don't require strong
> ordering.
> 
> Setting RO for *root ports* seems more problematic.  It allows the
> root port to issue PCIe writes that don't require strong ordering.
> These would be CPU MMIO writes to devices.  But Linux currently does
> not have a way for drivers to indicate that some MMIO writes need to
> be ordered while others do not, and I think drivers assume that all
> MMIO writes are performed in order. 
> 
> We merged this patch as 7be142caabc4 ("PCI: tegra: Enable Relaxed
> Ordering only for Tegra20 & Tegra30") [2], so Tegra20 and Tegra30 root
> ports are allowed (but again, not required) to set the RO bit for MMIO
> writes initiated by a CPU.
> 
> Because I think drivers *rely* on MMIO writes being strongly ordered,
> this is a potential problem.  I think we should probably consider
> explicitly *disabling* RO in all root ports (with exceptions for
> quirks like this) in case it's set by any firmware.
> 
> So the question is, how do Tegra20 and Tegra30 actually work?  Do they
> ever actually set the RO bit for these MMIO writes?  If so, I think
> drivers are really at risk, and we probably should log some kind of
> warning.

Keith reminded me that writel_relaxed() is a way for drivers to
specify that they don't need strong ordering.  So I think the question
really is whether Tegra20 and Tegra30 set the RO bit for normal
writel() writes.  If they set RO for writel_relaxed() writes, that
should be perfectly fine.

> But if Tegra20 and Tegra30 just need "Enable Relaxed Ordering" set as
> a bug workaround and they never actually initiate PCIe writes with the
> RO bit set, maybe we should add a comment to that effect, but there
> should be no actual problem.
> 
> [1] https://lore.kernel.org/r/20210830123704.221494-2-verdre@v0yd.nl
> [2] https://git.kernel.org/linus/7be142caabc4
> 
> > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > ---
> > V3:
> > * Modified commit message to make it more precise and explicit
> > 
> > V2:
> > * Modified commit message to include reference to Tegra20 TRM document.
> > 
> >  drivers/pci/controller/pci-tegra.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> > index 9cc03a2549c0..241760aa15bd 100644
> > --- a/drivers/pci/controller/pci-tegra.c
> > +++ b/drivers/pci/controller/pci-tegra.c
> > @@ -787,12 +787,15 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0bf1, tegra_pcie_fixup_class);
> >  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0e1c, tegra_pcie_fixup_class);
> >  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0e1d, tegra_pcie_fixup_class);
> >  
> > -/* Tegra PCIE requires relaxed ordering */
> > +/* Tegra20 and Tegra30 PCIE requires relaxed ordering */
> >  static void tegra_pcie_relax_enable(struct pci_dev *dev)
> >  {
> >  	pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN);
> >  }
> > -DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, tegra_pcie_relax_enable);
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0bf0, tegra_pcie_relax_enable);
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0bf1, tegra_pcie_relax_enable);
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0e1c, tegra_pcie_relax_enable);
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0e1d, tegra_pcie_relax_enable);
> >  
> >  static int tegra_pcie_request_resources(struct tegra_pcie *pcie)
> >  {
> > -- 
> > 2.17.1
> >