Received-SPF: pass (google.com: domain of linux-kernel+bounces-167512-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Fri, 3 May 2024 13:18:35 +0300 (EEST)
To: =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
cc: Alex Deucher <alexander.deucher@amd.com>, amd-gfx@lists.freedesktop.org, 
    Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@gmail.com>, 
    Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>, 
    dri-devel@lists.freedesktop.org, Jason Gunthorpe <jgg@ziepe.ca>, 
    Leon Romanovsky <leon@kernel.org>, LKML <linux-kernel@vger.kernel.org>, 
    linux-rdma@vger.kernel.org, "Pan, Xinhui" <Xinhui.Pan@amd.com>, 
    =?ISO-8859-15?Q?Christian_K=F6nig?= <christian.koenig@amd.com>, 
    Lukas Wunner <lukas@wunner.de>, 
    Dean Luick <dean.luick@cornelisnetworks.com>
Subject: Re: [PATCH 3/3] RDMA/hfi1: Use RMW accessors for changing LNKCTL2
In-Reply-To: <20240215133155.9198-4-ilpo.jarvinen@linux.intel.com>
Message-ID: <26be3948-e687-f510-0612-abcac5d919af@linux.intel.com>
References: <20240215133155.9198-1-ilpo.jarvinen@linux.intel.com> <20240215133155.9198-4-ilpo.jarvinen@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323328-818284186-1714731515=:1852"

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-818284186-1714731515=:1852
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: QUOTED-PRINTABLE

On Thu, 15 Feb 2024, Ilpo J=C3=A4rvinen wrote:

> Convert open coded RMW accesses for LNKCTL2 to use
> pcie_capability_clear_and_set_word() which makes its easier to
> understand what the code tries to do.
>=20
> LNKCTL2 is not really owned by any driver because it is a collection of
> control bits that PCI core might need to touch. RMW accessors already
> have support for proper locking for a selected set of registers
> (LNKCTL2 is not yet among them but likely will be in the future) to
> avoid losing concurrent updates.
>=20
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Ilpo J=C3=A4rvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: Dean Luick <dean.luick@cornelisnetworks.com>

I found out from Linux RDMA and InfiniBand patchwork that this patch had=20
been silently closed as "Not Applicable". Is there some reason for that?

I was sending this change independently out (among 2 similar ones that=20
already got applied) so I wouldn't need to keep carrying it within my PCIe=
=20
bandwidth controller series. It seemed useful enough as cleanups to stand=
=20
on its own legs w/o requiring it to be part of PCIe bw controller series.

Should I resend the patch or do RDMA/IB maintainers prefer it to remain=20
as a part of PCIe BW controller series?

--=20
 i.

> ---
>  drivers/infiniband/hw/hfi1/pcie.c | 30 ++++++++----------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)
>=20
> diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hf=
i1/pcie.c
> index 119ec2f1382b..7133964749f8 100644
> --- a/drivers/infiniband/hw/hfi1/pcie.c
> +++ b/drivers/infiniband/hw/hfi1/pcie.c
> @@ -1207,14 +1207,11 @@ int do_pcie_gen3_transition(struct hfi1_devdata *=
dd)
>  =09=09    (u32)lnkctl2);
>  =09/* only write to parent if target is not as high as ours */
>  =09if ((lnkctl2 & PCI_EXP_LNKCTL2_TLS) < target_vector) {
> -=09=09lnkctl2 &=3D ~PCI_EXP_LNKCTL2_TLS;
> -=09=09lnkctl2 |=3D target_vector;
> -=09=09dd_dev_info(dd, "%s: ..new link control2: 0x%x\n", __func__,
> -=09=09=09    (u32)lnkctl2);
> -=09=09ret =3D pcie_capability_write_word(parent,
> -=09=09=09=09=09=09 PCI_EXP_LNKCTL2, lnkctl2);
> +=09=09ret =3D pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL2=
,
> +=09=09=09=09=09=09=09 PCI_EXP_LNKCTL2_TLS,
> +=09=09=09=09=09=09=09 target_vector);
>  =09=09if (ret) {
> -=09=09=09dd_dev_err(dd, "Unable to write to PCI config\n");
> +=09=09=09dd_dev_err(dd, "Unable to change parent PCI target speed\n");
>  =09=09=09return_error =3D 1;
>  =09=09=09goto done;
>  =09=09}
> @@ -1223,22 +1220,11 @@ int do_pcie_gen3_transition(struct hfi1_devdata *=
dd)
>  =09}
> =20
>  =09dd_dev_info(dd, "%s: setting target link speed\n", __func__);
> -=09ret =3D pcie_capability_read_word(dd->pcidev, PCI_EXP_LNKCTL2, &lnkct=
l2);
> +=09ret =3D pcie_capability_clear_and_set_word(dd->pcidev, PCI_EXP_LNKCTL=
2,
> +=09=09=09=09=09=09 PCI_EXP_LNKCTL2_TLS,
> +=09=09=09=09=09=09 target_vector);
>  =09if (ret) {
> -=09=09dd_dev_err(dd, "Unable to read from PCI config\n");
> -=09=09return_error =3D 1;
> -=09=09goto done;
> -=09}
> -
> -=09dd_dev_info(dd, "%s: ..old link control2: 0x%x\n", __func__,
> -=09=09    (u32)lnkctl2);
> -=09lnkctl2 &=3D ~PCI_EXP_LNKCTL2_TLS;
> -=09lnkctl2 |=3D target_vector;
> -=09dd_dev_info(dd, "%s: ..new link control2: 0x%x\n", __func__,
> -=09=09    (u32)lnkctl2);
> -=09ret =3D pcie_capability_write_word(dd->pcidev, PCI_EXP_LNKCTL2, lnkct=
l2);
> -=09if (ret) {
> -=09=09dd_dev_err(dd, "Unable to write to PCI config\n");
> +=09=09dd_dev_err(dd, "Unable to change device PCI target speed\n");
>  =09=09return_error =3D 1;
>  =09=09goto done;
>  =09}
>=20
--8323328-818284186-1714731515=:1852--