irdma: Convert to IfAPI

[jhibbits]

Summary

Mostly mechanical changes, with some reworking in irdma_cm for iterating
over interfaces and addresses.

[bartosz.sobczak_intel.com]

some of the changes need to be back-ported to 13 and 12 OS, to not diverge too much from the core code.
The cm code is shared with linux driver, so i am going to check if we can take advantage of that.

sys/dev/irdma/irdma_cm.c

1680

name doesn't match the function name

[bartosz.sobczak_intel.com]

The changes may be applied to most recent code for OOT releases, so there is not much reason to hold back on that part.
Please fix the function description marked previously.

There's also a review set up for irdma upgrade to 1.1.11-k (https://reviews.freebsd.org/D39173)
it would be better for me, if this change would wait until 1.1.11-k would be accepted, although i understand it would get complicated either way.

sys/dev/irdma/irdma_cm.c

1747

name doesn't match the actual function

[jhibbits]

@bartosz.sobczak_intel.com I'm find holding off. When D39173 is committed I'll rebase on that and rework to also use the new iterator API which is cleaner than the callback API.

[bartosz.sobczak_intel.com]

d38960_2.diff30 KBDownload

Hi!
Please take a look at the attached file.
This is a rebased change onto recently upgraded irdma driver (now 1.1.11-k), with additional proposals of changes.
Please take a look and let me know if that looks good to you.

I'm not sure how (and if i can) do direct changes onto this review.

The change wasn't tested yet.

[jhibbits]

Rebase, address feedback.

@bartosz.sobczak_intel.com I can't view your file, it's marked as "Restricted file", but I imagine it's something like this change, with maybe some extras you added in.

[jhibbits]

Hi @bartosz.sobczak_intel.com I just checked, and the diffs are identical. I have no way of testing it, I've only compiled it for all targets.

[bartosz.sobczak_intel.com]

In D38960#899373, @jhibbits wrote:

Hi @bartosz.sobczak_intel.com I just checked, and the diffs are identical. I have no way of testing it, I've only compiled it for all targets.

Hi @jhibbits,
Yes, the core of changes matches. There are a few improvements though. For instance, i don't see the need to have irdma_add_mqh_4_ifa and irdma_add_mqh_6_ifa separate and there are additional changes in icrdma.c that we could apply.
I've been working on it yesterday, but i couldn't get the compilation checked yet. I'll provide the additional changes as soon as i will be able to.

[jhibbits]

Ah I see now. Not sure what I actually downloaded to check yesterday, now I got the right diff to compare.

[bartosz.sobczak_intel.com]

i still haven't had a chance to test this, because from what i can see the commit 3e142e07675be6d breaks iwarp for E810 devices. i'm trying to comprehend why this happens.

[jhibbits]

In D38960#902466, @bartosz.sobczak_intel.com wrote:

i still haven't had a chance to test this, because from what i can see the commit 3e142e07675be6d breaks iwarp for E810 devices. i'm trying to comprehend why this happens.

Crap. It should've been a purely mechanical change, with no functional changes. Unfortunately I have no way of testing. @hselasky are you able to test and debug? I hope I don't need to revert, I want to get D39621 in before the branch in May.

[bartosz.sobczak_intel.com]

In D38960#902485, @jhibbits wrote:

In D38960#902466, @bartosz.sobczak_intel.com wrote:

i still haven't had a chance to test this, because from what i can see the commit 3e142e07675be6d breaks iwarp for E810 devices. i'm trying to comprehend why this happens.

Crap. It should've been a purely mechanical change, with no functional changes. Unfortunately I have no way of testing. @hselasky are you able to test and debug? I hope I don't need to revert, I want to get D39621 in before the branch in May.

I know! right?
Ok, i think i found the change:

@@ -176,13 +176,13 @@ static inline int rdma_addr_gid_offset(struct rdma_dev_addr *dev_addr)
 	return dev_addr->dev_type == ARPHRD_INFINIBAND ? 4 : 0;
 }
 
-static inline u16 rdma_vlan_dev_vlan_id(const struct ifnet *dev)
+static inline u16 rdma_vlan_dev_vlan_id(if_t dev)
 {
 	uint16_t tag;
 
-	if (dev->if_type == IFT_ETHER && dev->if_pcp != IFNET_PCP_NONE)
+	if (if_gettype(dev) != IFT_ETHER || if_getpcp(dev) == IFNET_PCP_NONE)
 		return 0x0000;	/* prio-tagged traffic */
-	if (VLAN_TAG(__DECONST(struct ifnet *, dev), &tag) != 0)
+	if (VLAN_TAG(__DECONST(if_t, dev), &tag) != 0)
 		return 0xffff;
 	return tag;
 }

So we have a change from:
A && !B
into:
!A || B

Do you know why this change? It clearly is a negative of what was before. The effect is that when no vlan is set, we get vlan_id 0 instead of 0xffff, and this is used by irdma driver.

[zlei]

In D38960#903021, @bartosz.sobczak_intel.com wrote:

In D38960#902485, @jhibbits wrote:

In D38960#902466, @bartosz.sobczak_intel.com wrote:

i still haven't had a chance to test this, because from what i can see the commit 3e142e07675be6d breaks iwarp for E810 devices. i'm trying to comprehend why this happens.

Crap. It should've been a purely mechanical change, with no functional changes. Unfortunately I have no way of testing. @hselasky are you able to test and debug? I hope I don't need to revert, I want to get D39621 in before the branch in May.

I know! right?
Ok, i think i found the change:

@@ -176,13 +176,13 @@ static inline int rdma_addr_gid_offset(struct rdma_dev_addr *dev_addr)
 	return dev_addr->dev_type == ARPHRD_INFINIBAND ? 4 : 0;
 }
 
-static inline u16 rdma_vlan_dev_vlan_id(const struct ifnet *dev)
+static inline u16 rdma_vlan_dev_vlan_id(if_t dev)
 {
 	uint16_t tag;
 
-	if (dev->if_type == IFT_ETHER && dev->if_pcp != IFNET_PCP_NONE)
+	if (if_gettype(dev) != IFT_ETHER || if_getpcp(dev) == IFNET_PCP_NONE)

@jhibbits @hselasky
I believe this is wrong for a mechanically convert.

		return 0x0000;	/* prio-tagged traffic */

• if (VLAN_TAG(__DECONST(struct ifnet *, dev), &tag) != 0)

+ if (VLAN_TAG(__DECONST(if_t, dev), &tag) != 0)

		return 0xffff;
	return tag;

}

So we have a change from:
A && !B
into:
!A || B

Do you know why this change? It clearly is a negative of what was before. The effect is that when no vlan is set, we get vlan_id 0 instead of 0xffff, and this is used by irdma driver.

[jhibbits]

In D38960#903021, @bartosz.sobczak_intel.com wrote:

In D38960#902485, @jhibbits wrote:

In D38960#902466, @bartosz.sobczak_intel.com wrote:

i still haven't had a chance to test this, because from what i can see the commit 3e142e07675be6d breaks iwarp for E810 devices. i'm trying to comprehend why this happens.

Crap. It should've been a purely mechanical change, with no functional changes. Unfortunately I have no way of testing. @hselasky are you able to test and debug? I hope I don't need to revert, I want to get D39621 in before the branch in May.

I know! right?
Ok, i think i found the change:

@@ -176,13 +176,13 @@ static inline int rdma_addr_gid_offset(struct rdma_dev_addr *dev_addr)
 	return dev_addr->dev_type == ARPHRD_INFINIBAND ? 4 : 0;
 }
 
-static inline u16 rdma_vlan_dev_vlan_id(const struct ifnet *dev)
+static inline u16 rdma_vlan_dev_vlan_id(if_t dev)
 {
 	uint16_t tag;
 
-	if (dev->if_type == IFT_ETHER && dev->if_pcp != IFNET_PCP_NONE)
+	if (if_gettype(dev) != IFT_ETHER || if_getpcp(dev) == IFNET_PCP_NONE)
 		return 0x0000;	/* prio-tagged traffic */
-	if (VLAN_TAG(__DECONST(struct ifnet *, dev), &tag) != 0)
+	if (VLAN_TAG(__DECONST(if_t, dev), &tag) != 0)
 		return 0xffff;
 	return tag;
 }

So we have a change from:
A && !B
into:
!A || B

Do you know why this change? It clearly is a negative of what was before. The effect is that when no vlan is set, we get vlan_id 0 instead of 0xffff, and this is used by irdma driver.

Now that you point it out, it's clearly wrong. I'll fix it immediately. No idea how that got changed like that.

[jhibbits]

Hi @bartosz.sobczak_intel.com the ofed fix has been committed as 12e99b63d2fa, thanks!

[bartosz.sobczak_intel.com]

d38960_3.diff30 KBDownload

Thanks for bearing with me @jhibbits !

I'm attaching current proposal which i rebased on top of 14-cur today and run quickly rping on roce/iwarp and it seems fine.

[jhibbits]

In D38960#904247, @bartosz.sobczak_intel.com wrote:

d38960_3.diff30 KBDownload

Thanks for bearing with me @jhibbits !

I'm attaching current proposal which i rebased on top of 14-cur today and run quickly rping on roce/iwarp and it seems fine.

Not sure what phabricator is doing, but I'm unable to see your diff. But if it works, you can commandeer this change (take it over) and update the diff.

[jhibbits]

Rebase and use @bartosz.sobczak_intel.com 's diff instead.

[bartosz.sobczak_intel.com]

Thank you @jhibbits i started a testing automation on our side to double check.
Unfortunately i'm going out for holidays, so i won't be able to report on that. I'll ask someone to cover for me during this time, and to let you know if the testing went good.

Thanks!
Bartek

[mateusz.moga_intel.com]

In D38960#904739, @bartosz.sobczak_intel.com wrote:

Thank you @jhibbits i started a testing automation on our side to double check.
Unfortunately i'm going out for holidays, so i won't be able to report on that. I'll ask someone to cover for me during this time, and to let you know if the testing went good.

Thanks!
Bartek

Hello @jhibbits , green light from validation. thanks

[jhibbits]

@mateusz.moga_intel.com Thanks for validation, I'll push it today.