[PATCH] softmac: remove netif_tx_disable when scanning

[PATCH] softmac: remove netif_tx_disable when scanning

[Larry Finger]

From: Michael Buesch <mb-fseUSCV1ubazQB+pC5nmwQ@public.gmane.org>

In the scan section of ieee80211softmac, network transmits are disabled.
Clearly, this does not make any sense as transmit is necessary for active
scanning, and transmits are not used when passive scanning. In addition,
when SoftMAC re-enables transmits, it may override the wishes of a driver
that may have very good reasons for disabling transmits. No specific
problems can be blamed directly on this bug, but it may be responsible
for some unexplained behavior.

Signed-off-by: Michael Buesch <mb-fseUSCV1ubazQB+pC5nmwQ@public.gmane.org>
Signed-off-by: Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
---

John,

Please apply this to wireless-2.6 and push it upstream for inclusion in 2.6.19,
if possible. I will be submitting it to 2.6.18.Y.

Larry


Index: linux-2.6.19-rc5/net/ieee80211/softmac/ieee80211softmac_scan.c
===================================================================
--- linux-2.6.19-rc5.orig/net/ieee80211/softmac/ieee80211softmac_scan.c
+++ linux-2.6.19-rc5/net/ieee80211/softmac/ieee80211softmac_scan.c
@@ -47,7 +47,6 @@ ieee80211softmac_start_scan(struct ieee8
 	sm->scanning = 1;
 	spin_unlock_irqrestore(&sm->lock, flags);
 
-	netif_tx_disable(sm->ieee->dev);
 	ret = sm->start_scan(sm->dev);
 	if (ret) {
 		spin_lock_irqsave(&sm->lock, flags);
@@ -248,7 +246,6 @@ void ieee80211softmac_scan_finished(stru
 		if (net)
 			sm->set_channel(sm->dev, net->channel);
 	}
-	netif_wake_queue(sm->ieee->dev);
 	ieee80211softmac_call_events(sm, IEEE80211SOFTMAC_EVENT_SCAN_FINISHED, NULL);
 }
 EXPORT_SYMBOL_GPL(ieee80211softmac_scan_finished);

---

Re: [PATCH] softmac: remove netif_tx_disable when scanning

[Daniel Drake]

Larry Finger wrote:
> In the scan section of ieee80211softmac, network transmits are disabled.
> Clearly, this does not make any sense as transmit is necessary for active
> scanning, and transmits are not used when passive scanning.

Probe request frames are generated by softmac and do not go through the 
TX queue. Disabling the TX queue has no effect on that.

Surely disabling the queue actually makes sense, in order to avoid 
frames designated for the "current session" being transmitted on 
different channels during scanning?

Daniel

Re: [PATCH] softmac: remove netif_tx_disable when scanning

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 898 bytes --]

On Sat, 2006-11-25 at 23:05 -0500, Daniel Drake wrote:

> Surely disabling the queue actually makes sense, in order to avoid 
> frames designated for the "current session" being transmitted on 
> different channels during scanning?

The problem is that queue disabling isn't refcounted so that a scan that
collides with bcm43xx having disabled the queue for calibration might
re-enable the queue while bcm43xx is still calibrating.

Clearly, this doesn't fully fix the problem because softmac will try to
transmit frames during the calibration. Hence, a proper fix would be to
not remove the calls to netif_tx_disable but make them go through
softmac (ieee80211_tx_disable) to make sure that softmac doesn't try to
scan while the queues are disabled, which would fix the aforementioned
problem of softmac enabling the queue while the driver needs it disabled
for free.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] softmac: remove netif_tx_disable when scanning

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 670 bytes --]

On Sat, 2006-11-25 at 23:05 -0500, Daniel Drake wrote:

> Surely disabling the queue actually makes sense, in order to avoid 
> frames designated for the "current session" being transmitted on 
> different channels during scanning?

Also, for bcm43xx this isn't a problem since the firmware (optionally
but we use that afaik) takes care of not transmitting frames that are
tagged for a different channel than currently tuned to. This is
necessary because the packets travel through a FIFO and radio tuning is
done by the driver without it knowing how much is still in the FIFO and
a full disable is quite expensive (in terms of the time needed.)

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 179 bytes --]

_______________________________________________
Bcm43xx-dev mailing list
Bcm43xx-dev-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev

Re: [PATCH] softmac: remove netif_tx_disable when scanning

[Michael Buesch]

On Sunday 26 November 2006 11:03, Johannes Berg wrote:
> On Sat, 2006-11-25 at 23:05 -0500, Daniel Drake wrote:
> 
> > Surely disabling the queue actually makes sense, in order to avoid 
> > frames designated for the "current session" being transmitted on 
> > different channels during scanning?
> 
> The problem is that queue disabling isn't refcounted so that a scan that
> collides with bcm43xx having disabled the queue for calibration might
> re-enable the queue while bcm43xx is still calibrating.
> 
> Clearly, this doesn't fully fix the problem because softmac will try to
> transmit frames during the calibration. Hence, a proper fix would be to
> not remove the calls to netif_tx_disable but make them go through
> softmac (ieee80211_tx_disable) to make sure that softmac doesn't try to
> scan while the queues are disabled, which would fix the aforementioned
> problem of softmac enabling the queue while the driver needs it disabled
> for free.

Yeah, but I don't think it's worth fixing. ;)
Simply removing this tx_disable in softmac fixes a _big_ bug
and that must be applied. The bug of being able to scan while
calibrating is very minor and not worth to fix. Fixing this
will likely introduce other bugs. It's simply not worth it
anymore.

This tx_disable remove patch must be applied, because it fixes
actual locking issues in the driver(s). And I think it is responsible
for several mysterious bugs we have seen in the past. I cannot
prove that, but I am pretty sure.
It must also be pushed for inclusion in -stable.

-- 
Greetings Michael.

Re: [PATCH] softmac: remove netif_tx_disable when scanning

[Michael Buesch]

On Sunday 26 November 2006 05:05, Daniel Drake wrote:
> Larry Finger wrote:
> > In the scan section of ieee80211softmac, network transmits are disabled.
> > Clearly, this does not make any sense as transmit is necessary for active
> > scanning, and transmits are not used when passive scanning.
> 
> Probe request frames are generated by softmac and do not go through the 
> TX queue. Disabling the TX queue has no effect on that.

That's just yet another problem. ;)
The driver has its damn good reasons to disable the queue. (And only
the driver should be allowed to disable that queue). Softmac ignoring
this queue-disabled flag is just yet another bug.

> Surely disabling the queue actually makes sense, in order to avoid 
> frames designated for the "current session" being transmitted on 
> different channels during scanning?

This should be avoided by mechanisms in the firmware, as that's
the only race-free place where a "Am I on the correct channel"
check can happen. Additionally, the ieee80211 stack should
keep track of the currently set channel and don't transmit frames
on the wrong channel.

But I don't really think this is worth fixing, as it's all already
fixed in d80211 (Well, these bugs have never been in there afaik ;) ).

-- 
Greetings Michael.

Re: [PATCH] softmac: remove netif_tx_disable when scanning

[Larry Finger]

Michael Buesch wrote:
> On Sunday 26 November 2006 11:03, Johannes Berg wrote:
>> On Sat, 2006-11-25 at 23:05 -0500, Daniel Drake wrote:
>>
>>> Surely disabling the queue actually makes sense, in order to avoid 
>>> frames designated for the "current session" being transmitted on 
>>> different channels during scanning?
>> The problem is that queue disabling isn't refcounted so that a scan that
>> collides with bcm43xx having disabled the queue for calibration might
>> re-enable the queue while bcm43xx is still calibrating.
>>
>> Clearly, this doesn't fully fix the problem because softmac will try to
>> transmit frames during the calibration. Hence, a proper fix would be to
>> not remove the calls to netif_tx_disable but make them go through
>> softmac (ieee80211_tx_disable) to make sure that softmac doesn't try to
>> scan while the queues are disabled, which would fix the aforementioned
>> problem of softmac enabling the queue while the driver needs it disabled
>> for free.
> 
> Yeah, but I don't think it's worth fixing. ;)
> Simply removing this tx_disable in softmac fixes a _big_ bug
> and that must be applied. The bug of being able to scan while
> calibrating is very minor and not worth to fix. Fixing this
> will likely introduce other bugs. It's simply not worth it
> anymore.

I actually found one of these while testing WX locking as suggested by Dan Williams. His method has 
two scripts running at the same time with one of them doing an 'iwlist ethX scan' and the other 
executing an 'iwconfig ethX' as fast as possible. Running this test for 36 hours failed to indicate 
any locking errors, but running it without the patch under discussion led to a number of the 
following errors:

kernel: bcm43xx: ASSERTION FAILED (!ring->suspended) at: 
drivers/net/wireless/bcm43xx/bcm43xx_dma.c:71:request_slot()

This error _NEVER_ happened when softmac was not allowed to disable TX.

Larry

Re: [PATCH] softmac: remove netif_tx_disable when scanning

[Michael Buesch]

On Sunday 26 November 2006 14:05, Larry Finger wrote:
> Michael Buesch wrote:
> > On Sunday 26 November 2006 11:03, Johannes Berg wrote:
> >> On Sat, 2006-11-25 at 23:05 -0500, Daniel Drake wrote:
> >>
> >>> Surely disabling the queue actually makes sense, in order to avoid 
> >>> frames designated for the "current session" being transmitted on 
> >>> different channels during scanning?
> >> The problem is that queue disabling isn't refcounted so that a scan that
> >> collides with bcm43xx having disabled the queue for calibration might
> >> re-enable the queue while bcm43xx is still calibrating.
> >>
> >> Clearly, this doesn't fully fix the problem because softmac will try to
> >> transmit frames during the calibration. Hence, a proper fix would be to
> >> not remove the calls to netif_tx_disable but make them go through
> >> softmac (ieee80211_tx_disable) to make sure that softmac doesn't try to
> >> scan while the queues are disabled, which would fix the aforementioned
> >> problem of softmac enabling the queue while the driver needs it disabled
> >> for free.
> > 
> > Yeah, but I don't think it's worth fixing. ;)
> > Simply removing this tx_disable in softmac fixes a _big_ bug
> > and that must be applied. The bug of being able to scan while
> > calibrating is very minor and not worth to fix. Fixing this
> > will likely introduce other bugs. It's simply not worth it
> > anymore.
> 
> I actually found one of these while testing WX locking as suggested by Dan Williams. His method has 
> two scripts running at the same time with one of them doing an 'iwlist ethX scan' and the other 
> executing an 'iwconfig ethX' as fast as possible. Running this test for 36 hours failed to indicate 
> any locking errors, but running it without the patch under discussion led to a number of the 
> following errors:
> 
> kernel: bcm43xx: ASSERTION FAILED (!ring->suspended) at: 
> drivers/net/wireless/bcm43xx/bcm43xx_dma.c:71:request_slot()
> 
> This error _NEVER_ happened when softmac was not allowed to disable TX.

Yeah, that's the error I expected.
Nice that you proved my point for this being an important patch. ;)

-- 
Greetings Michael.

Re: [PATCH] softmac: remove netif_tx_disable when scanning

[Daniel Drake]

Johannes Berg wrote:
> The problem is that queue disabling isn't refcounted so that a scan that
> collides with bcm43xx having disabled the queue for calibration might
> re-enable the queue while bcm43xx is still calibrating.

I agree with that part. However the other reason for the patch (transmit 
queue needed for active scanning) is bogus, and the patch introduces a 
problem where session frames may be transmitted during scanning (using 
TX queue control avoids that problem).

> Clearly, this doesn't fully fix the problem because softmac will try to
> transmit frames during the calibration. Hence, a proper fix would be to
> not remove the calls to netif_tx_disable but make them go through
> softmac (ieee80211_tx_disable) to make sure that softmac doesn't try to
> scan while the queues are disabled, which would fix the aforementioned
> problem of softmac enabling the queue while the driver needs it disabled
> for free.

Stack-level refcounted TX control like this would also be beneficial for 
zd1211rw, currently we have a semi-ugly implementation inside the driver.

> Also, for bcm43xx this isn't a problem since the firmware (optionally
> but we use that afaik) takes care of not transmitting frames that are
> tagged for a different channel than currently tuned to.

zd1211 has no such functionality :(

Michael Buesch wrote:
> Softmac ignoring
> this queue-disabled flag is just yet another bug.

Agreed, but this one isn't going to be fixed any time soon. This was the 
first point I raised against softmac during early zd1211rw development a 
long while back.

I agree with the objectives of this patch but the way I see it is that 
it trades one bug for another. A proper solution, as suggested by 
Johannes (refcounted stack-level TX control) would not be hard to 
implement and would solve the bug without introducing another.

Daniel

Re: [PATCH] softmac: remove netif_tx_disable when scanning

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

On Sun, 2006-11-26 at 11:37 -0500, Daniel Drake wrote:

> However the other reason for the patch (transmit 
> queue needed for active scanning) is bogus,

I think that was just a misunderstanding.

> and the patch introduces a 
> problem where session frames may be transmitted during scanning (using 
> TX queue control avoids that problem).

Which is really the reason why we put that there in the first place :)

> Stack-level refcounted TX control like this would also be beneficial for 
> zd1211rw, currently we have a semi-ugly implementation inside the driver.

> I agree with the objectives of this patch but the way I see it is that 
> it trades one bug for another. A proper solution, as suggested by 
> Johannes (refcounted stack-level TX control) would not be hard to 
> implement and would solve the bug without introducing another.

Would you actually need a fully refcounted enable/disable? Because for
the stack it wouldn't be required if it'd simply not start scanning when
queue is disabled and stop scanning immediately when queue stop is
requested.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] softmac: remove netif_tx_disable when scanning

[Daniel Drake]

Johannes Berg wrote:
> Would you actually need a fully refcounted enable/disable?

No, we could stick with our existing setup if the stack did something 
different.

> Because for
> the stack it wouldn't be required if it'd simply not start scanning when
> queue is disabled and stop scanning immediately when queue stop is
> requested.

That would work, assuming there is a way to make the stack listen for 
the "TX queue stopped" event (or would the ieee80211_tx_disable wrapper 
take care of that?).

Daniel

Re: [PATCH] softmac: remove netif_tx_disable when scanning

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 660 bytes --]

On Sun, 2006-11-26 at 13:25 -0500, Daniel Drake wrote:

> No, we could stick with our existing setup if the stack did something 
> different.

But you do need full refcounting I guess.

> > Because for
> > the stack it wouldn't be required if it'd simply not start scanning when
> > queue is disabled and stop scanning immediately when queue stop is
> > requested.
> 
> That would work, assuming there is a way to make the stack listen for 
> the "TX queue stopped" event (or would the ieee80211_tx_disable wrapper 
> take care of that?).

A wrapper is what I had in mind, I don't think there's any notification
for these things.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] softmac: remove netif_tx_disable when scanning

[Larry Finger]

Johannes Berg wrote:
> On Sun, 2006-11-26 at 13:25 -0500, Daniel Drake wrote:
> 
>> No, we could stick with our existing setup if the stack did something 
>> different.
> 
> But you do need full refcounting I guess.
> 
>>> Because for
>>> the stack it wouldn't be required if it'd simply not start scanning when
>>> queue is disabled and stop scanning immediately when queue stop is
>>> requested.
>> That would work, assuming there is a way to make the stack listen for 
>> the "TX queue stopped" event (or would the ieee80211_tx_disable wrapper 
>> take care of that?).
> 
> A wrapper is what I had in mind, I don't think there's any notification
> for these things.

If someone will prepare a patch that will fix this, I can test it. Otherwise, I'm prepared to 
substitute the bug that doesn't hurt anything for the one that I have shown does cause problems.

Larry

Re: [PATCH] softmac: remove netif_tx_disable when scanning

[Daniel Drake]

Johannes Berg wrote:
> On Sun, 2006-11-26 at 13:25 -0500, Daniel Drake wrote:
> 
>> No, we could stick with our existing setup if the stack did something 
>> different.
> 
> But you do need full refcounting I guess.

No, I don't think we would need to move away from our existing method:

   spin_lock_irqsave(&mac->lock, flags);
   if (mac->updating_rts_rate == 0 && mac->updating_basic_rates == 0)
     netif_wake_queue(mac->netdev);
   spin_unlock_irqrestore(&mac->lock, flags);

That said, it is in some ways comparable to refcounting without a 
counter: if both flags are set to 0, the above code is guaranteed to be 
executed twice (each time after one flag is toggled)

Daniel

Re: [PATCH] softmac: remove netif_tx_disable when scanning

[Michael Buesch]

On Sunday 26 November 2006 17:37, Daniel Drake wrote:
> Johannes Berg wrote:
> > The problem is that queue disabling isn't refcounted so that a scan that
> > collides with bcm43xx having disabled the queue for calibration might
> > re-enable the queue while bcm43xx is still calibrating.
> 
> I agree with that part. However the other reason for the patch (transmit 
> queue needed for active scanning) is bogus, and the patch introduces a 
> problem where session frames may be transmitted during scanning (using 
> TX queue control avoids that problem).
> 
> > Clearly, this doesn't fully fix the problem because softmac will try to
> > transmit frames during the calibration. Hence, a proper fix would be to
> > not remove the calls to netif_tx_disable but make them go through
> > softmac (ieee80211_tx_disable) to make sure that softmac doesn't try to
> > scan while the queues are disabled, which would fix the aforementioned
> > problem of softmac enabling the queue while the driver needs it disabled
> > for free.
> 
> Stack-level refcounted TX control like this would also be beneficial for 
> zd1211rw, currently we have a semi-ugly implementation inside the driver.
> 
> > Also, for bcm43xx this isn't a problem since the firmware (optionally
> > but we use that afaik) takes care of not transmitting frames that are
> > tagged for a different channel than currently tuned to.
> 
> zd1211 has no such functionality :(
> 
> Michael Buesch wrote:
> > Softmac ignoring
> > this queue-disabled flag is just yet another bug.
> 
> Agreed, but this one isn't going to be fixed any time soon. This was the 
> first point I raised against softmac during early zd1211rw development a 
> long while back.
> 
> I agree with the objectives of this patch but the way I see it is that 
> it trades one bug for another. A proper solution, as suggested by 

Yeah, it trades one fat big bug against a very minor one.
AND!! Currently it's possible to transmit frames while scanning, too.
Don't forget that. Today we have both bugs. With this patch we have only
one left. The only "issue" is that today the "transmit packet while
scanning" bug is less likely to trigger. But it's still possible (because
of disabling not being refcounted).

> Johannes (refcounted stack-level TX control) would not be hard to 
> implement and would solve the bug without introducing another.

So, please do so.
But please apply this one regardless of what you do. ;)
It fixes a nasty kind of bug. With this trivial to fix bug
in the tree I won't look at periodic work "bugs" anymore, that
are reported now and then, because I really thing they are all
caused by this hidden bug.
I also saw a real life bugreport of the asserion in the DMA code
triggering. I didn't know back than how that was possible to happen,
but now I know.

Besides that, what's the problem with some bogus frames being transmitted
throughout the short period of scanning? I don't really see how that
can hurt anyone (besides consuming minor bandwidth).

I already said it in the past. We _have_ to trade one bug for the other
in softmac, because it's buggy by design. And it really doesn't make sense
to fix them all. Well, actually it does. Because fixing all these bugs
would mean replacing softmac by d80211. There's no real other way. ;)

-- 
Greetings Michael.