| Message ID | 1345021026-10886-2-git-send-email-B38951@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 08/15/2012 03:57 AM, Jia Hongtao wrote: > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > Some platforms like QEMU treat 0 as an invalid address for ISA IO base. > So we make sure that ISA IO base will never be zero. By functionality this > is equivalent to assgin the first pci bus detected as a primary bus. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Signed-off-by: Jia Hongtao <B38951@freescale.com> When did Ben post this? Suggesting a temporary workaround in an e-mail is *not* the same as posting a patch, and definitely not the same as providing a signed-off-by which AFAICT you forged. Don't do that. > --- > arch/powerpc/kernel/pci-common.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c > index 0f75bd5..2a09aa5 100644 > --- a/arch/powerpc/kernel/pci-common.c > +++ b/arch/powerpc/kernel/pci-common.c > @@ -734,7 +734,7 @@ void __devinit pci_process_bridge_OF_ranges(struct pci_controller *hose, > hose->io_base_virt = ioremap(cpu_addr, size); > > /* Expect trouble if pci_addr is not 0 */ > - if (primary) > + if (primary || !isa_io_base) > isa_io_base = > (unsigned long)hose->io_base_virt; > #endif /* CONFIG_PPC32 */ > Didn't I already point out that this has problems when the primary bus is not the first to be probed? If your answer is that you fix that in a later patch, that breaks bisectability. -Scott
On Wed, 2012-08-15 at 12:29 -0500, Scott Wood wrote: > > --- > > arch/powerpc/kernel/pci-common.c | 2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c > > index 0f75bd5..2a09aa5 100644 > > --- a/arch/powerpc/kernel/pci-common.c > > +++ b/arch/powerpc/kernel/pci-common.c > > @@ -734,7 +734,7 @@ void __devinit pci_process_bridge_OF_ranges(struct pci_controller *hose, > > hose->io_base_virt = ioremap(cpu_addr, size); > > > > /* Expect trouble if pci_addr is not 0 */ > > - if (primary) > > + if (primary || !isa_io_base) > > isa_io_base = > > (unsigned long)hose->io_base_virt; > > #endif /* CONFIG_PPC32 */ > > > > Didn't I already point out that this has problems when the primary bus > is not the first to be probed? If your answer is that you fix that in a > later patch, that breaks bisectability. Is it though ? ie, we will override it with the real primary in the above test, so it will only very temporarily be set to the "wrong" bus no ? IE, the test will still trip on the actual "primary" if there's one. Cheers, Ben.
On 08/15/2012 04:32 PM, Benjamin Herrenschmidt wrote: > On Wed, 2012-08-15 at 12:29 -0500, Scott Wood wrote: >>> --- >>> arch/powerpc/kernel/pci-common.c | 2 +- >>> 1 files changed, 1 insertions(+), 1 deletions(-) >>> >>> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c >>> index 0f75bd5..2a09aa5 100644 >>> --- a/arch/powerpc/kernel/pci-common.c >>> +++ b/arch/powerpc/kernel/pci-common.c >>> @@ -734,7 +734,7 @@ void __devinit pci_process_bridge_OF_ranges(struct pci_controller *hose, >>> hose->io_base_virt = ioremap(cpu_addr, size); >>> >>> /* Expect trouble if pci_addr is not 0 */ >>> - if (primary) >>> + if (primary || !isa_io_base) >>> isa_io_base = >>> (unsigned long)hose->io_base_virt; >>> #endif /* CONFIG_PPC32 */ >>> >> >> Didn't I already point out that this has problems when the primary bus >> is not the first to be probed? If your answer is that you fix that in a >> later patch, that breaks bisectability. > > Is it though ? ie, we will override it with the real primary in the > above test, so it will only very temporarily be set to the "wrong" bus > no ? IE, the test will still trip on the actual "primary" if there's > one Is there no lasting remnant of that temporary wrong isa_io_base? We won't have I/O resources that were calculated relative to that, which stop working once isa_io_base changes? Or does that happen later, after this function has been called on all buses (and would that continue to be the case if we change the PCI bus to a platform device)? -Scott
On Wed, 2012-08-15 at 16:57 -0500, Scott Wood wrote: > Is there no lasting remnant of that temporary wrong isa_io_base? We > won't have I/O resources that were calculated relative to that, which > stop working once isa_io_base changes? Or does that happen later, after > this function has been called on all buses (and would that continue to > be the case if we change the PCI bus to a platform device)? If you continue creating your PCI busses all at once early on you'll be fine. The platform device business is going to break that (and other things as well btw, such as pci_final_fixup). Maybe it's time to contemplate doing something more like ppc64 and reserve a piece of virtual address space (I know there isn't much, so make it 64k per bus max) and just map the busses in there with the first 64k being reserved for the ISA stuff if it exists ? Cheers, Ben.
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Thursday, August 16, 2012 1:29 AM > To: Jia Hongtao-B38951 > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; > benh@kernel.crashing.org; Li Yang-R58472; Wood Scott-B07421 > Subject: Re: [PATCH V7 1/3] powerpc/pci: Make sure ISA IO base is not > zero > > On 08/15/2012 03:57 AM, Jia Hongtao wrote: > > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > > > Some platforms like QEMU treat 0 as an invalid address for ISA IO base. > > So we make sure that ISA IO base will never be zero. By functionality > > this is equivalent to assgin the first pci bus detected as a primary > bus. > > > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > Signed-off-by: Jia Hongtao <B38951@freescale.com> > > When did Ben post this? > > Suggesting a temporary workaround in an e-mail is *not* the same as > posting a patch, and definitely not the same as providing a signed-off-by > which AFAICT you forged. Don't do that. > > > --- > > arch/powerpc/kernel/pci-common.c | 2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/arch/powerpc/kernel/pci-common.c > > b/arch/powerpc/kernel/pci-common.c > > index 0f75bd5..2a09aa5 100644 > > --- a/arch/powerpc/kernel/pci-common.c > > +++ b/arch/powerpc/kernel/pci-common.c > > @@ -734,7 +734,7 @@ void __devinit pci_process_bridge_OF_ranges(struct > pci_controller *hose, > > hose->io_base_virt = ioremap(cpu_addr, size); > > > > /* Expect trouble if pci_addr is not 0 */ > > - if (primary) > > + if (primary || !isa_io_base) > > isa_io_base = > > (unsigned long)hose->io_base_virt; #endif > /* CONFIG_PPC32 */ > > > > Didn't I already point out that this has problems when the primary bus is > not the first to be probed? If your answer is that you fix that in a > later patch, that breaks bisectability. > > -Scott Sorry, my answer is not that I fix that in later patch. My answer is, without this patch there is also problem with non-first-primary. That is to say the bisectability problem has been already there. The problem is not brought by this patch. - Hongtao.
> -----Original Message----- > From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org] > Sent: Thursday, August 16, 2012 6:43 AM > To: Wood Scott-B07421 > Cc: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org; > galak@kernel.crashing.org; Li Yang-R58472; Wood Scott-B07421 > Subject: Re: [PATCH V7 1/3] powerpc/pci: Make sure ISA IO base is not > zero > > On Wed, 2012-08-15 at 16:57 -0500, Scott Wood wrote: > > Is there no lasting remnant of that temporary wrong isa_io_base? We > > won't have I/O resources that were calculated relative to that, which > > stop working once isa_io_base changes? Or does that happen later, > > after this function has been called on all buses (and would that > > continue to be the case if we change the PCI bus to a platform device)? > > If you continue creating your PCI busses all at once early on you'll be > fine. The platform device business is going to break that (and other > things as well btw, such as pci_final_fixup). I have already done some investigation and the sequence of fixup (including early, header, final) will not be changed in platform driver. We register and init PCI controllers as platform devices at arch_initcall stage and PCI scanning (pcibios_init) is at subsys_initcall stage in which early and header fixup will be done in right sequence. The final fixup will be start at rootfs_initcall stage which is later than early and header fixup. - Hongtao. > > Maybe it's time to contemplate doing something more like ppc64 and > reserve a piece of virtual address space (I know there isn't much, so > make it 64k per bus max) and just map the busses in there with the first > 64k being reserved for the ISA stuff if it exists ? > > Cheers, > Ben. > >
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c index 0f75bd5..2a09aa5 100644 --- a/arch/powerpc/kernel/pci-common.c +++ b/arch/powerpc/kernel/pci-common.c @@ -734,7 +734,7 @@ void __devinit pci_process_bridge_OF_ranges(struct pci_controller *hose, hose->io_base_virt = ioremap(cpu_addr, size); /* Expect trouble if pci_addr is not 0 */ - if (primary) + if (primary || !isa_io_base) isa_io_base = (unsigned long)hose->io_base_virt; #endif /* CONFIG_PPC32 */