[PATCH v2 1/6] scsi/bfa: use pcie_set/get_readrq to simplify code

[PATCH v2 1/6] scsi/bfa: use pcie_set/get_readrq to simplify code

[Yijing Wang]

v1->v2: use pcie_get/set_readrq to simplify code
a lot suggestd by Bjorn.

Use pcie_get_readrq()/pcie_set_readrq() to simplify
code.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Jiang Liu <jiang.liu@huawei.com>
Cc: Anil Gurumurthy <agurumur@brocade.com>
Cc: Vijaya Mohan Guvva <vmohan@brocade.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/scsi/bfa/bfad.c |   48 +++++-----------------------------------------
 1 files changed, 6 insertions(+), 42 deletions(-)

diff --git a/drivers/scsi/bfa/bfad.c b/drivers/scsi/bfa/bfad.c
index f8ca7be..0a458db 100644
--- a/drivers/scsi/bfa/bfad.c
+++ b/drivers/scsi/bfa/bfad.c
@@ -766,50 +766,14 @@ bfad_pci_init(struct pci_dev *pdev, struct bfad_s *bfad)
 	bfad->pcidev = pdev;
 
 	/* Adjust PCIe Maximum Read Request Size */
-	if (pcie_max_read_reqsz > 0) {
-		int pcie_cap_reg;
-		u16 pcie_dev_ctl;
-		u16 mask = 0xffff;
-
-		switch (pcie_max_read_reqsz) {
-		case 128:
-			mask = 0x0;
-			break;
-		case 256:
-			mask = 0x1000;
-			break;
-		case 512:
-			mask = 0x2000;
-			break;
-		case 1024:
-			mask = 0x3000;
-			break;
-		case 2048:
-			mask = 0x4000;
-			break;
-		case 4096:
-			mask = 0x5000;
-			break;
-		default:
-			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 ((pcie_dev_ctl & 0x7000) != mask) {
-				printk(KERN_WARNING "BFA[%s]: "
+	if (pcie_max_read_reqsz > 0 && pci_is_pcie(pdev)) {
+		int max_rq = pcie_get_readrq(pdev);
+		if (max_rq > 128 && max_rq < 4096 && is_power_of_2(max_rq))
+			printk(KERN_WARNING "BFA[%s]: "
 				"pcie_max_read_request_size is %d, "
-				"reset to %d\n", bfad->pci_name,
-				(1 << ((pcie_dev_ctl & 0x7000) >> 12)) << 7,
+				"reset to %d\n", bfad->pci_name, max_rq,
 				pcie_max_read_reqsz);
-
-				pcie_dev_ctl &= ~0x7000;
-				pci_write_config_word(pdev, pcie_cap_reg,
-						pcie_dev_ctl | mask);
-			}
-		}
+		pcie_set_readrq(pdev, pcie_max_read_reqsz);
 	}
 
 	pci_save_state(pdev);
-- 
1.7.1

[PATCH v2 2/6] scsi/csiostor: use pcie_capability_xxx to simplify code

[Yijing Wang]

v1->v2: add #define for Completion Timeout Value, and use
        pcie_capability_clear_and_set_word() instead suggested by Bjorn.

Pcie_capability_xxx() interfaces were introduced 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 <wangyijing@huawei.com>
Cc: Jiang Liu <jiang.liu@huawei.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Naresh Kumar Inna <naresh@chelsio.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jesper Juhl <jj@chaosbits.net>
Cc: linux-scsi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/scsi/csiostor/csio_hw.c |   14 +++-----------
 include/uapi/linux/pci_regs.h   |    1 +
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/csiostor/csio_hw.c b/drivers/scsi/csiostor/csio_hw.c
index 0eb35b9..07f493a 100644
--- a/drivers/scsi/csiostor/csio_hw.c
+++ b/drivers/scsi/csiostor/csio_hw.c
@@ -855,17 +855,9 @@ csio_hw_get_flash_params(struct csio_hw *hw)
 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);
-		val &= 0xfff0;
-		val |= range ;
-		pci_write_config_word(hw->pdev,
-				      pcie_cap + PCI_EXP_DEVCTL2, val);
-	}
+	if (pci_is_pcie(hw->pdev))
+		pcie_capability_clear_and_set_word(hw->pdev, PCI_EXP_DEVCTL2,
+				PCI_EXP_DEVCTL2_COMP_TIME, range);
 }
 
 /*****************************************************************************/
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index baa7852..cd74182 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -558,6 +558,7 @@
 #define  PCI_EXP_DEVCAP2_OBFF_MSG	0x00040000 /* New message signaling */
 #define  PCI_EXP_DEVCAP2_OBFF_WAKE	0x00080000 /* Re-use WAKE# for OBFF */
 #define PCI_EXP_DEVCTL2		40	/* Device Control 2 */
+#define  PCI_EXP_DEVCTL2_COMP_TIME	0x0f	/* Completion Timeout Value */
 #define  PCI_EXP_DEVCTL2_ARI		0x20	/* Alternative Routing-ID */
 #define  PCI_EXP_DEVCTL2_IDO_REQ_EN	0x0100	/* Allow IDO for requests */
 #define  PCI_EXP_DEVCTL2_IDO_CMP_EN	0x0200	/* Allow IDO for completions */
-- 
1.7.1

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

[Yijing Wang]

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

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

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

[Bjorn Helgaas]

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

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

[Benjamin Herrenschmidt]

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

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

[Yijing Wang]

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/
> 
> .
> 


-- 
Thanks!
Yijing

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

[Yijing Wang]

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
> 
> 
> .
> 


-- 
Thanks!
Yijing

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

[Yijing Wang]

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
> 
> 
> .
> 


-- 
Thanks!
Yijing

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

[Yijing Wang]

>> 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
> 
> 
> .
> 


-- 
Thanks!
Yijing

[PATCH v2 4/6] x86/pci: use pcie_cap to simplify code

[Yijing Wang]

v1->v2: use pci_bus_set_ops to replace pci_ops
        and add a dev_info() to notify user that
        the pci_ops was replaced suggested by Bjorn.

PCI core saves PCIe Cap offset in pcie_cap,
use pcie_cap to simplify code. Also we should
use pci_bus_set_ops() to replace pci_ops for
safety and add a dev_info to notify user that
the pci_ops was replaced.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 arch/x86/pci/fixup.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index f5809fa..edd7879 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -252,7 +252,7 @@ static struct pci_ops quirk_pcie_aspm_ops = {
  */
 static void pcie_rootport_aspm_quirk(struct pci_dev *pdev)
 {
-	int cap_base, i;
+	int i;
 	struct pci_bus  *pbus;
 	struct pci_dev *dev;
 
@@ -278,7 +278,7 @@ static void pcie_rootport_aspm_quirk(struct pci_dev *pdev)
 		for (i = GET_INDEX(pdev->device, 0); i <= GET_INDEX(pdev->device, 7); ++i)
 			quirk_aspm_offset[i] = 0;
 
-		pbus->ops = pbus->parent->ops;
+		pci_bus_set_ops(pbus, pbus->parent->ops);
 	} else {
 		/*
 		 * If devices are attached to the root port at power-up or
@@ -286,13 +286,16 @@ static void pcie_rootport_aspm_quirk(struct pci_dev *pdev)
 		 * each root port to save the register offsets and replace the
 		 * bus ops.
 		 */
-		list_for_each_entry(dev, &pbus->devices, bus_list) {
+		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);
-			quirk_aspm_offset[GET_INDEX(pdev->device, dev->devfn)] = cap_base + 0x10;
-		}
-		pbus->ops = &quirk_pcie_aspm_ops;
+			quirk_aspm_offset[GET_INDEX(pdev->device, dev->devfn)] =
+				dev->pcie_cap + PCI_EXP_LNKCTL;
+
+		pci_bus_set_ops(pbus, &quirk_pcie_aspm_ops);
+		dev_info(&pbus->dev, "pci_ops is replaced by quirk_pcie_aspm_ops, "
+				"any writes to ASPM control bits will be ignored.\n");
 	}
+
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_MCH_PA,	pcie_rootport_aspm_quirk);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_MCH_PA1,	pcie_rootport_aspm_quirk);
-- 
1.7.1

[PATCH v2 5/6] PCI: use pci_is_pcie() to simplify code

[Yijing Wang]

Use pci_is_pcie() instead of pci_find_capability
to simplify code.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 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

[PATCH v2 6/6] scsi/qla2xxx: use pcie_is_pcie() to simplify code

[Yijing Wang]

Use pci_is_pcie() instead of pci_find_capability
to simplify code.

Acked-by: Chad Dupuis <chad.dupuis@qlogic.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Andrew Vasquez <andrew.vasquez@qlogic.com>
Cc: linux-driver@qlogic.com
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 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 2482975..5494d27 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

Re: [PATCH v2 1/6] scsi/bfa: use pcie_set/get_readrq to simplify code

[Jon Mason]

On Thu, Sep 5, 2013 at 12:55 AM, Yijing Wang <wangyijing@huawei.com> wrote:
> v1->v2: use pcie_get/set_readrq to simplify code
> a lot suggestd by Bjorn.
>
> Use pcie_get_readrq()/pcie_set_readrq() to simplify
> code.

Very similar to a patch I sent out in 2011
http://www.spinics.net/lists/linux-scsi/msg52990.html

Hopefully this one gets pulled in.

> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> Cc: Jiang Liu <jiang.liu@huawei.com>
> Cc: Anil Gurumurthy <agurumur@brocade.com>
> Cc: Vijaya Mohan Guvva <vmohan@brocade.com>
> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
> Cc: linux-scsi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/scsi/bfa/bfad.c |   48 +++++-----------------------------------------
>  1 files changed, 6 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/scsi/bfa/bfad.c b/drivers/scsi/bfa/bfad.c
> index f8ca7be..0a458db 100644
> --- a/drivers/scsi/bfa/bfad.c
> +++ b/drivers/scsi/bfa/bfad.c
> @@ -766,50 +766,14 @@ bfad_pci_init(struct pci_dev *pdev, struct bfad_s *bfad)
>         bfad->pcidev = pdev;
>
>         /* Adjust PCIe Maximum Read Request Size */
> -       if (pcie_max_read_reqsz > 0) {
> -               int pcie_cap_reg;
> -               u16 pcie_dev_ctl;
> -               u16 mask = 0xffff;
> -
> -               switch (pcie_max_read_reqsz) {
> -               case 128:
> -                       mask = 0x0;
> -                       break;
> -               case 256:
> -                       mask = 0x1000;
> -                       break;
> -               case 512:
> -                       mask = 0x2000;
> -                       break;
> -               case 1024:
> -                       mask = 0x3000;
> -                       break;
> -               case 2048:
> -                       mask = 0x4000;
> -                       break;
> -               case 4096:
> -                       mask = 0x5000;
> -                       break;
> -               default:
> -                       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 ((pcie_dev_ctl & 0x7000) != mask) {
> -                               printk(KERN_WARNING "BFA[%s]: "
> +       if (pcie_max_read_reqsz > 0 && pci_is_pcie(pdev)) {
> +               int max_rq = pcie_get_readrq(pdev);
> +               if (max_rq > 128 && max_rq < 4096 && is_power_of_2(max_rq))
> +                       printk(KERN_WARNING "BFA[%s]: "
>                                 "pcie_max_read_request_size is %d, "
> -                               "reset to %d\n", bfad->pci_name,
> -                               (1 << ((pcie_dev_ctl & 0x7000) >> 12)) << 7,
> +                               "reset to %d\n", bfad->pci_name, max_rq,
>                                 pcie_max_read_reqsz);
> -
> -                               pcie_dev_ctl &= ~0x7000;
> -                               pci_write_config_word(pdev, pcie_cap_reg,
> -                                               pcie_dev_ctl | mask);
> -                       }
> -               }
> +               pcie_set_readrq(pdev, pcie_max_read_reqsz);
>         }
>
>         pci_save_state(pdev);
> --
> 1.7.1
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/6] scsi/bfa: use pcie_set/get_readrq to simplify code

[Bjorn Helgaas]

On Thu, Sep 05, 2013 at 03:55:25PM +0800, Yijing Wang wrote:
> v1->v2: use pcie_get/set_readrq to simplify code
> a lot suggestd by Bjorn.
> 
> Use pcie_get_readrq()/pcie_set_readrq() to simplify
> code.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> Cc: Jiang Liu <jiang.liu@huawei.com>
> Cc: Anil Gurumurthy <agurumur@brocade.com>
> Cc: Vijaya Mohan Guvva <vmohan@brocade.com>
> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
> Cc: linux-scsi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/scsi/bfa/bfad.c |   48 +++++-----------------------------------------
>  1 files changed, 6 insertions(+), 42 deletions(-)

I applied all these with some tweaks to my pci/yijing-pci_is_pcie-v2
branch [1].  This will be rebased after v3.12-rc1, and may be amended
if any patches are picked up by others.

Hints (not just for you; I hope other people pay attention, too,
because I'm obsessive and I pay attention to these details):

  - Include a "[PATCH v2 0/6]" email.  That's a good place for you to
    put an overall description of the series, and a good place for
    responses like this one that apply to the whole series.

  - Pay attention to the order of your patches.  Yours seemed random,
    and I reordered them so the core PCI ones are first and the arch
    and driver ones are later.  That way I can easily drop the later
    ones if they are picked up by other maintainers.

  - Don't put "v1->v2" comments in your changelogs.  Those are fine
    in the "[0/6]" email, but they're useless in the git changelog, and
    I strip them out when I see them.  Or you can put them after the
    "---" line, in which case they get stripped out automatically.

  - Run "git log --oneline" on the files you touch.  You should follow
    the existing convention, including spacing, brackets, capitalization,
    etc.  I changed most of your subject lines for this reason.

  - Write titles that are sentences, starting with a verb, as suggested
    by Ingo [2].  You did this already; I just made changes for
    consistency of capitalization and the like.

  - Use real function names, not things like "pcie_capability_xxx".
    That makes it easier to search logs.

  - Be consistent about writing function names.  Some of your logs
    included, e.g., both "pci_bus_set_ops" and "dev_info()".  I prefer
    to always include the parentheses when writing a function name,
    but at least be consistent.

  - Don't put "Cc: <mailing-list>" in your changelog.  That tag is
    useful to show that a *person* has had the opportunity to comment
    on a patch but declined to do so.  I don't think it's meaningful
    for mailing lists.  If it were, every single commit would have
    that tag, since every single commit should appear on the relevant
    list.  I suspect you probably do this so that something like
    "git send-email --signed-off-by-cc" will automatically send mail
    to the right lists.  But that's a one-time convenience at the
    cost of useless info in the changelog that's there forever.

  - Put Signed-off-by, Acked-by, etc., tags in this order as suggested
    by Ingo [2]:

      Reported-by:
      Tested-by:
      Signed-off-by:
      Acked-by:
      Reviewed-by:
      Cc: stable@vger.kernel.org  # v3.11+
      Cc: others

[1] http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/yijing-pci_is_pcie-v2

[2] http://lkml.kernel.org/r/20120711080446.GA17713@gmail.com

> diff --git a/drivers/scsi/bfa/bfad.c b/drivers/scsi/bfa/bfad.c
> index f8ca7be..0a458db 100644
> --- a/drivers/scsi/bfa/bfad.c
> +++ b/drivers/scsi/bfa/bfad.c
> @@ -766,50 +766,14 @@ bfad_pci_init(struct pci_dev *pdev, struct bfad_s *bfad)
>  	bfad->pcidev = pdev;
>  
>  	/* Adjust PCIe Maximum Read Request Size */
> -	if (pcie_max_read_reqsz > 0) {
> -		int pcie_cap_reg;
> -		u16 pcie_dev_ctl;
> -		u16 mask = 0xffff;
> -
> -		switch (pcie_max_read_reqsz) {
> -		case 128:
> -			mask = 0x0;
> -			break;
> -		case 256:
> -			mask = 0x1000;
> -			break;
> -		case 512:
> -			mask = 0x2000;
> -			break;
> -		case 1024:
> -			mask = 0x3000;
> -			break;
> -		case 2048:
> -			mask = 0x4000;
> -			break;
> -		case 4096:
> -			mask = 0x5000;
> -			break;
> -		default:
> -			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 ((pcie_dev_ctl & 0x7000) != mask) {
> -				printk(KERN_WARNING "BFA[%s]: "
> +	if (pcie_max_read_reqsz > 0 && pci_is_pcie(pdev)) {
> +		int max_rq = pcie_get_readrq(pdev);
> +		if (max_rq > 128 && max_rq < 4096 && is_power_of_2(max_rq))

I think you meant to validate pcie_max_read_reqsz (the module parameter),
not max_rq.  I made this change on my branch.

> +			printk(KERN_WARNING "BFA[%s]: "
>  				"pcie_max_read_request_size is %d, "
> -				"reset to %d\n", bfad->pci_name,
> -				(1 << ((pcie_dev_ctl & 0x7000) >> 12)) << 7,
> +				"reset to %d\n", bfad->pci_name, max_rq,
>  				pcie_max_read_reqsz);
> -
> -				pcie_dev_ctl &= ~0x7000;
> -				pci_write_config_word(pdev, pcie_cap_reg,
> -						pcie_dev_ctl | mask);
> -			}
> -		}
> +		pcie_set_readrq(pdev, pcie_max_read_reqsz);
>  	}
>  
>  	pci_save_state(pdev);
> -- 
> 1.7.1
> 
> 

Re: [PATCH v2 1/6] scsi/bfa: use pcie_set/get_readrq to simplify code

[Yijing Wang]

On 2013/9/7 6:14, Bjorn Helgaas wrote:
> On Thu, Sep 05, 2013 at 03:55:25PM +0800, Yijing Wang wrote:
>> v1->v2: use pcie_get/set_readrq to simplify code
>> a lot suggestd by Bjorn.
>>
>> Use pcie_get_readrq()/pcie_set_readrq() to simplify
>> code.
>>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> Cc: Jiang Liu <jiang.liu@huawei.com>
>> Cc: Anil Gurumurthy <agurumur@brocade.com>
>> Cc: Vijaya Mohan Guvva <vmohan@brocade.com>
>> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
>> Cc: linux-scsi@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>  drivers/scsi/bfa/bfad.c |   48 +++++-----------------------------------------
>>  1 files changed, 6 insertions(+), 42 deletions(-)

Hi Bjorn,
   Thanks for your patience guidance! Now I know more about how to write
a patch or patchset. I will check my patch follow the advices below before
I send out every time. Make it easier for you and other maintainers to
review and apply.

> 
> I applied all these with some tweaks to my pci/yijing-pci_is_pcie-v2
> branch [1].  This will be rebased after v3.12-rc1, and may be amended
> if any patches are picked up by others.
> 
> Hints (not just for you; I hope other people pay attention, too,
> because I'm obsessive and I pay attention to these details):
> 
>   - Include a "[PATCH v2 0/6]" email.  That's a good place for you to
>     put an overall description of the series, and a good place for
>     responses like this one that apply to the whole series.
> 
>> -
>> -		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 ((pcie_dev_ctl & 0x7000) != mask) {
>> -				printk(KERN_WARNING "BFA[%s]: "
>> +	if (pcie_max_read_reqsz > 0 && pci_is_pcie(pdev)) {
>> +		int max_rq = pcie_get_readrq(pdev);
>> +		if (max_rq > 128 && max_rq < 4096 && is_power_of_2(max_rq))
> 
> I think you meant to validate pcie_max_read_reqsz (the module parameter),
> not max_rq.  I made this change on my branch.

Yes, thanks for your fix.

Thanks!
Yijing.

> 
>> +			printk(KERN_WARNING "BFA[%s]: "
>>  				"pcie_max_read_request_size is %d, "
>> -				"reset to %d\n", bfad->pci_name,
>> -				(1 << ((pcie_dev_ctl & 0x7000) >> 12)) << 7,
>> +				"reset to %d\n", bfad->pci_name, max_rq,
>>  				pcie_max_read_reqsz);
>> -
>> -				pcie_dev_ctl &= ~0x7000;
>> -				pci_write_config_word(pdev, pcie_cap_reg,
>> -						pcie_dev_ctl | mask);
>> -			}
>> -		}
>> +		pcie_set_readrq(pdev, pcie_max_read_reqsz);
>>  	}
>>  
>>  	pci_save_state(pdev);
>> -- 
>> 1.7.1
>>
>>
> 
> .
> 


-- 
Thanks!
Yijing