This looks correct to me. One of the folks actively working on the Intel networking drivers should take a look to make sure they're aware of it.

Update ice driver code

Update ice driver review to address feedback

It looks like similar work was done via r355942, with slightly different (better) function names, and various style changes. This can be closed as essentially resolved/committed now.

Fix typo of local variable name

Add missing break statement

Refactor check as suggested by review comments

I mean ixl_if_priv_ioctl(). I'm not sure what you mean by "that" ioctl, since ixl_if_priv_ioctl() handles multiple ioctls by virtue of not looking at the command argument. In particular, iflib passes driver-specific ioctls to ixl_if_priv_ioctl() and so would not seem to be the right place to check for privileges for exactly the reason that you state.

I put the check directly in ixl_handle_nvmupd_cmd, similar to the check that we have in the ice driver code.

In D22523#495245, @dougm wrote:

With the attached simple test program, compiled with -O3, the results without and with DM defined are:

In D22523#495149, @erj wrote:

If it helps @dougm, the maximum size we'd typically use for the bitstrings would be 2048 bits, with us maybe using 16384-bit ones in the future.

Hmm. I think I understand the change.. but it seems less intuitive.

Hah, thanks for fixing!

Update test cases per review feedback

fix test cases for bit_ffc_at

dereference _result properly before trying to assign it

add test cases to help cover the expected behavior

Updated the review to include additional test cases and fixed the manual page.

Update review based on feedback from Alan Somers

Ok, will add tests and fix the comments.

In D22398#490688, @asomers wrote:

Would you mind adding tests to tests/sys/sys/bitstring_test.c?

We use this implementation in the ice driver (current review at https://reviews.freebsd.org/D21959, will be updated to reflect moving the function here soon).

You should be able to verify this leak by doing something like:

I'm really not sure this is the correct fix. Possibly we should just add an ifa_free after the rtrequest1_fib()?

I found this while debugging an extra reference on the ifa refcount. I think I found a "solution" that isn't quite right, but this comment is misleading.

Remove ETHER_IS_ZERO definitions from USB networking drivers.

In D22203#486049, @erj wrote:

@jacob.e.keller_intel.com, is this ready to be committed?

We're in agreement that the refactor is a better solution, so I'm going to abandon this one.

add an IFNET_WLOCK_ASSERT in if_freegroup

I think the refactored version to avoid code duplication is probably a better fix. If you go with this one, perhaps remove the whitespace changes and only commit the added free().

Also, yay.. yet another memory leak fixed while playing whack-a-mole trying to fix leaks during driver load/unload testing. I think we still have at least one more on M_IFADDR somewhere.

This is likely the better approach to solving the leak described in https://reviews.freebsd.org/D22215

This is the simpler straight forward fix to the same issue described in https://reviews.freebsd.org/D22214

In D22203#485241, @gallatin wrote:

Thanks for fixing this. The NULL canaries were left over from when iflib had a separate code path which avoided busdma. When I removed this codepath, I neglected to remove those canaries.

The fact that the map pointer can be NULL is weird, but we found this to be true in several places, and without the removal of the NULL checks we end up leaking ~200 bytes of M_DEVBUF, and associated other DMA memory allocated outside of malloc.

We should be able to MFC this to 12-STABLE and 11-STABLE. Since the issue has existed since the beginning of iflib in-tree, I don't really think it's worth trying to rush it into 12.1... although it is a relatively small fix.

Updated the review to add vlan event handler unregister calls before iflib_stop as well. I kept the call in iflib_deregister as well so that they get cleaned up properly during error flows. Now it uses the "unregister -> assign NULL" pattern so that only one of the flows will actually perform an unregister check.

also move VLAN event handlers

In D22071#482228, @jacob.e.keller_intel.com wrote:

I did manage to eventually trigger another stale ifp pointer, though it was not reliable:

Kernel page fault with the following non-sleepable locks held:
exclusive sleep mutex ip6qlock (ip6qlock) r = 0 (0xfffffe00007aa848) locked @ /usr/src/sys/netinet6/frag6.c:849
shared rw vnet_rwlock (vnet_rwlock) r = 0 (0xffffffff820be700) locked @ /usr/src/sys/netinet6/frag6.c:845
stack backtrace:
#0 0xffffffff80bb6f83 at witness_debugger+0x73
#1 0xffffffff80bb7fa2 at witness_warn+0x442
#2 0xffffffff8108a0f3 at trap_pfault+0x53
#3 0xffffffff810896e4 at trap+0x2b4
#4 0xffffffff8106201c at calltrap+0x8
#5 0xffffffff80d8c07a at icmp6_error+0x4aa
#6 0xffffffff80d8b30e at frag6_freef+0x10e
#7 0xffffffff80d8b551 at frag6_slowtimo+0x111
#8 0xffffffff80bdcda4 at pfslowtimo+0x54
#9 0xffffffff80b65bdf at softclock_call_cc+0x13f
#10 0xffffffff80b65f9c at softclock+0x7c
#11 0xffffffff80b0f857 at ithread_loop+0x187
#12 0xffffffff80b0c4a4 at fork_exit+0x84
#13 0xffffffff8106305e at fork_trampoline+0xe


Fatal trap 12: page fault while in kernel mode
cpuid = 0; apic id = 00
fault virtual address   = 0xfffffe0000825dd8
fault code              = supervisor read data, page not present
instruction pointer     = 0x20:0xffffffff80d8c5b2
stack pointer           = 0x28:0xfffffe1fc28c6ff0
frame pointer           = 0x28:0xfffffe1fc28c7090
code segment            = base 0x0, limit 0xfffff, type 0x1b
                        = DPL 0, pres 1, long 1, def32 0, gran 1
processor eflags        = interrupt enabled, resume, IOPL = 0
current process         = 12 (swi4: clock (0))
trap number             = 12
panic: page fault
cpuid = 0
time = 1571354026
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe1fc28c6cb0
vpanic() at vpanic+0x19d/frame 0xfffffe1fc28c6d00
panic() at panic+0x43/frame 0xfffffe1fc28c6d60
trap_fatal() at trap_fatal+0x39c/frame 0xfffffe1fc28c6dc0
trap_pfault() at trap_pfault+0x62/frame 0xfffffe1fc28c6e10
trap() at trap+0x2b4/frame 0xfffffe1fc28c6f20
calltrap() at calltrap+0x8/frame 0xfffffe1fc28c6f20
--- trap 0xc, rip = 0xffffffff80d8c5b2, rsp = 0xfffffe1fc28c6ff0, rbp = 0xfffffe1fc28c7090 ---
icmp6_reflect() at icmp6_reflect+0x242/frame 0xfffffe1fc28c7090
icmp6_error() at icmp6_error+0x4aa/frame 0xfffffe1fc28c70e0
frag6_freef() at frag6_freef+0x10e/frame 0xfffffe1fc28c7130
frag6_slowtimo() at frag6_slowtimo+0x111/frame 0xfffffe1fc28c7180
pfslowtimo() at pfslowtimo+0x54/frame 0xfffffe1fc28c71b0
softclock_call_cc() at softclock_call_cc+0x13f/frame 0xfffffe1fc28c7260
softclock() at softclock+0x7c/frame 0xfffffe1fc28c7290
ithread_loop() at ithread_loop+0x187/frame 0xfffffe1fc28c72f0
fork_exit() at fork_exit+0x84/frame 0xfffffe1fc28c7330
fork_trampoline() at fork_trampoline+0xe/frame 0xfffffe1fc28c7330
--- trap 0, rip = 0, rsp = 0, rbp = 0 ---
KDB: enter: panic

Assuming this fix makes sense, this should be MFC'd to 11-STABLE and 12-STABLE too, IMHO.

I did manage to eventually trigger another stale ifp pointer, though it was not reliable:

In D21959#480017, @rpokala wrote:

Don't forget a manpage. :-)

In D21959#479856, @gallatin wrote:

I just looked for a short time.. more comments later..

Use the correct register value for the received pause frames

I agree with Eric, we should depend on the device to be configured properly, and push to get the images for the device updated if they're wrong.

This is specifically a fix for 11-STABLE. It's already fixed in CURRENT and STABLE-12. It looks like it was an accidental miss in MFC for STABLE-11 a few months ago.

In D21240#462131, @aleksandr.fedorov_itglobal.com wrote:

It’s seems I found another incorrect solution:

#include <stdio.h>

int main()
{

unsigned char a = (1 << 0) ||(1<<1)||(1 <<2)||(1<<3)||(1<<4)||(1<<5);
printf("momentum more : %d\n", a);

return 0;

}

Use bitwise OR instead of bitwise AND to correct implementation

Move the kobject refcount removal to a separate patch

In D21005#456595, @erj wrote:

In D21005#455734, @jacob.e.keller_intel.com wrote:

This is built on top of https://reviews.freebsd.org/D21004

It could probably be backported, but isn't as necessary as the previous patch which is why I kept them separate.

Do you mean https://reviews.freebsd.org/D21003?

This is built on top of https://reviews.freebsd.org/D21003

We discovered this as part of our rename from ixlv to iavf.