In D44827#1023217, @adrian wrote:

In D44827#1023209, @bz wrote:

In D44827#1023208, @adrian wrote:

eg ath(4) would do something like:

How do you want to implement ath_settkipmic() "nicely" without exposing the field?

oops my bad. it'll be a flag for hardware encryption support! I fixed the code snippet.

Also do you have any plan to update the man page regarding the new field ic_sw_cryptocaps besides ic_cryptocaps ?

Also please update the Summary section in this review, i.e. the mentioned new field ic_wpa_cryptocaps is actually the ic_sw_cryptocaps in the code change.

LGTM

I am intending to give approval, but I am afraid of missing the new response to my review comment for the code comment update. So let's be patient. :)

In D43966#1023788, @bz wrote:

@cc Adding LOCK asserts is a very good idea; not all the functions need them if I remember correctly. Should I really add it to this "fix" or should I do a separate full pass on the linux_80211_macops.c file?

In D44463#1014016, @adrian wrote:

you negotiate it as an extension, and then it's used to switch STA keys without dropping frames.

During association with the AP, sw/hw keys (indexes) are set. And before tx, the hw key is selected by sw key's index returned from ieee80211_crypto_get_txkey(), same way as without LKPI_80211_HW_CRYPTO.

I am working the "multi-key" support these days on top of D43634(this patch) and D43648, but it turns out this patch is causing more trouble than it is trying to solve.
In fact, it caused this "Abort trap" from the console and then my VM is shutdown silently. The kernel is main with LKPI_80211_HW_CRYPTO on top of this patch.

code update according to comments

I think LKPI_80211_LHW_LOCK_ASSERT can help remind us of such LKPI_80211_LHW_LOCK requirement for lkpi_80211_mo_* functions.

The ktls draws my attention. Hope you add reviewers.

Shall correct the typo first before committing.

Please add PR: 277095 in the summary, as this patch also fixes that.

Good catch! Always keep an eye on the cubic_data->t_epoch refresh.

In D43968#1003515, @bz wrote:

In D43968#1003463, @cc wrote:

Initial test shows this error when I changed the channel number in AP. Looks like a reproduce of PR 277100 without LKPI_80211_HW_CRYPTO enabled, or a separate issue?

Yes, looks likely same/similar than in the PR.

You've seen the comments (1)-(4) above?

Initial test shows this error when I changed the channel number in AP. Looks like a reproduce of PR 277100 without LKPI_80211_HW_CRYPTO enabled, or a separate issue?

My initial test on the three patches of D43389, D43725, D43753 looks to be good. No more panics. And I need to figure out some issues in my testbed, so I give the approval first as I don't want my test to delay the schedule.

My initial test on the three patches of D43389, D43725, D43753 looks to be good. No more panics. And I need to figure out some issues in my testbed, so I give the approval first as I don't want my test to delay the schedule.

My initial test on the three patches of D43389, D43725, D43753 looks to be good. No more panics. And I need to figure out some issues in my testbed, so I give the approval first as I don't want my test to delay the schedule.

In D43770#999610, @bz wrote:

Can we please NOT do this. I explained it in my other review (D43658) twice by now.

This is (a) only changing some defines to change and not all the others for the same structure, (b) it's been in at least 3 major releases this way why still change it? It fits with the entire HT code. For VHT which is not used yet we still can do and do.

Also, if doing I said replace them with the names from LinuxKPI so we do not have two versions. You are just trading one name for another name and still leaving us with more compat names.

In D43725#999202, @cc wrote:

In D43725#999194, @cc wrote:

Given D43389 is the FIRST of a series, is this the SECOND one or is there any dependence? Please help clarify.

I think there is dependence, as I applied this patch only, restarted netif, and hit the panic:

--- trap 0x9, rip = 0xffffffff80cf8661, rsp = 0xfffffe00ab111d00, rbp = 0xfffffe00ab111d10 ---
node_free() at node_free+0x11/frame 0xfffffe00ab111d10
lkpi_sta_auth_to_scan() at lkpi_sta_auth_to_scan+0x27f/frame 0xfffffe00ab111d80
lkpi_iv_newstate() at lkpi_iv_newstate+0x253/frame 0xfffffe00ab111df0
ieee80211_newstate_cb() at ieee80211_newstate_cb+0x226/frame 0xfffffe00ab111e40
taskqueue_run_locked() at taskqueue_run_locked+0xab/frame 0xfffffe00ab111ec0
taskqueue_thread_loop() at taskqueue_thread_loop+0xd3/frame 0xfffffe00ab111ef0
fork_exit() at fork_exit+0x82/frame 0xfffffe00ab111f30
fork_trampoline() at fork_trampoline+0xe/frame 0xfffffe00ab111f30
--- trap 0, rip = 0, rsp = 0, rbp = 0 ---
KDB: enter: panic
[ thread pid 0 tid 100192 ]
Stopped at      kdb_enter+0x32: movq    $0,0xe394d3(%rip)
db>

In D43658#999086, @emaste wrote:

I thought the ports tree contains user-space applications. Are you talking about some user-space drivers in the ports tree?

There are a number of kernel modules in the ports tree as well. drm-kmod is one well known example.

In D43725#999194, @cc wrote:

Given D43389 is the FIRST of a series, is this the SECOND one or is there any dependence? Please help clarify.

Given D43389 is the FIRST of a series, is this the SECOND one or is there any dependence? Please help clarify.

Remove Copyright headnote and myself from if_iwn.c, as noted in review.

With D43770 in mind, I think we can go ahead and commit this change.

add Copyright in if_iwn.c

Add missing update in if_iwn.c and keep IEEE80211_HTINFO_OPMODE_S from the kernel build lesson.

I have created D43770 to clarify/illustrate my idea.

In D43658#997148, @bz wrote:

If I understand correctly, these defines are from the Table 8-130—HT Operation element fields and subfields (continued) on page 615. I quoted them below. I think it's better to use cleaner names for these defines in sys/net80211/ieee80211.h and use an indent similar to the Linux version.

There's a lot to be said about the "native" net80211 version. But that's not part of this review request. I tend to not change the old stuff there if it is used (VHT is not used yet, so that's why there is cleanup going on there still). Simply MFCing it could be a pain without leaving compat defines. In this case it seems only iwn(4) would be affected so we could argue we can but I haven't checked all the other related fields and that's where the real pain comes in.

I think a separate patch "just rename these defines in sys/net80211/ieee80211.h" shall be OK, and can MFC, because their corresponding values are correct. If you agree, I can submit a patch for review.

The defines in net80211 are part of the public KPI and have been for a decade.
They are used at least in one device driver as well. I have not recently checked if there are any usable wlan(4) drivers in ports still supporting HT but that's due diligens to be done then as well.
If we are fine with breaking out-of-tree then we should probably just move the ones from LinuxKPI into net80211 and have a single definition of them.
But them we'll have inconsistent naming with the other fields for "HTINFO" etc. and that makes no sense either.
Hence my leaning of leaving the "legacy bits" as-are.

In D43658#996657, @bz wrote:

In D43658#996222, @cc wrote:

This patch also fixes the wrong/old values used in sys/compat/linuxkpi/common/include/linux/ieee80211.h. thanks

No, this patch ONLY fixes the wrong values there. The fact that they are defined to a native version is cosmetics.

I meant this patch fixes the wrong/old values here, in the LinuxKPI. For example, IEEE80211_HT_OP_MODE_PROTECTION_NONHT_MIXED should have a value 3, not the old 0x02.

Good catch!

This patch also fixes the wrong/old values used in sys/compat/linuxkpi/common/include/linux/ieee80211.h. thanks

I see two "MFC after" in the summary section, and the summary section seems to contain two parts that it looks you are going to split this patch into two when you MFC it. Am I understanding correctly? Are you going to manually MFC the two parts?

By the way, I forgot to ask, is it also helpful to add IEEE80211_LOCK_ASSERT in lkpi_iv_update_bss()?

I think this patch as a fix is targeting 271979#c28. If I am right, I will use the reproduce method to test this patch. thanks

Good catch!

Thanks for reminding me this so that I can make the right change of 93b7818226cf in MFC.

Thanks for the reminder.

Looks good to me now. Thanks for the update.

This is committed in https://cgit.freebsd.org/src/commit/?id=93b7818226cf5270646725805b4a8c17a1ad3761

Sorry, not sure how this is missing in my radar. Please allow one day or two to finish my review.

Tested in Emulab.net. Looks good to me.

code update with successful test

In D42765#979657, @imp wrote:

/rescue/tar is a requirement. What's wrong with that? the whole bug is because someone tried to grab the wrong image to start with and there's going to be way more than just this one detail wrong with the running system.

I think I have tested this method of separating packet processing and free before. But I want to make sure it is performing as expected. I will give my comment after testing.

remove debug echo

code update based on review comment

Add Reviewed by: carlavilla