[PATCH 1/1] ath9k: decrypt_error flag issue

[PATCH 1/1] ath9k: decrypt_error flag issue

[Lorenzo Bianconi]

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
---

--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -1780,7 +1780,6 @@
 	struct ieee80211_hw *hw = sc->hw;
 	struct ieee80211_hdr *hdr;
 	int retval;
-	bool decrypt_error = false;
 	struct ath_rx_status rs;
 	enum ath9k_rx_qtype qtype;
 	bool edma = !!(ah->caps.hw_caps & ATH9K_HW_CAP_EDMA);
@@ -1802,6 +1801,7 @@
 	tsf_lower = tsf & 0xffffffff;

 	do {
+		bool decrypt_error = false;
 		/* If handling rx interrupt and flush is in progress => exit */
 		if ((sc->sc_flags & SC_OP_RXFLUSH) && (flush == 0))
 			break;

Re: [PATCH 1/1] ath9k: decrypt_error flag issue

[Pavel Roskin]

On Wed, 8 Aug 2012 19:20:15 +0200
Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> wrote:

> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
> ---

Please see Documentation/SubmittingPatches in Linux sources.  In
particular, you separated the description of the patch from its
contents.  Single patch doesn't need to be send as series.  The
description for the series is not a substitute for a description of
every patch.

Also, the subject should summarize the patch.  I know, it may be hard
to fit, but not impossible.

When exactly would you have the problem that decrypt_error is not
unset, but should be?  It's important that you show your assumptions so
that others can see if they are correct, in addition to checking the
code.  Other developers are more likely to check your patch if you
show understanding of the code you have changed.

> --- a/drivers/net/wireless/ath/ath9k/recv.c
> +++ b/drivers/net/wireless/ath/ath9k/recv.c
> @@ -1780,7 +1780,6 @@

That's weird, recv.c is 1273 lines long in wireless-testing.git.

-- 
Regards,
Pavel Roskin

Re: [PATCH 1/1] ath9k: decrypt_error flag issue

[Lorenzo Bianconi]

> On Wed, 8 Aug 2012 19:20:15 +0200
> Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> wrote:
>
>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
>> ---
>
> Please see Documentation/SubmittingPatches in Linux sources.  In
> particular, you separated the description of the patch from its
> contents.  Single patch doesn't need to be send as series.  The
> description for the series is not a substitute for a description of
> every patch.
>
> Also, the subject should summarize the patch.  I know, it may be hard
> to fit, but not impossible.
>

Ack. I will rewrite the mail.

> When exactly would you have the problem that decrypt_error is not
> unset, but should be?  It's important that you show your assumptions so
> that others can see if they are correct, in addition to checking the
> code.  Other developers are more likely to check your patch if you
> show understanding of the code you have changed.
>

Assume hw reports a decryption error, the flag decrypt_error is set to
true in ath9k_rx_accept.
Since this flag is initialized to false just out of  ath_rx_tasklet while cycle,
all subsequent frames are marked as corrupted until ath_rx_tasklet ends.


>> --- a/drivers/net/wireless/ath/ath9k/recv.c
>> +++ b/drivers/net/wireless/ath/ath9k/recv.c
>> @@ -1780,7 +1780,6 @@
>
> That's weird, recv.c is 1273 lines long in wireless-testing.git.
>
> --
> Regards,
> Pavel Roskin

Regards

Lorenzo

Re: [PATCH 1/1] ath9k: decrypt_error flag issue

[Mohammed Shafi]

On Thu, Aug 9, 2012 at 5:57 PM, Lorenzo Bianconi
<lorenzo.bianconi83@gmail.com> wrote:
>> On Wed, 8 Aug 2012 19:20:15 +0200
>> Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> wrote:
>>
>>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
>>> ---
>>
>> Please see Documentation/SubmittingPatches in Linux sources.  In
>> particular, you separated the description of the patch from its
>> contents.  Single patch doesn't need to be send as series.  The
>> description for the series is not a substitute for a description of
>> every patch.
>>
>> Also, the subject should summarize the patch.  I know, it may be hard
>> to fit, but not impossible.
>>
>
> Ack. I will rewrite the mail.

more help
http://linuxwireless.org/en/developers/Documentation/git-guide :)

>
>> When exactly would you have the problem that decrypt_error is not
>> unset, but should be?  It's important that you show your assumptions so
>> that others can see if they are correct, in addition to checking the
>> code.  Other developers are more likely to check your patch if you
>> show understanding of the code you have changed.
>>
>
> Assume hw reports a decryption error, the flag decrypt_error is set to
> true in ath9k_rx_accept.
> Since this flag is initialized to false just out of  ath_rx_tasklet while cycle,
> all subsequent frames are marked as corrupted until ath_rx_tasklet ends.

seems to be a nice catch!
It would be great if we can simulate this situation (ie
decrypt_error is set to true, and subsequent frames are discarded)
may be we can try this some time, purposefully setting the decrypt_error 'true'
for the first instance and see if there is data loss/throughput diff
because of this.


>
>
>>> --- a/drivers/net/wireless/ath/ath9k/recv.c
>>> +++ b/drivers/net/wireless/ath/ath9k/recv.c
>>> @@ -1780,7 +1780,6 @@
>>
>> That's weird, recv.c is 1273 lines long in wireless-testing.git.

yeah you got to commit the patches by cloning the latest wireless testing
git clone git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-testing.git


-- 
thanks,
shafi

Re: [PATCH 1/1] ath9k: decrypt_error flag issue

[Pavel Roskin]

On Thu, 9 Aug 2012 18:21:59 +0530
Mohammed Shafi <shafi.wireless@gmail.com> wrote:

> > Assume hw reports a decryption error, the flag decrypt_error is set
> > to true in ath9k_rx_accept.
> > Since this flag is initialized to false just out of  ath_rx_tasklet
> > while cycle, all subsequent frames are marked as corrupted until
> > ath_rx_tasklet ends.
> 
> seems to be a nice catch!
> It would be great if we can simulate this situation (ie
> decrypt_error is set to true, and subsequent frames are discarded)
> may be we can try this some time, purposefully setting the
> decrypt_error 'true' for the first instance and see if there is data
> loss/throughput diff because of this.

That simulation might be too much work.  The only affected frames would
be those that are processed in the same tasklet as the frame with
decryption error.  What frame is processed in which tasklet is
timing-dependent.

It's enough to see that the code is wrong as it is.

ath9k_rx_accept() only sets decrypt_error to true and never to false.
It's not my favorite way of handling errors, but it's OK.

ath9k_rx_skb_preprocess() calls ath9k_rx_accept() and never writes or
reads decrypt_error in any other way.

ath_rx_tasklet() calls ath9k_rx_skb_preprocess() in a loop over the
frames to be processed.  Then it calls ath9k_rx_skb_postprocess() and
passes decrypt_error to it.  So we can have a leftover value from
another frame passed there.  In that case, the frame won't me marked as
decrypted.

I think it would be nice to examine all the variables initialized
outside the loop in ath_rx_tasklet().  There may be more catches
waiting to be discovered.

-- 
Regards,
Pavel Roskin