Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[Michael Wu]

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

On Sunday 29 July 2007 19:31, Daniel Drake wrote:
> This patch adds debug output to mac80211 operations. This is
> intended to sort out the issues with the interface semantics.
>
Ugh. I really hate enter/exit and especially these since they are open coded. 
What do you need clarified about mac80211 interface semantics?

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[John W. Linville]

On Sun, Jul 29, 2007 at 08:21:31PM -0700, Michael Wu wrote:
> On Sunday 29 July 2007 19:31, Daniel Drake wrote:
> > This patch adds debug output to mac80211 operations. This is
> > intended to sort out the issues with the interface semantics.
> >
> Ugh. I really hate enter/exit and especially these since they are open coded. 
> What do you need clarified about mac80211 interface semantics?

I agree, these are a bit ugly.  Do you intend them to be permanent?

-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[Ulrich Kunitz]

John W. Linville wrote:

> On Sun, Jul 29, 2007 at 08:21:31PM -0700, Michael Wu wrote:
> > On Sunday 29 July 2007 19:31, Daniel Drake wrote:
> > > This patch adds debug output to mac80211 operations. This is
> > > intended to sort out the issues with the interface semantics.
> > >
> > Ugh. I really hate enter/exit and especially these since they are open coded. 
> > What do you need clarified about mac80211 interface semantics?
> 
> I agree, these are a bit ugly.  Do you intend them to be permanent?

Guys, actually I looked into iwl-base.c and there are also leave
and enter statements for exactly the same mac80211 operations. I
would appreciate, if somebody could explain me the difference.

Please notify that the messages will only be compiled into the
driver if ZD1211_DEBUG is set. I could agree to take them out,
after we have fixed the multiple interface issues, but right now
they help to make the logfile quite readable. They help also to
check my understanding of the struct ieee80211_ops functions.

Asked what I don't understand about the mac80211 interfaces, I
have following questions:

Why does WIPHY has only a single MAC address?

Though ZD1211 has only one RF chip and one baseband processor --
which represents a single PHY device in my opinion -- it can
support two MAC addresses. Sure MAC80211 can give different IP
addresses to different interfaces, but currently I'm not sure, how
they relate to the address in the WIPHY.

What about add_interface/remove_interfaces and monitor interfaces?

There seems to be difference between "hard" and "soft" monitor
mode. Currently I'm not sure what the semantics is. There seems to
be also calls to add_interface/remote_interface with if_id -1. I
guess that means something like all interfaces. The expected
behaviour is not documented for that case. 

Cheers,

Uli

-- 
Uli Kunitz

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[Michael Buesch]

On Tuesday 31 July 2007, Ulrich Kunitz wrote:
> What about add_interface/remove_interfaces and monitor interfaces?
> 
> There seems to be difference between "hard" and "soft" monitor
> mode. Currently I'm not sure what the semantics is. There seems to
> be also calls to add_interface/remote_interface with if_id -1. I
> guess that means something like all interfaces. The expected
> behaviour is not documented for that case. 

I suggest you look at bcm43xx-mac80211.
You will need to introduce refcounting for your monitor interfaces
and I _really_ suggest to get rid of the zd_op_open() and zd_op_close()
functions. Just include that in add/remove interface. You need
to refcount your interfaces anyway.

I don't know what you mean by hard/soft monitor.

Don't care about the id_id. It's a cookie and it's opaque to
the driver.

Semantics are pretty easy for add/remove interface. The function
is called every time the user adds/removes an interface. :)

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[Andy Green]

Somebody in the thread at some point said:
> On Tuesday 31 July 2007, Ulrich Kunitz wrote:
>> What about add_interface/remove_interfaces and monitor interfaces?
>>
>> There seems to be difference between "hard" and "soft" monitor
>> mode. Currently I'm not sure what the semantics is. There seems to
>> be also calls to add_interface/remote_interface with if_id -1. I
>> guess that means something like all interfaces. The expected
>> behaviour is not documented for that case. 

> I don't know what you mean by hard/soft monitor.

Currently there are two distinct kinds of "monitor mode" result returned
by zd1211rw-mac80211 and iwl3945 at least.

The "hard" one to use Ulrich's name is what you want to see with Monitor
mode, the physical device is in hardware promisc and you see all kinds
of beacons and foreign network encrypted packets.

The "soft" one is a faked up repeating of incoming (and outgoing!
twice!) packets that happen to be around during the filtered operation
of the device, in the case where the device operates as Managed and is
not in hardware promisc.  In this mode, you see only your associated
station beacons and your own packets, not even packets from other
stations on your AP.

You get "hard" monitoring when you have only one virtual interface and
it is in monitor mode.  You get "soft" bogus monitoring when your
monitor mode interface is a second virtual one.

I posted a few times about this last week giving details of what you get
under what circumstances.  It really would be much better if the device
is kicked to hardware promisc if ANY virtual interface is in Monitor
mode, therefore the results are consistent regardless of the number of
virtual interfaces that happen to be around.

-Andy

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[Jiri Benc]

On Tue, 31 Jul 2007 00:25:21 +0100, Andy Green wrote:
> The "soft" one is a faked up repeating of incoming (and outgoing!
> twice!) packets

You mean you see each outgoing packet twice on soft monitor interface? That
would be a bug.

> It really would be much better if the device
> is kicked to hardware promisc if ANY virtual interface is in Monitor
> mode, therefore the results are consistent regardless of the number of
> virtual interfaces that happen to be around.

Of course. If the hardware supports that.

 Jiri

-- 
Jiri Benc
SUSE Labs

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[Michael Buesch]

On Tuesday 31 July 2007 23:12:49 Jiri Benc wrote:
> On Tue, 31 Jul 2007 00:25:21 +0100, Andy Green wrote:
> > The "soft" one is a faked up repeating of incoming (and outgoing!
> > twice!) packets
> 
> You mean you see each outgoing packet twice on soft monitor interface? That
> would be a bug.
> 
> > It really would be much better if the device
> > is kicked to hardware promisc if ANY virtual interface is in Monitor
> > mode, therefore the results are consistent regardless of the number of
> > virtual interfaces that happen to be around.
> 
> Of course. If the hardware supports that.

The device should _not_ be kicked to promisc, if we have a monitor
interface. The promisc bit is a completely seperate setting
and must be handled seperate from the monitor stuff.
Userspace does take care of setting the promisc bit, if it's
required.
We recently fixed this bug in bcm43xx.

-- 
Greetings Michael.

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[Andy Green]

Somebody in the thread at some point said:
> On Tuesday 31 July 2007 23:12:49 Jiri Benc wrote:
>> On Tue, 31 Jul 2007 00:25:21 +0100, Andy Green wrote:
>>> The "soft" one is a faked up repeating of incoming (and outgoing!
>>> twice!) packets
>> You mean you see each outgoing packet twice on soft monitor interface? That
>> would be a bug.
>>
>>> It really would be much better if the device
>>> is kicked to hardware promisc if ANY virtual interface is in Monitor
>>> mode, therefore the results are consistent regardless of the number of
>>> virtual interfaces that happen to be around.
>> Of course. If the hardware supports that.
> 
> The device should _not_ be kicked to promisc, if we have a monitor
> interface. The promisc bit is a completely seperate setting
> and must be handled seperate from the monitor stuff.
> Userspace does take care of setting the promisc bit, if it's
> required.
> We recently fixed this bug in bcm43xx.

Well that's fine... tcpdump for example does IFF_PROMISC while it runs
so nobody who doesn't care will notice the difference, but I guess you
get the power saving while IFF_PROMISC is off and the Monitor mode
interface is up.

But anyone who tcpdumps on a Monitor mode interface still always sees
the whole picture, not the synthesized one.  Sounds like a good general
way to me.

-Andy

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[Andy Green]

Somebody in the thread at some point said:
> On Tue, 31 Jul 2007 00:25:21 +0100, Andy Green wrote:
>> The "soft" one is a faked up repeating of incoming (and outgoing!
>> twice!) packets
> 
> You mean you see each outgoing packet twice on soft monitor interface? That
> would be a bug.

It's not all outgoing packets, just the injected ones.  I described it here

http://marc.info/?l=linux-wireless&m=118544939901024&w=2

>> It really would be much better if the device
>> is kicked to hardware promisc if ANY virtual interface is in Monitor
>> mode, therefore the results are consistent regardless of the number of
>> virtual interfaces that happen to be around.
> 
> Of course. If the hardware supports that.

Is there any hardware that can't do hardware promisc and let the stack
decide what to throw away?  I would imagine the problem of having too
many kinds of packet should be okay to solve by examining MAC addresses
and so on.

-Andy

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[Michael Wu]

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

On Monday 30 July 2007 16:05, Michael Buesch wrote:
> I suggest you look at bcm43xx-mac80211.
> You will need to introduce refcounting for your monitor interfaces
> and I _really_ suggest to get rid of the zd_op_open() and zd_op_close()
> functions. Just include that in add/remove interface. You need
> to refcount your interfaces anyway.
>
Requiring refcounting for monitor interfaces is fairly dumb since mac80211 
does the exact same refcounting. I haven't bothered to do it in any driver 
ports yet for this reason. This part of the mac80211 driver api needs to be 
fixed.

Also, I'm not entirely convinced that eliminating the open/close callbacks is 
a good idea, which is why I continued to use them in the zd1211rw port.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[Michael Buesch]

On Tuesday 31 July 2007, Michael Wu wrote:
> On Monday 30 July 2007 16:05, Michael Buesch wrote:
> > I suggest you look at bcm43xx-mac80211.
> > You will need to introduce refcounting for your monitor interfaces
> > and I _really_ suggest to get rid of the zd_op_open() and zd_op_close()
> > functions. Just include that in add/remove interface. You need
> > to refcount your interfaces anyway.
> >
> Requiring refcounting for monitor interfaces is fairly dumb since mac80211 
> does the exact same refcounting. I haven't bothered to do it in any driver 
> ports yet for this reason. This part of the mac80211 driver api needs to be 
> fixed.

I'm talking about _current_ API. And with current API you need refcounting.
If you want to change that, that's fine. But that doesn't matter here.

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[Jiri Benc]

On Mon, 30 Jul 2007 22:22:58 -0700, Michael Wu wrote:
> On Monday 30 July 2007 16:05, Michael Buesch wrote:
> > I suggest you look at bcm43xx-mac80211.
> > You will need to introduce refcounting for your monitor interfaces
> > and I _really_ suggest to get rid of the zd_op_open() and zd_op_close()
> > functions. Just include that in add/remove interface. You need
> > to refcount your interfaces anyway.
> >
> Requiring refcounting for monitor interfaces is fairly dumb since mac80211 
> does the exact same refcounting. I haven't bothered to do it in any driver 
> ports yet for this reason. This part of the mac80211 driver api needs to be 
> fixed.

Ouch, I thought we don't bother drivers with more than one monitor
interface at a time. Apparently I forgot to fix that.

> Also, I'm not entirely convinced that eliminating the open/close callbacks is 
> a good idea,

Why?

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[Michael Buesch]

On Tuesday 31 July 2007 23:06:13 Jiri Benc wrote:
> On Mon, 30 Jul 2007 22:22:58 -0700, Michael Wu wrote:
> > On Monday 30 July 2007 16:05, Michael Buesch wrote:
> > > I suggest you look at bcm43xx-mac80211.
> > > You will need to introduce refcounting for your monitor interfaces
> > > and I _really_ suggest to get rid of the zd_op_open() and zd_op_close()
> > > functions. Just include that in add/remove interface. You need
> > > to refcount your interfaces anyway.
> > >
> > Requiring refcounting for monitor interfaces is fairly dumb since mac80211 
> > does the exact same refcounting. I haven't bothered to do it in any driver 
> > ports yet for this reason. This part of the mac80211 driver api needs to be 
> > fixed.
> 
> Ouch, I thought we don't bother drivers with more than one monitor
> interface at a time. Apparently I forgot to fix that.
> 
> > Also, I'm not entirely convinced that eliminating the open/close callbacks is 
> > a good idea,
> 
> Why?

Yeah, I don't see the need for open/close, either.
We can live perfectly well without them. It even reduces code complexity
and makes drivers more readable.
In add/remove you better know anyway, if the interface is up or down.
So if it's down in add_interface, just init it. It's a matter of an if statement.
Really nothing more.

-- 
Greetings Michael.

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[Michael Wu]

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

On Monday 30 July 2007 15:52, Ulrich Kunitz wrote:
> John W. Linville wrote:
> > On Sun, Jul 29, 2007 at 08:21:31PM -0700, Michael Wu wrote:
> > > On Sunday 29 July 2007 19:31, Daniel Drake wrote:
> > > > This patch adds debug output to mac80211 operations. This is
> > > > intended to sort out the issues with the interface semantics.
> > >
> > > Ugh. I really hate enter/exit and especially these since they are open
> > > coded. What do you need clarified about mac80211 interface semantics?
> >
> > I agree, these are a bit ugly.  Do you intend them to be permanent?
>
> Guys, actually I looked into iwl-base.c and there are also leave
> and enter statements for exactly the same mac80211 operations. I
> would appreciate, if somebody could explain me the difference.
>
None. I didn't like the ones in iwlwifi either.

> Please notify that the messages will only be compiled into the
> driver if ZD1211_DEBUG is set. I could agree to take them out,
> after we have fixed the multiple interface issues, but right now
> they help to make the logfile quite readable. They help also to
> check my understanding of the struct ieee80211_ops functions.
>
I would prefer if you kept it in your tree.

> Asked what I don't understand about the mac80211 interfaces, I
> have following questions:
>
> Why does WIPHY has only a single MAC address?
>
That is for the mac address stored in the eeprom.

> Though ZD1211 has only one RF chip and one baseband processor --
> which represents a single PHY device in my opinion -- it can
> support two MAC addresses. Sure MAC80211 can give different IP
> addresses to different interfaces, but currently I'm not sure, how
> they relate to the address in the WIPHY.
>
The proper address to use is given in add_interface. Don't worry about the 
address in wiphy.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[Jiri Benc]

On Tue, 31 Jul 2007 00:52:05 +0200, Ulrich Kunitz wrote:
> What about add_interface/remove_interfaces and monitor interfaces?

It depends on the IEEE80211_HW_MONITOR_DURING_OPER flag in hw.flags. If you
set the flag in the driver, then monitor interfaces work in the exactly
same way as STA or IBSS interfaces - the add_interface callback is called
(with conf.type equal to IEEE80211_IF_TYPE_MNTR) whenever user brings up a
monitor interface. You are responsible for switching the card to a monitor
mode (e.g. turning off hardware packet filtering).

If your hardware can't support monitor and regular interfaces concurrently,
don't set the IEEE80211_HW_MONITOR_DURING_OPER flag. In this case, the
stack calls add_interface with IEEE80211_IF_TYPE_MNTR in conf.type only
when there is no other interface active. If the user brings up a monitor
interface while he has a regular interface active, the driver is not
notified about that and the monitor interface is emulated by the stack with
a limited capabilities (this is called "soft monitor mode").

> There seems to be difference between "hard" and "soft" monitor
> mode. Currently I'm not sure what the semantics is.

You don't need to care about that in a driver, there is no difference for a
driver as you will never see a soft monitor interface; it's a
mac80211-level thing only.

>From the perspective of a user (and user space), the difference is as
follows:

"Hard monitor mode" is the usual monitor mode, i.e. all incoming frames,
regardless of their source/destination MAC addresses, are received through
the monitor interface.

"Soft monitor mode" is relevant only when the hardware has limited
capabilities for filter settings _and_ you have both regular (e.g. STA) and
monitor interface active (ifconfig up'ed). In such case, you receive only
frames addressed to your MAC address through the monitor interface. This is
still useful e.g. for debugging of network problems as you see management
frames.

> There seems to
> be also calls to add_interface/remote_interface with if_id -1. I
> guess that means something like all interfaces. The expected
> behaviour is not documented for that case. 

As Michael correctly said, don't care about the value, just store it and
hand it back to the stack when the stack wants it.

 Jiri

-- 
Jiri Benc
SUSE Labs

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[Michael Buesch]

On Tuesday 31 July 2007 23:00:43 Jiri Benc wrote:
> On Tue, 31 Jul 2007 00:52:05 +0200, Ulrich Kunitz wrote:
> > What about add_interface/remove_interfaces and monitor interfaces?
> 
> It depends on the IEEE80211_HW_MONITOR_DURING_OPER flag in hw.flags. If you
> set the flag in the driver, then monitor interfaces work in the exactly
> same way as STA or IBSS interfaces - the add_interface callback is called
> (with conf.type equal to IEEE80211_IF_TYPE_MNTR) whenever user brings up a
> monitor interface. You are responsible for switching the card to a monitor
> mode (e.g. turning off hardware packet filtering).

I think you don't want to completely turn off packet filtering
(promisc) if a monitor interface is present. The promisc bit is to be
honoured seperately.
The only thing we do in bcm43xx is enable passing of ctl frames (ACKs, etc),
if we have a monitor interface.

> > There seems to
> > be also calls to add_interface/remote_interface with if_id -1. I
> > guess that means something like all interfaces. The expected
> > behaviour is not documented for that case. 
> 
> As Michael correctly said, don't care about the value, just store it and
> hand it back to the stack when the stack wants it.

That might raise the question, if it's required to store if_ids of monitor
interfaces. I think it's not, as you don't need any mac80211 callback,
that requires an if_id, for a monitor interface.
So in bcm43xx we only store the if_id of the "operating" interface.

-- 
Greetings Michael.

Re: [PATCH] zd1211rw-mac80211: debug output for mac80211 ops

[Jiri Benc]

On Tue, 31 Jul 2007 23:37:01 +0200, Michael Buesch wrote:
> On Tuesday 31 July 2007 23:00:43 Jiri Benc wrote:
> > It depends on the IEEE80211_HW_MONITOR_DURING_OPER flag in hw.flags. If you
> > set the flag in the driver, then monitor interfaces work in the exactly
> > same way as STA or IBSS interfaces - the add_interface callback is called
> > (with conf.type equal to IEEE80211_IF_TYPE_MNTR) whenever user brings up a
> > monitor interface. You are responsible for switching the card to a monitor
> > mode (e.g. turning off hardware packet filtering).
> 
> I think you don't want to completely turn off packet filtering
> (promisc) if a monitor interface is present. The promisc bit is to be
> honoured seperately.
> The only thing we do in bcm43xx is enable passing of ctl frames (ACKs, etc),
> if we have a monitor interface.

Hm, yes, you're right. I didn't realize that, thanks!

> > As Michael correctly said, don't care about the value, just store it and
> > hand it back to the stack when the stack wants it.
> 
> That might raise the question, if it's required to store if_ids of monitor
> interfaces. I think it's not, as you don't need any mac80211 callback,
> that requires an if_id, for a monitor interface.

Sure. If you don't need to call any callback which requires if_id (and
currently there is no such callback you can call for a monitor interface),
there is no point in storing the value. But it's also no harm.

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs