[PATCH] ath9k: Check for NULL sta in ath_tx_start

[PATCH] ath9k: Check for NULL sta in ath_tx_start

[greearb]

From: Ben Greear <greearb@candelatech.com>

It can be NULL according to docs, and logging showed it
to be NULL in practice.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 8b0b076... 5ddca08... M	drivers/net/wireless/ath/ath9k/xmit.c
 drivers/net/wireless/ath/ath9k/xmit.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 8b0b076..5ddca08 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -1764,7 +1764,10 @@ int ath_tx_start(struct ieee80211_hw *hw, struct sk_buff *skb,
 	int frmlen = skb->len + FCS_LEN;
 	int q;
 
-	txctl->an = (struct ath_node *)sta->drv_priv;
+	/* NOTE:  sta can be NULL according to net/mac80211.h */
+	if (sta)
+		txctl->an = (struct ath_node *)sta->drv_priv;
+
 	if (info->control.hw_key)
 		frmlen += info->control.hw_key->icv_len;
 
-- 
1.7.2.3

Re: [PATCH] ath9k: Check for NULL sta in ath_tx_start

[Luis R. Rodriguez]

On Mon, Dec 6, 2010 at 9:13 PM,  <greearb@gmail.com> wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> It can be NULL according to docs, and logging showed it
> to be NULL in practice.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>

Does this fix an oops? If so can you explain and provide the trace and
resubmit and cc stable in the commit log?

  Luis

Re: [PATCH] ath9k: Check for NULL sta in ath_tx_start

[Ben Greear]

On 12/06/2010 09:21 PM, Luis R. Rodriguez wrote:
> On Mon, Dec 6, 2010 at 9:13 PM,<greearb@gmail.com>  wrote:
>> From: Ben Greear<greearb@candelatech.com>
>>
>> It can be NULL according to docs, and logging showed it
>> to be NULL in practice.
>>
>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>
> Does this fix an oops? If so can you explain and provide the trace and
> resubmit and cc stable in the commit log?

I think it fixes the TID corruption I posted about earlier.  It seems
so obvious though, that I'm curious why no one else sees problems,
and why I didn't see more crashes in my setup.

(The paprd code appears to send with null STA, for instance, and my
printks showed lots of NULL stas in my 16-sta test setup, though I
don't think I'm using the paprd code path.)

I'll test some more and re-post tomorrow if all goes well.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] ath9k: Check for NULL sta in ath_tx_start

[Vasanthakumar Thiagarajan]

On Tue, Dec 07, 2010 at 11:06:24AM +0530, Ben Greear wrote:
> On 12/06/2010 09:21 PM, Luis R. Rodriguez wrote:
> > On Mon, Dec 6, 2010 at 9:13 PM,<greearb@gmail.com>  wrote:
> >> From: Ben Greear<greearb@candelatech.com>
> >>
> >> It can be NULL according to docs, and logging showed it
> >> to be NULL in practice.
> >>
> >> Signed-off-by: Ben Greear<greearb@candelatech.com>
> >
> > Does this fix an oops? If so can you explain and provide the trace and
> > resubmit and cc stable in the commit log?
> 
> I think it fixes the TID corruption I posted about earlier.  It seems
> so obvious though, that I'm curious why no one else sees problems,
> and why I didn't see more crashes in my setup.
> 
> (The paprd code appears to send with null STA, for instance, and my
> printks showed lots of NULL stas in my 16-sta test setup, though I
> don't think I'm using the paprd code path.)

paprd is used only with >= AR9003.

Vasanth

Re: [PATCH] ath9k: Check for NULL sta in ath_tx_start

[Ben Greear]

On 12/06/2010 11:42 PM, Vasanthakumar Thiagarajan wrote:
> On Tue, Dec 07, 2010 at 11:06:24AM +0530, Ben Greear wrote:
>> On 12/06/2010 09:21 PM, Luis R. Rodriguez wrote:
>>> On Mon, Dec 6, 2010 at 9:13 PM,<greearb@gmail.com>   wrote:
>>>> From: Ben Greear<greearb@candelatech.com>
>>>>
>>>> It can be NULL according to docs, and logging showed it
>>>> to be NULL in practice.
>>>>
>>>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>>>
>>> Does this fix an oops? If so can you explain and provide the trace and
>>> resubmit and cc stable in the commit log?
>>
>> I think it fixes the TID corruption I posted about earlier.  It seems
>> so obvious though, that I'm curious why no one else sees problems,
>> and why I didn't see more crashes in my setup.
>>
>> (The paprd code appears to send with null STA, for instance, and my
>> printks showed lots of NULL stas in my 16-sta test setup, though I
>> don't think I'm using the paprd code path.)
>
> paprd is used only with>= AR9003.

Whoever coded it up hopefully had that hardware...so why didn't
they see lots of crashes?

Thanks,
Ben

>
> Vasanth


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] ath9k: Check for NULL sta in ath_tx_start

[Vasanthakumar Thiagarajan]

On Tue, Dec 07, 2010 at 01:19:22PM +0530, Ben Greear wrote:
> On 12/06/2010 11:42 PM, Vasanthakumar Thiagarajan wrote:
> > On Tue, Dec 07, 2010 at 11:06:24AM +0530, Ben Greear wrote:
> >> On 12/06/2010 09:21 PM, Luis R. Rodriguez wrote:
> >>> On Mon, Dec 6, 2010 at 9:13 PM,<greearb@gmail.com>   wrote:
> >>>> From: Ben Greear<greearb@candelatech.com>
> >>>>
> >>>> It can be NULL according to docs, and logging showed it
> >>>> to be NULL in practice.
> >>>>
> >>>> Signed-off-by: Ben Greear<greearb@candelatech.com>
> >>>
> >>> Does this fix an oops? If so can you explain and provide the trace and
> >>> resubmit and cc stable in the commit log?
> >>
> >> I think it fixes the TID corruption I posted about earlier.  It seems
> >> so obvious though, that I'm curious why no one else sees problems,
> >> and why I didn't see more crashes in my setup.
> >>
> >> (The paprd code appears to send with null STA, for instance, and my
> >> printks showed lots of NULL stas in my 16-sta test setup, though I
> >> don't think I'm using the paprd code path.)
> >
> > paprd is used only with>= AR9003.
> 
> Whoever coded it up hopefully had that hardware...so why didn't
> they see lots of crashes?

I myself tested it lot of times, but did not see any crash, weird.

Vasanth

> 
> Thanks,
> Ben
> 
> >
> > Vasanth
> 
> 
> -- 
> Ben Greear <greearb@candelatech.com>
> Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] ath9k: Check for NULL sta in ath_tx_start

[Ben Greear]

On 12/07/2010 12:31 AM, Vasanthakumar Thiagarajan wrote:
> On Tue, Dec 07, 2010 at 01:19:22PM +0530, Ben Greear wrote:
>> On 12/06/2010 11:42 PM, Vasanthakumar Thiagarajan wrote:
>>> On Tue, Dec 07, 2010 at 11:06:24AM +0530, Ben Greear wrote:
>>>> On 12/06/2010 09:21 PM, Luis R. Rodriguez wrote:
>>>>> On Mon, Dec 6, 2010 at 9:13 PM,<greearb@gmail.com>    wrote:
>>>>>> From: Ben Greear<greearb@candelatech.com>
>>>>>>
>>>>>> It can be NULL according to docs, and logging showed it
>>>>>> to be NULL in practice.
>>>>>>
>>>>>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>>>>>
>>>>> Does this fix an oops? If so can you explain and provide the trace and
>>>>> resubmit and cc stable in the commit log?
>>>>
>>>> I think it fixes the TID corruption I posted about earlier.  It seems
>>>> so obvious though, that I'm curious why no one else sees problems,
>>>> and why I didn't see more crashes in my setup.
>>>>
>>>> (The paprd code appears to send with null STA, for instance, and my
>>>> printks showed lots of NULL stas in my 16-sta test setup, though I
>>>> don't think I'm using the paprd code path.)
>>>
>>> paprd is used only with>= AR9003.
>>
>> Whoever coded it up hopefully had that hardware...so why didn't
>> they see lots of crashes?
>
> I myself tested it lot of times, but did not see any crash, weird.

Looks like the offending change when in recently (11/4/10, one of Felix's
patches).

That is probably why no one else is hitting this yet, and it
isn't needed for stable I'm guessing....

Thanks,
Ben

>
> Vasanth
>
>>
>> Thanks,
>> Ben
>>
>>>
>>> Vasanth
>>
>>
>> --
>> Ben Greear<greearb@candelatech.com>
>> Candela Technologies Inc  http://www.candelatech.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] ath9k: Check for NULL sta in ath_tx_start

[Vasanthakumar Thiagarajan]

On Tue, Dec 07, 2010 at 10:43:38PM +0530, Ben Greear wrote:
> On 12/07/2010 12:31 AM, Vasanthakumar Thiagarajan wrote:
> > On Tue, Dec 07, 2010 at 01:19:22PM +0530, Ben Greear wrote:
> >> On 12/06/2010 11:42 PM, Vasanthakumar Thiagarajan wrote:
> >>> On Tue, Dec 07, 2010 at 11:06:24AM +0530, Ben Greear wrote:
> >>>> On 12/06/2010 09:21 PM, Luis R. Rodriguez wrote:
> >>>>> On Mon, Dec 6, 2010 at 9:13 PM,<greearb@gmail.com>    wrote:
> >>>>>> From: Ben Greear<greearb@candelatech.com>
> >>>>>>
> >>>>>> It can be NULL according to docs, and logging showed it
> >>>>>> to be NULL in practice.
> >>>>>>
> >>>>>> Signed-off-by: Ben Greear<greearb@candelatech.com>
> >>>>>
> >>>>> Does this fix an oops? If so can you explain and provide the trace and
> >>>>> resubmit and cc stable in the commit log?
> >>>>
> >>>> I think it fixes the TID corruption I posted about earlier.  It seems
> >>>> so obvious though, that I'm curious why no one else sees problems,
> >>>> and why I didn't see more crashes in my setup.
> >>>>
> >>>> (The paprd code appears to send with null STA, for instance, and my
> >>>> printks showed lots of NULL stas in my 16-sta test setup, though I
> >>>> don't think I'm using the paprd code path.)
> >>>
> >>> paprd is used only with>= AR9003.
> >>
> >> Whoever coded it up hopefully had that hardware...so why didn't
> >> they see lots of crashes?
> >
> > I myself tested it lot of times, but did not see any crash, weird.
> 
> Looks like the offending change when in recently (11/4/10, one of Felix's
> patches).
> 
> That is probably why no one else is hitting this yet, and it
> isn't needed for stable I'm guessing....

Nice, care to send a patch?. Yeah, it is not needed for stable.

Vasanth

Re: [PATCH] ath9k: Check for NULL sta in ath_tx_start

[Ben Greear]

On 12/07/2010 09:17 PM, Vasanthakumar Thiagarajan wrote:
> On Tue, Dec 07, 2010 at 10:43:38PM +0530, Ben Greear wrote:
>> On 12/07/2010 12:31 AM, Vasanthakumar Thiagarajan wrote:
>>> On Tue, Dec 07, 2010 at 01:19:22PM +0530, Ben Greear wrote:
>>>> On 12/06/2010 11:42 PM, Vasanthakumar Thiagarajan wrote:
>>>>> On Tue, Dec 07, 2010 at 11:06:24AM +0530, Ben Greear wrote:
>>>>>> On 12/06/2010 09:21 PM, Luis R. Rodriguez wrote:
>>>>>>> On Mon, Dec 6, 2010 at 9:13 PM,<greearb@gmail.com>     wrote:
>>>>>>>> From: Ben Greear<greearb@candelatech.com>
>>>>>>>>
>>>>>>>> It can be NULL according to docs, and logging showed it
>>>>>>>> to be NULL in practice.
>>>>>>>>
>>>>>>>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>>>>>>>
>>>>>>> Does this fix an oops? If so can you explain and provide the trace and
>>>>>>> resubmit and cc stable in the commit log?
>>>>>>
>>>>>> I think it fixes the TID corruption I posted about earlier.  It seems
>>>>>> so obvious though, that I'm curious why no one else sees problems,
>>>>>> and why I didn't see more crashes in my setup.
>>>>>>
>>>>>> (The paprd code appears to send with null STA, for instance, and my
>>>>>> printks showed lots of NULL stas in my 16-sta test setup, though I
>>>>>> don't think I'm using the paprd code path.)
>>>>>
>>>>> paprd is used only with>= AR9003.
>>>>
>>>> Whoever coded it up hopefully had that hardware...so why didn't
>>>> they see lots of crashes?
>>>
>>> I myself tested it lot of times, but did not see any crash, weird.
>>
>> Looks like the offending change when in recently (11/4/10, one of Felix's
>> patches).
>>
>> That is probably why no one else is hitting this yet, and it
>> isn't needed for stable I'm guessing....
>
> Nice, care to send a patch?. Yeah, it is not needed for stable.

The same patch I originally posted to to start this thread
should do just fine.

If someone really wants an oops report, I can add an artificial BUG_ON
to catch this, but for whatever reason, it normally just crashes things
weirdly and perhaps sometimes screwed up the file-system, so I'd rather
just have the patch go in as it is..I think it is obviously more correct
than the existing code.

Thanks,
Ben

>
> Vasanth


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com