| Message ID | 44d133d5ebd4f7b9e8b817aa8bae12f690e70000.1448270813.git.stanimir.varbanov@linaro.org |
|---|---|
| State | Accepted |
| 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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/ -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(-)