| Message ID | CAE9FiQU6NTEdKJi58bNZU4u=+-rzB3mddp6vKGc4JjbZWoa9zQ@mail.gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, Sep 15, 2015 at 12:58:20AM +0100, Yinghai Lu wrote: > On Mon, Sep 14, 2015 at 10:36 AM, Yinghai Lu <yinghai@kernel.org> wrote: > > On Mon, Sep 14, 2015 at 9:28 AM, Lorenzo Pieralisi > > <lorenzo.pieralisi@arm.com> wrote: > >> On Mon, Sep 14, 2015 at 05:05:50PM +0100, Yinghai Lu wrote: > >>> We could just revert > >>> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of > >>> arch code") > >>> instead. > >> > > > if arch code called pci_read_bridge_bases() via pcibios_fixup_bus(), > > then it need to have > > to call pcibios_allocate_bus_resources() later. > > > > but now arm (mips ?) does not have calling pcibios_allocate_bus_resources(). pcibios_allocate_bus_resources() is an arch specific function and arm and (and mips ?) does not need to create/call it because ARM reassigns ALL resources in ALL platforms, hoping FW can provide a reasonable PCI bridge apertures set-up on ARM is wishful thinking at present. If PCI core code is written with that assumption (ie that arch code zeroes the bridge apertures if they can't be claimed), pci_read_bridge_bases() can't be moved to PCI core code at present, sad and simple. I already asked many times why __pci_bus_size_bridges() cares about the old bridge size on first scan and got no answer so I would ask Bjorn to revert dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of arch code") or we apply an ARM specific plaster, we are making no progress on this. > Found other problem that is caused by > dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of > arch code") > > If that commit does not get reverted, will need to have attached patch I see what you mean and I see why there is a reason to apply the patch below if we do not revert dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of arch code"), but I am afraid the commit log has to be rewritten to explain the problem in a way that properly describes the issue, and that's not the first one I read in the last couple of weeks to figure out how to fix this regression. Thanks, Lorenzo > > > Subject: [PATCH] PCI: Skip reading bridge bases with existing ones on rescan > > Noticed there is noisy print out pci bridge bases read out for > existing devices that is caused by commit dff22d2054b5 > ("PCI: Call pci_read_bridge_bases() from core instead of arch code") > > pci_bus 0000:06: scanning bus > pcieport 0000:06:00.0: scanning [bus 07-09] behind bridge, pass 0 > pcieport 0000:06:00.0: PCI bridge to [bus 07-09] > pcieport 0000:06:00.0: bridge window [io 0x1000-0x2fff] > pcieport 0000:06:00.0: bridge window [mem 0xfda00000-0xfddfffff] > pcieport 0000:06:00.0: bridge window [mem 0xfc000000-0xfc3fffff 64bit pref] > pci_bus 0000:07: scanning bus > pcieport 0000:07:00.0: scanning [bus 08-08] behind bridge, pass 0 > pcieport 0000:07:00.0: PCI bridge to [bus 08] > pcieport 0000:07:00.0: bridge window [io 0x2000-0x2fff] > pcieport 0000:07:00.0: bridge window [mem 0xfdc00000-0xfddfffff] > pcieport 0000:07:00.0: bridge window [mem 0xfc200000-0xfc3fffff 64bit pref] > pci_bus 0000:08: scanning bus > pcieport 0000:07:00.1: scanning [bus 09-09] behind bridge, pass 0 > pcieport 0000:07:00.1: PCI bridge to [bus 09] > pcieport 0000:07:00.1: bridge window [io 0x1000-0x1fff] > pcieport 0000:07:00.1: bridge window [mem 0xfda00000-0xfdbfffff] > pcieport 0000:07:00.1: bridge window [mem 0xfc000000-0xfc1fffff 64bit pref] > > Add is_added checking to avoid those wrong reading. > > Fixes: dff22d2054b5 ("PCI: Call pci_read_bridge_bases() from core > instead of arch code") > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > > --- > drivers/pci/probe.c | 3 +++ > 1 file changed, 3 insertions(+) > > Index: linux-2.6/drivers/pci/probe.c > =================================================================== > --- linux-2.6.orig/drivers/pci/probe.c > +++ linux-2.6/drivers/pci/probe.c > @@ -455,6 +455,9 @@ void pci_read_bridge_bases(struct pci_bu > struct resource *res; > int i; > > + if (child->is_added) > + return; > + > if (pci_is_root_bus(child)) /* It's a host bus, nothing to read */ > return; > Subject: [PATCH] PCI: Skip reading bridge bases with existing ones on rescan > > Noticed there is noisy print out pci bridge bases read out for > existing devices that is caused by commit dff22d2054b5 > ("PCI: Call pci_read_bridge_bases() from core instead of arch code") > > pci_bus 0000:06: scanning bus > pcieport 0000:06:00.0: scanning [bus 07-09] behind bridge, pass 0 > pcieport 0000:06:00.0: PCI bridge to [bus 07-09] > pcieport 0000:06:00.0: bridge window [io 0x1000-0x2fff] > pcieport 0000:06:00.0: bridge window [mem 0xfda00000-0xfddfffff] > pcieport 0000:06:00.0: bridge window [mem 0xfc000000-0xfc3fffff 64bit pref] > pci_bus 0000:07: scanning bus > pcieport 0000:07:00.0: scanning [bus 08-08] behind bridge, pass 0 > pcieport 0000:07:00.0: PCI bridge to [bus 08] > pcieport 0000:07:00.0: bridge window [io 0x2000-0x2fff] > pcieport 0000:07:00.0: bridge window [mem 0xfdc00000-0xfddfffff] > pcieport 0000:07:00.0: bridge window [mem 0xfc200000-0xfc3fffff 64bit pref] > pci_bus 0000:08: scanning bus > pcieport 0000:07:00.1: scanning [bus 09-09] behind bridge, pass 0 > pcieport 0000:07:00.1: PCI bridge to [bus 09] > pcieport 0000:07:00.1: bridge window [io 0x1000-0x1fff] > pcieport 0000:07:00.1: bridge window [mem 0xfda00000-0xfdbfffff] > pcieport 0000:07:00.1: bridge window [mem 0xfc000000-0xfc1fffff 64bit pref] > > Add is_added checking to avoid those wrong reading. > > Fixes: dff22d2054b5 ("PCI: Call pci_read_bridge_bases() from core instead of arch code") > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > > --- > drivers/pci/probe.c | 3 +++ > 1 file changed, 3 insertions(+) > > Index: linux-2.6/drivers/pci/probe.c > =================================================================== > --- linux-2.6.orig/drivers/pci/probe.c > +++ linux-2.6/drivers/pci/probe.c > @@ -455,6 +455,9 @@ void pci_read_bridge_bases(struct pci_bu > struct resource *res; > int i; > > + if (child->is_added) > + return; > + > if (pci_is_root_bus(child)) /* It's a host bus, nothing to read */ > return; > -- 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 Tue, Sep 15, 2015 at 4:46 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Tue, Sep 15, 2015 at 12:58:20AM +0100, Yinghai Lu wrote: >> On Mon, Sep 14, 2015 at 10:36 AM, Yinghai Lu <yinghai@kernel.org> wrote: >> > On Mon, Sep 14, 2015 at 9:28 AM, Lorenzo Pieralisi >> > <lorenzo.pieralisi@arm.com> wrote: >> >> On Mon, Sep 14, 2015 at 05:05:50PM +0100, Yinghai Lu wrote: >> >>> We could just revert >> >>> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of >> >>> arch code") >> >>> instead. >> >> >> >> > if arch code called pci_read_bridge_bases() via pcibios_fixup_bus(), >> > then it need to have >> > to call pcibios_allocate_bus_resources() later. >> > >> > but now arm (mips ?) does not have calling pcibios_allocate_bus_resources(). > > pcibios_allocate_bus_resources() is an arch specific function and arm > and (and mips ?) does not need to create/call it because ARM reassigns > ALL resources in ALL platforms, hoping FW can provide a reasonable PCI > bridge apertures set-up on ARM is wishful thinking at present. > > If PCI core code is written with that assumption (ie that arch code zeroes > the bridge apertures if they can't be claimed), pci_read_bridge_bases() > can't be moved to PCI core code at present, sad and simple. > > I already asked many times why __pci_bus_size_bridges() cares about > the old bridge size on first scan and got no answer so I would ask Bjorn > to revert dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core > instead of arch code") or we apply an ARM specific plaster, we are making > no progress on this. > >> Found other problem that is caused by >> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of >> arch code") >> >> If that commit does not get reverted, will need to have attached patch > > I see what you mean and I see why there is a reason to apply the patch > below if we do not revert dff22d2054b5 (" PCI: Call pci_read_bridge_bases() > from core instead of arch code"), but I am afraid the commit log has to > be rewritten to explain the problem in a way that properly describes > the issue, and that's not the first one I read in the last couple of > weeks to figure out how to fix this regression. I'm inclined to revert dff22d2054b5 ("PCI: Call pci_read_bridge_bases() from core instead of arch code") until we can figure this out. I think the idea of moving that work from arch-specific code to the core is good, but it seems like it leads to more hacky workarounds than real cleanup right now. What would break if we simply reverted it? Would that reintroduce any problems? PCI enumeration and resource assignment is currently split, with some ACPI stuff in the middle: - PCI enumeration - ACPI resource reservation - PCI resource assignment I think it would make sense to do some PCI resource validation and assignment during enumeration, but I don't think it's possible as long as we have that ACPI stuff in the middle. Sometimes ACPI reports non-PCI devices in the middle of space we think is available for PCI (either from _CRS or our legacy "everything after top-of-RAM" idea), so we can't assign space to PCI devices until we know where those ACPI things are. I think this is totally broken; ACPI is logically more fundamental than PCI, and we should look at all the ACPI resources *before* we enumerate PCI devices, but there's a lot of ancient history there and it's a lot of work to change it. Bjorn -- 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 Tue, Sep 15, 2015 at 04:57:07PM +0100, Bjorn Helgaas wrote: > On Tue, Sep 15, 2015 at 4:46 AM, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > On Tue, Sep 15, 2015 at 12:58:20AM +0100, Yinghai Lu wrote: > >> On Mon, Sep 14, 2015 at 10:36 AM, Yinghai Lu <yinghai@kernel.org> wrote: > >> > On Mon, Sep 14, 2015 at 9:28 AM, Lorenzo Pieralisi > >> > <lorenzo.pieralisi@arm.com> wrote: > >> >> On Mon, Sep 14, 2015 at 05:05:50PM +0100, Yinghai Lu wrote: > >> >>> We could just revert > >> >>> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of > >> >>> arch code") > >> >>> instead. > >> >> > >> > >> > if arch code called pci_read_bridge_bases() via pcibios_fixup_bus(), > >> > then it need to have > >> > to call pcibios_allocate_bus_resources() later. > >> > > >> > but now arm (mips ?) does not have calling pcibios_allocate_bus_resources(). > > > > pcibios_allocate_bus_resources() is an arch specific function and arm > > and (and mips ?) does not need to create/call it because ARM reassigns > > ALL resources in ALL platforms, hoping FW can provide a reasonable PCI > > bridge apertures set-up on ARM is wishful thinking at present. > > > > If PCI core code is written with that assumption (ie that arch code zeroes > > the bridge apertures if they can't be claimed), pci_read_bridge_bases() > > can't be moved to PCI core code at present, sad and simple. > > > > I already asked many times why __pci_bus_size_bridges() cares about > > the old bridge size on first scan and got no answer so I would ask Bjorn > > to revert dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core > > instead of arch code") or we apply an ARM specific plaster, we are making > > no progress on this. > > > >> Found other problem that is caused by > >> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of > >> arch code") > >> > >> If that commit does not get reverted, will need to have attached patch > > > > I see what you mean and I see why there is a reason to apply the patch > > below if we do not revert dff22d2054b5 (" PCI: Call pci_read_bridge_bases() > > from core instead of arch code"), but I am afraid the commit log has to > > be rewritten to explain the problem in a way that properly describes > > the issue, and that's not the first one I read in the last couple of > > weeks to figure out how to fix this regression. > > I'm inclined to revert dff22d2054b5 ("PCI: Call > pci_read_bridge_bases() from core instead of arch code") until we can > figure this out. I think the idea of moving that work from > arch-specific code to the core is good, but it seems like it leads to > more hacky workarounds than real cleanup right now. What would break > if we simply reverted it? Would that reintroduce any problems? None that I am aware of, I know Guenter required it for this series: https://lkml.org/lkml/2015/7/30/861 but it was not merged so, as far as I understand, reverting the patch should get things to normal. I think it unearthed a couple of niggles in core code though that should be changed regardless (eg I still think that __pci_bus_size_bridges() taking the "old" bridge aperture size for granted is wrong and that's the only reason why bridge apertures can only be read in arch code, that claims them and reset them if the claiming fail). > PCI enumeration and resource assignment is currently split, with some > ACPI stuff in the middle: > > - PCI enumeration > - ACPI resource reservation > - PCI resource assignment > > I think it would make sense to do some PCI resource validation and > assignment during enumeration, but I don't think it's possible as long > as we have that ACPI stuff in the middle. Sometimes ACPI reports > non-PCI devices in the middle of space we think is available for PCI > (either from _CRS or our legacy "everything after top-of-RAM" idea), > so we can't assign space to PCI devices until we know where those ACPI > things are. I think there are too many hidden assumptions in the code that assigns resources owing to history and they are really difficult to untangle, that's certain and that's why the resource validation and assignment, that should be arch-agnostic, is very arch dependent at present. Reading bridge bases in core code, which should be harmless, triggered a couple of regressions already, imagine what happens if we started claiming resources in core PCI code. Resource validation can be done in core code IMO, it will take time to figure it out though but it is something worth doing. Thank you ! Lorenzo -- 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 09/15/2015 09:30 AM, Lorenzo Pieralisi wrote: > On Tue, Sep 15, 2015 at 04:57:07PM +0100, Bjorn Helgaas wrote: >> On Tue, Sep 15, 2015 at 4:46 AM, Lorenzo Pieralisi >> <lorenzo.pieralisi@arm.com> wrote: >>> On Tue, Sep 15, 2015 at 12:58:20AM +0100, Yinghai Lu wrote: >>>> On Mon, Sep 14, 2015 at 10:36 AM, Yinghai Lu <yinghai@kernel.org> wrote: >>>>> On Mon, Sep 14, 2015 at 9:28 AM, Lorenzo Pieralisi >>>>> <lorenzo.pieralisi@arm.com> wrote: >>>>>> On Mon, Sep 14, 2015 at 05:05:50PM +0100, Yinghai Lu wrote: >>>>>>> We could just revert >>>>>>> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of >>>>>>> arch code") >>>>>>> instead. >>>>>> >>>> >>>>> if arch code called pci_read_bridge_bases() via pcibios_fixup_bus(), >>>>> then it need to have >>>>> to call pcibios_allocate_bus_resources() later. >>>>> >>>>> but now arm (mips ?) does not have calling pcibios_allocate_bus_resources(). >>> >>> pcibios_allocate_bus_resources() is an arch specific function and arm >>> and (and mips ?) does not need to create/call it because ARM reassigns >>> ALL resources in ALL platforms, hoping FW can provide a reasonable PCI >>> bridge apertures set-up on ARM is wishful thinking at present. >>> >>> If PCI core code is written with that assumption (ie that arch code zeroes >>> the bridge apertures if they can't be claimed), pci_read_bridge_bases() >>> can't be moved to PCI core code at present, sad and simple. >>> >>> I already asked many times why __pci_bus_size_bridges() cares about >>> the old bridge size on first scan and got no answer so I would ask Bjorn >>> to revert dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core >>> instead of arch code") or we apply an ARM specific plaster, we are making >>> no progress on this. >>> >>>> Found other problem that is caused by >>>> dff22d2054b5 (" PCI: Call pci_read_bridge_bases() from core instead of >>>> arch code") >>>> >>>> If that commit does not get reverted, will need to have attached patch >>> >>> I see what you mean and I see why there is a reason to apply the patch >>> below if we do not revert dff22d2054b5 (" PCI: Call pci_read_bridge_bases() >>> from core instead of arch code"), but I am afraid the commit log has to >>> be rewritten to explain the problem in a way that properly describes >>> the issue, and that's not the first one I read in the last couple of >>> weeks to figure out how to fix this regression. >> >> I'm inclined to revert dff22d2054b5 ("PCI: Call >> pci_read_bridge_bases() from core instead of arch code") until we can >> figure this out. I think the idea of moving that work from >> arch-specific code to the core is good, but it seems like it leads to >> more hacky workarounds than real cleanup right now. What would break >> if we simply reverted it? Would that reintroduce any problems? > > None that I am aware of, I know Guenter required it for this series: > > https://lkml.org/lkml/2015/7/30/861 > > but it was not merged so, as far as I understand, reverting the patch > should get things to normal. I think it unearthed a couple of niggles It looks like me and Yinghai disagree how the problem I was trying to fix should be handled, I don't understand Yinghai's concerns, and unfortunately I just don't have the time I would need to get a better understanding. It is fine with me to revert your patch and abandon mine. Guenter -- 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
Subject: [PATCH] PCI: Skip reading bridge bases with existing ones on rescan
Noticed there is noisy print out pci bridge bases read out for
existing devices that is caused by commit dff22d2054b5
("PCI: Call pci_read_bridge_bases() from core instead of arch code")
pci_bus 0000:06: scanning bus
pcieport 0000:06:00.0: scanning [bus 07-09] behind bridge, pass 0
pcieport 0000:06:00.0: PCI bridge to [bus 07-09]
pcieport 0000:06:00.0: bridge window [io 0x1000-0x2fff]
pcieport 0000:06:00.0: bridge window [mem 0xfda00000-0xfddfffff]
pcieport 0000:06:00.0: bridge window [mem 0xfc000000-0xfc3fffff 64bit pref]
pci_bus 0000:07: scanning bus
pcieport 0000:07:00.0: scanning [bus 08-08] behind bridge, pass 0
pcieport 0000:07:00.0: PCI bridge to [bus 08]
pcieport 0000:07:00.0: bridge window [io 0x2000-0x2fff]
pcieport 0000:07:00.0: bridge window [mem 0xfdc00000-0xfddfffff]
pcieport 0000:07:00.0: bridge window [mem 0xfc200000-0xfc3fffff 64bit pref]
pci_bus 0000:08: scanning bus
pcieport 0000:07:00.1: scanning [bus 09-09] behind bridge, pass 0
pcieport 0000:07:00.1: PCI bridge to [bus 09]
pcieport 0000:07:00.1: bridge window [io 0x1000-0x1fff]
pcieport 0000:07:00.1: bridge window [mem 0xfda00000-0xfdbfffff]
pcieport 0000:07:00.1: bridge window [mem 0xfc000000-0xfc1fffff 64bit pref]
Add is_added checking to avoid those wrong reading.
Fixes: dff22d2054b5 ("PCI: Call pci_read_bridge_bases() from core instead of arch code")
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
drivers/pci/probe.c | 3 +++
1 file changed, 3 insertions(+)
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -455,6 +455,9 @@ void pci_read_bridge_bases(struct pci_bu
struct resource *res;
int i;
+ if (child->is_added)
+ return;
+
if (pci_is_root_bus(child)) /* It's a host bus, nothing to read */
return;