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