Hi David,
"Liu CF/TW" <cfliu.tw@xxxxxxxxx> writes:
> - ath10k: add rawtxrx param for 10.2+ firmware to support sw, hw crypto
> engine and raw Tx injection.
> - mac80211: Add IEEE80211_KEY_FLAG_RESERVE_TAILROOM support for TKIP and
> CCMP required by ath10k and add per BSS(vif) based sw_crypto
> control.
This patch has few major problems. First of all, ath10k and mac80211
changes should be in separate patches. Secondly the patch is whitespace
damaged, I recommend using git-send-email to avoid that.
And there's just too much feature changes in one patch, it's usually
better to split the features into separate patches. For example, you
could first add a simple raw mode support to ath10k (ie. the bare
minimum needed to get the feature) and then adding more advanced
features per patch.
> This change enables the raw Tx/Rx feature in ath10k 10.2+ firmware with a
> module parameter 'rawtxrx'. With rawtxrx=1, the ath10k hardware crypto
> engine could be optionally skipped to support use cases such as enabling
> mac80211 sw crypto engine, user level crypto engine, raw Tx frame
> injection.
Lots of people, especially in Qualcomm, seem to call this feature as
"raw mode". Would it be more descriptive to name the module paramer as
'rawmode'?
> Testing: used QCA988x hw 2.0 with 10.2 firmware.
>
> ath10k ath10k nl80211
> rawtxrx nohwcrypt SW_CRYPTO
> param param attribute Testing Status
> ------- --------- --------- ---------------------------------
> 0 0 - HW CCMP/TKIP tested ok.
> 0 1 - Not supported by ath10k hw.
> 1 0 - HW CCMP/TKIP tested ok.
> 1 0 0 BSS 1 tested HW CCMP/TKIP ok.
> 1 0 1 BSS 2 can bypass HW engine.
> - mac80211 SW crypto tested ok.
> - raw Tx frame injection tested ok.
> 1 1 - HW crypto globally disabled.
> - mac80211 SW crypto tested ok.
> - raw Tx frame injection tested ok.
I wonder does it make any sense to have nohwcrypt parameter? Especially
if ath10k doesn't support case rawtxrx=0 and nohwcrypt=1. One
possibility I came up is to have multiple values for rawtxrx, for
example is rawtxrx=1 means HW crypt enabled and rawtxrx=2 HW crypt
disabled. Ideas welcome.
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -31,16 +31,22 @@
> #include "wmi-ops.h"
>
> unsigned int ath10k_debug_mask;
> +bool ath10k_modparam_nohwcrypt;
> +bool ath10k_modparam_rawtxrx;
Instead of making these public I would prefer to set a flag in struct
ath10k, for example dev_flags.
> ath10k_core_init_firmware_features(struct ath10k *ar)
> return -EINVAL;
> }
>
> + if ((ath10k_modparam_rawtxrx || ath10k_modparam_nohwcrypt) &&
> + !test_bit(ATH10K_FW_FEATURE_WMI_10_2, ar->fw_features)) {
> + ath10k_err(ar, "rawtxrx mode supported only in 10.2+ firmware");
> + return -EINVAL;
> + }
I think we should add a flag to enum ath10k_fw_features and check for
that.
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index 241220c..cdfa8a8 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -1761,6 +1761,9 @@ enum nl80211_commands {
> * @NL80211_ATTR_REG_INDOOR: flag attribute, if set indicates that the device
> * is operating in an indoor environment.
> *
> + * @NL80211_ATTR_SW_CRYPTO: use software crypto instead of hardware crypto for
> + * the BSS.
> + *
> * @NUM_NL80211_ATTR: total number of nl80211_attrs available
> * @NL80211_ATTR_MAX: highest attribute number currently defined
> * @__NL80211_ATTR_AFTER_LAST: internal use
> @@ -2130,6 +2133,8 @@ enum nl80211_attrs {
>
> NL80211_ATTR_REG_INDOOR,
>
> + NL80211_ATTR_SW_CRYPTO,
Like I said above, nl80211/cfg80211/mac80211 changes need to be in
separate patches. And I suspect that the changes of getting this
accepted is low. Maybe a debugfs is a better choise?
--
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
