Re: [PATCH 3/3] RDMA/hfi1: Use RMW accessors for changing LNKCTL2
On Fri, May 03, 2024 at 10:04:16AM -0300, Jason Gunthorpe wrote: > On Fri, May 03, 2024 at 01:18:35PM +0300, Ilpo Järvinen wrote: > > On Thu, 15 Feb 2024, Ilpo Järvinen 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. > > > > > > 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. > > > > > > Suggested-by: Lukas Wunner > > > Signed-off-by: Ilpo Järvinen > > > Reviewed-by: Dean Luick > > > > I found out from Linux RDMA and InfiniBand patchwork that this patch had > > been silently closed as "Not Applicable". Is there some reason for > > that? > > It is part of a series that crosses subsystems, series like that > usually go through some other trees. Exactly, this is why I marked it as "Not Applicable". > > If you want single patches applied then please send single > patches.. It is hard to understand intent from mixed series. > > Jason
Re: [PATCH 3/3] RDMA/hfi1: Use RMW accessors for changing LNKCTL2
On Fri, May 03, 2024 at 01:18:35PM +0300, Ilpo Järvinen wrote: > On Thu, 15 Feb 2024, Ilpo Järvinen 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. > > > > 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. > > > > Suggested-by: Lukas Wunner > > Signed-off-by: Ilpo Järvinen > > Reviewed-by: Dean Luick > > I found out from Linux RDMA and InfiniBand patchwork that this patch had > been silently closed as "Not Applicable". Is there some reason for > that? It is part of a series that crosses subsystems, series like that usually go through some other trees. If you want single patches applied then please send single patches.. It is hard to understand intent from mixed series. Jason
Re: [PATCH 3/3] RDMA/hfi1: Use RMW accessors for changing LNKCTL2
On Thu, 15 Feb 2024, Ilpo Järvinen 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.
>
> 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.
>
> Suggested-by: Lukas Wunner
> Signed-off-by: Ilpo Järvinen
> Reviewed-by: Dean Luick
I found out from Linux RDMA and InfiniBand patchwork that this patch had
been silently closed as "Not Applicable". Is there some reason for that?
I was sending this change independently out (among 2 similar ones that
already got applied) so I wouldn't need to keep carrying it within my PCIe
bandwidth controller series. It seemed useful enough as cleanups to stand
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
as a part of PCIe BW controller series?
--
i.
> ---
> 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 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)
> (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;
> }
> @@ -1223,22 +1220,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, );
> + 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;
> }
>
[PATCH 3/3] RDMA/hfi1: Use RMW accessors for changing LNKCTL2
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.
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.
Suggested-by: Lukas Wunner
Signed-off-by: Ilpo Järvinen
Reviewed-by: Dean Luick
---
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 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)
(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;
}
@@ -1223,22 +1220,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, );
+ 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;
}
--
2.39.2
