more nl80211/iw tool code comments

more nl80211/iw tool code comments

[Johannes Berg]

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

Hi,

Just briefly looked at your iw tool. Code looks really good, very
readable and understandable :)

A few thoughts/comments:

get_phymode should probably be more strict and not accept things like
"IEEEabg" as A mode. IMHO the only valid things should be
"IEEE<space>802.11<single char>"
"802.11<single char>"
"<single char>"


get_iftype should accept "ap-vlan" which str_iftype returns, although
it's not really useful for use by hand anyway, I think.


You have

iw phy set      [ phy ] DEVICE [ CHANSPEC ] [ name NEWNAME ]

and we discussed on IRC that it might make sense to have the same for
nl80211. On the surface, that makes sense, however, it does add a
complication in that we need to either specify that you cannot combine
some attributes (which doesn't really make sense), or we need to take
quite a bit of care with atomicity; setting the channel and changing the
phy name can both fail individually but having them in one netlink
message implies that it's one transaction. I'm not sure the somewhat
cleaner API and saving one command number is worth the additional
transactional safety we need to be careful with then.

So I'd like to reverse my previously stated opinion and say that I now
think that putting these orthogonal things into different commands would
be better so that we don't run into this transaction problem.

Of course, that doesn't counter the other thing, namely that commands
could (should?) be named NL80211_CMD_WIPHY_NAME_{SET,GET,NEW} (del?) and
occur in groups etc.


Another thing: Maybe it should be possible to say "phy# 1" in addition
to "phy phy1" so that it's easier to write scripts that don't care about
concurrent phy name changes? Just a thought.




johannes

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

Re: more nl80211/iw tool code comments

[David Lamparter]

Hi!

On Wed, Jun 13, 2007 at 08:23:37PM +0200, Johannes Berg wrote:
> get_phymode should probably be more strict and not accept things like
> "IEEEabg" as A mode. 
Changed.

> get_iftype should accept "ap-vlan" which str_iftype returns, although
> it's not really useful for use by hand anyway, I think.
"Oups."

> Another thing: Maybe it should be possible to say "phy# 1" in addition
> to "phy phy1" so that it's easier to write scripts that don't care about
> concurrent phy name changes? Just a thought.
Added, using "phy %0" syntax.

The entire iw tool is really just a hack btw...


> iw phy set      [ phy ] DEVICE [ CHANSPEC ] [ name NEWNAME ]
> 
> and we discussed on IRC that it might make sense to have the same for
> nl80211. On the surface, that makes sense, however, it does add a
> complication in that we need to either specify that you cannot combine
> some attributes (which doesn't really make sense), 
Hmm, I can't come up with any example other than using some 11n attribute
with a 11abg phymode... care to hit me with a hint?

>                                                    or we need to take
> quite a bit of care with atomicity; setting the channel and changing the
> phy name can both fail individually but having them in one netlink
> message implies that it's one transaction. I'm not sure the somewhat
> cleaner API and saving one command number is worth the additional
> transactional safety we need to be careful with then.
> 
> So I'd like to reverse my previously stated opinion and say that I now
> think that putting these orthogonal things into different commands would
> be better so that we don't run into this transaction problem.

Well, ... take a look at net/core/rtnetlink.c line 716:

	if (err < 0 && modified && net_ratelimit())
		printk(KERN_WARNING "A link change request failed with "
		       "some changes comitted already. Interface %s may "
		       "have been left with an inconsistent configuration, "
		       "please check.\n", dev->name);

So, if rtnetlink doesn't bother too much about "transaction safety" either,
why should we? If an app wants to know what failed, it can still send SET
requests broken down into pieces, so they will know which piece failed.
Obviously they need to leave some stuff grouped (e.g. PHYMODE and CHANNEL),
but I don't think it's useful to force them do so by breaking stuff into
multiple commands...

(There is no difference really between
	CMD_SET_PHY name=myphy
	CMD_SET_PHY phymode=a channel=1
and
	CMD_SET_PHYNAME name=myphy
	CMD_SET_CHANNEL phymode=a channel=1
but the former allows, if we don't care, to just batch it.)


-David

P.S.: I'm a bit busy and won't have time to work on stuff until approx.
next Monday. Oh and I overdid a bit on the "documentation"...
http://git.spaceboyz.net/nl80211/nl80211-meta.git/master:/nl80211doc.html
(note the quad'ified attempt at associating ;)

Re: more nl80211/iw tool code comments

[Johannes Berg]

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

Hi David,

> The entire iw tool is really just a hack btw...

doesn't look too bad to me, unlike my python tool... :)

> > iw phy set      [ phy ] DEVICE [ CHANSPEC ] [ name NEWNAME ]
> > 
> > and we discussed on IRC that it might make sense to have the same for
> > nl80211. On the surface, that makes sense, however, it does add a
> > complication in that we need to either specify that you cannot combine
> > some attributes (which doesn't really make sense), 
> Hmm, I can't come up with any example other than using some 11n attribute
> with a 11abg phymode... care to hit me with a hint?

I was thinking of the name vs. phy parameters.

> 	if (err < 0 && modified && net_ratelimit())
> 		printk(KERN_WARNING "A link change request failed with "
> 		       "some changes comitted already. Interface %s may "
> 		       "have been left with an inconsistent configuration, "
> 		       "please check.\n", dev->name);
> 
> So, if rtnetlink doesn't bother too much about "transaction safety" either,
> why should we?

Heh, dunno, I just like to think that one thing posted to the kernel is
done atomically. We can drop that requirement and document it, but that
won't be easy since we'd have to document exactly what is done
atomically.

> If an app wants to know what failed, it can still send SET
> requests broken down into pieces, so they will know which piece failed.
> Obviously they need to leave some stuff grouped (e.g. PHYMODE and CHANNEL),
> but I don't think it's useful to force them do so by breaking stuff into
> multiple commands...
> 
> (There is no difference really between
> 	CMD_SET_PHY name=myphy
> 	CMD_SET_PHY phymode=a channel=1
> and
> 	CMD_SET_PHYNAME name=myphy
> 	CMD_SET_CHANNEL phymode=a channel=1
> but the former allows, if we don't care, to just batch it.)

True. I guess we can leave it as-is and see if we run into problems with
some software or something and if we do document it better ;)

johannes

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

phy mode, channel -> freq mapping (was RE: more nl80211/iw tool code comments)

[Sandesh Goel]

Hi,

I have a high level comment on the whole phy mode, channel and frequency
mapping business.

I think it is cleaner to define a parameter called BAND which can take
values 2.4 GHz, 5 GHz and so on. Then, the combination of BAND and
CHANNEL will uniquely define the operating frequency.

Using PHY_MODE for this purpose is redundant and confusing in my
opinion. Following is the relationship between BAND and PHY_MODE. 

If BAND is 2.4 GHz, PHY_MODE can be either b, g or n.
If BAND is 5 GHZ, PHY_MODE can be a or n.

Note that 'n' is common to both bands and hence PHY_MODE of 'n' alone
can not define the frequency uniquely. PHY_MODE and BAND were almost
equivalent when only 802.11a and 802.11b were around, but certainly not
any more. If this is not handled properly, it can get quite ugly down
the road.

I wasn't sure if this has already been thought about. I look forward to
being educated if I am missing something.

Thanks,
Sandesh

PS: David, I appreciate your attempt at documenting the API; it got me
interested in reviewing this.

-----Original Message-----
From: linux-wireless-owner@vger.kernel.org
[mailto:linux-wireless-owner@vger.kernel.org] On Behalf Of David
Lamparter
Sent: Thursday, June 14, 2007 7:59 PM
To: Johannes Berg
Cc: linux-wireless
Subject: Re: more nl80211/iw tool code comments

Hi!

On Wed, Jun 13, 2007 at 08:23:37PM +0200, Johannes Berg wrote:
> get_phymode should probably be more strict and not accept things like
> "IEEEabg" as A mode. 
Changed.

> get_iftype should accept "ap-vlan" which str_iftype returns, although
> it's not really useful for use by hand anyway, I think.
"Oups."

> Another thing: Maybe it should be possible to say "phy# 1" in addition
> to "phy phy1" so that it's easier to write scripts that don't care
about
> concurrent phy name changes? Just a thought.
Added, using "phy %0" syntax.

The entire iw tool is really just a hack btw...


> iw phy set      [ phy ] DEVICE [ CHANSPEC ] [ name NEWNAME ]
> 
> and we discussed on IRC that it might make sense to have the same for
> nl80211. On the surface, that makes sense, however, it does add a
> complication in that we need to either specify that you cannot combine
> some attributes (which doesn't really make sense), 
Hmm, I can't come up with any example other than using some 11n
attribute
with a 11abg phymode... care to hit me with a hint?

>                                                    or we need to take
> quite a bit of care with atomicity; setting the channel and changing
the
> phy name can both fail individually but having them in one netlink
> message implies that it's one transaction. I'm not sure the somewhat
> cleaner API and saving one command number is worth the additional
> transactional safety we need to be careful with then.
> 
> So I'd like to reverse my previously stated opinion and say that I now
> think that putting these orthogonal things into different commands
would
> be better so that we don't run into this transaction problem.

Well, ... take a look at net/core/rtnetlink.c line 716:

	if (err < 0 && modified && net_ratelimit())
		printk(KERN_WARNING "A link change request failed with "
		       "some changes comitted already. Interface %s may
"
		       "have been left with an inconsistent
configuration, "
		       "please check.\n", dev->name);

So, if rtnetlink doesn't bother too much about "transaction safety"
either,
why should we? If an app wants to know what failed, it can still send
SET
requests broken down into pieces, so they will know which piece failed.
Obviously they need to leave some stuff grouped (e.g. PHYMODE and
CHANNEL),
but I don't think it's useful to force them do so by breaking stuff into
multiple commands...

(There is no difference really between
	CMD_SET_PHY name=myphy
	CMD_SET_PHY phymode=a channel=1
and
	CMD_SET_PHYNAME name=myphy
	CMD_SET_CHANNEL phymode=a channel=1
but the former allows, if we don't care, to just batch it.)


-David

P.S.: I'm a bit busy and won't have time to work on stuff until approx.
next Monday. Oh and I overdid a bit on the "documentation"...
http://git.spaceboyz.net/nl80211/nl80211-meta.git/master:/nl80211doc.htm
l
(note the quad'ified attempt at associating ;)

-
To unsubscribe from this list: send the line "unsubscribe
linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: phy mode, channel -> freq mapping (was RE: more nl80211/iw tool code comments)

[Johannes Berg]

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

Hi,

> I think it is cleaner to define a parameter called BAND which can take
> values 2.4 GHz, 5 GHz and so on. Then, the combination of BAND and
> CHANNEL will uniquely define the operating frequency.

<phymode stuff snipped>

> I wasn't sure if this has already been thought about. I look forward to
> being educated if I am missing something.

Good point. I hadn't really considered this but a/g or n cards really
make it necessary to distinguish here, and regulatory stuff would also
benefit from defining it that way.

johannes

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

Re: phy mode, channel -> freq mapping (was RE: more nl80211/iw tool code comments)

[Tomas Winkler]

On 6/18/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> Hi,
>
> > I think it is cleaner to define a parameter called BAND which can take
> > values 2.4 GHz, 5 GHz and so on. Then, the combination of BAND and
> > CHANNEL will uniquely define the operating frequency.
>
> <phymode stuff snipped>
>
> > I wasn't sure if this has already been thought about. I look forward to
> > being educated if I am missing something.
>
> Good point. I hadn't really considered this but a/g or n cards really
> make it necessary to distinguish here, and regulatory stuff would also
> benefit from defining it that way.
>
I have to agree that the phymode lost is relevance. and band channel
couple has more meaning

In this context, what atheros turbo phy mode is? What channel, band
does it operate?

Thanks
Tomas

> johannes
>
>

RE: phy mode, channel -> freq mapping (was RE: more nl80211/iw tool code comments)

[Sandesh Goel]

The so called "turbo" mode extensions provided by various vendors
roughly fall into 3 categories

1. MAC extensions such as frame bursting, aggregation, using more
aggressive transmit parameters etc
2. PHY extensions such as using proprietary modulation/coding, MIMO etc
3. RF extensions such as using larger channel width, also sometimes
called channel bonding

All of these have been adequately addressed by 802.11e, WMM and 802.11n
and there exist modes which allow these to be done in a standard
compliant and interoperable manner. In my opinion, the linux wireless
stack would do well to stay away from the proprietary extensions and
thus minimize confusion. 

At most, we could allow an API for enable/disable turbo mode which would
allow the specific driver/firmware to do whatever magic it wants under
the hood without polluting the rest of the stack.

Thanks,
Sandesh

-----Original Message-----
From: Tomas Winkler [mailto:tomasw@gmail.com] 
Sent: Monday, June 18, 2007 5:55 PM
To: Johannes Berg
Cc: Sandesh Goel; David Lamparter; linux-wireless
Subject: Re: phy mode, channel -> freq mapping (was RE: more nl80211/iw
tool code comments)

On 6/18/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> Hi,
>
> > I think it is cleaner to define a parameter called BAND which can
take
> > values 2.4 GHz, 5 GHz and so on. Then, the combination of BAND and
> > CHANNEL will uniquely define the operating frequency.
>
> <phymode stuff snipped>
>
> > I wasn't sure if this has already been thought about. I look forward
to
> > being educated if I am missing something.
>
> Good point. I hadn't really considered this but a/g or n cards really
> make it necessary to distinguish here, and regulatory stuff would also
> benefit from defining it that way.
>
I have to agree that the phymode lost is relevance. and band channel
couple has more meaning

In this context, what atheros turbo phy mode is? What channel, band
does it operate?

Thanks
Tomas

> johannes
>
>

Re: phy mode, channel -> freq mapping (was RE: more nl80211/iw tool code comments)

[Jiri Benc]

On Mon, 18 Jun 2007 02:42:01 -0700, Sandesh Goel wrote:
> I have a high level comment on the whole phy mode, channel and frequency
> mapping business.
> 
> I think it is cleaner to define a parameter called BAND which can take
> values 2.4 GHz, 5 GHz and so on. Then, the combination of BAND and
> CHANNEL will uniquely define the operating frequency.

I completely agree. Actually, I thought about this in the past (see my
old patches for ieee80211) but it was not worth the effort to rewrite
mac80211 (as the changes would need to be quite invasive). But now,
with 802.11n, it's probably worth reconsidering.

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs