From: Kalle Valo <kvalo@qca.qualcomm.com> To: Liu CF/TW <cfliu.tw@gmail.com> Cc: "ath10k@lists.infradead.org" <ath10k@lists.infradead.org>, <linux-wireless@vger.kernel.org> Subject: Re: [PATCH] ath10k/mac80211: add rawtxrx, nohwcrypt module param for raw tx injection, sw crypto support. Date: Fri, 8 May 2015 14:54:58 +0300 [thread overview] Message-ID: <873837qo6l.fsf@kamboji.qca.qualcomm.com> (raw) In-Reply-To: <CAG5N3qG7090vB6XoUaQSHvQw7GpE+gXvJAbKrv7def3nV0OFJw@mail.gmail.com> (Liu CF's message of "Tue, 5 May 2015 18:06:46 -0700") Hi David, "Liu CF/TW" <cfliu.tw@gmail.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Kalle Valo <kvalo@qca.qualcomm.com> To: Liu CF/TW <cfliu.tw@gmail.com> Cc: linux-wireless@vger.kernel.org, "ath10k@lists.infradead.org" <ath10k@lists.infradead.org> Subject: Re: [PATCH] ath10k/mac80211: add rawtxrx, nohwcrypt module param for raw tx injection, sw crypto support. Date: Fri, 8 May 2015 14:54:58 +0300 [thread overview] Message-ID: <873837qo6l.fsf@kamboji.qca.qualcomm.com> (raw) In-Reply-To: <CAG5N3qG7090vB6XoUaQSHvQw7GpE+gXvJAbKrv7def3nV0OFJw@mail.gmail.com> (Liu CF's message of "Tue, 5 May 2015 18:06:46 -0700") Hi David, "Liu CF/TW" <cfliu.tw@gmail.com> 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 _______________________________________________ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
next prev parent reply other threads:[~2015-05-08 11:55 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-05-06 1:06 [PATCH] ath10k/mac80211: add rawtxrx, nohwcrypt module param for raw tx injection, sw crypto support Liu CF/TW 2015-05-06 1:06 ` Liu CF/TW 2015-05-08 11:54 ` Kalle Valo [this message] 2015-05-08 11:54 ` Kalle Valo 2015-05-08 16:28 ` Ben Greear 2015-05-08 16:28 ` Ben Greear 2015-05-08 17:35 ` Liu CF/TW 2015-05-08 17:35 ` Liu CF/TW 2015-05-08 17:52 ` Ben Greear 2015-05-08 17:52 ` Ben Greear 2015-05-11 12:12 ` Kalle Valo 2015-05-11 12:12 ` Kalle Valo 2015-05-11 16:17 ` Ben Greear 2015-05-11 16:17 ` Ben Greear 2015-05-12 22:44 ` Liu CF/TW 2015-05-12 22:44 ` Liu CF/TW 2015-05-12 23:01 ` Liu CF/TW 2015-05-12 23:01 ` Liu CF/TW 2015-05-14 15:12 ` Kalle Valo 2015-05-14 15:12 ` Kalle Valo 2015-05-14 19:35 ` Liu CF/TW 2015-05-14 19:35 ` Liu CF/TW 2015-05-14 21:16 ` Ben Greear 2015-05-14 21:16 ` Ben Greear 2015-05-11 12:24 ` Kalle Valo 2015-05-11 12:24 ` Kalle Valo
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=873837qo6l.fsf@kamboji.qca.qualcomm.com \ --to=kvalo@qca.qualcomm.com \ --cc=ath10k@lists.infradead.org \ --cc=cfliu.tw@gmail.com \ --cc=linux-wireless@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.