[v2,3/6] powerpc/pci: use pci_is_pcie() to simplify code

Commit Message

Yijing Wang Sept. 5, 2013, 7:55 a.m. UTC

Use pci_is_pcie() to simplify code.

Acked-by: Kumar Gala <[e-mail]>
Reviewed-by: Gavin Shan <[e-mail]>
Signed-off-by: Yijing Wang <[e-mail]>
Cc: Gavin Shan <[e-mail]>
Cc: Benjamin Herrenschmidt <[e-mail]>
Cc: Paul Mackerras <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
 arch/powerpc/kernel/eeh.c     |    3 +--
 arch/powerpc/sysdev/fsl_pci.c |    2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

Comments

Bjorn Helgaas Sept. 6, 2013, 8:30 p.m. UTC | #1

On Thu, Sep 05, 2013 at 03:55:27PM +0800, Yijing Wang wrote:
> Use pci_is_pcie() to simplify code.
> 
> Acked-by: Kumar Gala <[e-mail]>
> Reviewed-by: Gavin Shan <[e-mail]>
> Signed-off-by: Yijing Wang <[e-mail]>
> Cc: Gavin Shan <[e-mail]>
> Cc: Benjamin Herrenschmidt <[e-mail]>
> Cc: Paul Mackerras <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  arch/powerpc/kernel/eeh.c     |    3 +--
>  arch/powerpc/sysdev/fsl_pci.c |    2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)

Ben, Paul, this has no dependencies on anything new to PCI or any
other patches in this series, so you can take it through the POWERPC
tree.  If you don't want to do that, let me know and I can take it.

If you want it:

Acked-by: Bjorn Helgaas <[e-mail]>

> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index 55593ee..6ebbe54 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -189,8 +189,7 @@ static size_t eeh_gather_pci_data(struct eeh_dev *edev, char * buf, size_t len)
>  	}
>  
>  	/* If PCI-E capable, dump PCI-E cap 10, and the AER */
> -	cap = pci_find_capability(dev, PCI_CAP_ID_EXP);
> -	if (cap) {
> +	if (pci_is_pcie(dev)) {
>  		n += scnprintf(buf+n, len-n, "pci-e cap10:\n");
>  		printk(KERN_WARNING
>  		       "EEH: PCI-E capabilities and status follow:\n");
> diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
> index 46ac1dd..5402a1d 100644
> --- a/arch/powerpc/sysdev/fsl_pci.c
> +++ b/arch/powerpc/sysdev/fsl_pci.c
> @@ -41,7 +41,7 @@ static void quirk_fsl_pcie_header(struct pci_dev *dev)
>  	u8 hdr_type;
>  
>  	/* if we aren't a PCIe don't bother */
> -	if (!pci_find_capability(dev, PCI_CAP_ID_EXP))
> +	if (!pci_is_pcie(dev))
>  		return;
>  
>  	/* if we aren't in host mode don't bother */
> -- 
> 1.7.1
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Benjamin Herrenschmidt Oct. 11, 2013, 5:49 a.m. UTC | #2

On Fri, 2013-09-06 at 14:30 -0600, Bjorn Helgaas wrote:
> On Thu, Sep 05, 2013 at 03:55:27PM +0800, Yijing Wang wrote:
> > Use pci_is_pcie() to simplify code.
> > 
> > Acked-by: Kumar Gala <[e-mail]>
> > Reviewed-by: Gavin Shan <[e-mail]>
> > Signed-off-by: Yijing Wang <[e-mail]>
> > Cc: Gavin Shan <[e-mail]>
> > Cc: Benjamin Herrenschmidt <[e-mail]>
> > Cc: Paul Mackerras <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > ---
> >  arch/powerpc/kernel/eeh.c     |    3 +--
> >  arch/powerpc/sysdev/fsl_pci.c |    2 +-
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> Ben, Paul, this has no dependencies on anything new to PCI or any
> other patches in this series, so you can take it through the POWERPC
> tree.  If you don't want to do that, let me know and I can take it.
> 
> If you want it:
> 
> Acked-by: Bjorn Helgaas <[e-mail]>

It's also quite broken :-)

See below:

> > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> > index 55593ee..6ebbe54 100644
> > --- a/arch/powerpc/kernel/eeh.c
> > +++ b/arch/powerpc/kernel/eeh.c
> > @@ -189,8 +189,7 @@ static size_t eeh_gather_pci_data(struct eeh_dev *edev, char * buf, size_t len)
> >  	}
> >  
> >  	/* If PCI-E capable, dump PCI-E cap 10, and the AER */
> > -	cap = pci_find_capability(dev, PCI_CAP_ID_EXP);
> > -	if (cap) {
> > +	if (pci_is_pcie(dev)) {
> >  		n += scnprintf(buf+n, len-n, "pci-e cap10:\n");
> >  		printk(KERN_WARNING
> >  		       "EEH: PCI-E capabilities and status follow:\n");

So we remove reading of "cap", but slightly further down the code does:

		for (i=0; i<=8; i++) {
			eeh_ops->read_config(dn, cap+4*i, 4, &cfg);
			n += scnprintf(buf+n, len-n, "%02x:%x\n", 4*i, cfg);
			printk(KERN_WARNING "EEH: PCI-E %02x: %08x\n", i, cfg);
		}

Which actually *uses* the value of "cap" ... oops :-)

> > diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
> > index 46ac1dd..5402a1d 100644
> > --- a/arch/powerpc/sysdev/fsl_pci.c
> > +++ b/arch/powerpc/sysdev/fsl_pci.c
> > @@ -41,7 +41,7 @@ static void quirk_fsl_pcie_header(struct pci_dev *dev)
> >  	u8 hdr_type;
> >  
> >  	/* if we aren't a PCIe don't bother */
> > -	if (!pci_find_capability(dev, PCI_CAP_ID_EXP))
> > +	if (!pci_is_pcie(dev))
> >  		return;
> >  
> >  	/* if we aren't in host mode don't bother */
> > -- 
> > 1.7.1
> > 
> > 


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Yijing Wang Oct. 11, 2013, 6:28 a.m. UTC | #3

On 2013/10/11 13:49, Benjamin Herrenschmidt wrote:
> On Fri, 2013-09-06 at 14:30 -0600, Bjorn Helgaas wrote:
>> On Thu, Sep 05, 2013 at 03:55:27PM +0800, Yijing Wang wrote:
>>> Use pci_is_pcie() to simplify code.
>>>
>>> Acked-by: Kumar Gala <[e-mail]>
>>> Reviewed-by: Gavin Shan <[e-mail]>
>>> Signed-off-by: Yijing Wang <[e-mail]>
>>> Cc: Gavin Shan <[e-mail]>
>>> Cc: Benjamin Herrenschmidt <[e-mail]>
>>> Cc: Paul Mackerras <[e-mail]>
>>> Cc: [e-mail]
>>> Cc: [e-mail]
>>> ---
>>>  arch/powerpc/kernel/eeh.c     |    3 +--
>>>  arch/powerpc/sysdev/fsl_pci.c |    2 +-
>>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> Ben, Paul, this has no dependencies on anything new to PCI or any
>> other patches in this series, so you can take it through the POWERPC
>> tree.  If you don't want to do that, let me know and I can take it.
>>
>> If you want it:
>>
>> Acked-by: Bjorn Helgaas <[e-mail]>
> 
> It's also quite broken :-)
> 
> See below:
> 
>>> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>>> index 55593ee..6ebbe54 100644
>>> --- a/arch/powerpc/kernel/eeh.c
>>> +++ b/arch/powerpc/kernel/eeh.c
>>> @@ -189,8 +189,7 @@ static size_t eeh_gather_pci_data(struct eeh_dev *edev, char * buf, size_t len)
>>>  	}
>>>  
>>>  	/* If PCI-E capable, dump PCI-E cap 10, and the AER */
>>> -	cap = pci_find_capability(dev, PCI_CAP_ID_EXP);
>>> -	if (cap) {
>>> +	if (pci_is_pcie(dev)) {
>>>  		n += scnprintf(buf+n, len-n, "pci-e cap10:\n");
>>>  		printk(KERN_WARNING
>>>  		       "EEH: PCI-E capabilities and status follow:\n");
> 
> So we remove reading of "cap", but slightly further down the code does:
> 
> 		for (i=0; i<=8; i++) {
> 			eeh_ops->read_config(dn, cap+4*i, 4, &cfg);
> 			n += scnprintf(buf+n, len-n, "%02x:%x\n", 4*i, cfg);
> 			printk(KERN_WARNING "EEH: PCI-E %02x: %08x\n", i, cfg);
> 		}
> 
> Which actually *uses* the value of "cap" ... oops :-)

Hi Benjamin,
   Thanks for your review and comments!  I will update it at once.

Thanks!
Yijing.

> 
>>> diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
>>> index 46ac1dd..5402a1d 100644
>>> --- a/arch/powerpc/sysdev/fsl_pci.c
>>> +++ b/arch/powerpc/sysdev/fsl_pci.c
>>> @@ -41,7 +41,7 @@ static void quirk_fsl_pcie_header(struct pci_dev *dev)
>>>  	u8 hdr_type;
>>>  
>>>  	/* if we aren't a PCIe don't bother */
>>> -	if (!pci_find_capability(dev, PCI_CAP_ID_EXP))
>>> +	if (!pci_is_pcie(dev))
>>>  		return;
>>>  
>>>  	/* if we aren't in host mode don't bother */
>>> -- 
>>> 1.7.1
>>>
>>>
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> .
>

Yijing Wang Oct. 11, 2013, 6:33 a.m. UTC | #4

On 2013/10/11 14:16, Gavin Shan wrote:
> On Fri, Oct 11, 2013 at 04:49:56PM +1100, Benjamin Herrenschmidt wrote:
>> On Fri, 2013-09-06 at 14:30 -0600, Bjorn Helgaas wrote:
>>> On Thu, Sep 05, 2013 at 03:55:27PM +0800, Yijing Wang wrote:
>>>> Use pci_is_pcie() to simplify code.
>>>>
>>>> Acked-by: Kumar Gala <[e-mail]>
>>>> Reviewed-by: Gavin Shan <[e-mail]>
>>>> Signed-off-by: Yijing Wang <[e-mail]>
>>>> Cc: Gavin Shan <[e-mail]>
>>>> Cc: Benjamin Herrenschmidt <[e-mail]>
>>>> Cc: Paul Mackerras <[e-mail]>
>>>> Cc: [e-mail]
>>>> Cc: [e-mail]
>>>> ---
>>>>  arch/powerpc/kernel/eeh.c     |    3 +--
>>>>  arch/powerpc/sysdev/fsl_pci.c |    2 +-
>>>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> Ben, Paul, this has no dependencies on anything new to PCI or any
>>> other patches in this series, so you can take it through the POWERPC
>>> tree.  If you don't want to do that, let me know and I can take it.
>>>
>>> If you want it:
>>>
>>> Acked-by: Bjorn Helgaas <[e-mail]>
>>
>> It's also quite broken :-)
>>
>> See below:
>>
>>>> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>>>> index 55593ee..6ebbe54 100644
>>>> --- a/arch/powerpc/kernel/eeh.c
>>>> +++ b/arch/powerpc/kernel/eeh.c
>>>> @@ -189,8 +189,7 @@ static size_t eeh_gather_pci_data(struct eeh_dev *edev, char * buf, size_t len)
>>>>  	}
>>>>  
>>>>  	/* If PCI-E capable, dump PCI-E cap 10, and the AER */
>>>> -	cap = pci_find_capability(dev, PCI_CAP_ID_EXP);
>>>> -	if (cap) {
>>>> +	if (pci_is_pcie(dev)) {
>>>>  		n += scnprintf(buf+n, len-n, "pci-e cap10:\n");
>>>>  		printk(KERN_WARNING
>>>>  		       "EEH: PCI-E capabilities and status follow:\n");
>>
>> So we remove reading of "cap", but slightly further down the code does:
>>
>> 		for (i=0; i<=8; i++) {
>> 			eeh_ops->read_config(dn, cap+4*i, 4, &cfg);
>> 			n += scnprintf(buf+n, len-n, "%02x:%x\n", 4*i, cfg);
>> 			printk(KERN_WARNING "EEH: PCI-E %02x: %08x\n", i, cfg);
>> 		}
>>
>> Which actually *uses* the value of "cap" ... oops :-)
>>
> 
> It's my fault and I should have looked into the changes more closely.
> How about changing it like this:
> 
> 	cap = pci_is_pcie(dev) ? pci_pcie_cap(dev) :
> 	      pci_find_capability(dev, PCI_CAP_ID_EXP);
> 	if (cap) {
> 		...
> 	}
> 
> It would save some PCI-CFG access cycles for most cases :-)

Hi Gavin,  it's not your fault, it's my fault. :)

Because pci_pcie_cap(dev) == dev->pcie_cap == pci_find_capability(dev, PCI_CAP_ID_EXP);

so I think it's ok to use dev->pcie_cap instead of stale "cap".

I have updated this patch.

Thanks for your review and comments!

Thanks!
Yijing.


> 
>>>> diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
>>>> index 46ac1dd..5402a1d 100644
>>>> --- a/arch/powerpc/sysdev/fsl_pci.c
>>>> +++ b/arch/powerpc/sysdev/fsl_pci.c
>>>> @@ -41,7 +41,7 @@ static void quirk_fsl_pcie_header(struct pci_dev *dev)
>>>>  	u8 hdr_type;
>>>>  
>>>>  	/* if we aren't a PCIe don't bother */
>>>> -	if (!pci_find_capability(dev, PCI_CAP_ID_EXP))
>>>> +	if (!pci_is_pcie(dev))
>>>>  		return;
>>>>  
>>>>  	/* if we aren't in host mode don't bother */
> 
> Thanks,
> Gavin
> 
> 
> .
>

Yijing Wang Oct. 11, 2013, 7:28 a.m. UTC | #5

On 2013/10/11 14:53, Gavin Shan wrote:
> On Fri, Oct 11, 2013 at 02:33:58PM +0800, Yijing Wang wrote:
>> On 2013/10/11 14:16, Gavin Shan wrote:
>>> On Fri, Oct 11, 2013 at 04:49:56PM +1100, Benjamin Herrenschmidt wrote:
>>>> On Fri, 2013-09-06 at 14:30 -0600, Bjorn Helgaas wrote:
>>>>> On Thu, Sep 05, 2013 at 03:55:27PM +0800, Yijing Wang wrote:
> 
> .../...
> 
>>>>>> Use pci_is_pcie() to simplify code.
>>>>>> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>>>>>> index 55593ee..6ebbe54 100644
>>>>>> --- a/arch/powerpc/kernel/eeh.c
>>>>>> +++ b/arch/powerpc/kernel/eeh.c
>>>>>> @@ -189,8 +189,7 @@ static size_t eeh_gather_pci_data(struct eeh_dev *edev, char * buf, size_t len)
>>>>>>  	}
>>>>>>  
>>>>>>  	/* If PCI-E capable, dump PCI-E cap 10, and the AER */
>>>>>> -	cap = pci_find_capability(dev, PCI_CAP_ID_EXP);
>>>>>> -	if (cap) {
>>>>>> +	if (pci_is_pcie(dev)) {
>>>>>>  		n += scnprintf(buf+n, len-n, "pci-e cap10:\n");
>>>>>>  		printk(KERN_WARNING
>>>>>>  		       "EEH: PCI-E capabilities and status follow:\n");
>>>>
>>>> So we remove reading of "cap", but slightly further down the code does:
>>>>
>>>> 		for (i=0; i<=8; i++) {
>>>> 			eeh_ops->read_config(dn, cap+4*i, 4, &cfg);
>>>> 			n += scnprintf(buf+n, len-n, "%02x:%x\n", 4*i, cfg);
>>>> 			printk(KERN_WARNING "EEH: PCI-E %02x: %08x\n", i, cfg);
>>>> 		}
>>>>
>>>> Which actually *uses* the value of "cap" ... oops :-)
>>>>
>>>
>>> It's my fault and I should have looked into the changes more closely.
>>> How about changing it like this:
>>>
>>> 	cap = pci_is_pcie(dev) ? pci_pcie_cap(dev) :
>>> 	      pci_find_capability(dev, PCI_CAP_ID_EXP);
>>> 	if (cap) {
>>> 		...
>>> 	}
>>>
>>> It would save some PCI-CFG access cycles for most cases :-)
>>
>> Hi Gavin,  it's not your fault, it's my fault. :)
>>
>> Because pci_pcie_cap(dev) == dev->pcie_cap == pci_find_capability(dev, PCI_CAP_ID_EXP);
>>
>> so I think it's ok to use dev->pcie_cap instead of stale "cap".
>>
> 
> Yijing, There has one exception: dev->pcie_cap isn't updated yet.

In my idea, dev->pcie_cap(here is pci_dev->pcie_cap) will update in set_pcie_port_type() function,
and this function always be called after allocate pci device. We get pci_dev by eeh_dev_to_pci_dev(),
I think pci_dev has been initialized completely.

> This function has possibility to be invoked before that. However,
> we don't have the binding (eeh device <-> PCI device) for the case.
> So the piece of code shouldn't be running

In PCI core, I knew

pci_scan_device()
   pci_setup_device()
       set_pcie_port_type()
            pci_dev->pcie_cap = pci_find_capability(pdev, PCI_CAP_ID_EXP);

In powerpc, I also found

of_scan_pci_dev()
   of_create_pci_dev()
       set_pcie_port_type()
	    pci_dev->pcie_cap = pci_find_capability(pdev, PCI_CAP_ID_EXP);
> 
> However, it's a bit safer to have pci_find_capability(dev, PCI_CAP_ID_EXP)
> as well even though we needn't it for 99.9% cases if you agree :-)

I agree, this function is not the performance bottleneck,
safety is more important. :)
So if Bjorn and Benjamin think it's not safe, it's ok to drop it. :)

Thanks!
Yijing.

> 
> Thanks,
> Gavin
> 
> 
> .
>

Yijing Wang Oct. 11, 2013, 8:22 a.m. UTC | #6

>> In my idea, dev->pcie_cap(here is pci_dev->pcie_cap) will update in set_pcie_port_type() function,
>> and this function always be called after allocate pci device. We get pci_dev by eeh_dev_to_pci_dev(),
>> I think pci_dev has been initialized completely.
>>
>>> This function has possibility to be invoked before that. However,
>>> we don't have the binding (eeh device <-> PCI device) for the case.
>>> So the piece of code shouldn't be running
>>
>> In PCI core, I knew
>>
>> pci_scan_device()
>>   pci_setup_device()
>>       set_pcie_port_type()
>>            pci_dev->pcie_cap = pci_find_capability(pdev, PCI_CAP_ID_EXP);
>>
>> In powerpc, I also found
>>
>> of_scan_pci_dev()
>>   of_create_pci_dev()
>>       set_pcie_port_type()
>> 	    pci_dev->pcie_cap = pci_find_capability(pdev, PCI_CAP_ID_EXP);
>>>
>>> However, it's a bit safer to have pci_find_capability(dev, PCI_CAP_ID_EXP)
>>> as well even though we needn't it for 99.9% cases if you agree :-)
>>
>> I agree, this function is not the performance bottleneck,
>> safety is more important. :)
>> So if Bjorn and Benjamin think it's not safe, it's ok to drop it. :)
>>
> 
> No, it's not what I mean. Anyway, "v3" looks good to me.
> At least, it can save PCI-CFG access cycles find locate
> the PCIe capability position :-)

Thanks! :)

> 
> Thanks,
> Gavin
> 
> 
> .
>

Patch

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 55593ee..6ebbe54 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -189,8 +189,7 @@  static size_t eeh_gather_pci_data(struct eeh_dev *edev, char * buf, size_t len)
 	}
 
 	/* If PCI-E capable, dump PCI-E cap 10, and the AER */
-	cap = pci_find_capability(dev, PCI_CAP_ID_EXP);
-	if (cap) {
+	if (pci_is_pcie(dev)) {
 		n += scnprintf(buf+n, len-n, "pci-e cap10:\n");
 		printk(KERN_WARNING
 		       "EEH: PCI-E capabilities and status follow:\n");
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index 46ac1dd..5402a1d 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -41,7 +41,7 @@  static void quirk_fsl_pcie_header(struct pci_dev *dev)
 	u8 hdr_type;
 
 	/* if we aren't a PCIe don't bother */
-	if (!pci_find_capability(dev, PCI_CAP_ID_EXP))
+	if (!pci_is_pcie(dev))
 		return;
 
 	/* if we aren't in host mode don't bother */