Can we shorten hardware/software to hw/sw?

Why do we need wrappers from general code into crypto? Isn't one of them enough to set (and probably also clear[1]) a field in the (currently) ic?

[1] the current API won't really help you for all use cases as at least ath(4) has one "clear bit" case as well. Possibly using a KPI like ifnet does (ptr, set, clear) would help. I am also wondering if we generally want to have the KPI in a way to |= for set an &~ for clear as multiple callers are doing it bit-by-bit.

I think if we were to adjust the few callers then we could hide the field name entirely and solve that naming problem right away as well with adding the 2nd struct field.

In D44827#1023017, @bz wrote:

Can we shorten hardware/software to hw/sw?

Why do we need wrappers from general code into crypto? Isn't one of them enough to set (and probably also clear[1]) a field in the (currently) ic?

Just habit. The routines in ieee80211.c are normally the public facing ones. The crypto layer ones know about the crypto stuff in more detail.

[1] the current API won't really help you for all use cases as at least ath(4) has one "clear bit" case as well. Possibly using a KPI like ifnet does (ptr, set, clear) would help. I am also wondering if we generally want to have the KPI in a way to |= for set an &~ for clear as multiple callers are doing it bit-by-bit.

My intent here is that drivers call it to set them explicitly, not mask them. So eg, ath would simply just call it on the set of bits they care about for both hardware and software crypto.
The ath10k driver would call them based on whether software encryption was enabled or disabled at module load time.
Hardware that say, didn't support TKIP at all (hardware or software) would call the registration without those bits set.

I think if we were to adjust the few callers then we could hide the field name entirely and solve that naming problem right away as well with adding the 2nd struct field.

eg ath(4) would do something like:

        uint32_t hw_cryptocaps;
	if (ath_hal_ciphersupported(ah, HAL_CIPHER_WEP))
		hw_cryptocaps |= IEEE80211_CRYPTO_WEP;
	if (ath_hal_ciphersupported(ah, HAL_CIPHER_AES_OCB))
		hw_cryptocaps |= IEEE80211_CRYPTO_AES_OCB;
	if (ath_hal_ciphersupported(ah, HAL_CIPHER_AES_CCM))
		hw_cryptocaps |= IEEE80211_CRYPTO_AES_CCM;
	if (ath_hal_ciphersupported(ah, HAL_CIPHER_CKIP))
		hw_cryptocaps |= IEEE80211_CRYPTO_CKIP;
	if (ath_hal_ciphersupported(ah, HAL_CIPHER_TKIP)) {
		hw_cryptocaps |= IEEE80211_CRYPTO_TKIP;	
/*
		 * Check if h/w does the MIC and/or whether the
		 * separate key cache entries are required to
		 * handle both tx+rx MIC keys.
		 */
		if (ath_hal_ciphersupported(ah, HAL_CIPHER_MIC))
			hw_cryptocaps |= IEEE80211_CRYPTO_TKIPMIC;
}

ieee80211_set_hardware_cipher_set(hw_cryptocaps);
/* All if_ath(4) HAL providers allow for pass-through unencrypted frames, so
  * software encryption is possible.
 */
ieee80211_set_software_cipher_set(
IEEE80211_CRYPTO_WEP |
 IEEE80211_CRYPTO_AES_OCB |
 IEEE80211_CRYPTO_AES_CCM |
 IEEE80211_CRYPTO_CKIP |
 IEEE80211_CRYPTO_TKIP);

Yes it means that if we add new cipher suites then we will have to add them to each of the drivers, but then by doing this we actually ensure we don't break things by enabling support for drivers when new ciphers (and later new key exchange algorithms) are added without verifying and manually enabling it on each driver.

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?

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.

I find it a little confusing with the verb & noun use of set in set_software_cipher_set - do you think set_software_ciphers (or set_sw_ciphers) is reasonable?

In D44827#1023673, @emaste wrote:

I find it a little confusing with the verb & noun use of set in set_software_cipher_set - do you think set_software_ciphers (or set_sw_ciphers) is reasonable?

I think renaming it to set_software_ciphers() / set_hardware_ciphers() is fine!

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.

I don't get it. Without exposing the field ic_cryptocaps? And where is the "fixed the code snippet"?

Are you going to change the few drivers which need change to get rid of the public exposure as well? Otherwise this is a dead code before added.

In D44827#1024561, @bz wrote:

Are you going to change the few drivers which need change to get rid of the public exposure as well? Otherwise this is a dead code before added.

Yup a diff later in the stack (D44936) uses it.

In D44827#1024357, @cc wrote:

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.

I don't get it. Without exposing the field ic_cryptocaps? And where is the "fixed the code snippet"?

oh THAT. yeah, that's dirty and I hate it.

I'll think about it a bit. I may end up just removing doing HW TKIP support for NICs that can't do it with WME because honestly it's 2024 and those NICs, if still being used, would likely be used on something with enough CPU for it not to matter. (And I'd honestly be surprised if someone really complains about the performance change.)

This still has changes not marked "done".
Is there a change in the stack which does change the drivers to use this?

In D44827#1027553, @bz wrote:

This still has changes not marked "done".
Is there a change in the stack which does change the drivers to use this?

There's no native drivers in our tree right now that require the software cipher call, but in the linux side ath10k will likely require it.
D44936 replaces most of the ic_cryptocaps references to use the hardware cipher call.