Re: [PATCH 1/2] ath10k: add cryptmode param to support sw crypto and raw tx injection.

[Michal Kazior <michal.kazior@tieto.com>]

On 1 June 2015 at 21:44, David Liu <cfliu.tw@gmail.com> 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ł