Hi,
This is a patch series for IEEE802.11e/WMM support against
linux-2.6.22-rc4.
[PATCH 1/3] mac80211: add IEEE802.11e/WMM structures
[PATCH 2/3] mac80211: IEEE802.11e/WMM TS management and DLS support
[PATCH 3/3] mac80211: debugfs support for TSM and DLS
Thanks,
-yi
Jiri Benc wrote:
> On Wed, 6 Jun 2007 16:21:54 +0800, Zhu Yi wrote:
>> This is a patch series for IEEE802.11e/WMM support against
>> linux-2.6.22-rc4.
>>
>> [PATCH 1/3] mac80211: add IEEE802.11e/WMM structures
>> [PATCH 2/3] mac80211: IEEE802.11e/WMM TS management and DLS support
>> [PATCH 3/3] mac80211: debugfs support for TSM and DLS
>
> John,
>
> I see you already applied those patches. I haven't had a time to look
> at the second patch in this resent version yet, but even from the first
> and third one it's clear they still need a lot of work (for example,
> adding cfg80211 support instead of that debugfs interface isn't going
> to be a few lines long patch).
>
> Please revert them.
Or better yet, let's develop them as a community by submitting patches to fix them.
James
On Thursday 07 June 2007 21:41, jketreno wrote:
> Nothing is perfect the first time--and nothing will ever get done so long
> as an aggressive and offensive position is held in regard to outside
> contribution.
>
Sure. But frankly, this code sucks. The original patch that I reviewed had
problems that indicated a total lack of testing.. and that was just for a
subset of the patch series that I actually reviewed in detail. Even the
design of the entire patch series is questionable even if we weren't trying
to move to a userspace MLME (and we need to, because kernel space MLME is a
*dead end*). The patch series also introduced sparse errors.
Do I need to go on? Make the code suck less and we won't have to go through
this again. It's that simple. It's not about being aggressive or offensive.
It's about having some standards.
Now of course, this also happened because Linville didn't see much and assumed
it was okay. If a patch needs review (and a patch series of this size *needs*
review), go ahead and ask for a review (or remind me I need to do one!). I
don't have much time this summer for writing code, but I will make sure I
have time to review code, especially if Jiri Benc is not around.
-Michael Wu
On Thu, 07 Jun 2007 21:41:51 -0700, jketreno wrote:
> In addition to the updated patches against wireless-dev incorporating
> feedback, John asked Yi to provide the WME patches against Linus' tree.
> He did..
>
> Now, a month after Yi's patches were originally sent out, Jiri cited things
> he doesn't like
I apologize it took me so long. On the other hand, I wrote at least some of
the comments when the patches were sent out first time many months ago and
they still haven't been addressed.
> --things which can be addressed through patches to the code
> vs. having to re-apply a monolithic patch.
That could work if there are just minor issues in the code. That's not this
case, the patches have still major ones.
> Nothing is perfect the first time--and nothing will ever get done so long
> as an aggressive and offensive position is held in regard to outside
> contribution.
I hope I wasn't aggressive. If it sounded that way, I apologize, I didn't
mean to. Be assured that nobody is trying to offense "outside
contributions" (there is nothing like that, btw, everyone can contribute
and is considered the same), the only merit is a quality of code.
Let's work on fixing those patches and when they are ready I will be more
than happy to let them be applied - everybody will profit from them.
Unfortunately, I won't have a time to look deeply into the second patch
until a beginning of next week. (But you can use that time for fixing the
third patch ;-))
Thanks,
Jiri
--
Jiri Benc
SUSE Labs
On Thursday 07 June 2007 20:52, jketreno wrote:
> Or better yet, let's develop them as a community by submitting patches to
> fix them.
>
Easier to ask forgiveness than to get permission, eh?
No thanks.
-Michael Wu
On Thu, 2007-06-07 at 15:40 -0700, Michael Wu wrote:
> Sure. But frankly, this code sucks. The original patch that I reviewed had
> problems that indicated a total lack of testing.. and that was just for a
> subset of the patch series that I actually reviewed in detail. Even the
> design of the entire patch series is questionable even if we weren't trying
> to move to a userspace MLME (and we need to, because kernel space MLME is a
> *dead end*).
user space MLME is good, I'd also agree to move to it if it is ready.
But the key point for the patch series is not dealing with MLME. They
are about the TS managment and DLS. Please concentrate on the key point.
Did you ever read IEEE 802.11e or WMM specs?
> The patch series also introduced sparse errors.
$ cd wireless-dev.git
$ touch net/mac80211/*.[ch]
$ make modules C=1
Sparse does generate a lot of errors. But there is only one from my
patches (because I define int dls_sta:1 which is followed the int
assoc_ap:1 defined already there).
> Do I need to go on? Make the code suck less and we won't have to go through
> this again. It's that simple. It's not about being aggressive or offensive.
> It's about having some standards.
>
> Now of course, this also happened because Linville didn't see much and assumed
> it was okay. If a patch needs review (and a patch series of this size *needs*
> review), go ahead and ask for a review (or remind me I need to do one!). I
> don't have much time this summer for writing code, but I will make sure I
> have time to review code, especially if Jiri Benc is not around.
I have addressed all your comments in this series, right? And you are in
the cc list. You have more, just speak out!
-yi
On Wed, 6 Jun 2007 16:21:54 +0800, Zhu Yi wrote:
> This is a patch series for IEEE802.11e/WMM support against
> linux-2.6.22-rc4.
>
> [PATCH 1/3] mac80211: add IEEE802.11e/WMM structures
> [PATCH 2/3] mac80211: IEEE802.11e/WMM TS management and DLS support
> [PATCH 3/3] mac80211: debugfs support for TSM and DLS
John,
I see you already applied those patches. I haven't had a time to look
at the second patch in this resent version yet, but even from the first
and third one it's clear they still need a lot of work (for example,
adding cfg80211 support instead of that debugfs interface isn't going
to be a few lines long patch).
Please revert them.
Thanks,
Jiri
--
Jiri Benc
SUSE Labs
Michael Wu wrote:
> On Thursday 07 June 2007 20:52, jketreno wrote:
>> Or better yet, let's develop them as a community by submitting patches to
>> fix them.
>>
> Easier to ask forgiveness than to get permission, eh?
>
> No thanks.
Forgiveness for what?
In addition to the updated patches against wireless-dev incorporating
feedback, John asked Yi to provide the WME patches against Linus' tree.
He did..
Now, a month after Yi's patches were originally sent out, Jiri cited things
he doesn't like--things which can be addressed through patches to the code
vs. having to re-apply a monolithic patch.
Nothing is perfect the first time--and nothing will ever get done so long
as an aggressive and offensive position is held in regard to outside
contribution.
James
>
> -Michael Wu
On Wed, 2007-06-06 at 22:00 +0200, Johannes Berg wrote:
> On Wed, 2007-06-06 at 16:21 +0800, Zhu Yi wrote:
>
> > This is a patch series for IEEE802.11e/WMM support against
> > linux-2.6.22-rc4.
>
> Great :)
>
> What I'm missing though is a sane userspace interface. Having all the
> information in debugfs is great, but I don't think debugfs should be
> writable here. Could you define this in terms of nl80211/cfg80211? I'm
> not sure what exactly would be required.
Yes, the user space API should be in the cfg80211 format. End users
definitely don't want to play with debugfs, it's only for developer's
debugging. Defining a good API is not easy, but since the core code is
there, we can begin the API discussion now.
> Also, can you explain how you plan to use this? I don't think that users
> will want to manually use the interfaces to add a direct link etc.
> Hence, it has to be done automatically. What do we base the decision on?
> If we're sending a whole bunch of data to another STA do we
> automatically try to negotiate DLS? If so, do we do that in the kernel?
> Or do we have enough information to do that in userspace?
>
> Essentially I see two choices:
> 1) we do it in the kernel, and do it automagically somehow
> 2) we do it in userspace in some daemon
>
> In the first case, somehow I'm missing how the kernel is supposed to
> make the decision that it wants to use a direct link. But in case 2),
> why do we need the kernel to send all these management frames? Couldn't
> we just have some API to tell the kernel that we have negotiated a
> direct link (yes, it'd require the injection patches, but that work is
> available)?
I think 2) is the way to go. Kernel shouldn't involve in the decision
making of DLS. Switch to DLS intelligently in the user space sounds good
but it doesn't relate to the kernel <-> user space API, you can do it
all in the user space. I think there should be 2 APIs for DLS.
1) User requests kernel to setup or tear down a DLS link with another
STA (sync and async)
2) Kernel notifies the user space (daemon) a DLS request is sent from
another STA and let user space to make the decision or an existed DLS
connection is torn down by the peer (via event).
BTW, I'd agree move the kernel MLME to user space if possible. I
implemented in the kernel because a) it's simple and other MLMEs are
also there, i.e ASSOC_REQ. b) I don't know where to implement it in the
user space, wpa_supplicant? But if we
Thanks,
-yi
On Fri, 2007-06-08 at 16:16 +0800, Zhu Yi wrote:
> Yes, the user space API should be in the cfg80211 format. End users
> definitely don't want to play with debugfs, it's only for developer's
> debugging. Defining a good API is not easy, but since the core code is
> there, we can begin the API discussion now.
Good :)
Can we start with use cases? I don't really know about TSM. DLS seems
easy enough though:
DLS_UC1:
* Ariel wants to transfer large file from wireless peer
DLS_UC2:
* Bert wants to stream video from a wireless peer
DLS_UC3:
* Carla is developing firmware for a wireless file server and wants to
support DLS for better performance
DLS_UC4:
* Dorothy is using Carla's file server with a stock linux/mac80211
laptop and wants to do the least thing possible to get DLS working
when offered
Any more?
> I think 2) is the way to go. Kernel shouldn't involve in the decision
> making of DLS.
Alright.
> Switch to DLS intelligently in the user space sounds good
> but it doesn't relate to the kernel <-> user space API, you can do it
> all in the user space.
Actually, it does, because you do need all the information to make that
decision. I don't think we have per-MAC-address traffic statistics or
something.
> 1) User requests kernel to setup or tear down a DLS link with another
> STA (sync and async)
>
> 2) Kernel notifies the user space (daemon) a DLS request is sent from
> another STA and let user space to make the decision or an existed DLS
> connection is torn down by the peer (via event).
I'd think this should both be part of nl80211. It shouldn't be too hard
to add (1) (except I don't see the point in doing it synchronously and
would rather not) to nl80211, but I do appreciate that (2) isn't
something that's easily possible for lack of event API in
cfg80211/nl80211.
> BTW, I'd agree move the kernel MLME to user space if possible. I
> implemented in the kernel because a) it's simple and other MLMEs are
> also there, i.e ASSOC_REQ. b) I don't know where to implement it in the
> user space, wpa_supplicant? But if we
But if we [...]?
I'm not sure if we can live with having parts of the MLME in userspace
and parts in the kernel like if we'd put DLS setup into userspace now.
What somebody really needs to do is
1) integrate the injection patches
2) implement a nl80211 API for the userspace MLME
3) fix the userspace MLME implementation stuff in mac80211 to work with
the injection patches and the new nl80211 API
4) publish an adapted userspace MLME that uses this (or patches to wpa
supplicant)
5) write some documentation
6) put that userspace MLME into git, publish the tree on kernel.org and
maintain it
7) decree that wpa supplicant is on-topic for linux-wireless
Some of these have sub-points:
2a) in mac80211, rip out all the userspace MLME stuff and analyse what
is required
2b) add the commands to nl80211 (make sure that only one userspace MLME
can drive a single interface)
2c) add the commands to cfg80211
2d) re-implement the commands in mac80211 based on cfg80211
2e) since we want to be able to configure the userspace MLME through
the kernel, implement nl80211 command forwarding to the userspace
MLME [1]
[1] Actually, I'd think that *all* commands should be forwarded to the
userspace MLME except for commands *from* the userspace MLME. That way,
the userspace MLME can act as a filter for commands which should make a
whole bunch of things easier.
Btw, Jouni, your email address in the wpa_supplicant developer docs is
still the old one :)
johannes
>> Switch to DLS intelligently in the user space sounds good
>> but it doesn't relate to the kernel <-> user space API, you can do it
>> all in the user space.
>
> Actually, it does, because you do need all the information to make
that
> decision. I don't think we have per-MAC-address traffic statistics or
> something.
FWIW, need to take into consideration:
1) STA1 and STA2 are close to each other, but far from the AP. DLS will
most probably improve the link between them and the medium utilization.
2) STA1 and STA2 are far away from each other and the AP is between
them. DLS _may_ deteriorate the connection between STAs and the medium
utilization, or even may not be possible at all.
IMO, The decision to initiate a DLS link should _ideally_ be based not
only on the traffic properties but also on statistics of the actual rate
used between each of the participants including packets sent to/from the
peer STA (This is not easy as it requires the initiating STA to get into
promiscuous mode for some time while it is connected to the AP, prior to
establishing the DLS).
Perhaps providing the user a manual way to configure DLS connection
between two STAs is good enough for now and can be useful also later
when an intelligent method is in place.
Guy.
On Fri, 2007-06-08 at 00:40 +0200, Jiri Benc wrote:
> I apologize it took me so long. On the other hand, I wrote at least some of
> the comments when the patches were sent out first time many months ago and
> they still haven't been addressed.
I think I have all addressed all the comments from the list last time.
If you still have _valid_ comments, please point out.
-yi
On Wed, 2007-06-06 at 16:21 +0800, Zhu Yi wrote:
> This is a patch series for IEEE802.11e/WMM support against
> linux-2.6.22-rc4.
Great :)
What I'm missing though is a sane userspace interface. Having all the
information in debugfs is great, but I don't think debugfs should be
writable here. Could you define this in terms of nl80211/cfg80211? I'm
not sure what exactly would be required.
Also, can you explain how you plan to use this? I don't think that users
will want to manually use the interfaces to add a direct link etc.
Hence, it has to be done automatically. What do we base the decision on?
If we're sending a whole bunch of data to another STA do we
automatically try to negotiate DLS? If so, do we do that in the kernel?
Or do we have enough information to do that in userspace?
Essentially I see two choices:
1) we do it in the kernel, and do it automagically somehow
2) we do it in userspace in some daemon
In the first case, somehow I'm missing how the kernel is supposed to
make the decision that it wants to use a direct link. But in case 2),
why do we need the kernel to send all these management frames? Couldn't
we just have some API to tell the kernel that we have negotiated a
direct link (yes, it'd require the injection patches, but that work is
available)?
johannes
On Thu, Jun 07, 2007 at 10:43:15PM +0200, Jiri Benc wrote:
> On Wed, 6 Jun 2007 16:21:54 +0800, Zhu Yi wrote:
> > This is a patch series for IEEE802.11e/WMM support against
> > linux-2.6.22-rc4.
> >
> > [PATCH 1/3] mac80211: add IEEE802.11e/WMM structures
> > [PATCH 2/3] mac80211: IEEE802.11e/WMM TS management and DLS support
> > [PATCH 3/3] mac80211: debugfs support for TSM and DLS
> I see you already applied those patches. I haven't had a time to look
> at the second patch in this resent version yet, but even from the first
> and third one it's clear they still need a lot of work (for example,
> adding cfg80211 support instead of that debugfs interface isn't going
> to be a few lines long patch).
>
> Please revert them.
I think you will agree that Yi has been making a good effort to correct
problems identified with this patch series. I think it is best to
continue integrating those patches into the wireless-dev tree for now.
Once the code is agreeable to those concerned (including a reasonable
API for DLS establishment), then we can ask for Yi (or someone else)
to submit a clean patchset for sending upstream. As long as the
individual patches are focused on a particular feature, git provides
facilities for combining patches that are fairly robust and easy
to manage. So, creating a clean patchset should be no great hardship.
John
P.S. The future of wireless-dev is still open for dicussion. I think
it can still be useful for integrating "in progress" patches and
feeding them to -mm for early testing. However, I definitely would
like to reduce the delta between wireless-dev and wireless-2.6 as
much as possible and as soon as possible.
--
John W. Linville
[email protected]