[PATCH] mac80211: refine ieee80211_rx() context requirement

[PATCH] mac80211: refine ieee80211_rx() context requirement

[Wei.Yang]

From: Wei Yang <Wei.Yang@windriver.com>

In case of RT kernel, the return value of softirq_count() always
equal to 0, we need to use in_serving_softirq to decide whether
the current context is in softirq context.

Signed-off-by: Wei Yang <Wei.Yang@windriver.com>
---
 net/mac80211/rx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index c6844ad..80fac46 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -3226,7 +3226,7 @@ void ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb)
 	struct ieee80211_supported_band *sband;
 	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
 
-	WARN_ON_ONCE(softirq_count() == 0);
+	WARN_ON_ONCE(!in_serving_softirq());
 
 	if (WARN_ON(status->band >= IEEE80211_NUM_BANDS))
 		goto drop;
-- 
1.7.9.5

Re: [PATCH] mac80211: refine ieee80211_rx() context requirement

[Johannes Berg]

On Wed, 2013-05-08 at 16:31 +0800, Wei.Yang@windriver.com wrote:
> From: Wei Yang <Wei.Yang@windriver.com>
> 
> In case of RT kernel, the return value of softirq_count() always
> equal to 0, we need to use in_serving_softirq to decide whether
> the current context is in softirq context.

> -	WARN_ON_ONCE(softirq_count() == 0);
> +	WARN_ON_ONCE(!in_serving_softirq());

As I understand the code, I don't believe this change to be correct. The
function can happily run with softirqs disabled (e.g.
local_bh_disable()), for example by being called via ieee80211_rx_ni().
As I understand in_serving_softirq(), it checks that it's actually
inside handling a softirq, no?

johannes

Re: [PATCH] mac80211: refine ieee80211_rx() context requirement

[Johannes Berg]

On Wed, 2013-05-08 at 10:36 +0200, Johannes Berg wrote:
> On Wed, 2013-05-08 at 16:31 +0800, Wei.Yang@windriver.com wrote:
> > From: Wei Yang <Wei.Yang@windriver.com>
> > 
> > In case of RT kernel, the return value of softirq_count() always
> > equal to 0, we need to use in_serving_softirq to decide whether
> > the current context is in softirq context.
> 
> > -	WARN_ON_ONCE(softirq_count() == 0);
> > +	WARN_ON_ONCE(!in_serving_softirq());
> 
> As I understand the code, I don't believe this change to be correct. The
> function can happily run with softirqs disabled (e.g.
> local_bh_disable()), for example by being called via ieee80211_rx_ni().
> As I understand in_serving_softirq(), it checks that it's actually
> inside handling a softirq, no?

Arguably, it should be
	WARN_ON_ONCE(!in_softirq());

but that's equivalent:

include/linux/hardirq.h

 * in_softirq - Are we currently processing softirq or have bh disabled?

 #define in_softirq()           (softirq_count())

johannes

Re: [PATCH] mac80211: refine ieee80211_rx() context requirement

[wyang1]

On 05/08/2013 04:36 PM, Johannes Berg wrote:
> On Wed, 2013-05-08 at 16:31 +0800, Wei.Yang@windriver.com wrote:
>> From: Wei Yang <Wei.Yang@windriver.com>
>>
>> In case of RT kernel, the return value of softirq_count() always
>> equal to 0, we need to use in_serving_softirq to decide whether
>> the current context is in softirq context.
>> -	WARN_ON_ONCE(softirq_count() == 0);
>> +	WARN_ON_ONCE(!in_serving_softirq());
> As I understand the code, I don't believe this change to be correct. The
> function can happily run with softirqs disabled (e.g.
> local_bh_disable()), for example by being called via ieee80211_rx_ni().
> As I understand in_serving_softirq(), it checks that it's actually
> inside handling a softirq, no?

Yes, I am not an expert on wireless, so I am not sure whether 
ieee80211_rx will be called with softirqs disabled.
Just as you said, if it can run with softirqs diabled, the fix is not 
correct. sorry for it.

Thanks
Wei
>
> johannes
>
>
>

Re: [PATCH] mac80211: refine ieee80211_rx() context requirement

[wyang1]

On 05/08/2013 04:37 PM, Johannes Berg wrote:
> On Wed, 2013-05-08 at 10:36 +0200, Johannes Berg wrote:
>> On Wed, 2013-05-08 at 16:31 +0800, Wei.Yang@windriver.com wrote:
>>> From: Wei Yang <Wei.Yang@windriver.com>
>>>
>>> In case of RT kernel, the return value of softirq_count() always
>>> equal to 0, we need to use in_serving_softirq to decide whether
>>> the current context is in softirq context.
>>> -	WARN_ON_ONCE(softirq_count() == 0);
>>> +	WARN_ON_ONCE(!in_serving_softirq());
>> As I understand the code, I don't believe this change to be correct. The
>> function can happily run with softirqs disabled (e.g.
>> local_bh_disable()), for example by being called via ieee80211_rx_ni().
>> As I understand in_serving_softirq(), it checks that it's actually
>> inside handling a softirq, no?
> Arguably, it should be
> 	WARN_ON_ONCE(!in_softirq());
>
> but that's equivalent:

Yeah, but the softirq is threaded in rt kernel, the softirq_count always 
return 0. The warning should always happen with rt kernel.

Wei
>
> include/linux/hardirq.h
>
>   * in_softirq - Are we currently processing softirq or have bh disabled?
>
>   #define in_softirq()           (softirq_count())
>
> johannes
>
>
>

Re: [PATCH] mac80211: refine ieee80211_rx() context requirement

[Johannes Berg]

On Wed, 2013-05-08 at 16:47 +0800, wyang1 wrote:
> On 05/08/2013 04:37 PM, Johannes Berg wrote:
> > On Wed, 2013-05-08 at 10:36 +0200, Johannes Berg wrote:
> >> On Wed, 2013-05-08 at 16:31 +0800, Wei.Yang@windriver.com wrote:
> >>> From: Wei Yang <Wei.Yang@windriver.com>
> >>>
> >>> In case of RT kernel, the return value of softirq_count() always
> >>> equal to 0, we need to use in_serving_softirq to decide whether
> >>> the current context is in softirq context.
> >>> -	WARN_ON_ONCE(softirq_count() == 0);
> >>> +	WARN_ON_ONCE(!in_serving_softirq());
> >> As I understand the code, I don't believe this change to be correct. The
> >> function can happily run with softirqs disabled (e.g.
> >> local_bh_disable()), for example by being called via ieee80211_rx_ni().
> >> As I understand in_serving_softirq(), it checks that it's actually
> >> inside handling a softirq, no?
> > Arguably, it should be
> > 	WARN_ON_ONCE(!in_softirq());
> >
> > but that's equivalent:
> 
> Yeah, but the softirq is threaded in rt kernel, the softirq_count always 
> return 0. The warning should always happen with rt kernel.

So maybe then the RT kernel defines in_softirq() appropriately and you
should submit a patch to change to using that? I find it hard to believe
that the RT kernel breaks _all_ the softirq APIs, but hey, I know
nothing about RT :-)

johannes