Patchwork [V7,1/3] powerpc/pci: Make sure ISA IO base is not zero

login
register
mail settings
Submitter Hongtao Jia
Date Aug. 15, 2012, 8:57 a.m.
Message ID <1345021026-10886-2-git-send-email-B38951@freescale.com>
Download mbox | patch
Permalink /patch/177594/
State Superseded
Headers show

Comments

Hongtao Jia - Aug. 15, 2012, 8:57 a.m.
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>
---
 arch/powerpc/kernel/pci-common.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Scott Wood - Aug. 15, 2012, 5:29 p.m.
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
Benjamin Herrenschmidt - Aug. 15, 2012, 9:32 p.m.
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.
Scott Wood - Aug. 15, 2012, 9:57 p.m.
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
Benjamin Herrenschmidt - Aug. 15, 2012, 10:42 p.m.
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.
Hongtao Jia - Aug. 16, 2012, 3:11 a.m.
> -----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.
Hongtao Jia - Aug. 16, 2012, 3:37 a.m.
> -----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.
> 
>

Patch

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 */