[PATCH] b43: fix ieee80211_rx() context

[PATCH] b43: fix ieee80211_rx() context

[Johannes Berg]

Due to the way it interacts with the networking
stack and other parts of mac80211, ieee80211_rx()
must be called with disabled softirqs.

Michael, the former maintainer of this driver,
has refused to fix the problem this way instead
proposing a much more invasive patch that could
not even be proved correct wrt. locking inside
mac80211. Regardless of that, he believes this
to be a bug in mac80211, and has also publicly
stated [1] that he does not care about this even
though it is a regression introduced by his own
patches.

Since nobody else seems to be wanting to fix the
problem, I'll just fix it for the benefit of the
many users of this driver.

[1] http://thread.gmane.org/gmane.linux.kernel.wireless.general/39440/focus=40266

Reported-by: Dave Young <hidave.darkstar@gmail.com>
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 drivers/net/wireless/b43/xmit.c |    3 +++
 1 file changed, 3 insertions(+)

--- wireless-testing.orig/drivers/net/wireless/b43/xmit.c	2009-10-11 12:11:50.000000000 +0200
+++ wireless-testing/drivers/net/wireless/b43/xmit.c	2009-10-11 12:12:06.000000000 +0200
@@ -690,7 +690,10 @@ void b43_rx(struct b43_wldev *dev, struc
 	}
 
 	memcpy(IEEE80211_SKB_RXCB(skb), &status, sizeof(status));
+
+	local_bh_disable();
 	ieee80211_rx(dev->wl->hw, skb);
+	local_bh_enable();
 
 #if B43_DEBUG
 	dev->rx_count++;

Re: [PATCH] b43: fix ieee80211_rx() context

[Michael Buesch]

On Sunday 11 October 2009 12:19:21 Johannes Berg wrote:
> Due to the way it interacts with the networking
> stack and other parts of mac80211, ieee80211_rx()
> must be called with disabled softirqs.

Is this stated in the documentation somewhere?

> Michael, the former maintainer of this driver,
> has refused to fix the problem this way instead
> proposing a much more invasive patch that could
> not even be proved correct wrt. locking inside
> mac80211. Regardless of that, he believes this
> to be a bug in mac80211, and has also publicly
> stated [1] that he does not care about this even
> though it is a regression introduced by his own
> patches.

What if we leave slander out of the commit messages?

> Since nobody else seems to be wanting to fix the
> problem, I'll just fix it for the benefit of the
> many users of this driver.
> 
> [1] http://thread.gmane.org/gmane.linux.kernel.wireless.general/39440/focus=40266
> 
> Reported-by: Dave Young <hidave.darkstar@gmail.com>
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
>  drivers/net/wireless/b43/xmit.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> --- wireless-testing.orig/drivers/net/wireless/b43/xmit.c	2009-10-11 12:11:50.000000000 +0200
> +++ wireless-testing/drivers/net/wireless/b43/xmit.c	2009-10-11 12:12:06.000000000 +0200
> @@ -690,7 +690,10 @@ void b43_rx(struct b43_wldev *dev, struc
>  	}
>  
>  	memcpy(IEEE80211_SKB_RXCB(skb), &status, sizeof(status));
> +
> +	local_bh_disable();
>  	ieee80211_rx(dev->wl->hw, skb);
> +	local_bh_enable();
>  
>  #if B43_DEBUG
>  	dev->rx_count++;



-- 
Greetings, Michael.

Re: [PATCH] b43: fix ieee80211_rx() context

[Johannes Berg]

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

On Sun, 2009-10-11 at 12:26 +0200, Michael Buesch wrote:
> On Sunday 11 October 2009 12:19:21 Johannes Berg wrote:
> > Due to the way it interacts with the networking
> > stack and other parts of mac80211, ieee80211_rx()
> > must be called with disabled softirqs.
> 
> Is this stated in the documentation somewhere?

No. However, there are many things that aren't in the documentation, I'm
working on a patch to add a note.

> > Michael, the former maintainer of this driver,
> > has refused to fix the problem this way instead
> > proposing a much more invasive patch that could
> > not even be proved correct wrt. locking inside
> > mac80211. Regardless of that, he believes this
> > to be a bug in mac80211, and has also publicly
> > stated [1] that he does not care about this even
> > though it is a regression introduced by his own
> > patches.
> 
> What if we leave slander out of the commit messages?

As far as I know, it is an accurate account of what happened in the
other thread, and as such is not slander. I just wanted to provide a
rationale for me fixing this bug instead of you. If you disagree that
this is an accurate representation, I invite you to summarise the thread
that caused this miserable situation of a known bug not being fixed for
a very long time despite appropriate fixes being known in your own words
for the commit message.

johannes

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

Re: [PATCH] b43: fix ieee80211_rx() context

[Michael Buesch]

On Sunday 11 October 2009 12:31:06 Johannes Berg wrote:
> On Sun, 2009-10-11 at 12:26 +0200, Michael Buesch wrote:
> > On Sunday 11 October 2009 12:19:21 Johannes Berg wrote:
> > > Due to the way it interacts with the networking
> > > stack and other parts of mac80211, ieee80211_rx()
> > > must be called with disabled softirqs.
> > 
> > Is this stated in the documentation somewhere?
> 
> No. However, there are many things that aren't in the documentation, I'm
> working on a patch to add a note.

Ok, thanks a lot.

> I just wanted to provide a
> rationale for me fixing this bug instead of you.

Since when do we require that in commit messages?

> If you disagree that 
> this is an accurate representation, I invite you to summarise the thread
> that caused this miserable situation of a known bug not being fixed for
> a very long time despite appropriate fixes being known in your own words
> for the commit message.

If you'd care _that_ much, you could have just reverted my commit.
Yes, I introduced the regression and I was unable to cook up a fix for it. So the logical
reaction to that would be to revert my commit.

-- 
Greetings, Michael.

Re: [PATCH] b43: fix ieee80211_rx() context

[Holger Schurig]

> As far as I know, it is an accurate account of what happened in the
> other thread, and as such is not slander. 

It might not be slander, but it's still not polite.

-- 
http://www.holgerschurig.de

Re: [PATCH] b43: fix ieee80211_rx() context

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Sun, 11 Oct 2009 12:19:21 +0200

> Due to the way it interacts with the networking
> stack and other parts of mac80211, ieee80211_rx()
> must be called with disabled softirqs.
> 
> Michael, the former maintainer of this driver,
> has refused to fix the problem this way instead
> proposing a much more invasive patch that could
> not even be proved correct wrt. locking inside
> mac80211. Regardless of that, he believes this
> to be a bug in mac80211, and has also publicly
> stated [1] that he does not care about this even
> though it is a regression introduced by his own
> patches.
> 
> Since nobody else seems to be wanting to fix the
> problem, I'll just fix it for the benefit of the
> many users of this driver.
> 
> [1] http://thread.gmane.org/gmane.linux.kernel.wireless.general/39440/focus=40266
> 
> Reported-by: Dave Young <hidave.darkstar@gmail.com>
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH] b43: fix ieee80211_rx() context

[Dave Young]

On Sun, Oct 11, 2009 at 6:39 PM, David Miller <davem@davemloft.net> wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Sun, 11 Oct 2009 12:19:21 +0200
>
>> Due to the way it interacts with the networking
>> stack and other parts of mac80211, ieee80211_rx()
>> must be called with disabled softirqs.
>>
>> Michael, the former maintainer of this driver,
>> has refused to fix the problem this way instead
>> proposing a much more invasive patch that could
>> not even be proved correct wrt. locking inside
>> mac80211. Regardless of that, he believes this
>> to be a bug in mac80211, and has also publicly
>> stated [1] that he does not care about this even
>> though it is a regression introduced by his own
>> patches.
>>
>> Since nobody else seems to be wanting to fix the
>> problem, I'll just fix it for the benefit of the
>> many users of this driver.
>>
>> [1] http://thread.gmane.org/gmane.linux.kernel.wireless.general/39440/focus=40266
>>
>> Reported-by: Dave Young <hidave.darkstar@gmail.com>
>> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
>
> Acked-by: David S. Miller <davem@davemloft.net>
>

Tested-by: Dave Young <hidave.darkstar@gmail.com>

-- 
Regards
dave

Re: [PATCH] b43: fix ieee80211_rx() context

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> Due to the way it interacts with the networking
> stack and other parts of mac80211, ieee80211_rx()
> must be called with disabled softirqs.

[...]

> +	local_bh_disable();
>  	ieee80211_rx(dev->wl->hw, skb);
> +	local_bh_enable();

This is a bit awkward from drivers' point of view, we have to add the
same code to all mac80211 drivers using either SPI or SDIO buses.

What about adding a new inline function ieee80211_rx_ni() which would
disable bottom halves like above and call ieee80211_rx()? IMHO that's
easier for the driver developers to understand and also easier to
document ("use this function when calling from process context"). If
this is acceptable, I can create a patch.

-- 
Kalle Valo

Re: [PATCH] b43: fix ieee80211_rx() context

[Johannes Berg]

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

On Sun, 2009-10-11 at 18:59 +0300, Kalle Valo wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
> 
> > Due to the way it interacts with the networking
> > stack and other parts of mac80211, ieee80211_rx()
> > must be called with disabled softirqs.
> 
> [...]
> 
> > +	local_bh_disable();
> >  	ieee80211_rx(dev->wl->hw, skb);
> > +	local_bh_enable();
> 
> This is a bit awkward from drivers' point of view, we have to add the
> same code to all mac80211 drivers using either SPI or SDIO buses.
> 
> What about adding a new inline function ieee80211_rx_ni() which would
> disable bottom halves like above and call ieee80211_rx()? IMHO that's
> easier for the driver developers to understand and also easier to
> document ("use this function when calling from process context"). If
> this is acceptable, I can create a patch.

I really don't see the point, since it's just three lines of code, but I
wouldn't mind all that much either.

johannes

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

Re: [PATCH] b43: fix ieee80211_rx() context

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

>> > +	local_bh_disable();
>> >  	ieee80211_rx(dev->wl->hw, skb);
>> > +	local_bh_enable();
>> 
>> This is a bit awkward from drivers' point of view, we have to add the
>> same code to all mac80211 drivers using either SPI or SDIO buses.
>> 
>> What about adding a new inline function ieee80211_rx_ni() which would
>> disable bottom halves like above and call ieee80211_rx()? IMHO that's
>> easier for the driver developers to understand and also easier to
>> document ("use this function when calling from process context"). If
>> this is acceptable, I can create a patch.
>
> I really don't see the point, since it's just three lines of code, but I
> wouldn't mind all that much either.

My worry are the developers who even don't know what is a bottom half
and might get it all wrong. (Yes, there really are such people.)

But if you don't see any benefit from adding a new function, I'll drop
the idea. No need to complicate this anymore :)

-- 
Kalle Valo

Re: [PATCH] b43: fix ieee80211_rx() context

[David Miller]

From: Kalle Valo <kalle.valo@iki.fi>
Date: Sun, 11 Oct 2009 19:08:58 +0300

> Johannes Berg <johannes@sipsolutions.net> writes:
> 
>>> > +	local_bh_disable();
>>> >  	ieee80211_rx(dev->wl->hw, skb);
>>> > +	local_bh_enable();
>>> 
>>> This is a bit awkward from drivers' point of view, we have to add the
>>> same code to all mac80211 drivers using either SPI or SDIO buses.
>>> 
>>> What about adding a new inline function ieee80211_rx_ni() which would
>>> disable bottom halves like above and call ieee80211_rx()? IMHO that's
>>> easier for the driver developers to understand and also easier to
>>> document ("use this function when calling from process context"). If
>>> this is acceptable, I can create a patch.
>>
>> I really don't see the point, since it's just three lines of code, but I
>> wouldn't mind all that much either.
> 
> My worry are the developers who even don't know what is a bottom half
> and might get it all wrong. (Yes, there really are such people.)

And the difference between this and knowing you need to call the
ieee80211_rx_ni() thing is?

You have to know what the heck a bottom half is to even know that you
would need to call the ieee80211_rx_ni() thing.

And that's the same amount of knowledge necessary to simply wrap the
thing in a BH disable/enable sequence.

Re: [PATCH] b43: fix ieee80211_rx() context

[Kalle Valo]

David Miller <davem@davemloft.net> writes:

>>> I really don't see the point, since it's just three lines of code, but I
>>> wouldn't mind all that much either.
>> 
>> My worry are the developers who even don't know what is a bottom half
>> and might get it all wrong. (Yes, there really are such people.)
>
> And the difference between this and knowing you need to call the
> ieee80211_rx_ni() thing is?
>
> You have to know what the heck a bottom half is to even know that you
> would need to call the ieee80211_rx_ni() thing.
>
> And that's the same amount of knowledge necessary to simply wrap the
> thing in a BH disable/enable sequence.

I was thinking that it's possible to document it something like this:

o in irq context use ieee80211_rx_irqsafe()
o in a tasklet use ieee80211_rx()
o in process context use ieee80211_rx_ni()

Also in the future it might be easier to optimise something based on
these functions. Maybe.

But as Johannes didn't like the idea, and neither do you, I'm going to
drop the idea. I'll add the BH disable/enable to wl1251 instead and
hopefully Luciano does the same to wl1271.

-- 
Kalle Valo

Re: [PATCH] b43: fix ieee80211_rx() context

[Luciano Coelho]

ext Kalle Valo wrote:
> David Miller <davem@davemloft.net> writes:
> 
>>>> I really don't see the point, since it's just three lines of code, but I
>>>> wouldn't mind all that much either.
>>> My worry are the developers who even don't know what is a bottom half
>>> and might get it all wrong. (Yes, there really are such people.)
>> And the difference between this and knowing you need to call the
>> ieee80211_rx_ni() thing is?
>>
>> You have to know what the heck a bottom half is to even know that you
>> would need to call the ieee80211_rx_ni() thing.
>>
>> And that's the same amount of knowledge necessary to simply wrap the
>> thing in a BH disable/enable sequence.
> 
> I was thinking that it's possible to document it something like this:
> 
> o in irq context use ieee80211_rx_irqsafe()
> o in a tasklet use ieee80211_rx()
> o in process context use ieee80211_rx_ni()
> 
> Also in the future it might be easier to optimise something based on
> these functions. Maybe.
> 
> But as Johannes didn't like the idea, and neither do you, I'm going to
> drop the idea. I'll add the BH disable/enable to wl1251 instead and
> hopefully Luciano does the same to wl1271.

Yeps, I can do the same for wl1271.

-- 
Cheers,
Luca.

Re: [PATCH] b43: fix ieee80211_rx() context

[John W. Linville]

On Sun, Oct 11, 2009 at 08:08:57PM -0700, David Miller wrote:
> From: Kalle Valo <kalle.valo@iki.fi>
> Date: Sun, 11 Oct 2009 19:08:58 +0300
> 
> > Johannes Berg <johannes@sipsolutions.net> writes:
> > 
> >>> > +	local_bh_disable();
> >>> >  	ieee80211_rx(dev->wl->hw, skb);
> >>> > +	local_bh_enable();
> >>> 
> >>> This is a bit awkward from drivers' point of view, we have to add the
> >>> same code to all mac80211 drivers using either SPI or SDIO buses.
> >>> 
> >>> What about adding a new inline function ieee80211_rx_ni() which would
> >>> disable bottom halves like above and call ieee80211_rx()? IMHO that's
> >>> easier for the driver developers to understand and also easier to
> >>> document ("use this function when calling from process context"). If
> >>> this is acceptable, I can create a patch.
> >>
> >> I really don't see the point, since it's just three lines of code, but I
> >> wouldn't mind all that much either.
> > 
> > My worry are the developers who even don't know what is a bottom half
> > and might get it all wrong. (Yes, there really are such people.)
> 
> And the difference between this and knowing you need to call the
> ieee80211_rx_ni() thing is?
> 
> You have to know what the heck a bottom half is to even know that you
> would need to call the ieee80211_rx_ni() thing.
> 
> And that's the same amount of knowledge necessary to simply wrap the
> thing in a BH disable/enable sequence.

I'm not sure I see the difference between this and the rationale for
having netif_rx_ni vs. an open-coded version of it?  ieee80211_rx_ni
seems like a small amount of code (could even be inline) that
potentially avoids some stupid bugs...?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] b43: fix ieee80211_rx() context

[David Miller]

From: "John W. Linville" <linville@tuxdriver.com>
Date: Mon, 12 Oct 2009 09:38:36 -0400

> I'm not sure I see the difference between this and the rationale for
> having netif_rx_ni vs. an open-coded version of it?  ieee80211_rx_ni
> seems like a small amount of code (could even be inline) that
> potentially avoids some stupid bugs...?

Sure, no problem, feel free to add ieee80211_rx_ni().