2012-03-10 00:40:18

by Ashok Nagarajan

[permalink] [raw]
Subject: [PATCH] ath9k: Fix mactime from being clobbered in rx_status

mactime was being overwritten by the function ath9k_rx_skb_preprocess. Fixed
by calling the function before setting the mactime.

Signed-off-by: Ashok Nagarajan <[email protected]>
Signed-off-by: Javier Cardona <[email protected]>
---
drivers/net/wireless/ath/ath9k/recv.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 1b1b279..52a0466 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -1841,6 +1841,9 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
if (sc->sc_flags & SC_OP_RXFLUSH)
goto requeue_drop_frag;

+ retval = ath9k_rx_skb_preprocess(common, hw, hdr, &rs,
+ rxs, &decrypt_error);
+
rxs->mactime = (tsf & ~0xffffffffULL) | rs.rs_tstamp;
if (rs.rs_tstamp > tsf_lower &&
unlikely(rs.rs_tstamp - tsf_lower > 0x10000000))
@@ -1850,8 +1853,6 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
unlikely(tsf_lower - rs.rs_tstamp > 0x10000000))
rxs->mactime += 0x100000000ULL;

- retval = ath9k_rx_skb_preprocess(common, hw, hdr, &rs,
- rxs, &decrypt_error);
if (retval)
goto requeue_drop_frag;

--
1.7.5.4



2012-03-10 01:06:51

by Christian Lamparter

[permalink] [raw]
Subject: Re: [PATCH] ath9k: Fix mactime from being clobbered in rx_status

On Saturday 10 March 2012 01:40:08 Ashok Nagarajan wrote:
> mactime was being overwritten by the function
> ath9k_rx_skb_preprocess. Fixed by calling the
> function before setting the mactime.

This looks rather odd... In essence you're trying to revert
"ath9k: trivial: reorder rx_tasklet processing"

Anyway, I 'CC'ed the original author Zefir Kurtisi. So if
there are any open question, now is the time.

> Signed-off-by: Ashok Nagarajan <[email protected]>
> Signed-off-by: Javier Cardona <[email protected]>
> ---
> drivers/net/wireless/ath/ath9k/recv.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
> index 1b1b279..52a0466 100644
> --- a/drivers/net/wireless/ath/ath9k/recv.c
> +++ b/drivers/net/wireless/ath/ath9k/recv.c
> @@ -1841,6 +1841,9 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
> if (sc->sc_flags & SC_OP_RXFLUSH)
> goto requeue_drop_frag;
>
> + retval = ath9k_rx_skb_preprocess(common, hw, hdr, &rs,
> + rxs, &decrypt_error);
> +

Instead of moving ath9k_rx_skb_preprocess around, you could just
put the memset(rx_status, 0, sizeof(struct ieee80211_rx_status));
right here.

[Also, why leave the if (retval) check behind?]

> rxs->mactime = (tsf & ~0xffffffffULL) | rs.rs_tstamp;
> if (rs.rs_tstamp > tsf_lower &&
> unlikely(rs.rs_tstamp - tsf_lower > 0x10000000))
> @@ -1850,8 +1853,6 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
> unlikely(tsf_lower - rs.rs_tstamp > 0x10000000))
> rxs->mactime += 0x100000000ULL;
>
> - retval = ath9k_rx_skb_preprocess(common, hw, hdr, &rs,
> - rxs, &decrypt_error);
> if (retval)
> goto requeue_drop_frag;
^^ that one

2012-03-10 14:19:00

by Mohammed Shafi

[permalink] [raw]
Subject: Re: [PATCH] ath9k: Fix mactime from being clobbered in rx_status

On Sat, Mar 10, 2012 at 7:11 PM, Zefir Kurtisi
<[email protected]> wrote:
> Thanks for the ping.
>
> The patch that is reverted here was meant as pre-work for modifications
> required to generate DFS events with correctly set mactime that are still
> pending. It was obviously not as 'trivial' as claimed, since I overlooked
> the memset() in ath9k_rx_skb_preprocess() that effectively set the time to 0
> for all fragments.
>
> Therefore full ACK to revert it. Just wondering why it was not noticed for
> nearly 4 months now.

blame me, i also missed the memset in ath9k_rx_skb_preprocess. also we
missed quite a few of patches that caused some other regression and
they were missed

>
>
> On 10.03.2012 02:06, Christian Lamparter wrote:
>>
>> On Saturday 10 March 2012 01:40:08 Ashok Nagarajan wrote:
>>>
>>> mactime was being overwritten by the function
>>> ath9k_rx_skb_preprocess. ?Fixed by calling the
>>> function before setting the mactime.
>>
>>
>> This looks rather odd... In essence you're trying to revert
>> "ath9k: trivial: reorder rx_tasklet processing"
>>
>> Anyway, I 'CC'ed the original author Zefir Kurtisi. So if
>> there are any open question, now is the time.
>>
>>> Signed-off-by: Ashok Nagarajan<[email protected]>
>>> Signed-off-by: Javier Cardona<[email protected]>
>>> ---
>>> ?drivers/net/wireless/ath/ath9k/recv.c | ? ?5 +++--
>>> ?1 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath9k/recv.c
>>> b/drivers/net/wireless/ath/ath9k/recv.c
>>> index 1b1b279..52a0466 100644
>>> --- a/drivers/net/wireless/ath/ath9k/recv.c
>>> +++ b/drivers/net/wireless/ath/ath9k/recv.c
>>> @@ -1841,6 +1841,9 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush,
>>> bool hp)
>>> ? ? ? ? ? ? ? ?if (sc->sc_flags& ?SC_OP_RXFLUSH)
>>>
>>> ? ? ? ? ? ? ? ? ? ? ? ?goto requeue_drop_frag;
>>>
>>> + ? ? ? ? ? ? ? retval = ath9k_rx_skb_preprocess(common, hw, hdr,&rs,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?rxs,&decrypt_error);
>>> +
>>
>>
>> Instead of moving ath9k_rx_skb_preprocess around, you could just
>> put the memset(rx_status, 0, sizeof(struct ieee80211_rx_status));
>> right here.
>>
>> [Also, why leave the if (retval) check behind?]
>>
>>> ? ? ? ? ? ? ? ?rxs->mactime = (tsf& ?~0xffffffffULL) | rs.rs_tstamp;
>>> ? ? ? ? ? ? ? ?if (rs.rs_tstamp> ?tsf_lower&&
>>> ? ? ? ? ? ? ? ?unlikely(rs.rs_tstamp - tsf_lower> ?0x10000000))
>>> @@ -1850,8 +1853,6 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush,
>>> bool hp)
>>> ? ? ? ? ? ? ? ? ? ?unlikely(tsf_lower - rs.rs_tstamp> ?0x10000000))
>>> ? ? ? ? ? ? ? ? ? ? ? ?rxs->mactime += 0x100000000ULL;
>>>
>>> - ? ? ? ? ? ? ? retval = ath9k_rx_skb_preprocess(common, hw, hdr,&rs,
>>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?rxs,&decrypt_error);
>>> ? ? ? ? ? ? ? ?if (retval)
>>> ? ? ? ? ? ? ? ? ? ? ? ?goto requeue_drop_frag;
>>
>> ? ? ? ? ? ? ? ?^^ that one
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html



--
thanks,
shafi

2012-03-10 13:41:53

by Zefir Kurtisi

[permalink] [raw]
Subject: Re: [PATCH] ath9k: Fix mactime from being clobbered in rx_status

Thanks for the ping.

The patch that is reverted here was meant as pre-work for modifications
required to generate DFS events with correctly set mactime that are
still pending. It was obviously not as 'trivial' as claimed, since I
overlooked the memset() in ath9k_rx_skb_preprocess() that effectively
set the time to 0 for all fragments.

Therefore full ACK to revert it. Just wondering why it was not noticed
for nearly 4 months now.

On 10.03.2012 02:06, Christian Lamparter wrote:
> On Saturday 10 March 2012 01:40:08 Ashok Nagarajan wrote:
>> mactime was being overwritten by the function
>> ath9k_rx_skb_preprocess. Fixed by calling the
>> function before setting the mactime.
>
> This looks rather odd... In essence you're trying to revert
> "ath9k: trivial: reorder rx_tasklet processing"
>
> Anyway, I 'CC'ed the original author Zefir Kurtisi. So if
> there are any open question, now is the time.
>
>> Signed-off-by: Ashok Nagarajan<[email protected]>
>> Signed-off-by: Javier Cardona<[email protected]>
>> ---
>> drivers/net/wireless/ath/ath9k/recv.c | 5 +++--
>> 1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
>> index 1b1b279..52a0466 100644
>> --- a/drivers/net/wireless/ath/ath9k/recv.c
>> +++ b/drivers/net/wireless/ath/ath9k/recv.c
>> @@ -1841,6 +1841,9 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
>> if (sc->sc_flags& SC_OP_RXFLUSH)
>> goto requeue_drop_frag;
>>
>> + retval = ath9k_rx_skb_preprocess(common, hw, hdr,&rs,
>> + rxs,&decrypt_error);
>> +
>
> Instead of moving ath9k_rx_skb_preprocess around, you could just
> put the memset(rx_status, 0, sizeof(struct ieee80211_rx_status));
> right here.
>
> [Also, why leave the if (retval) check behind?]
>
>> rxs->mactime = (tsf& ~0xffffffffULL) | rs.rs_tstamp;
>> if (rs.rs_tstamp> tsf_lower&&
>> unlikely(rs.rs_tstamp - tsf_lower> 0x10000000))
>> @@ -1850,8 +1853,6 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
>> unlikely(tsf_lower - rs.rs_tstamp> 0x10000000))
>> rxs->mactime += 0x100000000ULL;
>>
>> - retval = ath9k_rx_skb_preprocess(common, hw, hdr,&rs,
>> - rxs,&decrypt_error);
>> if (retval)
>> goto requeue_drop_frag;
> ^^ that one