| Message ID | 20230915120142.32987-5-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Add PCIe Bandwidth Controller | expand |
On 9/15/2023 7:01 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> I believe the subject should begin with "RDMA/hfi1:", the current expectation for all devices in drivers/infiniband. > --- > 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..60a177f52eb5 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 PCI target speed\n"); To differentiate from the dev_err below, add "parent", i.e. "Unable to change parent PCI target speed". > 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 PCI target speed\n"); To differentiate from the dev_err above, add "device", i.e. "Unable to change device PCI target speed". > return_error = 1; > goto done; > } External recipient
On Fri, 15 Sep 2023, Dean Luick wrote: > On 9/15/2023 7:01 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> > > I believe the subject should begin with "RDMA/hfi1:", the current expectation for all devices in drivers/infiniband. Thanks, I'll update it. I hadn't realized given the shortlogs of the other commits (no idea where I managed to get that "drm" from, but it's also gone now). > > --- > > 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..60a177f52eb5 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 PCI target speed\n"); > > To differentiate from the dev_err below, add "parent", i.e. "Unable to change parent PCI target speed". > > > > 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 PCI target speed\n"); > > To differentiate from the dev_err above, add "device", i.e. "Unable to change device PCI target speed". Okay, I'll change those. Thanks for taking a look.
diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c index 08732e1ac966..60a177f52eb5 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 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 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(-)