diff mbox series

[v2,04/10] drm/IB/hfi1: Use RMW accessors for changing LNKCTL2

Message ID 20230915120142.32987-5-ilpo.jarvinen@linux.intel.com (mailing list archive)
State Superseded, archived
Headers show
Series Add PCIe Bandwidth Controller | expand

Commit Message

Ilpo Järvinen Sept. 15, 2023, 12:01 p.m. UTC
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(-)

Comments

Dean Luick Sept. 15, 2023, 1:51 p.m. UTC | #1
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
Ilpo Järvinen Sept. 15, 2023, 2:02 p.m. UTC | #2
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 mbox series

Patch

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;
 	}