wlan: update drivers to use ieee80211_set_hardware_ciphers()
Needs RevisionPublic
Actions

Authored by adrian on Apr 25 2024, 4:08 AM.

Details

Reviewers

cc
bz

Group Reviewers

wireless

Summary

This migrates drivers to use ieee80211_set_hardware_ciphers() instead
of setting ic_cryptocaps directly.

The only driver that hasn't entirely had ic_cryptocaps access removed
is if_ath(4); it's doing something tricksy to work around a hardware
issue (it looks like TKIP + WME/QoS frames may be buggy or not
supported.) I'll need to think about this and address it in a future
commit.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 57341
Build 54229: arc lint + arc unit

Event Timeline

adrian created this revision.Apr 25 2024, 4:08 AM

Herald added a subscriber: imp. · View Herald TranscriptApr 25 2024, 4:08 AM

adrian requested review of this revision.Apr 25 2024, 4:08 AM

Harbormaster completed remote builds in B57340: Diff 137641.Apr 25 2024, 4:08 AM

update w/ diff url

Harbormaster completed remote builds in B57341: Diff 137642.Apr 25 2024, 4:09 AM

adrian added a parent revision: D44922: wpa: add cipher flag support for the new ciphers.Apr 25 2024, 4:09 AM

adrian added a reviewer: wireless.

adrian added a project: wireless.

adrian mentioned this in D44827: net80211: add driver / crypto methods to set the hardware / software cipher suites.Apr 25 2024, 4:17 AM

adrian added a child revision: D44937: wlan: add some calls to ieee80211_set_driver_keymgmt_suites().Apr 25 2024, 4:45 AM

Looks reasonable to me

I see you made comments of "ieee80211_set_software_ciphers() is not needed" in if_rtwn.c. Do you mind to add it in the rest of the drivers files? Or is there any better/central place to put such comment or document it?

sys/dev/ath/if_ath.c
608	I would use `hwcryptocaps` instead, in case there will be `swcryptocaps` in the future, even if most drivers don't have software crypto. Also `hwcryptocaps` makes it clear that this is not software crypto caps. So saves comment. I think this can also help to reduce the rename work once the member name `ic_cryptocaps` be renamed as `ic_hw_cryptocaps` later.
sys/dev/rtwn/if_rtwn.c
238–239	Do you mean this? As from ieee80211_crypto_attach() in 1116e8b95c60: `rtwn supports the default set of net80211 supported ciphers, no more no less,`

This revision now requires changes to proceed.Mon, May 13, 2:37 PM

adrian added inline comments.Wed, Jun 5, 2:11 AM

sys/dev/rtwn/if_rtwn.c
238–239	yes, are you requesting a wording change?

cc added inline comments.Wed, Jun 5, 3:55 PM

sys/dev/rtwn/if_rtwn.c
238–239	yes, are you requesting a wording change? I was thinking maybe the default set of supported ciphers in ieee80211_crypto_attach() could change in the future. So maybe the comment here can be more precise? like this: rtwn supports the default set of net80211 supported ciphers (IEEE80211_CRYPTO_[WEP, TKIP, AES_CCM])

rename field, from cc@

Harbormaster completed remote builds in B58080: Diff 139542.Wed, Jun 5, 11:36 PM

What about the missing ones?

bz requested changes to this revision.Wed, Jun 5, 11:41 PM

bz added inline comments.

sys/dev/ath/if_ath.c
2583	Why are we still directly manipulating these fields here and below?

This revision now requires changes to proceed.Wed, Jun 5, 11:41 PM

adrian marked an inline comment as done.Thu, Jun 6, 5:26 PM

adrian added inline comments.

sys/dev/ath/if_ath.c
2583	Because I don't have a neat way to clean this up yet, and I may just delete it all eventually and put a note in the driver (and wiki) about the hw capabilities / bugs and why this is disabled. I'll tackle it in a subsequent commit, once I've grabbed the box of relevant hardware to test it with and properly test all the options together.

Other than the comment wording like "rtwn supports the default set of net80211 supported ciphers (IEEE80211_CRYPTO_[WEP, TKIP, AES_CCM])", looks good to me.