| Message ID | 20230511131441.45704-7-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: Improve LNKCTL & LNKCTL2 concurrency control | expand |
On 5/11/2023 8:14 AM, Ilpo Järvinen wrote: > Don't assume that only the driver would be accessing LNKCTL/LNKCTL2. > ASPM policy changes can trigger write to LNKCTL outside of driver's > control. And in the case of upstream (parent), the driver does not even > own the device it's changing the registers for. > > Use pcie_lnkctl_clear_and_set() and pcie_lnkctl2_clear_and_set() which > do proper locking to avoid losing concurrent updates to the register > value. > > Suggested-by: Lukas Wunner <lukas@wunner.de> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > drivers/infiniband/hw/hfi1/aspm.c | 16 ++++++---------- > drivers/infiniband/hw/hfi1/pcie.c | 28 ++++++---------------------- > 2 files changed, 12 insertions(+), 32 deletions(-) > > diff --git a/drivers/infiniband/hw/hfi1/aspm.c b/drivers/infiniband/hw/hfi1/aspm.c > index a3c53be4072c..d3f3b7e9b833 100644 > --- a/drivers/infiniband/hw/hfi1/aspm.c > +++ b/drivers/infiniband/hw/hfi1/aspm.c > @@ -66,12 +66,10 @@ static void aspm_hw_enable_l1(struct hfi1_devdata *dd) > return; > > /* Enable ASPM L1 first in upstream component and then downstream */ > - pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL, > - PCI_EXP_LNKCTL_ASPMC, > - PCI_EXP_LNKCTL_ASPM_L1); > - pcie_capability_clear_and_set_word(dd->pcidev, PCI_EXP_LNKCTL, > - PCI_EXP_LNKCTL_ASPMC, > - PCI_EXP_LNKCTL_ASPM_L1); > + pcie_lnkctl_clear_and_set(parent, PCI_EXP_LNKCTL_ASPMC, > + PCI_EXP_LNKCTL_ASPM_L1); > + pcie_lnkctl_clear_and_set(dd->pcidev, PCI_EXP_LNKCTL_ASPMC, > + PCI_EXP_LNKCTL_ASPM_L1); > } > > void aspm_hw_disable_l1(struct hfi1_devdata *dd) > @@ -79,11 +77,9 @@ void aspm_hw_disable_l1(struct hfi1_devdata *dd) > struct pci_dev *parent = dd->pcidev->bus->self; > > /* Disable ASPM L1 first in downstream component and then upstream */ > - pcie_capability_clear_and_set_word(dd->pcidev, PCI_EXP_LNKCTL, > - PCI_EXP_LNKCTL_ASPMC, 0x0); > + pcie_lnkctl_clear_and_set(dd->pcidev, PCI_EXP_LNKCTL_ASPMC, 0); > if (parent) > - pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL, > - PCI_EXP_LNKCTL_ASPMC, 0x0); > + pcie_lnkctl_clear_and_set(parent, PCI_EXP_LNKCTL_ASPMC, 0); > } > > static void aspm_enable(struct hfi1_devdata *dd) > diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c > index 08732e1ac966..fe7324d38d64 100644 > --- a/drivers/infiniband/hw/hfi1/pcie.c > +++ b/drivers/infiniband/hw/hfi1/pcie.c > @@ -1212,14 +1212,10 @@ 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); > + pcie_lnkctl2_clear_and_set(parent, PCI_EXP_LNKCTL2_TLS, > + target_vector); You are missing an assignment to "ret" above. -Dean External recipient
On Thu, 11 May 2023, Dean Luick wrote: > On 5/11/2023 8:14 AM, Ilpo Järvinen wrote: > > Don't assume that only the driver would be accessing LNKCTL/LNKCTL2. > > ASPM policy changes can trigger write to LNKCTL outside of driver's > > control. And in the case of upstream (parent), the driver does not even > > own the device it's changing the registers for. > > > > Use pcie_lnkctl_clear_and_set() and pcie_lnkctl2_clear_and_set() which > > do proper locking to avoid losing concurrent updates to the register > > value. > > > > Suggested-by: Lukas Wunner <lukas@wunner.de> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > --- > > diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c > > index 08732e1ac966..fe7324d38d64 100644 > > --- a/drivers/infiniband/hw/hfi1/pcie.c > > +++ b/drivers/infiniband/hw/hfi1/pcie.c > > @@ -1212,14 +1212,10 @@ 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); > > + pcie_lnkctl2_clear_and_set(parent, PCI_EXP_LNKCTL2_TLS, > > + target_vector); > > You are missing an assignment to "ret" above. Thanks for noticing, I'll fix it in the next version.
diff --git a/drivers/infiniband/hw/hfi1/aspm.c b/drivers/infiniband/hw/hfi1/aspm.c index a3c53be4072c..d3f3b7e9b833 100644 --- a/drivers/infiniband/hw/hfi1/aspm.c +++ b/drivers/infiniband/hw/hfi1/aspm.c @@ -66,12 +66,10 @@ static void aspm_hw_enable_l1(struct hfi1_devdata *dd) return; /* Enable ASPM L1 first in upstream component and then downstream */ - pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL, - PCI_EXP_LNKCTL_ASPMC, - PCI_EXP_LNKCTL_ASPM_L1); - pcie_capability_clear_and_set_word(dd->pcidev, PCI_EXP_LNKCTL, - PCI_EXP_LNKCTL_ASPMC, - PCI_EXP_LNKCTL_ASPM_L1); + pcie_lnkctl_clear_and_set(parent, PCI_EXP_LNKCTL_ASPMC, + PCI_EXP_LNKCTL_ASPM_L1); + pcie_lnkctl_clear_and_set(dd->pcidev, PCI_EXP_LNKCTL_ASPMC, + PCI_EXP_LNKCTL_ASPM_L1); } void aspm_hw_disable_l1(struct hfi1_devdata *dd) @@ -79,11 +77,9 @@ void aspm_hw_disable_l1(struct hfi1_devdata *dd) struct pci_dev *parent = dd->pcidev->bus->self; /* Disable ASPM L1 first in downstream component and then upstream */ - pcie_capability_clear_and_set_word(dd->pcidev, PCI_EXP_LNKCTL, - PCI_EXP_LNKCTL_ASPMC, 0x0); + pcie_lnkctl_clear_and_set(dd->pcidev, PCI_EXP_LNKCTL_ASPMC, 0); if (parent) - pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL, - PCI_EXP_LNKCTL_ASPMC, 0x0); + pcie_lnkctl_clear_and_set(parent, PCI_EXP_LNKCTL_ASPMC, 0); } static void aspm_enable(struct hfi1_devdata *dd) diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c index 08732e1ac966..fe7324d38d64 100644 --- a/drivers/infiniband/hw/hfi1/pcie.c +++ b/drivers/infiniband/hw/hfi1/pcie.c @@ -1212,14 +1212,10 @@ 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); + pcie_lnkctl2_clear_and_set(parent, 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 +1224,10 @@ 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_lnkctl2_clear_and_set(dd->pcidev, 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 link control2\n"); return_error = 1; goto done; }
Don't assume that only the driver would be accessing LNKCTL/LNKCTL2. ASPM policy changes can trigger write to LNKCTL outside of driver's control. And in the case of upstream (parent), the driver does not even own the device it's changing the registers for. Use pcie_lnkctl_clear_and_set() and pcie_lnkctl2_clear_and_set() which do proper locking to avoid losing concurrent updates to the register value. Suggested-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- drivers/infiniband/hw/hfi1/aspm.c | 16 ++++++---------- drivers/infiniband/hw/hfi1/pcie.c | 28 ++++++---------------------- 2 files changed, 12 insertions(+), 32 deletions(-)