2010-10-04 17:22:38

by Ben Greear

[permalink] [raw]
Subject: Putting APs into bridges?

I got a bug report from someone using my ath5k VAP patch.

It seems he put two VAPs into a bridge device, and got an
assert here (nevermind the printk, I just added that to
help debug the issue).

static void __ieee80211_wake_queue(struct ieee80211_hw *hw, int queue,
enum queue_stop_reason reason)
{
struct ieee80211_local *local = hw_to_local(hw);
struct ieee80211_sub_if_data *sdata;

trace_wake_queue(local, queue, reason);

if (WARN_ON(queue >= hw->queues)) {
printk(KERN_WARNING "%s: queue: %i hw->queues: %i\n",
sdata->name, queue, hw->queues);
return;
}


Before I try to reproduce this, it is valid to add APs to bridge
devices in the first place?

Thanks,
Ben

--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com



2010-10-05 03:56:27

by Bruno Randolf

[permalink] [raw]
Subject: Re: Putting APs into bridges?

On Tue October 5 2010 12:45:36 Ben Greear wrote:
> On 10/04/2010 08:43 PM, Bruno Randolf wrote:
> > On Tue October 5 2010 03:29:29 Johannes Berg wrote:
> >> On Mon, 2010-10-04 at 10:22 -0700, Ben Greear wrote:
> >>> It seems he put two VAPs into a bridge device, and got an
> >>> assert here (nevermind the printk, I just added that to
> >>> help debug the issue).
> >>>
> >>> static void __ieee80211_wake_queue(struct ieee80211_hw *hw, int queue,
> >>>
> >>> enum queue_stop_reason reason)
> >>>
> >>> {
> >>>
> >>> struct ieee80211_local *local = hw_to_local(hw);
> >>> struct ieee80211_sub_if_data *sdata;
> >>>
> >>> trace_wake_queue(local, queue, reason);
> >>>
> >>> if (WARN_ON(queue>= hw->queues)) {
> >>>
> >>> printk(KERN_WARNING "%s: queue: %i hw->queues: %i\n",
> >>>
> >>> sdata->name, queue, hw->queues);
> >>>
> >>> return;
> >>>
> >>> }
> >>>
> >>> Before I try to reproduce this, it is valid to add APs to bridge
> >>> devices in the first place?
> >>
> >> Yes, it's valid, we catch the invalid cases in cfg80211.
> >>
> >> Hitting the assert there is rather strange though.
> >
> > hey!
> >
> > i'm seeing the same. i think it's due to a bug in ath5k concerning power
> > save. we put frames in the CAB queue, but obviously we shouldn't tell
> > mac80211 to wake this queue (number 6) since mac80211 knows nothing
> > about it.
>
> Interesting...we couldn't reproduce it at all (two APs in a bridge worked
> as expected, as far as we could tell).

i think this has nothing to do with bridges. i can get the same problem with a
single AP interface. i can reproduce it by connecting 2 STA, one of which has
to use power-save. i'll come up with a simple patch soon.

bruno

2010-10-05 03:45:43

by Ben Greear

[permalink] [raw]
Subject: Re: Putting APs into bridges?

On 10/04/2010 08:43 PM, Bruno Randolf wrote:
> On Tue October 5 2010 03:29:29 Johannes Berg wrote:
>> On Mon, 2010-10-04 at 10:22 -0700, Ben Greear wrote:
>>> It seems he put two VAPs into a bridge device, and got an
>>> assert here (nevermind the printk, I just added that to
>>> help debug the issue).
>>>
>>> static void __ieee80211_wake_queue(struct ieee80211_hw *hw, int queue,
>>>
>>> enum queue_stop_reason reason)
>>>
>>> {
>>>
>>> struct ieee80211_local *local = hw_to_local(hw);
>>> struct ieee80211_sub_if_data *sdata;
>>>
>>> trace_wake_queue(local, queue, reason);
>>>
>>> if (WARN_ON(queue>= hw->queues)) {
>>>
>>> printk(KERN_WARNING "%s: queue: %i hw->queues: %i\n",
>>>
>>> sdata->name, queue, hw->queues);
>>>
>>> return;
>>>
>>> }
>>>
>>> Before I try to reproduce this, it is valid to add APs to bridge
>>> devices in the first place?
>>
>> Yes, it's valid, we catch the invalid cases in cfg80211.
>>
>> Hitting the assert there is rather strange though.
>
> hey!
>
> i'm seeing the same. i think it's due to a bug in ath5k concerning power save.
> we put frames in the CAB queue, but obviously we shouldn't tell mac80211 to
> wake this queue (number 6) since mac80211 knows nothing about it.

Interesting...we couldn't reproduce it at all (two APs in a bridge worked as
expected, as far as we could tell).

Thanks,
Ben


--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com

2010-10-04 18:29:32

by Johannes Berg

[permalink] [raw]
Subject: Re: Putting APs into bridges?

On Mon, 2010-10-04 at 10:22 -0700, Ben Greear wrote:

> It seems he put two VAPs into a bridge device, and got an
> assert here (nevermind the printk, I just added that to
> help debug the issue).
>
> static void __ieee80211_wake_queue(struct ieee80211_hw *hw, int queue,
> enum queue_stop_reason reason)
> {
> struct ieee80211_local *local = hw_to_local(hw);
> struct ieee80211_sub_if_data *sdata;
>
> trace_wake_queue(local, queue, reason);
>
> if (WARN_ON(queue >= hw->queues)) {
> printk(KERN_WARNING "%s: queue: %i hw->queues: %i\n",
> sdata->name, queue, hw->queues);
> return;
> }
>
>
> Before I try to reproduce this, it is valid to add APs to bridge
> devices in the first place?

Yes, it's valid, we catch the invalid cases in cfg80211.

Hitting the assert there is rather strange though.

johannes


2010-10-05 03:43:47

by Bruno Randolf

[permalink] [raw]
Subject: Re: Putting APs into bridges?

On Tue October 5 2010 03:29:29 Johannes Berg wrote:
> On Mon, 2010-10-04 at 10:22 -0700, Ben Greear wrote:
> > It seems he put two VAPs into a bridge device, and got an
> > assert here (nevermind the printk, I just added that to
> > help debug the issue).
> >
> > static void __ieee80211_wake_queue(struct ieee80211_hw *hw, int queue,
> >
> > enum queue_stop_reason reason)
> >
> > {
> >
> > struct ieee80211_local *local = hw_to_local(hw);
> > struct ieee80211_sub_if_data *sdata;
> >
> > trace_wake_queue(local, queue, reason);
> >
> > if (WARN_ON(queue >= hw->queues)) {
> >
> > printk(KERN_WARNING "%s: queue: %i hw->queues: %i\n",
> >
> > sdata->name, queue, hw->queues);
> >
> > return;
> >
> > }
> >
> > Before I try to reproduce this, it is valid to add APs to bridge
> > devices in the first place?
>
> Yes, it's valid, we catch the invalid cases in cfg80211.
>
> Hitting the assert there is rather strange though.

hey!

i'm seeing the same. i think it's due to a bug in ath5k concerning power save.
we put frames in the CAB queue, but obviously we shouldn't tell mac80211 to
wake this queue (number 6) since mac80211 knows nothing about it.

bruno