* [PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK [not found] <20241127103016.3481128-8-cassel@kernel.org> @ 2024-11-27 10:30 ` Niklas Cassel 2024-11-30 8:23 ` Manivannan Sadhasivam 2024-12-04 17:33 ` Bjorn Helgaas 0 siblings, 2 replies; 5+ messages in thread From: Niklas Cassel @ 2024-11-27 10:30 UTC (permalink / raw) To: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas, Kishon Vijay Abraham I Cc: Damien Le Moal, Frank Li, Jesper Nilsson, Niklas Cassel, stable, linux-pci 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK 2024-11-27 10:30 ` [PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK Niklas Cassel @ 2024-11-30 8:23 ` Manivannan Sadhasivam 2024-12-04 17:33 ` Bjorn Helgaas 1 sibling, 0 replies; 5+ messages in thread From: Manivannan Sadhasivam @ 2024-11-30 8:23 UTC (permalink / raw) To: Niklas Cassel Cc: Jingoo Han, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas, Kishon Vijay Abraham I, Damien Le Moal, Frank Li, Jesper Nilsson, stable, linux-pci 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 > -- மணிவண்ணன் சதாசிவம் ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK 2024-11-27 10:30 ` [PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK Niklas Cassel 2024-11-30 8:23 ` Manivannan Sadhasivam @ 2024-12-04 17:33 ` Bjorn Helgaas 2024-12-13 13:34 ` Niklas Cassel 1 sibling, 1 reply; 5+ messages in thread From: Bjorn Helgaas @ 2024-12-04 17:33 UTC (permalink / raw) To: Niklas Cassel Cc: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas, Kishon Vijay Abraham I, Damien Le Moal, Frank Li, Jesper Nilsson, stable, linux-pci 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK 2024-12-04 17:33 ` Bjorn Helgaas @ 2024-12-13 13:34 ` Niklas Cassel 2024-12-13 14:38 ` Niklas Cassel 0 siblings, 1 reply; 5+ messages in thread From: Niklas Cassel @ 2024-12-13 13:34 UTC (permalink / raw) To: Bjorn Helgaas Cc: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas, Kishon Vijay Abraham I, Damien Le Moal, Frank Li, Jesper Nilsson, stable, linux-pci 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK 2024-12-13 13:34 ` Niklas Cassel @ 2024-12-13 14:38 ` Niklas Cassel 0 siblings, 0 replies; 5+ messages in thread From: Niklas Cassel @ 2024-12-13 14:38 UTC (permalink / raw) To: Bjorn Helgaas Cc: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas, Kishon Vijay Abraham I, Damien Le Moal, Frank Li, Jesper Nilsson, stable, linux-pci 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-12-13 14:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20241127103016.3481128-8-cassel@kernel.org>
2024-11-27 10:30 ` [PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK Niklas Cassel
2024-11-30 8:23 ` Manivannan Sadhasivam
2024-12-04 17:33 ` Bjorn Helgaas
2024-12-13 13:34 ` Niklas Cassel
2024-12-13 14:38 ` Niklas Cassel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox