Return-path: Received: from mail-wi0-f171.google.com ([209.85.212.171]:33088 "EHLO mail-wi0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754811AbbFBHRa convert rfc822-to-8bit (ORCPT ); Tue, 2 Jun 2015 03:17:30 -0400 Received: by wiwd19 with SMTP id d19so9624723wiw.0 for ; Tue, 02 Jun 2015 00:17:29 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1433187843-20698-1-git-send-email-cfliu.tw@gmail.com> References: <1433187843-20698-1-git-send-email-cfliu.tw@gmail.com> Date: Tue, 2 Jun 2015 09:17:29 +0200 Message-ID: (sfid-20150602_091747_815265_7E64694A) Subject: Re: [PATCH 1/2] ath10k: add cryptmode param to support sw crypto and raw tx injection. From: Michal Kazior To: David Liu Cc: "ath10k@lists.infradead.org" , linux-wireless , Kalle Valo Content-Type: text/plain; charset=UTF-8 Sender: linux-wireless-owner@vger.kernel.org List-ID: On 1 June 2015 at 21:44, David Liu wrote: [...] > --- a/drivers/net/wireless/ath/ath10k/core.h > +++ b/drivers/net/wireless/ath/ath10k/core.h > @@ -91,6 +91,7 @@ struct ath10k_skb_cb { > u8 tid; > u16 freq; > bool is_offchan; > + bool nohwcrypt; > struct ath10k_htt_txbuf *txbuf; > u32 txbuf_paddr; > } __packed htt; > @@ -349,6 +350,7 @@ struct ath10k_vif { > } u; > > bool use_cts_prot; > + bool nohwcrypt; So this is a bit confusing. This is used only for tx policy only, right? It should be named accordingly then. The other nohwcrypt in skb_cb pretty much implies Tx already. [...] > @@ -484,6 +491,12 @@ enum ath10k_dev_flags { > * waiters should immediately cancel instead of waiting for a time out. > */ > ATH10K_FLAG_CRASH_FLUSH, > + > + /* Use Raw mode for Tx and Rx */ > + ATH10K_RAW_MODE, > + > + /* Disable HW crypto engine */ > + ATH10K_HW_CRYPTO_DISABLED, You're missing the _FLAG prefix. Also this isn't documented well enough. RAW_MODE implies sw crypto rx, no? Perhaps a more fine grained approach would be better: ATH10K_FLAG_RAW_TX, ATH10K_FLAG_RAW_RX, ATH10K_FLAG_SW_TX_CRYPTO, ATH10K_FLAG_SW_RX_CRYPTO, Obviously not all combinations are valid/doable but I think this will make the code look more obvious. > }; > > enum ath10k_cal_mode { > @@ -492,6 +505,15 @@ enum ath10k_cal_mode { > ATH10K_CAL_MODE_DT, > }; > > +enum ath10k_crypt_mode { > + /* Use HW crypto engine only */ > + ATH10K_CRYPT_MODE_HW, > + /* HW SW crypto engine only (ie. HW crypto engine disabled) */ > + ATH10K_CRYPT_MODE_SW, > + /* Both SW & HW crypto engine supported */ > + ATH10K_CRYPT_MODE_HW_SW, I don't think this is clear enough (and the comments don't help at all): ATH10K_CRYPT_MODE_HW, ATH10K_CRYPT_MODE_SW, ATH10K_CRYPT_MODE_SW_RX_HW_TX, It would also be nice to have some sort of indication in driver logs/dmesg (without any debug_masks) as to what crypto mode driver uses so that if someone reports a bug we can quickly see their base configuration. Having it completely configurable during runtime is a bit of a pain in this regard though.. but most people will likely just set cryptmode in modprobe.conf or something. Thoughts? > +}; > + > static inline const char *ath10k_cal_mode_str(enum ath10k_cal_mode mode) > { > switch (mode) { > diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c > index 89eb16b..a7df05d 100644 > --- a/drivers/net/wireless/ath/ath10k/htt_rx.c > +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c > @@ -1018,8 +1018,7 @@ static void ath10k_htt_rx_h_undecap_raw(struct ath10k *ar, > > /* In most cases this will be true for sniffed frames. It makes sense > * to deliver them as-is without stripping the crypto param. This would > - * also make sense for software based decryption (which is not > - * implemented in ath10k). > + * also make sense for software based decryption. I guess you should update the comment even more. The "would" doesn't fit anymore. Instead: "This is necessary for software crypto too. " Nonetheless kudos for taking care to update comments. [...] > diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h > index 85cca29..37fd2f83 100644 > --- a/drivers/net/wireless/ath/ath10k/hw.h > +++ b/drivers/net/wireless/ath/ath10k/hw.h > @@ -296,7 +296,7 @@ enum ath10k_hw_rate_cck { > #define TARGET_10X_RX_SKIP_DEFRAG_TIMEOUT_DUP_DETECTION_CHECK 1 > #define TARGET_10X_VOW_CONFIG 0 > #define TARGET_10X_NUM_MSDU_DESC (1024 + 400) > -#define TARGET_10X_MAX_FRAG_ENTRIES 0 > +#define TARGET_10X_MAX_FRAG_ENTRIES 10 This is probably enough at "2" (ath10k doesn't send more than 2 tx fragments now). I assume fw crashes with raw tx if this isn't fixed, correct? Sidenote: I guess TARGET_MAX_FRAG_ENTRIES could be fixed as well. It might make sense for QCA61X4 hw2.1 which still uses the old Rx indication event and might be able to do raw txrx + swcrypto. But I'm a bit reluctant to change this without any testing. [...] > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c > index 77220b0..1202150 100644 > --- a/drivers/net/wireless/ath/ath10k/wmi.c > +++ b/drivers/net/wireless/ath/ath10k/wmi.c > @@ -508,7 +508,7 @@ static struct wmi_vdev_param_map wmi_10x_vdev_param_map = { > .txbf = WMI_VDEV_PARAM_UNSUPPORTED, > .packet_powersave = WMI_VDEV_PARAM_UNSUPPORTED, > .drop_unencry = WMI_VDEV_PARAM_UNSUPPORTED, > - .tx_encap_type = WMI_VDEV_PARAM_UNSUPPORTED, > + .tx_encap_type = WMI_10X_VDEV_PARAM_TX_ENCAP_TYPE, Hmm.. Technically this isn't correct because 10.1 doesn't support this vdev parameter. Practically this might not matter since 10.1 won't ever have the appropriate fw_feature set (ATH10K_FW_FEATURE_RAW_MODE_SUPPORT).. unless Ben implements it in his 10.1 CT fork. Ideally you should create wmi_10_2_vdev_param_map and use it for the 10.2 op_version. Also you should probably update wmi_10_2_4_vdev_param_map as well - if 10.2 works then 10.2.4 will mostly like do too. I would actually even suggest for this tx_encap_type to be done in a separate patch as it'll change ath10k behaviour on its own. It'll be easier to bisect the driver later in case something will have gone wrong. Michał