rt2x00: Fix NULL pointer error in adhoc/master mode
PATCH 1/1 rt2x00: Fix NULL pointer error in adhoc/master mode
As soon as an interface is enabled, and that interface is in adhoc or master mode, the device will start raising beacondone interrupts. But before the first interrupt is raised, mac80211 will probably not have send any beacons to the device yet, which results in a NULL pointer error when the skb is being freed.
Note that the "raise beacondone interrupts without a beacon" is also a bug, and will be addressed later. The more important bug however is preventing the NULL pointer failt itself, since there might be other conditions that could trigger it as well.
---
drivers/net/wireless/rt2x00/rt2x00queue.c +3 -0
@@ -107,6 +107,9 @@ void rt2x00queue_unmap_skb(struct rt2x00_dev *rt2x00dev, struct sk_buff *skb)void rt2x00queue_free_skb (struct rt2x00_dev *rt2x00dev,struct sk_buff *skb) { +if (!skb) +return ; +rt2x00queue_unmap_skb (rt2x00dev, skb);dev_kfree_skb_any (skb); }
Ivo van Doorn wrote:
...
Quoted 2 lines
Note that the "raise beacondone interrupts without a beacon" is also a bug, and will be addressed later.
...
FWIW, I've never been able to figure out a way to shut it off in the legacy drivers. Unless there's some mechanism I've missed, I would consider this a bug, but its a hardware bug because - in the rt2500 driver at least - even disabling it in the interrupt mask register has no effect.
I think it would be more accurately termed a target beacon transmission time interrupt. The rt61 driver's ISR simply does not test for the condition. The rt2400 and rt2500 drivers do nothing if not in adhoc mode. If in adhoc mode, they both assume a beacon frame is ready to go and send it. This relies on a canned frame that is set up during initialization.
Hope this is helpful,
Signature · 5 lines
Bryan - "Beyond the point of no return, there's no going back." Narration to "Mars Rising" on the Science Channel
Quoted 11 lines · Bryan Batten
Ivo van Doorn wrote:
...Quoted 2 lines
Note that the "raise beacondone interrupts without a beacon" is also a bug, and will be addressed later.
...
FWIW, I've never been able to figure out a way to shut it off in the legacy drivers. Unless there's some mechanism I've missed, I would consider this a bug, but its a hardware bug because - in the rt2500 driver at least - even disabling it in the interrupt mask register has no effect.
That is interesting, have you tried controlling CSR14 and more specifically the TBCN or BeaconGen fields?
Quoted 6 lines
I think it would be more accurately termed a target beacon transmission time interrupt. The rt61 driver's ISR simply does not test for the condition. The rt2400 and rt2500 drivers do nothing if not in adhoc mode. If in adhoc mode, they both assume a beacon frame is ready to go and send it. This relies on a canned frame that is set up during initialization.
Hmm, in that case I have to fix something in the beacondone handler to prevent it requesting a beacon when mac80211 hasn't supplied anything yet..
Thanks for the notice. :)
Ivo
Johannes Berg
maintainerQuoted 4 lines
Hmm, in that case I have to fix something in the beacondone handler to prevent it requesting a beacon when mac80211 hasn't supplied anything yet..
I was working in this area... Can you take a quick look at this patch and let me know what you think?
http://johannes.sipsolutions.net/patches/kernel/all/2008-07-05-18%3a42/mac80211-beacon-revamp.patch
johannes
Quoted 8 lines · Johannes Berg
Quoted 4 lines
Hmm, in that case I have to fix something in the beacondone handler to prevent it requesting a beacon when mac80211 hasn't supplied anything yet..
I was working in this area... Can you take a quick look at this patch and let me know what you think?
http://johannes.sipsolutions.net/patches/kernel/all/2008-07-05-18%3a42/mac80211-beacon-revamp.patch
Patch looks good to me, it wouldn't change anything about the above since t2x00 would still call ieee80211_beacon_get() when the beacon wasn't provided by mac80211 yet.
But the flags that mark what has changed is definately a good idea, that will prevent some redundant register writes. :)
Ivo
Johannes Berg
maintainerQuoted 6 lines · Ivo van Doorn
Quoted 5 lines · Johannes Berg
Quoted 4 lines
Hmm, in that case I have to fix something in the beacondone handler to prevent it requesting a beacon when mac80211 hasn't supplied anything yet..
Quoted 6 lines
Quoted 3 lines
I was working in this area... Can you take a quick look at this patch and let me know what you think?
http://johannes.sipsolutions.net/patches/kernel/all/2008-07-05-18%3a42/mac80211-beacon-revamp.patchPatch looks good to me, it wouldn't change anything about the above since t2x00 would still call
ieee80211_beacon_get()when the beacon wasn't provided by mac80211 yet.
Oh, of course, I just was writing on the patch and read your mail and replied instead of sending you another one :)
One thing I'm worried about is the sequence numbers, but that bug is already there... IIRC you needed software sequence numbers for your drivers, but what about the beacons? I haven't _changed_ anything, but IBSS beacons seem to not get a software sequence number.
Quoted 2 lines
But the flags that mark what has changed is definately a good idea, that will prevent some redundant register writes. :)
:)
Only once you use the flags. My patch is intentionally minimal.
johannes
Quoted 14 lines · Johannes Berg
Quoted 6 lines · Ivo van Doorn
Quoted 5 lines · Johannes Berg
Quoted 4 lines
Hmm, in that case I have to fix something in the beacondone handler to prevent it requesting a beacon when mac80211 hasn't supplied anything yet..
Quoted 6 lines
Quoted 3 lines
I was working in this area... Can you take a quick look at this patch and let me know what you think?
http://johannes.sipsolutions.net/patches/kernel/all/2008-07-05-18%3a42/mac80211-beacon-revamp.patchPatch looks good to me, it wouldn't change anything about the above since t2x00 would still call
ieee80211_beacon_get()when the beacon wasn't provided by mac80211 yet.
Quoted 7 lines
Oh, of course, I just was writing on the patch and read your mail and replied instead of sending you another one :)
One thing I'm worried about is the sequence numbers, but that bug is already there... IIRC you needed software sequence numbers for your drivers, but what about the beacons? I haven't _changed_ anything, but IBSS beacons seem to not get a software sequence number.
That problem is only in rt2400pci and rt2500pci who raise the beacon interrupt, all other drivers have the template and auto sequence numbers.
But no sequence numbers for the beacons, I think that will definately cause problems for those 2 drivers.
But the function ieee80211_include_sequence() is being called from within the ieee80211_beacon_get() function. Only for the Mesh it doesn't seem to be called.
Quoted 5 lines
Quoted 2 lines
But the flags that mark what has changed is definately a good idea, that will prevent some redundant register writes. :)
:)
Only once you use the flags. My patch is intentionally minimal.
Not a problem, with that patch some other beaconing in the rt2x00 code will change anyway. :)
Ivo
Johannes Berg
maintainerQuoted 3 lines
That problem is only in rt2400pci and rt2500pci who raise the beacon interrupt, all other drivers have the template and auto sequence numbers.
Ok.
Quoted 5 lines
But no sequence numbers for the beacons, I think that will definately cause problems for those 2 drivers.
But the function
ieee80211_include_sequence()is being called from within theieee80211_beacon_get()function. Only for the Mesh it doesn't seem to be called.
Well it's not called from where the IBSS beacon is set either, and the IBSS beacon isn't generated every time anyway. So I'm confused now. Do rt2{4,5}00pci not support IBSS?
johannes
Quoted 9 lines · Johannes Berg
Quoted 3 lines
That problem is only in rt2400pci and rt2500pci who raise the beacon interrupt, all other drivers have the template and auto sequence numbers.
Ok.
Quoted 3 lines
But no sequence numbers for the beacons, I think that will definately cause problems for those 2 drivers.
Quoted 6 lines
Quoted 2 lines
But the function
ieee80211_include_sequence()is being called from within theieee80211_beacon_get()function. Only for the Mesh it doesn't seem to be called.Well it's not called from where the IBSS beacon is set either, and the IBSS beacon isn't generated every time anyway. So I'm confused now. Do rt2{4,5}00pci not support IBSS?
Well it does claim support for it, but it has been enabled only since 2.6.26, so the number of people who have tested it is very limited. And so far little to no success stories...
The main trick in rt2x00lib is that it doesn't care if it is an IBSS or AP beacon, it will always use the update_beacon() callback function to write the beacon to the hardware. It doesn't need to differentiate between the beacon types (as far as I know, master mode isn't implemented in the legacy drivers, and the only claim about if it is possible cones from Asus who owns the closed source driver with AP functionality).
So for the Ralink hardware it just needs a completely valid beacon which it can transmit, only for rt2400pci/rt2500pci it requires that the sequence number is set due to lacking hardware sequence counting.
After that those 2 will raise an interrupt every beacon interval to request a new beacon, which rt2x00lib fulfills by calling ieee80211_beacon_get().
Ivo
Ivo van Doorn wrote:
...
Quoted 2 lines
That is interesting, have you tried controlling CSR14 and more specifically the TBCN or BeaconGen fields?
No. It turns out that if I set the RT2500's CSR14 to zero (the default power on state according to the data sheet) at driver initialization, there are no beacon interrupts until enabled in AsicEnableIbssSync(). I deem this a bug, the fix for which in now in CVS.
Thanks,
Signature · 5 lines
Bryan - "Beyond the point of no return, there's no going back." Narration to "Mars Rising" on the Science Channel
Quoted 9 lines · Bryan Batten
Ivo van Doorn wrote:
...Quoted 2 lines
That is interesting, have you tried controlling CSR14 and more specifically the TBCN or BeaconGen fields?
No. It turns out that if I set the RT2500's CSR14 to zero (the default power on state according to the data sheet) at driver initialization, there are no beacon interrupts until enabled in
AsicEnableIbssSync(). I deem this a bug, the fix for which in now in CVS.
Heh, that will bring down the number of raised interrupts in the legacy driver. :) You might want to add it to rt2400 as well, I expect the same behavior for that. ;)
I'll put the initialization into rt2x00 as well, since it doesn't set it to 0 by default either.
Ivo
Quoted 2 lines · Ivo van Doorn
Quoted 1 line · Bryan Batten
Ivo van Doorn wrote:
...
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Note that the "raise beacondone interrupts without a beacon" is
also a bug, and will be addressed later.
...
Quoted 1 line
Quoted 1 line
FWIW, I've never been able to figure out a way to shut it off in the legacy drivers. Unless there's some mechanism I've missed, I would consider this a bug, but its a hardware bug because - in the rt2500 driver at least - even disabling it in the interrupt mask register has no effect.
Looking thru the RT2400 and RT2500 datasheets, I notice that on pp. 12 of the 2400 datasheet, CSR8 bits 0 and 1 are described. On the same page of the RT2500 datasheet, CSR8 bits 0 and 1 are not described. If that is not a typo, that means to me that on the RT2400 you can mask off beacon timer and wakeup timer interrupts with CSR8, but on the RT2500 you cannot.
Would it be possible to verify with Ralink that this is not a typo in the RT2500 datasheet?
Thanks,
Signature · 5 lines
Bryan - "Beyond the point of no return, there's no going back." Narration to "Mars Rising" on the Science Channel
Quoted 13 lines · Bryan Batten
Quoted 2 lines · Ivo van Doorn
Quoted 1 line · Bryan Batten
Ivo van Doorn wrote:
...
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Note that the "raise beacondone interrupts without a beacon" is
also a bug, and will be addressed later....
Quoted 5 lines
Quoted 5 lines
FWIW, I've never been able to figure out a way to shut it off in the legacy drivers. Unless there's some mechanism I've missed, I would consider this a bug, but its a hardware bug because - in the rt2500 driver at least - even disabling it in the interrupt mask register has no effect.
Quoted 6 lines
Looking thru the RT2400 and RT2500 datasheets, I notice that on pp. 12 of the 2400 datasheet, CSR8 bits 0 and 1 are described. On the same page of the RT2500 datasheet, CSR8 bits 0 and 1 are not described. If that is not a typo, that means to me that on the RT2400 you can mask off beacon timer and wakeup timer interrupts with CSR8, but on the RT2500 you cannot.
Yes, but if you look again you see that the legacy driver does define it, and even includes the description of bits 9 -> 19
And the legacy driver source code has always been more reliable then the datasheet, so I would put my money on the code. ;)
Quoted 2 lines
Would it be possible to verify with Ralink that this is not a typo in the RT2500 datasheet?
I'll see what I can find out.
Ivo
Ivo van Doorn wrote:
...
Quoted 3 lines
Yes, but if you look again you see that the legacy driver does
define it, and even includes the description of bits 9 -> 19 And
the legacy driver source code has always been more reliable then the datasheet, so I would put my money on the code. ;)
I know. But I suspect that in this case, the datasheet just *may* be correct.
Quoted 5 lines
Quoted 2 lines
Would it be possible to verify with Ralink that this is not a typo in the RT2500 datasheet?
I'll see what I can find out.
Thanks,
Signature · 5 lines
Bryan - "Beyond the point of no return, there's no going back." Narration to "Mars Rising" on the Science Channel