| Message ID | 20230929115723.7864-5-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Add PCIe Bandwidth Controller | expand |
On 9/29/2023 6:57 AM, Ilpo Järvinen wrote: > Don't assume that only the driver would be accessing LNKCTL2. In the > case of upstream (parent), the driver does not even own the device it's > changing the registers for. > > Use RMW capability accessors which do proper locking to avoid losing > concurrent updates to the register value. This change is also useful as > a cleanup. > > Suggested-by: Lukas Wunner <lukas@wunner.de> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > drivers/infiniband/hw/hfi1/pcie.c | 30 ++++++++---------------------- > 1 file changed, 8 insertions(+), 22 deletions(-) > > diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c > index 08732e1ac966..4487a05bea04 100644 > --- a/drivers/infiniband/hw/hfi1/pcie.c > +++ b/drivers/infiniband/hw/hfi1/pcie.c > @@ -1212,14 +1212,11 @@ int do_pcie_gen3_transition(struct hfi1_devdata *dd) > (u32)lnkctl2); > /* only write to parent if target is not as high as ours */ > if ((lnkctl2 & PCI_EXP_LNKCTL2_TLS) < target_vector) { > - lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS; > - lnkctl2 |= target_vector; > - dd_dev_info(dd, "%s: ..new link control2: 0x%x\n", __func__, > - (u32)lnkctl2); > - ret = pcie_capability_write_word(parent, > - PCI_EXP_LNKCTL2, lnkctl2); > + ret = pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL2, > + PCI_EXP_LNKCTL2_TLS, > + target_vector); > if (ret) { > - dd_dev_err(dd, "Unable to write to PCI config\n"); > + dd_dev_err(dd, "Unable to change parent PCI target speed\n"); > return_error = 1; > goto done; > } > @@ -1228,22 +1225,11 @@ int do_pcie_gen3_transition(struct hfi1_devdata *dd) > } > > dd_dev_info(dd, "%s: setting target link speed\n", __func__); > - ret = pcie_capability_read_word(dd->pcidev, PCI_EXP_LNKCTL2, &lnkctl2); > + ret = pcie_capability_clear_and_set_word(dd->pcidev, PCI_EXP_LNKCTL2, > + PCI_EXP_LNKCTL2_TLS, > + target_vector); > if (ret) { > - dd_dev_err(dd, "Unable to read from PCI config\n"); > - return_error = 1; > - goto done; > - } > - > - dd_dev_info(dd, "%s: ..old link control2: 0x%x\n", __func__, > - (u32)lnkctl2); > - lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS; > - lnkctl2 |= target_vector; > - dd_dev_info(dd, "%s: ..new link control2: 0x%x\n", __func__, > - (u32)lnkctl2); > - ret = pcie_capability_write_word(dd->pcidev, PCI_EXP_LNKCTL2, lnkctl2); > - if (ret) { > - dd_dev_err(dd, "Unable to write to PCI config\n"); > + dd_dev_err(dd, "Unable to change device PCI target speed\n"); > return_error = 1; > goto done; > } Looks good. Thank you for my suggested changes. Reviewed-by: Dean Luick <dean.luick@cornelisnetworks.com> -Dean External recipient
diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c index 08732e1ac966..4487a05bea04 100644 --- a/drivers/infiniband/hw/hfi1/pcie.c +++ b/drivers/infiniband/hw/hfi1/pcie.c @@ -1212,14 +1212,11 @@ int do_pcie_gen3_transition(struct hfi1_devdata *dd) (u32)lnkctl2); /* only write to parent if target is not as high as ours */ if ((lnkctl2 & PCI_EXP_LNKCTL2_TLS) < target_vector) { - lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS; - lnkctl2 |= target_vector; - dd_dev_info(dd, "%s: ..new link control2: 0x%x\n", __func__, - (u32)lnkctl2); - ret = pcie_capability_write_word(parent, - PCI_EXP_LNKCTL2, lnkctl2); + ret = pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL2, + PCI_EXP_LNKCTL2_TLS, + target_vector); if (ret) { - dd_dev_err(dd, "Unable to write to PCI config\n"); + dd_dev_err(dd, "Unable to change parent PCI target speed\n"); return_error = 1; goto done; } @@ -1228,22 +1225,11 @@ int do_pcie_gen3_transition(struct hfi1_devdata *dd) } dd_dev_info(dd, "%s: setting target link speed\n", __func__); - ret = pcie_capability_read_word(dd->pcidev, PCI_EXP_LNKCTL2, &lnkctl2); + ret = pcie_capability_clear_and_set_word(dd->pcidev, PCI_EXP_LNKCTL2, + PCI_EXP_LNKCTL2_TLS, + target_vector); if (ret) { - dd_dev_err(dd, "Unable to read from PCI config\n"); - return_error = 1; - goto done; - } - - dd_dev_info(dd, "%s: ..old link control2: 0x%x\n", __func__, - (u32)lnkctl2); - lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS; - lnkctl2 |= target_vector; - dd_dev_info(dd, "%s: ..new link control2: 0x%x\n", __func__, - (u32)lnkctl2); - ret = pcie_capability_write_word(dd->pcidev, PCI_EXP_LNKCTL2, lnkctl2); - if (ret) { - dd_dev_err(dd, "Unable to write to PCI config\n"); + dd_dev_err(dd, "Unable to change device PCI target speed\n"); return_error = 1; goto done; }
Don't assume that only the driver would be accessing LNKCTL2. In the case of upstream (parent), the driver does not even own the device it's changing the registers for. Use RMW capability accessors which do proper locking to avoid losing concurrent updates to the register value. This change is also useful as a cleanup. Suggested-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- drivers/infiniband/hw/hfi1/pcie.c | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-)