[PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK

[PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK

[Niklas Cassel]

The DWC Databook description for the LWR_TARGET_RW and LWR_TARGET_HW fields
in the IATU_LWR_TARGET_ADDR_OFF_INBOUND_i registers state that:
"Field size depends on log2(BAR_MASK+1) in BAR match mode."

I.e. only the upper bits are writable, and the number of writable bits is
dependent on the configured BAR_MASK.

If we do not write the BAR_MASK before writing the iATU registers, we are
relying the reset value of the BAR_MASK being larger than the requested
size of the first set_bar() call. The reset value of the BAR_MASK is SoC
dependent.

Thus, if the first set_bar() call requests a size that is larger than the
reset value of the BAR_MASK, the iATU will try to write to read-only bits,
which will cause the iATU to end up redirecting to a physical address that
is different from the address that was intended.

Thus, we should always write the iATU registers after writing the BAR_MASK.

Cc: stable@vger.kernel.org
Fixes: f8aed6ec624f ("PCI: dwc: designware: Add EP mode support")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 .../pci/controller/dwc/pcie-designware-ep.c   | 28 ++++++++++---------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index f3ac7d46a855..bad588ef69a4 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -222,19 +222,10 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 	if ((flags & PCI_BASE_ADDRESS_MEM_TYPE_64) && (bar & 1))
 		return -EINVAL;
 
-	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
-
-	if (!(flags & PCI_BASE_ADDRESS_SPACE))
-		type = PCIE_ATU_TYPE_MEM;
-	else
-		type = PCIE_ATU_TYPE_IO;
-
-	ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
-	if (ret)
-		return ret;
-
 	if (ep->epf_bar[bar])
-		return 0;
+		goto config_atu;
+
+	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
 
 	dw_pcie_dbi_ro_wr_en(pci);
 
@@ -246,9 +237,20 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 		dw_pcie_ep_writel_dbi(ep, func_no, reg + 4, 0);
 	}
 
-	ep->epf_bar[bar] = epf_bar;
 	dw_pcie_dbi_ro_wr_dis(pci);
 
+config_atu:
+	if (!(flags & PCI_BASE_ADDRESS_SPACE))
+		type = PCIE_ATU_TYPE_MEM;
+	else
+		type = PCIE_ATU_TYPE_IO;
+
+	ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
+	if (ret)
+		return ret;
+
+	ep->epf_bar[bar] = epf_bar;
+
 	return 0;
 }
 
-- 
2.47.0

Re: [PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK

[Manivannan Sadhasivam]

On Wed, Nov 27, 2024 at 11:30:17AM +0100, Niklas Cassel wrote:
> The DWC Databook description for the LWR_TARGET_RW and LWR_TARGET_HW fields
> in the IATU_LWR_TARGET_ADDR_OFF_INBOUND_i registers state that:
> "Field size depends on log2(BAR_MASK+1) in BAR match mode."
> 
> I.e. only the upper bits are writable, and the number of writable bits is
> dependent on the configured BAR_MASK.
> 
> If we do not write the BAR_MASK before writing the iATU registers, we are
> relying the reset value of the BAR_MASK being larger than the requested
> size of the first set_bar() call. The reset value of the BAR_MASK is SoC
> dependent.
> 
> Thus, if the first set_bar() call requests a size that is larger than the
> reset value of the BAR_MASK, the iATU will try to write to read-only bits,
> which will cause the iATU to end up redirecting to a physical address that
> is different from the address that was intended.
> 
> Thus, we should always write the iATU registers after writing the BAR_MASK.
> 
> Cc: stable@vger.kernel.org
> Fixes: f8aed6ec624f ("PCI: dwc: designware: Add EP mode support")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 28 ++++++++++---------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index f3ac7d46a855..bad588ef69a4 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -222,19 +222,10 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	if ((flags & PCI_BASE_ADDRESS_MEM_TYPE_64) && (bar & 1))
>  		return -EINVAL;
>  
> -	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> -
> -	if (!(flags & PCI_BASE_ADDRESS_SPACE))
> -		type = PCIE_ATU_TYPE_MEM;
> -	else
> -		type = PCIE_ATU_TYPE_IO;
> -
> -	ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
> -	if (ret)
> -		return ret;
> -
>  	if (ep->epf_bar[bar])
> -		return 0;
> +		goto config_atu;
> +
> +	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>  
>  	dw_pcie_dbi_ro_wr_en(pci);
>  
> @@ -246,9 +237,20 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  		dw_pcie_ep_writel_dbi(ep, func_no, reg + 4, 0);
>  	}
>  
> -	ep->epf_bar[bar] = epf_bar;
>  	dw_pcie_dbi_ro_wr_dis(pci);
>  
> +config_atu:
> +	if (!(flags & PCI_BASE_ADDRESS_SPACE))
> +		type = PCIE_ATU_TYPE_MEM;
> +	else
> +		type = PCIE_ATU_TYPE_IO;
> +
> +	ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
> +	if (ret)
> +		return ret;
> +
> +	ep->epf_bar[bar] = epf_bar;
> +
>  	return 0;
>  }
>  
> -- 
> 2.47.0
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK

[Bjorn Helgaas]

In subject, maybe "Write BAR_MASK before iATU registers"

I guess writing BAR_MASK is really configuring the *size* of the BAR?
Maybe the size is the important semantic connection with iATU config?

On Wed, Nov 27, 2024 at 11:30:17AM +0100, Niklas Cassel wrote:
> The DWC Databook description for the LWR_TARGET_RW and LWR_TARGET_HW fields
> in the IATU_LWR_TARGET_ADDR_OFF_INBOUND_i registers state that:
> "Field size depends on log2(BAR_MASK+1) in BAR match mode."

Can we include a databook revision and section here to help future
maintainers?

> I.e. only the upper bits are writable, and the number of writable bits is
> dependent on the configured BAR_MASK.
> 
> If we do not write the BAR_MASK before writing the iATU registers, we are
> relying the reset value of the BAR_MASK being larger than the requested
> size of the first set_bar() call. The reset value of the BAR_MASK is SoC
> dependent.
> 
> Thus, if the first set_bar() call requests a size that is larger than the
> reset value of the BAR_MASK, the iATU will try to write to read-only bits,
> which will cause the iATU to end up redirecting to a physical address that
> is different from the address that was intended.
> 
> Thus, we should always write the iATU registers after writing the BAR_MASK.

Apparently we write BAR_MASK and the iATU registers in the wrong
order?  I assume dw_pcie_ep_inbound_atu() writes the iATU registers.

I can't quite connect the commit log with the code change.  I assume
the dw_pcie_ep_writel_dbi2() and dw_pcie_ep_writel_dbi() writes update
BAR_MASK?

And I guess the problem is that the previous code does:

  dw_pcie_ep_inbound_atu        # iATU
  dw_pcie_ep_writel_dbi2        # BAR_MASK (?)
  dw_pcie_ep_writel_dbi

and the new code basically does this:

  if (ep->epf_bar[bar]) {
    dw_pcie_ep_writel_dbi2      # BAR_MASK (?)
    dw_pcie_ep_writel_dbi
  }
  dw_pcie_ep_inbound_atu        # iATU
  ep->epf_bar[bar] = epf_bar

so the first time we call dw_pcie_ep_set_bar(), we write BAR_MASK
before iATU, and if we call dw_pcie_ep_set_bar() again, we skip the 
BAR_MASK update?

> Cc: stable@vger.kernel.org
> Fixes: f8aed6ec624f ("PCI: dwc: designware: Add EP mode support")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 28 ++++++++++---------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index f3ac7d46a855..bad588ef69a4 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -222,19 +222,10 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	if ((flags & PCI_BASE_ADDRESS_MEM_TYPE_64) && (bar & 1))
>  		return -EINVAL;
>  
> -	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> -
> -	if (!(flags & PCI_BASE_ADDRESS_SPACE))
> -		type = PCIE_ATU_TYPE_MEM;
> -	else
> -		type = PCIE_ATU_TYPE_IO;
> -
> -	ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
> -	if (ret)
> -		return ret;
> -
>  	if (ep->epf_bar[bar])
> -		return 0;
> +		goto config_atu;
> +
> +	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>  
>  	dw_pcie_dbi_ro_wr_en(pci);
>  
> @@ -246,9 +237,20 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  		dw_pcie_ep_writel_dbi(ep, func_no, reg + 4, 0);
>  	}
>  
> -	ep->epf_bar[bar] = epf_bar;
>  	dw_pcie_dbi_ro_wr_dis(pci);
>  
> +config_atu:
> +	if (!(flags & PCI_BASE_ADDRESS_SPACE))
> +		type = PCIE_ATU_TYPE_MEM;
> +	else
> +		type = PCIE_ATU_TYPE_IO;
> +
> +	ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
> +	if (ret)
> +		return ret;
> +
> +	ep->epf_bar[bar] = epf_bar;
> +
>  	return 0;
>  }
>  
> -- 
> 2.47.0
> 

Re: [PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK

[Niklas Cassel]

Hello Bjorn,

On Wed, Dec 04, 2024 at 11:33:52AM -0600, Bjorn Helgaas wrote:
> In subject, maybe "Write BAR_MASK before iATU registers"

Ok. Will fix in v6.


> 
> I guess writing BAR_MASK is really configuring the *size* of the BAR?

I am quite sure that you know how host software determines the size of
a BAR :)

But yes, writing the BAR_MASK will directly decide a BARs size:
https://wiki.osdev.org/PCI#Address_and_size_of_the_BAR

So BAR_MASK is "BAR size - 1".


> Maybe the size is the important semantic connection with iATU config?

The connection is:
"Field size depends on log2(BAR_MASK+1) in BAR match mode."

So I think it makes sense for the subject to include BAR_MASK
rather than BAR size.


> 
> On Wed, Nov 27, 2024 at 11:30:17AM +0100, Niklas Cassel wrote:
> > The DWC Databook description for the LWR_TARGET_RW and LWR_TARGET_HW fields
> > in the IATU_LWR_TARGET_ADDR_OFF_INBOUND_i registers state that:
> > "Field size depends on log2(BAR_MASK+1) in BAR match mode."
> 
> Can we include a databook revision and section here to help future
> maintainers?

Ok. Will fix in v6.


> 
> > I.e. only the upper bits are writable, and the number of writable bits is
> > dependent on the configured BAR_MASK.
> > 
> > If we do not write the BAR_MASK before writing the iATU registers, we are
> > relying the reset value of the BAR_MASK being larger than the requested
> > size of the first set_bar() call. The reset value of the BAR_MASK is SoC
> > dependent.
> > 
> > Thus, if the first set_bar() call requests a size that is larger than the
> > reset value of the BAR_MASK, the iATU will try to write to read-only bits,
> > which will cause the iATU to end up redirecting to a physical address that
> > is different from the address that was intended.
> > 
> > Thus, we should always write the iATU registers after writing the BAR_MASK.
> 
> Apparently we write BAR_MASK and the iATU registers in the wrong
> order?  I assume dw_pcie_ep_inbound_atu() writes the iATU registers.

Yes.


> 
> I can't quite connect the commit log with the code change.  I assume
> the dw_pcie_ep_writel_dbi2() and dw_pcie_ep_writel_dbi() writes update
> BAR_MASK?

dw_pcie_ep_writel_dbi2() writes the BAR_MASK.
dw_pcie_ep_writel_dbi() writes the BAR type.


> 
> And I guess the problem is that the previous code does:
> 
>   dw_pcie_ep_inbound_atu        # iATU
>   dw_pcie_ep_writel_dbi2        # BAR_MASK (?)
>   dw_pcie_ep_writel_dbi
> 
> and the new code basically does this:
> 
>   if (ep->epf_bar[bar]) {
>     dw_pcie_ep_writel_dbi2      # BAR_MASK (?)
>     dw_pcie_ep_writel_dbi
>   }
>   dw_pcie_ep_inbound_atu        # iATU
>   ep->epf_bar[bar] = epf_bar
> 
> so the first time we call dw_pcie_ep_set_bar(), we write BAR_MASK
> before iATU, and if we call dw_pcie_ep_set_bar() again, we skip the 
> BAR_MASK update?

The problem is as described in the commit message:
"If we do not write the BAR_MASK before writing the iATU registers, we are
relying the reset value of the BAR_MASK being larger than the requested
size of the first set_bar() call. The reset value of the BAR_MASK is SoC
dependent."


Before:
dw_pcie_ep_inbound_atu() # iATU - the writable bits in this write depends on
                         # BAR_MASK, which has not been written yet, thus the
			 # write will be at the mercy of the reset value of
			 # BAR_MASK, which is SoC dependent.
dw_pcie_ep_writel_dbi2() # BAR_MASK
dw_pcie_ep_writel_dbi()  # BAR type

After:
dw_pcie_ep_writel_dbi2() # BAR_MASK
dw_pcie_ep_writel_dbi()  # BAR type
dw_pcie_ep_inbound_atu() # iATU - this write is now done after BAR_MASK has
			 # been written, so we know that all the bits in this
			 # write is writable.


Kind regards,
Niklas

Re: [PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK

[Niklas Cassel]

Hello Bjorn,

On Fri, Dec 13, 2024 at 02:34:35PM +0100, Niklas Cassel wrote:
> > 
> > And I guess the problem is that the previous code does:
> > 
> >   dw_pcie_ep_inbound_atu        # iATU
> >   dw_pcie_ep_writel_dbi2        # BAR_MASK (?)
> >   dw_pcie_ep_writel_dbi
> > 
> > and the new code basically does this:
> > 
> >   if (ep->epf_bar[bar]) {
> >     dw_pcie_ep_writel_dbi2      # BAR_MASK (?)
> >     dw_pcie_ep_writel_dbi
> >   }
> >   dw_pcie_ep_inbound_atu        # iATU
> >   ep->epf_bar[bar] = epf_bar
> > 
> > so the first time we call dw_pcie_ep_set_bar(), we write BAR_MASK
> > before iATU, and if we call dw_pcie_ep_set_bar() again, we skip the 
> > BAR_MASK update?
> 
> The problem is as described in the commit message:
> "If we do not write the BAR_MASK before writing the iATU registers, we are
> relying the reset value of the BAR_MASK being larger than the requested
> size of the first set_bar() call. The reset value of the BAR_MASK is SoC
> dependent."

Re-reading this commit message, I can see that it is a bit confusing.

I re-wrote the commit messages for patch 1/2 and patch 2/6 in this series,
based on your feedback. (I also updated the code comment in patch 2/6.)

I hope that it slightly clearer now :)

Please review:
https://lore.kernel.org/linux-pci/20241213143301.4158431-8-cassel@kernel.org/T/#u


Kind regards,
Niklas