fix the problem where pcix node is probed again as pci node.

Submitted by fkan@amcc.com on March 30, 2010, 5:41 p.m.

Details

Message ID 1269970878-5080-1-git-send-email-fkan@amcc.com
State Rejected
Headers show

Commit Message

fkan@amcc.com March 30, 2010, 5:41 p.m.
From: Feng Kan <fkan@appliedmicro.com>

The current matching scheme make the pci node match to pcix or pciex node.
To avoid the match, change the method so only one type of initialization
is called per node.

Signed-off-by: Feng Kan <fkan@appliedmicro.com>
Signed-off-by: Tirumala R Marri <tmarri@appliedmicro.com>
---
 arch/powerpc/sysdev/ppc4xx_pci.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

Comments

Benjamin Herrenschmidt March 30, 2010, 8:48 p.m.
On Tue, 2010-03-30 at 10:41 -0700, Feng Kan wrote:
> From: Feng Kan <fkan@appliedmicro.com>
> 
> The current matching scheme make the pci node match to pcix or pciex node.
> To avoid the match, change the method so only one type of initialization
> is called per node.

No, your patch is not right. The problem was introduced by a patch from
Grant that incorrectly made of_device_is_compatible do a substring
match. Grant should have fixed that now. Grant ? Is your fix upstream
yet ? If not, can you send that ASAP ?

Cheers,
Ben.


> Signed-off-by: Feng Kan <fkan@appliedmicro.com>
> Signed-off-by: Tirumala R Marri <tmarri@appliedmicro.com>
> ---
>  arch/powerpc/sysdev/ppc4xx_pci.c |   14 ++++++++------
>  1 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/ppc4xx_pci.c b/arch/powerpc/sysdev/ppc4xx_pci.c
> index 8aa3302..1e67c74 100644
> --- a/arch/powerpc/sysdev/ppc4xx_pci.c
> +++ b/arch/powerpc/sysdev/ppc4xx_pci.c
> @@ -1842,14 +1842,16 @@ static int __init ppc4xx_pci_find_bridges(void)
>  
>  	ppc_pci_flags |= PPC_PCI_ENABLE_PROC_DOMAINS | PPC_PCI_COMPAT_DOMAIN_0;
>  
> +	for_each_compatible_node(np, NULL, "ibm,plb-pci") {
> +		if (of_device_is_compatible(np, "ibm,plb-pcix"))
> +			ppc4xx_probe_pcix_bridge(np);
>  #ifdef CONFIG_PPC4xx_PCI_EXPRESS
> -	for_each_compatible_node(np, NULL, "ibm,plb-pciex")
> -		ppc4xx_probe_pciex_bridge(np);
> +		else if (of_device_is_compatible(np, "ibm,plb-pciex"))
> +			ppc4xx_probe_pciex_bridge(np);
>  #endif
> -	for_each_compatible_node(np, NULL, "ibm,plb-pcix")
> -		ppc4xx_probe_pcix_bridge(np);
> -	for_each_compatible_node(np, NULL, "ibm,plb-pci")
> -		ppc4xx_probe_pci_bridge(np);
> +		else
> +			ppc4xx_probe_pci_bridge(np);
> +	}
>  
>  	return 0;
>  }
Benjamin Herrenschmidt March 30, 2010, 9:14 p.m.
On Wed, 2010-03-31 at 07:48 +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2010-03-30 at 10:41 -0700, Feng Kan wrote:
> > From: Feng Kan <fkan@appliedmicro.com>
> > 
> > The current matching scheme make the pci node match to pcix or pciex node.
> > To avoid the match, change the method so only one type of initialization
> > is called per node.
> 
> No, your patch is not right. The problem was introduced by a patch from
> Grant that incorrectly made of_device_is_compatible do a substring
> match. Grant should have fixed that now. Grant ? Is your fix upstream
> yet ? If not, can you send that ASAP ?

Better if I CC him too :-)

Cheers,
Ben.

> Cheers,
> Ben.
> 
> 
> > Signed-off-by: Feng Kan <fkan@appliedmicro.com>
> > Signed-off-by: Tirumala R Marri <tmarri@appliedmicro.com>
> > ---
> >  arch/powerpc/sysdev/ppc4xx_pci.c |   14 ++++++++------
> >  1 files changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/powerpc/sysdev/ppc4xx_pci.c b/arch/powerpc/sysdev/ppc4xx_pci.c
> > index 8aa3302..1e67c74 100644
> > --- a/arch/powerpc/sysdev/ppc4xx_pci.c
> > +++ b/arch/powerpc/sysdev/ppc4xx_pci.c
> > @@ -1842,14 +1842,16 @@ static int __init ppc4xx_pci_find_bridges(void)
> >  
> >  	ppc_pci_flags |= PPC_PCI_ENABLE_PROC_DOMAINS | PPC_PCI_COMPAT_DOMAIN_0;
> >  
> > +	for_each_compatible_node(np, NULL, "ibm,plb-pci") {
> > +		if (of_device_is_compatible(np, "ibm,plb-pcix"))
> > +			ppc4xx_probe_pcix_bridge(np);
> >  #ifdef CONFIG_PPC4xx_PCI_EXPRESS
> > -	for_each_compatible_node(np, NULL, "ibm,plb-pciex")
> > -		ppc4xx_probe_pciex_bridge(np);
> > +		else if (of_device_is_compatible(np, "ibm,plb-pciex"))
> > +			ppc4xx_probe_pciex_bridge(np);
> >  #endif
> > -	for_each_compatible_node(np, NULL, "ibm,plb-pcix")
> > -		ppc4xx_probe_pcix_bridge(np);
> > -	for_each_compatible_node(np, NULL, "ibm,plb-pci")
> > -		ppc4xx_probe_pci_bridge(np);
> > +		else
> > +			ppc4xx_probe_pci_bridge(np);
> > +	}
> >  
> >  	return 0;
> >  }
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
fkan@amcc.com March 30, 2010, 10:02 p.m.
Ok thanks. This short string match may be useful in some cases, but I  
agree it plays havoc with the current code.

Feng Kan

On Mar 30, 2010, at 14:14, "Benjamin Herrenschmidt" <benh@kernel.crashing.org 
 > wrote:

> On Wed, 2010-03-31 at 07:48 +1100, Benjamin Herrenschmidt wrote:
>> On Tue, 2010-03-30 at 10:41 -0700, Feng Kan wrote:
>>> From: Feng Kan <fkan@appliedmicro.com>
>>>
>>> The current matching scheme make the pci node match to pcix or  
>>> pciex node.
>>> To avoid the match, change the method so only one type of  
>>> initialization
>>> is called per node.
>>
>> No, your patch is not right. The problem was introduced by a patch  
>> from
>> Grant that incorrectly made of_device_is_compatible do a substring
>> match. Grant should have fixed that now. Grant ? Is your fix upstream
>> yet ? If not, can you send that ASAP ?
>
> Better if I CC him too :-)
>
> Cheers,
> Ben.
>
>> Cheers,
>> Ben.
>>
>>
>>> Signed-off-by: Feng Kan <fkan@appliedmicro.com>
>>> Signed-off-by: Tirumala R Marri <tmarri@appliedmicro.com>
>>> ---
>>> arch/powerpc/sysdev/ppc4xx_pci.c |   14 ++++++++------
>>> 1 files changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/powerpc/sysdev/ppc4xx_pci.c b/arch/powerpc/ 
>>> sysdev/ppc4xx_pci.c
>>> index 8aa3302..1e67c74 100644
>>> --- a/arch/powerpc/sysdev/ppc4xx_pci.c
>>> +++ b/arch/powerpc/sysdev/ppc4xx_pci.c
>>> @@ -1842,14 +1842,16 @@ static int __init ppc4xx_pci_find_bridges 
>>> (void)
>>>
>>>    ppc_pci_flags |= PPC_PCI_ENABLE_PROC_DOMAINS |  
>>> PPC_PCI_COMPAT_DOMAIN_0;
>>>
>>> +    for_each_compatible_node(np, NULL, "ibm,plb-pci") {
>>> +        if (of_device_is_compatible(np, "ibm,plb-pcix"))
>>> +            ppc4xx_probe_pcix_bridge(np);
>>> #ifdef CONFIG_PPC4xx_PCI_EXPRESS
>>> -    for_each_compatible_node(np, NULL, "ibm,plb-pciex")
>>> -        ppc4xx_probe_pciex_bridge(np);
>>> +        else if (of_device_is_compatible(np, "ibm,plb-pciex"))
>>> +            ppc4xx_probe_pciex_bridge(np);
>>> #endif
>>> -    for_each_compatible_node(np, NULL, "ibm,plb-pcix")
>>> -        ppc4xx_probe_pcix_bridge(np);
>>> -    for_each_compatible_node(np, NULL, "ibm,plb-pci")
>>> -        ppc4xx_probe_pci_bridge(np);
>>> +        else
>>> +            ppc4xx_probe_pci_bridge(np);
>>> +    }
>>>
>>>    return 0;
>>> }
>>
>>
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
>
Grant Likely March 30, 2010, 10:28 p.m.
On Tue, Mar 30, 2010 at 3:14 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2010-03-31 at 07:48 +1100, Benjamin Herrenschmidt wrote:
>> On Tue, 2010-03-30 at 10:41 -0700, Feng Kan wrote:
>> > From: Feng Kan <fkan@appliedmicro.com>
>> >
>> > The current matching scheme make the pci node match to pcix or pciex node.
>> > To avoid the match, change the method so only one type of initialization
>> > is called per node.
>>
>> No, your patch is not right. The problem was introduced by a patch from
>> Grant that incorrectly made of_device_is_compatible do a substring
>> match. Grant should have fixed that now. Grant ? Is your fix upstream
>> yet ? If not, can you send that ASAP ?
>
> Better if I CC him too :-)

The fix is in upstream.  Commit 1976152fd8e706135deed6cf333e347c08416056

g.
Benjamin Herrenschmidt March 30, 2010, 10:28 p.m.
On Tue, 2010-03-30 at 15:02 -0700, Feng Kan wrote:
> Ok thanks. This short string match may be useful in some cases, but I
> agree it plays havoc with the current code.

Yeah well, it's not actually -that- useful and is definitely broken :-)

If you want multiples matches, then it's the compatible property itself
that should contain multiple entries since it's a list. The string match
should be exact.

Cheers,
Ben.

Patch hide | download patch | download mbox

diff --git a/arch/powerpc/sysdev/ppc4xx_pci.c b/arch/powerpc/sysdev/ppc4xx_pci.c
index 8aa3302..1e67c74 100644
--- a/arch/powerpc/sysdev/ppc4xx_pci.c
+++ b/arch/powerpc/sysdev/ppc4xx_pci.c
@@ -1842,14 +1842,16 @@  static int __init ppc4xx_pci_find_bridges(void)
 
 	ppc_pci_flags |= PPC_PCI_ENABLE_PROC_DOMAINS | PPC_PCI_COMPAT_DOMAIN_0;
 
+	for_each_compatible_node(np, NULL, "ibm,plb-pci") {
+		if (of_device_is_compatible(np, "ibm,plb-pcix"))
+			ppc4xx_probe_pcix_bridge(np);
 #ifdef CONFIG_PPC4xx_PCI_EXPRESS
-	for_each_compatible_node(np, NULL, "ibm,plb-pciex")
-		ppc4xx_probe_pciex_bridge(np);
+		else if (of_device_is_compatible(np, "ibm,plb-pciex"))
+			ppc4xx_probe_pciex_bridge(np);
 #endif
-	for_each_compatible_node(np, NULL, "ibm,plb-pcix")
-		ppc4xx_probe_pcix_bridge(np);
-	for_each_compatible_node(np, NULL, "ibm,plb-pci")
-		ppc4xx_probe_pci_bridge(np);
+		else
+			ppc4xx_probe_pci_bridge(np);
+	}
 
 	return 0;
 }