| Message ID | 44d133d5ebd4f7b9e8b817aa8bae12f690e70000.1448270813.git.stanimir.varbanov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 23 November 2015 11:28:58 Stanimir Varbanov wrote: > The io_base is used to keep the cpu physical address parsed > from ranges dt property. After issue pci_remap_iospace the > io_base has been assigned with io->start, which is not correct > cause io->start is a PCI bus address. > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > drivers/pci/host/pcie-designware.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > index 540f077c37ea..02a7452bdf23 100644 > --- a/drivers/pci/host/pcie-designware.c > +++ b/drivers/pci/host/pcie-designware.c > @@ -440,7 +440,6 @@ int dw_pcie_host_init(struct pcie_port *pp) > ret, pp->io); > continue; > } > - pp->io_base = pp->io->start; > break; > case IORESOURCE_MEM: > pp->mem = win->res; I was surprised to see such an obvious bug here, as we had spent a lot of time trying to get it right. However, it broke only recently and it's worth mentioning what commit did it, so Fixes: 0021d22b73d6 ("PCI: designware: Use of_pci_get_host_bridge_resources() to parse DT") Reviewed-by: Arnd Bergmann <arnd@arndb.de> The bug is present in 4.4-rc1 and we should get your fix merged into 4.4 as well, while all the other patches in your series are presumably for 4.5. Arnd
Hi Stanimir, Many Thanks for this fix > -----Original Message----- > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel- > owner@vger.kernel.org] On Behalf Of Arnd Bergmann > Sent: 23 November 2015 10:00 > To: Stanimir Varbanov > Cc: linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; devicetree@vger.kernel.org; linux- > pci@vger.kernel.org; Bjorn Helgaas; Srinivas Kandagatla; Rob Herring; Rob > Herring; Mark Rutland; Pawel Moll; Ian Campbell; Jingoo Han; Pratyush Anand; > Bjorn Andersson > Subject: Re: [PATCH v3 1/6] PCI: designware: remove wrong io_base assignment > > On Monday 23 November 2015 11:28:58 Stanimir Varbanov wrote: > > The io_base is used to keep the cpu physical address parsed > > from ranges dt property. After issue pci_remap_iospace the > > io_base has been assigned with io->start, which is not correct > > cause io->start is a PCI bus address. > > > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > > --- > > drivers/pci/host/pcie-designware.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie- > designware.c > > index 540f077c37ea..02a7452bdf23 100644 > > --- a/drivers/pci/host/pcie-designware.c > > +++ b/drivers/pci/host/pcie-designware.c > > @@ -440,7 +440,6 @@ int dw_pcie_host_init(struct pcie_port *pp) > > ret, pp->io); > > continue; > > } > > - pp->io_base = pp->io->start; > > break; > > case IORESOURCE_MEM: > > pp->mem = win->res; > > I was surprised to see such an obvious bug here, as we had spent a lot of > time trying to get it right. However, it broke only recently and it's > worth mentioning what commit did it, so Looking at "[PATCH v12 0/8] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05": actually the bug came in as patch 3/6 of v11 patchset split [...] Change from v11: - Split 3/6 in v11 to 3/8, 4/8, 5/8 in v12. [...] This was not present in v11...sorry about this. Gab > > Fixes: 0021d22b73d6 ("PCI: designware: Use of_pci_get_host_bridge_resources() > to parse DT") > Reviewed-by: Arnd Bergmann <arnd@arndb.de> > > The bug is present in 4.4-rc1 and we should get your fix merged into 4.4 > as well, while all the other patches in your series are presumably for 4.5. > > Arnd > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On 11/23/2015 12:27 PM, Gabriele Paoloni wrote: > Hi Stanimir, Many Thanks for this fix > >> -----Original Message----- >> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel- >> owner@vger.kernel.org] On Behalf Of Arnd Bergmann >> Sent: 23 November 2015 10:00 >> To: Stanimir Varbanov >> Cc: linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- >> kernel@lists.infradead.org; devicetree@vger.kernel.org; linux- >> pci@vger.kernel.org; Bjorn Helgaas; Srinivas Kandagatla; Rob Herring; Rob >> Herring; Mark Rutland; Pawel Moll; Ian Campbell; Jingoo Han; Pratyush Anand; >> Bjorn Andersson >> Subject: Re: [PATCH v3 1/6] PCI: designware: remove wrong io_base assignment >> >> On Monday 23 November 2015 11:28:58 Stanimir Varbanov wrote: >>> The io_base is used to keep the cpu physical address parsed >>> from ranges dt property. After issue pci_remap_iospace the >>> io_base has been assigned with io->start, which is not correct >>> cause io->start is a PCI bus address. >>> >>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >>> --- >>> drivers/pci/host/pcie-designware.c | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie- >> designware.c >>> index 540f077c37ea..02a7452bdf23 100644 >>> --- a/drivers/pci/host/pcie-designware.c >>> +++ b/drivers/pci/host/pcie-designware.c >>> @@ -440,7 +440,6 @@ int dw_pcie_host_init(struct pcie_port *pp) >>> ret, pp->io); >>> continue; >>> } >>> - pp->io_base = pp->io->start; >>> break; >>> case IORESOURCE_MEM: >>> pp->mem = win->res; >> >> I was surprised to see such an obvious bug here, as we had spent a lot of >> time trying to get it right. However, it broke only recently and it's >> worth mentioning what commit did it, so > > Looking at > "[PATCH v12 0/8] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05": > > actually the bug came in as patch 3/6 of v11 patchset split > > [...] > > Change from v11: > - Split 3/6 in v11 to 3/8, 4/8, 5/8 in v12. > > [...] > > This was not present in v11...sorry about this. > > Gab > >> >> Fixes: 0021d22b73d6 ("PCI: designware: Use of_pci_get_host_bridge_resources() >> to parse DT") >> Reviewed-by: Arnd Bergmann <arnd@arndb.de> I think the bug is introduced in: cbce7900598c ("PCI: designware: Make driver arch-agnostic") cause the io_base is correctly calculated in 0021d22b73d6, do you agree?
On Monday 23 November 2015 18:23:47 Stanimir Varbanov wrote: > >> > >> Fixes: 0021d22b73d6 ("PCI: designware: Use of_pci_get_host_bridge_resources() > >> to parse DT") > >> Reviewed-by: Arnd Bergmann <arnd@arndb.de> > > I think the bug is introduced in: > > cbce7900598c ("PCI: designware: Make driver arch-agnostic") > > cause the io_base is correctly calculated in 0021d22b73d6, do you agree? I think cbce7900598c just slightly changes the io_base value, but it's still referring to a bus address, not a cpu address, both before and after the patch. 0021d22b73d6 hower seems to remove the correct 'pp->io_base = range.cpu_addr;' and replaces it with 'pp->io_base = pp->io->start;', so it's now in the wrong address space. Arnd
On 11/23/2015 06:40 PM, Arnd Bergmann wrote: > On Monday 23 November 2015 18:23:47 Stanimir Varbanov wrote: >>>> >>>> Fixes: 0021d22b73d6 ("PCI: designware: Use of_pci_get_host_bridge_resources() >>>> to parse DT") >>>> Reviewed-by: Arnd Bergmann <arnd@arndb.de> >> >> I think the bug is introduced in: >> >> cbce7900598c ("PCI: designware: Make driver arch-agnostic") >> >> cause the io_base is correctly calculated in 0021d22b73d6, do you agree? > > > I think cbce7900598c just slightly changes the io_base value, but > it's still referring to a bus address, not a cpu address, both before > and after the patch. > > 0021d22b73d6 hower seems to remove the correct 'pp->io_base = range.cpu_addr;' > and replaces it with 'pp->io_base = pp->io->start;', so it's now in > the wrong address space. > > Arnd > OK, I will resend this as separate patch, and will drop it from this series.
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c index 540f077c37ea..02a7452bdf23 100644 --- a/drivers/pci/host/pcie-designware.c +++ b/drivers/pci/host/pcie-designware.c @@ -440,7 +440,6 @@ int dw_pcie_host_init(struct pcie_port *pp) ret, pp->io); continue; } - pp->io_base = pp->io->start; break; case IORESOURCE_MEM: pp->mem = win->res;
The io_base is used to keep the cpu physical address parsed from ranges dt property. After issue pci_remap_iospace the io_base has been assigned with io->start, which is not correct cause io->start is a PCI bus address. Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> --- drivers/pci/host/pcie-designware.c | 1 - 1 file changed, 1 deletion(-)