Re: [PATCH 4/7] ath10k: unify rx undecapping

[Kalle Valo <kvalo@qca.qualcomm.com>]

Michal Kazior <michal.kazior@tieto.com> writes:

> On 17 November 2014 16:11, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Michal Kazior <michal.kazior@tieto.com> writes:
>>> On 17 November 2014 15:32, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> [...]
>>>> This patch had few checkpatch warnings. I fixed them with the folded
>>>> patch and full patch here:
>>>>
>>>> https://github.com/kvalo/ath/commit/71fbd07d43e54f5f9f442bc5f2f4f9ef83aead63
> [...]
>>>> @@ -1132,7 +1133,7 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
>>>>         bool has_fcs_err;
>>>>         bool has_crypto_err;
>>>>         bool has_tkip_err;
>>>> -       bool has_peer_idx_invalid;
>>>> +       bool has_idx_invalid;
>>>>         bool is_decrypted;
>>>
>>> I don't really like the has_idx_invalid. Perhaps has_peer_err conveys
>>> a bit more of the original meaning?
>>
>> What about just peer_idx_invalid? IMHO we really don't need the has_
>> prefix in that relatively small function.
>
> Good point but the assignment line with `peer_idx_invalid` will still
> be over 80 chars long, no?

I actually test with 83 char limit nowadays. I should add the checkpatch
command line to the wiki.

> So perhaps instead of doing `rxd->attention.flags &
> __cpu_to_le32(FLAG)` it could be `attention =
> __le32_to_cpu(rxd->attention.flags)` and then `attention & FLAG` ?
> This way it shouldn't exceed the 80 char limit and var names won't
> need to be changed. Hopefully compiler will optimize it away.

That's much better and actually has a lot less calls to cpu_le32(). I
folded the patch below.

But aren't the '!!' operators useless? 'has_*' variables are booleans,
doesn't compiler handle that automatically?

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 6f58b7f912cd..6abb3b6a348f 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1133,8 +1133,9 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
 	bool has_fcs_err;
 	bool has_crypto_err;
 	bool has_tkip_err;
-	bool has_idx_invalid;
+	bool has_peer_idx_invalid;
 	bool is_decrypted;
+	u32 attention;
 
 	if (skb_queue_empty(amsdu))
 		return;
@@ -1162,14 +1163,12 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
 	/* Some attention flags are valid only in the last MSDU. */
 	last = skb_peek_tail(amsdu);
 	rxd = (void *)last->data - sizeof(*rxd);
-	has_fcs_err = !!(rxd->attention.flags &
-			 __cpu_to_le32(RX_ATTENTION_FLAGS_FCS_ERR));
-	has_crypto_err = !!(rxd->attention.flags &
-			    __cpu_to_le32(RX_ATTENTION_FLAGS_DECRYPT_ERR));
-	has_tkip_err = !!(rxd->attention.flags &
-			  __cpu_to_le32(RX_ATTENTION_FLAGS_TKIP_MIC_ERR));
-	has_idx_invalid = !!(rxd->attention.flags &
-			     __cpu_to_le32(RX_ATTENTION_FLAGS_PEER_IDX_INVALID));
+	attention = __le32_to_cpu(rxd->attention.flags);
+
+	has_fcs_err = !!(attention & RX_ATTENTION_FLAGS_FCS_ERR);
+	has_crypto_err = !!(attention & RX_ATTENTION_FLAGS_DECRYPT_ERR);
+	has_tkip_err = !!(attention & RX_ATTENTION_FLAGS_TKIP_MIC_ERR);
+	has_peer_idx_invalid = !!(attention & RX_ATTENTION_FLAGS_PEER_IDX_INVALID);
 
 	/* Note: If hardware captures an encrypted frame that it can't decrypt,
 	 * e.g. due to fcs error, missing peer or invalid key data it will
@@ -1178,7 +1177,7 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
 	is_decrypted = (enctype != HTT_RX_MPDU_ENCRYPT_NONE &&
 			!has_fcs_err &&
 			!has_crypto_err &&
-			!has_idx_invalid);
+			!has_peer_idx_invalid);
 
 	/* Clear per-MPDU flags while leaving per-PPDU flags intact. */
 	status->flag &= ~(RX_FLAG_FAILED_FCS_CRC |




-- 
Kalle Valo