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

login
register
mail settings
Submitter fkan@amcc.com
Date March 30, 2010, 5:41 p.m.
Message ID <1269970878-5080-1-git-send-email-fkan@amcc.com>
Download mbox | patch
Permalink /patch/49003/
State Rejected
Headers show

Comments

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(-)
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

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;
 }