2013-09-03 07:38:31

by Yijing Wang

[permalink] [raw]
Subject: [PATCH 1/7] scsi/bfa: use pcie_capability_xxx to simplify code

Pcie_capability_xxx() interfaces were introudced to
simplify code to access PCIe Cap config space. And
because PCI core saves the PCIe Cap offset in
set_pcie_port_type() when device is enumerated.
So we can use pci_is_pcie() instead.

Signed-off-by: Yijing Wang <[email protected]>
Cc: Jiang Liu <[email protected]>
Cc: Anil Gurumurthy <[email protected]>
Cc: Vijaya Mohan Guvva <[email protected]>
Cc: "James E.J. Bottomley" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/scsi/bfa/bfad.c | 9 +++------
1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/bfa/bfad.c b/drivers/scsi/bfa/bfad.c
index 9611195..d726b81 100644
--- a/drivers/scsi/bfa/bfad.c
+++ b/drivers/scsi/bfa/bfad.c
@@ -767,7 +767,6 @@ bfad_pci_init(struct pci_dev *pdev, struct bfad_s *bfad)

/* Adjust PCIe Maximum Read Request Size */
if (pcie_max_read_reqsz > 0) {
- int pcie_cap_reg;
u16 pcie_dev_ctl;
u16 mask = 0xffff;

@@ -794,10 +793,8 @@ bfad_pci_init(struct pci_dev *pdev, struct bfad_s *bfad)
break;
}

- pcie_cap_reg = pci_find_capability(pdev, PCI_CAP_ID_EXP);
- if (mask != 0xffff && pcie_cap_reg) {
- pcie_cap_reg += 0x08;
- pci_read_config_word(pdev, pcie_cap_reg, &pcie_dev_ctl);
+ if (mask != 0xffff && pci_is_pcie(pdev)) {
+ pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &pcie_dev_ctl);
if ((pcie_dev_ctl & 0x7000) != mask) {
printk(KERN_WARNING "BFA[%s]: "
"pcie_max_read_request_size is %d, "
@@ -806,7 +803,7 @@ bfad_pci_init(struct pci_dev *pdev, struct bfad_s *bfad)
pcie_max_read_reqsz);

pcie_dev_ctl &= ~0x7000;
- pci_write_config_word(pdev, pcie_cap_reg,
+ pcie_capability_write_word(pdev, PCI_EXP_DEVCTL,
pcie_dev_ctl | mask);
}
}
--
1.7.1


2013-09-03 07:36:01

by Yijing Wang

[permalink] [raw]
Subject: [PATCH 2/7] scsi/csiostor: use pcie_capability_xxx to simplify code

Pcie_capability_xxx() interfaces were introudced to
simplify code to access PCIe Cap config space. And
because PCI core saves the PCIe Cap offset in
set_pcie_port_type() when device is enumerated.
So we can use pci_is_pcie() instead.

Signed-off-by: Yijing Wang <[email protected]>
Cc: Jiang Liu <[email protected]>
Cc: "James E.J. Bottomley" <[email protected]>
Cc: Naresh Kumar Inna <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Jesper Juhl <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/scsi/csiostor/csio_hw.c | 9 +++------
1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/csiostor/csio_hw.c b/drivers/scsi/csiostor/csio_hw.c
index 0eb35b9..be9a6ef 100644
--- a/drivers/scsi/csiostor/csio_hw.c
+++ b/drivers/scsi/csiostor/csio_hw.c
@@ -856,15 +856,12 @@ static void
csio_set_pcie_completion_timeout(struct csio_hw *hw, u8 range)
{
uint16_t val;
- int pcie_cap;

- if (!csio_pci_capability(hw->pdev, PCI_CAP_ID_EXP, &pcie_cap)) {
- pci_read_config_word(hw->pdev,
- pcie_cap + PCI_EXP_DEVCTL2, &val);
+ if (pci_is_pcie(hw->pdev)) {
+ pcie_capability_read_word(hw->pdev, PCI_EXP_DEVCTL2, &val);
val &= 0xfff0;
val |= range ;
- pci_write_config_word(hw->pdev,
- pcie_cap + PCI_EXP_DEVCTL2, val);
+ pcie_capability_write_word(hw->pdev, PCI_EXP_DEVCTL2, val);
}
}

--
1.7.1

2013-09-03 07:36:08

by Yijing Wang

[permalink] [raw]
Subject: [PATCH 7/7] scsi/qla2xxx: use pcie_is_pcie() to simplify code

Use pci_is_pcie() instead of pci_find_capability
to simplify code.

Signed-off-by: Yijing Wang <[email protected]>
Cc: Andrew Vasquez <[email protected]>
Cc: [email protected]
Cc: "James E.J. Bottomley" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/scsi/qla2xxx/qla_mr.c | 6 ++----
1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c
index d799379..5f74271 100644
--- a/drivers/scsi/qla2xxx/qla_mr.c
+++ b/drivers/scsi/qla2xxx/qla_mr.c
@@ -507,7 +507,7 @@ qlafx00_pci_config(scsi_qla_host_t *vha)
pci_write_config_word(ha->pdev, PCI_COMMAND, w);

/* PCIe -- adjust Maximum Read Request Size (2048). */
- if (pci_find_capability(ha->pdev, PCI_CAP_ID_EXP))
+ if (pci_is_pcie(ha->pdev))
pcie_set_readrq(ha->pdev, 2048);

ha->chip_revision = ha->pdev->revision;
@@ -660,10 +660,8 @@ char *
qlafx00_pci_info_str(struct scsi_qla_host *vha, char *str)
{
struct qla_hw_data *ha = vha->hw;
- int pcie_reg;

- pcie_reg = pci_find_capability(ha->pdev, PCI_CAP_ID_EXP);
- if (pcie_reg) {
+ if (pci_is_pcie(ha->pdev)) {
strcpy(str, "PCIe iSA");
return str;
}
--
1.7.1

2013-09-03 07:36:20

by Yijing Wang

[permalink] [raw]
Subject: [PATCH 5/7] ixgbe: use pcie_capability_read_word() to simplify code

use pcie_capability_read_word() to simplify code.

Signed-off-by: Yijing Wang <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++----
1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index bad8f14..bfa0b06 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -152,7 +152,6 @@ MODULE_VERSION(DRV_VERSION);
static int ixgbe_read_pci_cfg_word_parent(struct ixgbe_adapter *adapter,
u32 reg, u16 *value)
{
- int pos = 0;
struct pci_dev *parent_dev;
struct pci_bus *parent_bus;

@@ -164,11 +163,10 @@ static int ixgbe_read_pci_cfg_word_parent(struct ixgbe_adapter *adapter,
if (!parent_dev)
return -1;

- pos = pci_find_capability(parent_dev, PCI_CAP_ID_EXP);
- if (!pos)
+ if (!pci_is_pcie(parent_dev))
return -1;

- pci_read_config_word(parent_dev, pos + reg, value);
+ pcie_capability_read_word(parent_dev, reg, value);
return 0;
}

--
1.7.1

2013-09-03 07:36:06

by Yijing Wang

[permalink] [raw]
Subject: [PATCH 6/7] PCI: use pci_is_pcie() to simplify code

Use pci_is_pcie() instead of pci_find_capability
to simplify code.

Signed-off-by: Yijing Wang <[email protected]>
---
drivers/pci/probe.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index eeb50bd..0fa9075 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -641,8 +641,7 @@ static void pci_set_bus_speed(struct pci_bus *bus)
return;
}

- pos = pci_find_capability(bridge, PCI_CAP_ID_EXP);
- if (pos) {
+ if (pci_is_pcie(bridge)) {
u32 linkcap;
u16 linksta;

--
1.7.1

2013-09-03 07:36:52

by Yijing Wang

[permalink] [raw]
Subject: [PATCH 3/7] powerpc/pci: use pci_is_pcie() to simplify code

Use pci_is_pcie() to simplify code.

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

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 39954fe..b0bd41a 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

2013-09-03 07:38:10

by Yijing Wang

[permalink] [raw]
Subject: [PATCH 4/7] x86/pci: use pcie_cap to simplify code

PCI core saves PCIe Cap offset in pcie_cap,
use pcie_cap to simplify code.

Signed-off-by: Yijing Wang <[email protected]>
---
arch/x86/pci/fixup.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index f5809fa..ee8330d 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -288,7 +288,7 @@ static void pcie_rootport_aspm_quirk(struct pci_dev *pdev)
*/
list_for_each_entry(dev, &pbus->devices, bus_list) {
/* There are 0 to 8 devices attached to this bus */
- cap_base = pci_find_capability(dev, PCI_CAP_ID_EXP);
+ cap_base = dev->pcie_cap;
quirk_aspm_offset[GET_INDEX(pdev->device, dev->devfn)] = cap_base + 0x10;
}
pbus->ops = &quirk_pcie_aspm_ops;
--
1.7.1

2013-09-03 20:18:48

by Chad Dupuis

[permalink] [raw]
Subject: Re: [PATCH 7/7] scsi/qla2xxx: use pcie_is_pcie() to simplify code

Looks good.

Acked-by: Chad Dupuis <[email protected]>

On Tue, 3 Sep 2013, Yijing Wang wrote:

> Use pci_is_pcie() instead of pci_find_capability
> to simplify code.
>
> Signed-off-by: Yijing Wang <[email protected]>
> Cc: Andrew Vasquez <[email protected]>
> Cc: [email protected]
> Cc: "James E.J. Bottomley" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/scsi/qla2xxx/qla_mr.c | 6 ++----
> 1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c
> index d799379..5f74271 100644
> --- a/drivers/scsi/qla2xxx/qla_mr.c
> +++ b/drivers/scsi/qla2xxx/qla_mr.c
> @@ -507,7 +507,7 @@ qlafx00_pci_config(scsi_qla_host_t *vha)
> pci_write_config_word(ha->pdev, PCI_COMMAND, w);
>
> /* PCIe -- adjust Maximum Read Request Size (2048). */
> - if (pci_find_capability(ha->pdev, PCI_CAP_ID_EXP))
> + if (pci_is_pcie(ha->pdev))
> pcie_set_readrq(ha->pdev, 2048);
>
> ha->chip_revision = ha->pdev->revision;
> @@ -660,10 +660,8 @@ char *
> qlafx00_pci_info_str(struct scsi_qla_host *vha, char *str)
> {
> struct qla_hw_data *ha = vha->hw;
> - int pcie_reg;
>
> - pcie_reg = pci_find_capability(ha->pdev, PCI_CAP_ID_EXP);
> - if (pcie_reg) {
> + if (pci_is_pcie(ha->pdev)) {
> strcpy(str, "PCIe iSA");
> return str;
> }
>

2013-09-03 23:34:46

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH 1/7] scsi/bfa: use pcie_capability_xxx to simplify code

On Tue, Sep 03, 2013 at 03:35:09PM +0800, Yijing Wang wrote:
> Pcie_capability_xxx() interfaces were introudced to
> simplify code to access PCIe Cap config space. And
> because PCI core saves the PCIe Cap offset in
> set_pcie_port_type() when device is enumerated.
> So we can use pci_is_pcie() instead.
>
> Signed-off-by: Yijing Wang <[email protected]>
> Cc: Jiang Liu <[email protected]>
> Cc: Anil Gurumurthy <[email protected]>
> Cc: Vijaya Mohan Guvva <[email protected]>
> Cc: "James E.J. Bottomley" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/scsi/bfa/bfad.c | 9 +++------
> 1 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/bfa/bfad.c b/drivers/scsi/bfa/bfad.c
> index 9611195..d726b81 100644
> --- a/drivers/scsi/bfa/bfad.c
> +++ b/drivers/scsi/bfa/bfad.c
> @@ -767,7 +767,6 @@ bfad_pci_init(struct pci_dev *pdev, struct bfad_s *bfad)
>
> /* Adjust PCIe Maximum Read Request Size */
> if (pcie_max_read_reqsz > 0) {
> - int pcie_cap_reg;
> u16 pcie_dev_ctl;
> u16 mask = 0xffff;
>
> @@ -794,10 +793,8 @@ bfad_pci_init(struct pci_dev *pdev, struct bfad_s *bfad)
> break;
> }
>
> - pcie_cap_reg = pci_find_capability(pdev, PCI_CAP_ID_EXP);
> - if (mask != 0xffff && pcie_cap_reg) {
> - pcie_cap_reg += 0x08;
> - pci_read_config_word(pdev, pcie_cap_reg, &pcie_dev_ctl);
> + if (mask != 0xffff && pci_is_pcie(pdev)) {

Please move the pci_is_pcie() test up to the
"if (pcie_mas_read_reqsz ..." statement. There's no point in doing
the switch statement if this isn't a PCIe device.

> + pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &pcie_dev_ctl);
> if ((pcie_dev_ctl & 0x7000) != mask) {
> printk(KERN_WARNING "BFA[%s]: "
> "pcie_max_read_request_size is %d, "
> @@ -806,7 +803,7 @@ bfad_pci_init(struct pci_dev *pdev, struct bfad_s *bfad)
> pcie_max_read_reqsz);
>
> pcie_dev_ctl &= ~0x7000;
> - pci_write_config_word(pdev, pcie_cap_reg,
> + pcie_capability_write_word(pdev, PCI_EXP_DEVCTL,
> pcie_dev_ctl | mask);

Please rework this to use pcie_set_readrq() instead of writing
the capability directly. If we write the capability directly, we
risk writing a value that is incompatible with the MPS
configuration done by the PCI core.

> }
> }
> --
> 1.7.1
>
>

2013-09-03 23:43:36

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH 2/7] scsi/csiostor: use pcie_capability_xxx to simplify code

On Tue, Sep 03, 2013 at 03:35:10PM +0800, Yijing Wang wrote:
> Pcie_capability_xxx() interfaces were introudced to

s/introudced/introduced/

> simplify code to access PCIe Cap config space. And
> because PCI core saves the PCIe Cap offset in
> set_pcie_port_type() when device is enumerated.
> So we can use pci_is_pcie() instead.
>
> Signed-off-by: Yijing Wang <[email protected]>
> Cc: Jiang Liu <[email protected]>
> Cc: "James E.J. Bottomley" <[email protected]>
> Cc: Naresh Kumar Inna <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Jesper Juhl <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/scsi/csiostor/csio_hw.c | 9 +++------
> 1 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/csiostor/csio_hw.c b/drivers/scsi/csiostor/csio_hw.c
> index 0eb35b9..be9a6ef 100644
> --- a/drivers/scsi/csiostor/csio_hw.c
> +++ b/drivers/scsi/csiostor/csio_hw.c
> @@ -856,15 +856,12 @@ static void
> csio_set_pcie_completion_timeout(struct csio_hw *hw, u8 range)
> {
> uint16_t val;
> - int pcie_cap;
>
> - if (!csio_pci_capability(hw->pdev, PCI_CAP_ID_EXP, &pcie_cap)) {
> - pci_read_config_word(hw->pdev,
> - pcie_cap + PCI_EXP_DEVCTL2, &val);
> + if (pci_is_pcie(hw->pdev)) {
> + pcie_capability_read_word(hw->pdev, PCI_EXP_DEVCTL2, &val);
> val &= 0xfff0;
> val |= range ;
> - pci_write_config_word(hw->pdev,
> - pcie_cap + PCI_EXP_DEVCTL2, val);
> + pcie_capability_write_word(hw->pdev, PCI_EXP_DEVCTL2, val);

Please add a #define for the Completion Timeout Value field and use
pcie_capability_clear_and_set_word() instead of writing it out the
long way here.

> }
> }
>
> --
> 1.7.1
>
>

2013-09-04 02:38:17

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH 1/7] scsi/bfa: use pcie_capability_xxx to simplify code

On Tue, Sep 3, 2013 at 5:34 PM, Bjorn Helgaas <[email protected]> wrote:
> On Tue, Sep 03, 2013 at 03:35:09PM +0800, Yijing Wang wrote:
>> Pcie_capability_xxx() interfaces were introudced to
>> simplify code to access PCIe Cap config space. And
>> because PCI core saves the PCIe Cap offset in
>> set_pcie_port_type() when device is enumerated.
>> So we can use pci_is_pcie() instead.
>>
>> Signed-off-by: Yijing Wang <[email protected]>
>> Cc: Jiang Liu <[email protected]>
>> Cc: Anil Gurumurthy <[email protected]>
>> Cc: Vijaya Mohan Guvva <[email protected]>
>> Cc: "James E.J. Bottomley" <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> drivers/scsi/bfa/bfad.c | 9 +++------
>> 1 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/scsi/bfa/bfad.c b/drivers/scsi/bfa/bfad.c
>> index 9611195..d726b81 100644
>> --- a/drivers/scsi/bfa/bfad.c
>> +++ b/drivers/scsi/bfa/bfad.c
>> @@ -767,7 +767,6 @@ bfad_pci_init(struct pci_dev *pdev, struct bfad_s *bfad)
>>
>> /* Adjust PCIe Maximum Read Request Size */
>> if (pcie_max_read_reqsz > 0) {
>> - int pcie_cap_reg;
>> u16 pcie_dev_ctl;
>> u16 mask = 0xffff;
>>
>> @@ -794,10 +793,8 @@ bfad_pci_init(struct pci_dev *pdev, struct bfad_s *bfad)
>> break;
>> }
>>
>> - pcie_cap_reg = pci_find_capability(pdev, PCI_CAP_ID_EXP);
>> - if (mask != 0xffff && pcie_cap_reg) {
>> - pcie_cap_reg += 0x08;
>> - pci_read_config_word(pdev, pcie_cap_reg, &pcie_dev_ctl);
>> + if (mask != 0xffff && pci_is_pcie(pdev)) {
>
> Please move the pci_is_pcie() test up to the
> "if (pcie_mas_read_reqsz ..." statement. There's no point in doing
> the switch statement if this isn't a PCIe device.
>
>> + pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &pcie_dev_ctl);
>> if ((pcie_dev_ctl & 0x7000) != mask) {

I forgot to mention that this 0x7000 constant should be
PCI_EXP_DEVCTL_READRQ instead.

>> printk(KERN_WARNING "BFA[%s]: "
>> "pcie_max_read_request_size is %d, "
>> @@ -806,7 +803,7 @@ bfad_pci_init(struct pci_dev *pdev, struct bfad_s *bfad)
>> pcie_max_read_reqsz);
>>
>> pcie_dev_ctl &= ~0x7000;
>> - pci_write_config_word(pdev, pcie_cap_reg,
>> + pcie_capability_write_word(pdev, PCI_EXP_DEVCTL,
>> pcie_dev_ctl | mask);
>
> Please rework this to use pcie_set_readrq() instead of writing
> the capability directly. If we write the capability directly, we
> risk writing a value that is incompatible with the MPS
> configuration done by the PCI core.
>
>> }
>> }
>> --
>> 1.7.1
>>
>>

2013-09-04 02:59:46

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH 4/7] x86/pci: use pcie_cap to simplify code

On Tue, Sep 03, 2013 at 03:35:12PM +0800, Yijing Wang wrote:
> PCI core saves PCIe Cap offset in pcie_cap,
> use pcie_cap to simplify code.
>
> Signed-off-by: Yijing Wang <[email protected]>
> ---
> arch/x86/pci/fixup.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index f5809fa..ee8330d 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -288,7 +288,7 @@ static void pcie_rootport_aspm_quirk(struct pci_dev *pdev)
> */
> list_for_each_entry(dev, &pbus->devices, bus_list) {
> /* There are 0 to 8 devices attached to this bus */
> - cap_base = pci_find_capability(dev, PCI_CAP_ID_EXP);
> + cap_base = dev->pcie_cap;
> quirk_aspm_offset[GET_INDEX(pdev->device, dev->devfn)] = cap_base + 0x10;

This should use PCI_EXP_LNKCTL instead of "0x10".

> }
> pbus->ops = &quirk_pcie_aspm_ops;

This quirk replaces the config accessors with ones that silently ignore
writes to ASPM control bits. That really warrants at least a dev_info()
note here, and we should be using pci_bus_set_ops().

Even that is a little bit dubious because I don't think this is really
safe -- what happens if this quirk replaces the ops, then somebody
else replaces the ops again? aer_inject.c at least keeps track of
the old ops and seems to fall back to them, but it seems fragile to
depend on every caller of pci_bus_set_ops() to do the right thing
there.

But this is beyond the scope of your patch, so if you just
add a dev_info() note and use pci_bus_set_ops(), that should be
enough for now.

2013-09-04 09:26:49

by Jeff Kirsher

[permalink] [raw]
Subject: Re: [E1000-devel] [PATCH 5/7] ixgbe: use pcie_capability_read_word() to simplify code

On Tue, 2013-09-03 at 15:35 +0800, Yijing Wang wrote:
> use pcie_capability_read_word() to simplify code.
>
> Signed-off-by: Yijing Wang <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++----
> 1 files changed, 2 insertions(+), 4 deletions(-)

Thanks Yijing, I will add it to my queue.

Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2013-09-04 16:20:29

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH 5/7] ixgbe: use pcie_capability_read_word() to simplify code

[+cc Jacob, Jeff]

On Tue, Sep 03, 2013 at 03:35:13PM +0800, Yijing Wang wrote:
> use pcie_capability_read_word() to simplify code.
>
> Signed-off-by: Yijing Wang <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++----
> 1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index bad8f14..bfa0b06 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -152,7 +152,6 @@ MODULE_VERSION(DRV_VERSION);
> static int ixgbe_read_pci_cfg_word_parent(struct ixgbe_adapter *adapter,
> u32 reg, u16 *value)
> {
> - int pos = 0;
> struct pci_dev *parent_dev;
> struct pci_bus *parent_bus;
>
> @@ -164,11 +163,10 @@ static int ixgbe_read_pci_cfg_word_parent(struct ixgbe_adapter *adapter,
> if (!parent_dev)
> return -1;
>
> - pos = pci_find_capability(parent_dev, PCI_CAP_ID_EXP);
> - if (!pos)
> + if (!pci_is_pcie(parent_dev))
> return -1;
>
> - pci_read_config_word(parent_dev, pos + reg, value);
> + pcie_capability_read_word(parent_dev, reg, value);
> return 0;
> }
>

Here's the caller of ixgbe_read_pci_cfg_word_parent():

/* Get the negotiated link width and speed from PCI config space of the
* parent, as this device is behind a switch
*/
err = ixgbe_read_pci_cfg_word_parent(adapter, 18, &link_status);

This should be using PCI_EXP_LNKSTA instead of "18".

But it would be even better if we could drop ixgbe_get_parent_bus_info()
completely. It seems redundant after merging Jacob's new
pcie_get_minimum_link() stuff [1].

ixgbe_disable_pcie_master() looks like it should be using
pcie_capability_read_word() with PCI_EXP_DEVSTA instead of using
IXGBE_PCI_DEVICE_STATUS. If fact, it looks like it could use the
new pci_wait_for_pending_transaction() interface [2].

It looks like all the #defines in the "PCI Bus Info" block
(IXGBE_PCI_DEVICE_STATUS, IXGBE_PCI_DEVICE_STATUS_TRANSACTION_PENDING,
IXGBE_PCI_LINK_STATUS, etc.) [3] are really for PCIe-generic things. If
so, the IXGBE-specific ones should be dropped in favor of the generic
ones.

[1] http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c?id=e027d1aec4bb49030646d2c186a721f94372d7f2
[2] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pci/pci.c?id=3775a209d38aa3a0c7ed89a7d0f529e0230f280e
[3] http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h#n1833

2013-09-04 17:24:43

by Jacob Keller

[permalink] [raw]
Subject: Re: [PATCH 5/7] ixgbe: use pcie_capability_read_word() to simplify code

On Wed, 2013-09-04 at 10:20 -0600, Bjorn Helgaas wrote:
> [+cc Jacob, Jeff]
>
> On Tue, Sep 03, 2013 at 03:35:13PM +0800, Yijing Wang wrote:
> > use pcie_capability_read_word() to simplify code.
> >
> > Signed-off-by: Yijing Wang <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++----
> > 1 files changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > index bad8f14..bfa0b06 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > @@ -152,7 +152,6 @@ MODULE_VERSION(DRV_VERSION);
> > static int ixgbe_read_pci_cfg_word_parent(struct ixgbe_adapter *adapter,
> > u32 reg, u16 *value)
> > {
> > - int pos = 0;
> > struct pci_dev *parent_dev;
> > struct pci_bus *parent_bus;
> >
> > @@ -164,11 +163,10 @@ static int ixgbe_read_pci_cfg_word_parent(struct ixgbe_adapter *adapter,
> > if (!parent_dev)
> > return -1;
> >
> > - pos = pci_find_capability(parent_dev, PCI_CAP_ID_EXP);
> > - if (!pos)
> > + if (!pci_is_pcie(parent_dev))
> > return -1;
> >
> > - pci_read_config_word(parent_dev, pos + reg, value);
> > + pcie_capability_read_word(parent_dev, reg, value);
> > return 0;
> > }
> >
>
> Here's the caller of ixgbe_read_pci_cfg_word_parent():
>
> /* Get the negotiated link width and speed from PCI config space of the
> * parent, as this device is behind a switch
> */
> err = ixgbe_read_pci_cfg_word_parent(adapter, 18, &link_status);
>
> This should be using PCI_EXP_LNKSTA instead of "18".

Absolutely.. Not sure why I didn't do this originally.....

>
> But it would be even better if we could drop ixgbe_get_parent_bus_info()
> completely. It seems redundant after merging Jacob's new
> pcie_get_minimum_link() stuff [1].

I don't know if we can fully drop it. We need this in order to read the
parent device on some quad port Ethernet adapters which have an internal
PCIe switch to link two parts together. There are a few places we read
the parent cfg word. At least one for sure.. but maybe others.. Can't
recall. The parent bus info is still used to print out the slot width
and speed. I don't know if it would be better to just print the response
from get_minimum_link or still print the actual slot.

>
> ixgbe_disable_pcie_master() looks like it should be using
> pcie_capability_read_word() with PCI_EXP_DEVSTA instead of using
> IXGBE_PCI_DEVICE_STATUS. If fact, it looks like it could use the
> new pci_wait_for_pending_transaction() interface [2].
>

I can look at doing this. I know it was done this way for historic
reasons, (and likely for code share with non Linux drivers.. but that's
not really an excuse)

> It looks like all the #defines in the "PCI Bus Info" block
> (IXGBE_PCI_DEVICE_STATUS, IXGBE_PCI_DEVICE_STATUS_TRANSACTION_PENDING,
> IXGBE_PCI_LINK_STATUS, etc.) [3] are really for PCIe-generic things. If
> so, the IXGBE-specific ones should be dropped in favor of the generic
> ones.

Agreed.

Thanks,
Jake


????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2013-09-04 21:10:32

by Kumar Gala

[permalink] [raw]
Subject: Re: [PATCH 3/7] powerpc/pci: use pci_is_pcie() to simplify code


On Sep 3, 2013, at 2:35 AM, Yijing Wang wrote:

> Use pci_is_pcie() to simplify code.
>
> Signed-off-by: Yijing Wang <[email protected]>
> Cc: Gavin Shan <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> arch/powerpc/kernel/eeh.c | 3 +--
> arch/powerpc/sysdev/fsl_pci.c | 2 +-
> 2 files changed, 2 insertions(+), 3 deletions(-)

Acked-by: Kumar Gala <[email protected]>

(for the fsl_pci.c) change

- k

2013-09-05 06:34:53

by Yijing Wang

[permalink] [raw]
Subject: Re: [PATCH 4/7] x86/pci: use pcie_cap to simplify code

On 2013/9/4 10:59, Bjorn Helgaas wrote:
> On Tue, Sep 03, 2013 at 03:35:12PM +0800, Yijing Wang wrote:
>> PCI core saves PCIe Cap offset in pcie_cap,
>> use pcie_cap to simplify code.
>>
>> Signed-off-by: Yijing Wang <[email protected]>
>> ---
>> arch/x86/pci/fixup.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
>> index f5809fa..ee8330d 100644
>> --- a/arch/x86/pci/fixup.c
>> +++ b/arch/x86/pci/fixup.c
>> @@ -288,7 +288,7 @@ static void pcie_rootport_aspm_quirk(struct pci_dev *pdev)
>> */
>> list_for_each_entry(dev, &pbus->devices, bus_list) {
>> /* There are 0 to 8 devices attached to this bus */
>> - cap_base = pci_find_capability(dev, PCI_CAP_ID_EXP);
>> + cap_base = dev->pcie_cap;
>> quirk_aspm_offset[GET_INDEX(pdev->device, dev->devfn)] = cap_base + 0x10;
>
> This should use PCI_EXP_LNKCTL instead of "0x10".
>
>> }
>> pbus->ops = &quirk_pcie_aspm_ops;

Hi Bjorn,
Thanks for your review and comments!

>
> This quirk replaces the config accessors with ones that silently ignore
> writes to ASPM control bits. That really warrants at least a dev_info()
> note here, and we should be using pci_bus_set_ops().

Good idea, I will update it, thanks!

>
> Even that is a little bit dubious because I don't think this is really
> safe -- what happens if this quirk replaces the ops, then somebody
> else replaces the ops again? aer_inject.c at least keeps track of
> the old ops and seems to fall back to them, but it seems fragile to
> depend on every caller of pci_bus_set_ops() to do the right thing
> there.
>
> But this is beyond the scope of your patch, so if you just
> add a dev_info() note and use pci_bus_set_ops(), that should be
> enough for now.
>
>


--
Thanks!
Yijing

2013-09-05 07:22:35

by Yijing Wang

[permalink] [raw]
Subject: Re: [PATCH 1/7] scsi/bfa: use pcie_capability_xxx to simplify code

>> @@ -794,10 +793,8 @@ bfad_pci_init(struct pci_dev *pdev, struct bfad_s *bfad)
>> break;
>> }
>>
>> - pcie_cap_reg = pci_find_capability(pdev, PCI_CAP_ID_EXP);
>> - if (mask != 0xffff && pcie_cap_reg) {
>> - pcie_cap_reg += 0x08;
>> - pci_read_config_word(pdev, pcie_cap_reg, &pcie_dev_ctl);
>> + if (mask != 0xffff && pci_is_pcie(pdev)) {
>
> Please move the pci_is_pcie() test up to the
> "if (pcie_mas_read_reqsz ..." statement. There's no point in doing
> the switch statement if this isn't a PCIe device.

Right, will update.

>
>> + pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &pcie_dev_ctl);
>> if ((pcie_dev_ctl & 0x7000) != mask) {
>> printk(KERN_WARNING "BFA[%s]: "
>> "pcie_max_read_request_size is %d, "
>> @@ -806,7 +803,7 @@ bfad_pci_init(struct pci_dev *pdev, struct bfad_s *bfad)
>> pcie_max_read_reqsz);
>>
>> pcie_dev_ctl &= ~0x7000;
>> - pci_write_config_word(pdev, pcie_cap_reg,
>> + pcie_capability_write_word(pdev, PCI_EXP_DEVCTL,
>> pcie_dev_ctl | mask);
>
> Please rework this to use pcie_set_readrq() instead of writing
> the capability directly. If we write the capability directly, we
> risk writing a value that is incompatible with the MPS
> configuration done by the PCI core.

Ah, this code is Long-winded, use pcie_set_readrq()/pcie_get_readrq() can simplify
this code much.

Thanks!
Yijing.

>
>> }
>> }
>> --
>> 1.7.1
>>
>>
>
> .
>


--
Thanks!
Yijing

2013-09-05 07:38:08

by Yijing Wang

[permalink] [raw]
Subject: Re: [PATCH 2/7] scsi/csiostor: use pcie_capability_xxx to simplify code

On 2013/9/4 7:43, Bjorn Helgaas wrote:
> On Tue, Sep 03, 2013 at 03:35:10PM +0800, Yijing Wang wrote:
>> Pcie_capability_xxx() interfaces were introudced to
>
> s/introudced/introduced/

Will update it.

>
>> simplify code to access PCIe Cap config space. And
>> because PCI core saves the PCIe Cap offset in
>> set_pcie_port_type() when device is enumerated.
>> So we can use pci_is_pcie() instead.
>>
>> Signed-off-by: Yijing Wang <[email protected]>
>> Cc: Jiang Liu <[email protected]>
>> Cc: "James E.J. Bottomley" <[email protected]>
>> Cc: Naresh Kumar Inna <[email protected]>
>> Cc: "David S. Miller" <[email protected]>
>> Cc: Jesper Juhl <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> drivers/scsi/csiostor/csio_hw.c | 9 +++------
>> 1 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/scsi/csiostor/csio_hw.c b/drivers/scsi/csiostor/csio_hw.c
>> index 0eb35b9..be9a6ef 100644
>> --- a/drivers/scsi/csiostor/csio_hw.c
>> +++ b/drivers/scsi/csiostor/csio_hw.c
>> @@ -856,15 +856,12 @@ static void
>> csio_set_pcie_completion_timeout(struct csio_hw *hw, u8 range)
>> {
>> uint16_t val;
>> - int pcie_cap;
>>
>> - if (!csio_pci_capability(hw->pdev, PCI_CAP_ID_EXP, &pcie_cap)) {
>> - pci_read_config_word(hw->pdev,
>> - pcie_cap + PCI_EXP_DEVCTL2, &val);
>> + if (pci_is_pcie(hw->pdev)) {
>> + pcie_capability_read_word(hw->pdev, PCI_EXP_DEVCTL2, &val);
>> val &= 0xfff0;
>> val |= range ;
>> - pci_write_config_word(hw->pdev,
>> - pcie_cap + PCI_EXP_DEVCTL2, val);
>> + pcie_capability_write_word(hw->pdev, PCI_EXP_DEVCTL2, val);
>
> Please add a #define for the Completion Timeout Value field and use
> pcie_capability_clear_and_set_word() instead of writing it out the
> long way here.

Will update it, thanks!

>
>> }
>> }
>>
>> --
>> 1.7.1
>>
>>
>
> .
>


--
Thanks!
Yijing