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 <[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 | 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
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 <[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 | 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
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 <[email protected]>
---
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
Use pci_is_pcie() to simplify code.
Acked-by: Kumar Gala <[email protected]>
Reviewed-by: Gavin Shan <[email protected]>
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 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
Use pci_is_pcie() instead of pci_find_capability
to simplify code.
Acked-by: Chad Dupuis <[email protected]>
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 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
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
On Thu, Sep 5, 2013 at 12:55 AM, Yijing Wang <[email protected]> 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 <[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 | 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 05, 2013 at 03:55:27PM +0800, Yijing Wang wrote:
> Use pci_is_pcie() to simplify code.
>
> Acked-by: Kumar Gala <[email protected]>
> Reviewed-by: Gavin Shan <[email protected]>
> 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(-)
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 <[email protected]>
> 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 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 <[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 | 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: [email protected] # 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/[email protected]
> 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
>
>
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 <[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 | 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
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 <[email protected]>
> > Reviewed-by: Gavin Shan <[email protected]>
> > 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(-)
>
> 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 <[email protected]>
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 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 <[email protected]>
>>> Reviewed-by: Gavin Shan <[email protected]>
>>> 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(-)
>>
>> 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 <[email protected]>
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
> .
>
--
Thanks!
Yijing
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 <[email protected]>
>>>> Reviewed-by: Gavin Shan <[email protected]>
>>>> 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(-)
>>>
>>> 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 <[email protected]>
>>
>> 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
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
>> 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