| Message ID | 1378367730-25996-3-git-send-email-wangyijing@huawei.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Benjamin Herrenschmidt |
| Headers | show |
On Thu, Sep 05, 2013 at 03:55:27PM +0800, Yijing Wang wrote: > Use pci_is_pcie() to simplify code. > > Acked-by: Kumar Gala <galak@kernel.crashing.org> > Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com> > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > Cc: Gavin Shan <shangw@linux.vnet.ibm.com> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Paul Mackerras <paulus@samba.org> > Cc: linuxppc-dev@lists.ozlabs.org > Cc: linux-kernel@vger.kernel.org > --- > 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 <bhelgaas@google.com> > 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 > >
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 <galak@kernel.crashing.org> > > Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com> > > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > > Cc: Gavin Shan <shangw@linux.vnet.ibm.com> > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > Cc: Paul Mackerras <paulus@samba.org> > > Cc: linuxppc-dev@lists.ozlabs.org > > Cc: linux-kernel@vger.kernel.org > > --- > > 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 <bhelgaas@google.com> 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 > > > >
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 <galak@kernel.crashing.org> >> > Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com> >> > Signed-off-by: Yijing Wang <wangyijing@huawei.com> >> > Cc: Gavin Shan <shangw@linux.vnet.ibm.com> >> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> > Cc: Paul Mackerras <paulus@samba.org> >> > Cc: linuxppc-dev@lists.ozlabs.org >> > Cc: linux-kernel@vger.kernel.org >> > --- >> > 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 <bhelgaas@google.com> > >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 :-) >> > 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
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 <galak@kernel.crashing.org> >>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com> >>> Signed-off-by: Yijing Wang <wangyijing@huawei.com> >>> Cc: Gavin Shan <shangw@linux.vnet.ibm.com> >>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>> Cc: Paul Mackerras <paulus@samba.org> >>> Cc: linuxppc-dev@lists.ozlabs.org >>> Cc: linux-kernel@vger.kernel.org >>> --- >>> 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 <bhelgaas@google.com> > > 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > > . >
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 <galak@kernel.crashing.org> >>>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com> >>>> Signed-off-by: Yijing Wang <wangyijing@huawei.com> >>>> Cc: Gavin Shan <shangw@linux.vnet.ibm.com> >>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>>> Cc: Paul Mackerras <paulus@samba.org> >>>> Cc: linuxppc-dev@lists.ozlabs.org >>>> Cc: linux-kernel@vger.kernel.org >>>> --- >>>> 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 <bhelgaas@google.com> >> >> 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 > > > . >
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. 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 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 :-) Thanks, Gavin
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 > > > . >
On Fri, Oct 11, 2013 at 03:28:02PM +0800, Yijing Wang wrote: >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. :) > 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, Gavin
>> 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 > > > . >
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 */