Because everybody has already seen these patches I won't be sending them
to the list again, uploaded here:
http://johannes.sipsolutions.net/patches/net-2.6.24/
That contains ports of my and Jiri Slaby's recently merged patches to
the net-2.6.24 tree minus those that don't apply because they delete
ioctls that aren't in that tree (patches 0..23) plus some other patches
I cherry-picked that I think should go in (024, 025), some whitespace
and comment cleanups to make it more like wireless-dev (026, 027). Then
there's your multicast fix (028) and those parts of the series I posted
yesterday that don't depend on nl80211 (029..035).
I've merged the event.c copyright patch (that I accidentally forgot to
copy to the list but it only added 6 lines copyright statement) into the
relevant cleanup patch (015).
johannes
On Mon, 2007-08-27 at 00:29 -0400, Michael Wu wrote:
> Patch 10:
[...]
I addressed your comments with the patch against wireless-dev I sent to
linux-wireless a few minutes ago. The same patch I rolled into the
updated patch 10 at
http://johannes.sipsolutions.net/patches/net-2.6.24/all/2007-08-27-13:38/
> Patch 17 (by Jiri Slaby):
> + pkt_data->flags &= ~(IEEE80211_TXPD_REQ_TX_STATUS |
> + IEEE80211_TXPD_DO_NOT_ENCRYPT | IEEE80211_TXPD_REQUEUE |
> + IEEE80211_TXPD_MGMT_IFACE);
> Or set it to 0?
Yup, changed, and will post change to wireless-dev.
> I'll review the rest of the patches tomorrow.. Patch 22 (revamp key handling)
> is a bit too big and scary for me to review tonight.
Don't go any further than 027 and then review those against wireless-dev
I posted instead, though I expect I'll be reposting today to fix the few
bugs people found.
johannes
On Tue, 2007-08-28 at 10:40 +0200, Johannes Berg wrote:
> On Mon, 2007-08-27 at 19:45 -0400, Michael Wu wrote:
> > On Saturday 25 August 2007 03:37, Johannes Berg wrote:
> > > http://johannes.sipsolutions.net/patches/net-2.6.24/
> > >
> > Patch 22:
> > + ap = sta_info_get(key->local, key->sdata->u.sta.bssid);
> > How does this work when we're setting a multicast/broadcast key on an AP
> > interface?
>
> Hmm. I don't think that ever worked correctly for when we ourselves are
> using WMM. I can fix that.
Ah no, I see, I just missed a (type == STA) condition. You never send
multicast traffic as QoS frames anyway.
johannes
On Saturday 25 August 2007 03:37, Johannes Berg wrote:
> http://johannes.sipsolutions.net/patches/net-2.6.24/
>
Patches 1-9: ACK
Patch 10:
+ /*
+ * No point in finding a key if the frame is neither
+ * addressed to us nor a multicast frame.
+ */
+ if (!rx->u.rx.ra_match)
+ return TXRX_DROP;
If you really want to drop the frame, it should be done elsewhere. It doesn't
seem appropriate here. TXRX_CONTINUE instead should be okay.
+ /*
+ * The device doesn't give us the IV so won't be
+ * able to look up the key. That's ok though, we
+ * don't need to decrypt the frame, we just won't
+ * be able to keep statistics accurate.
+ * Except for key threshold notifications, should
+ * we somehow allow the driver to tell us which key
+ * the hardware used if this flag is set?
+ */
Who won't be able to look up the key?
Second sentence seems a bit sketchy too but as long as I can understand it on
the first try.. ;)
+ if (rx->skb->len < 8 + hdrlen)
+ /* COUNT THIS? */
+ return TXRX_DROP;
For comments like these, I try to put the comment on the same line since I
expect to see code immediately following an if/for/while/etc. statement that
doesn't have curly braces. No problem if you prefer this style, however..
- if (rx->fc & IEEE80211_FCTL_PROTECTED && rx->key && rx->u.rx.ra_match) {
+ if (rx->key && rx->u.rx.ra_match) {
There's no way rx->u.rx.ra_match can be 0 now, can it?
I think the goto find_by_index can be avoided easily by testing for
(!is_multicast_ether_addr(hdr->addr1) && rx->sta) instead.
Patch 11: Well, okay. I'll probably add something like this back in the future
for improved rate control but it's not being used now so it should be
removed. ACK
Patch 12:
- int allow_broadcast_always; /* whether to allow TX of broadcast frames
- * even when there are no associated STAs
- */
-
Hmm.. that seems useful.. yet nothing is using it? What happen here?
ACK
Patch 13-14: ACK
Patch 15:
+ /*
+ * TODO: re-add support for sending MIC failure indication
+ * with all info via nl80211
+ */
Re-add? Don't think nl80211 ever had support for MIC failure event reporting,
but I guess I'm reading this differently. ACK
Patch 16: ACK
Patch 17 (by Jiri Slaby):
+ pkt_data->flags &= ~(IEEE80211_TXPD_REQ_TX_STATUS |
+ IEEE80211_TXPD_DO_NOT_ENCRYPT | IEEE80211_TXPD_REQUEUE |
+ IEEE80211_TXPD_MGMT_IFACE);
Or set it to 0?
Patch 18-21: ACK
I'll review the rest of the patches tomorrow.. Patch 22 (revamp key handling)
is a bit too big and scary for me to review tonight.
Thanks,
-Michael Wu
On Mon, 2007-08-27 at 19:45 -0400, Michael Wu wrote:
> On Saturday 25 August 2007 03:37, Johannes Berg wrote:
> > http://johannes.sipsolutions.net/patches/net-2.6.24/
> >
> Patch 22:
> + ap = sta_info_get(key->local, key->sdata->u.sta.bssid);
> How does this work when we're setting a multicast/broadcast key on an AP
> interface?
Hmm. I don't think that ever worked correctly for when we ourselves are
using WMM. I can fix that.
>
> + list_add(&key->list, &sdata->key_list);
> Should we be holding the key mutex here?
Yeah.
> This patch does too many things. If you can split some things out into
> separate patches, like moving code into key.c, it would be easier to review.
Not easily since it's really intermingled with the ioctl code. Have you
looked at the original?
johannes
Johannes Berg wrote:
> Because everybody has already seen these patches I won't be sending them
> to the list again, uploaded here:
>
> http://johannes.sipsolutions.net/patches/net-2.6.24/
>
I have tested the patches and they work with zd1211rw-mac80211 and
WPA. They version I tested can be accessed using
git://deine-taler.de/git/zd1211rw.git branch zd1211rw-net.
--
Uli Kunitz
On Mon, 2007-08-27 at 00:29 -0400, Michael Wu wrote:
> Patch 10:
> + /*
> + * No point in finding a key if the frame is neither
> + * addressed to us nor a multicast frame.
> + */
> + if (!rx->u.rx.ra_match)
> + return TXRX_DROP;
> If you really want to drop the frame, it should be done elsewhere. It doesn't
> seem appropriate here. TXRX_CONTINUE instead should be okay.
Hmm. I wasn't sure what would happen if I let it pass further up and we
won't be able to decrypt it (unless it's WEP etc... but then we don't
want the frame either) so the only thing we can possibly do is drop it.
It seemed appropriate to do it here.
> + /*
> + * The device doesn't give us the IV so won't be
> + * able to look up the key. That's ok though, we
> + * don't need to decrypt the frame, we just won't
> + * be able to keep statistics accurate.
> + * Except for key threshold notifications, should
> + * we somehow allow the driver to tell us which key
> + * the hardware used if this flag is set?
> + */
> Who won't be able to look up the key?
"we" aka mac80211. Not sure where that got lost.
> Second sentence seems a bit sketchy too but as long as I can understand it on
> the first try.. ;)
Heh.
> + if (rx->skb->len < 8 + hdrlen)
> + /* COUNT THIS? */
> + return TXRX_DROP;
> For comments like these, I try to put the comment on the same line since I
> expect to see code immediately following an if/for/while/etc. statement that
> doesn't have curly braces. No problem if you prefer this style, however..
Oh, I see what you mean. Don't think it matters much to me though I tend
to then decide "ah, remove the comment" and not update the braces.
> - if (rx->fc & IEEE80211_FCTL_PROTECTED && rx->key && rx->u.rx.ra_match) {
> + if (rx->key && rx->u.rx.ra_match) {
> There's no way rx->u.rx.ra_match can be 0 now, can it?
Heh, no.
> I think the goto find_by_index can be avoided easily by testing for
> (!is_multicast_ether_addr(hdr->addr1) && rx->sta) instead.
Like this:
if (!is_multicast_ether_addr(...) && rx->sta && rx->sta->key) {
} else {
trying_wep = rx->sta && !rx->sta->key;
}
or something like that. But the whole !rx->sta semantics aren't quite
clear, and I thought this was easier to understand. I can change it if
you prefer.
> Patch 11: Well, okay. I'll probably add something like this back in the future
> for improved rate control but it's not being used now so it should be
> removed. ACK
Could be, but should a be per-sta setting in nl80211. I'm trying to
reduce baggage here.
> Patch 12:
> - int allow_broadcast_always; /* whether to allow TX of broadcast frames
> - * even when there are no associated STAs
> - */
> -
> Hmm.. that seems useful.. yet nothing is using it? What happen here?
It's "used" in wireless-dev in ioctls, but talking to Jouni he seemed to
have a better idea instead so was OK with removing it.
> Patch 15:
> + /*
> + * TODO: re-add support for sending MIC failure indication
> + * with all info via nl80211
> + */
> Re-add? Don't think nl80211 ever had support for MIC failure event reporting,
> but I guess I'm reading this differently. ACK
(re-add support for sending ...) (with all info [and] via nl80211)
> Patch 17 (by Jiri Slaby):
> + pkt_data->flags &= ~(IEEE80211_TXPD_REQ_TX_STATUS |
> + IEEE80211_TXPD_DO_NOT_ENCRYPT | IEEE80211_TXPD_REQUEUE |
> + IEEE80211_TXPD_MGMT_IFACE);
> Or set it to 0?
Not my patch :P
I think he just made the patch so it's semantically identical to before,
but I guess that this actually touched all existing flags so 0 would
probably be appropriate here.
Thanks,
johannes
On Saturday 25 August 2007 03:37, Johannes Berg wrote:
> http://johannes.sipsolutions.net/patches/net-2.6.24/
>
Patch 22:
+ ap = sta_info_get(key->local, key->sdata->u.sta.bssid);
How does this work when we're setting a multicast/broadcast key on an AP
interface?
+ list_add(&key->list, &sdata->key_list);
Should we be holding the key mutex here?
This patch does too many things. If you can split some things out into
separate patches, like moving code into key.c, it would be easier to review.
Patch 23-29: ACK
Thanks,
-Michael Wu
On Sat, 2007-08-25 at 09:37 +0200, Johannes Berg wrote:
> Because everybody has already seen these patches I won't be sending them
> to the list again, uploaded here:
>
> http://johannes.sipsolutions.net/patches/net-2.6.24/
A new series is now at
http://johannes.sipsolutions.net/patches/net-2.6.24/all/2007-08-27-13:47/ that changes all those patch to address comments from Michael. Please push 001 through 027 to davem once Michael got a chance to review the changes I made to patch 10 (or the corresponding wireless-dev update) and patches 022 through 027.
johannes
On Sun, 2007-08-26 at 17:22 +0200, Ulrich Kunitz wrote:
> I have tested the patches and they work with zd1211rw-mac80211 and
> WPA. They version I tested can be accessed using
> git://deine-taler.de/git/zd1211rw.git branch zd1211rw-net.
Great, thanks for the heads-up.
johannes
On Mon, 2007-09-03 at 17:20 -0700, Jouni Malinen wrote:
> For the original reason for adding this (making it easier to do some
> continuous TX tests), yes, it can be done in other ways. The only thing
> I can think of here would be some special frames (like broadcast data
> frames as a beacon of some upper layer service) that would be used by
> clients that are capable of receiving frames when not associated (e.g.,
> they use monitor mode). This is getting a bit speculative here, so
> unless someone can point out a real example that depends on this,
This sounds like something that Andy was working on with radiotap
injection etc. Penumbra.
> I
> would go ahead and remove it now.
I think it's already gone.
johannes
On Mon, Aug 27, 2007 at 12:46:52PM +0200, Johannes Berg wrote:
> On Mon, 2007-08-27 at 00:29 -0400, Michael Wu wrote:
> > Patch 12:
> > - int allow_broadcast_always; /* whether to allow TX of broadcast frames
> > - * even when there are no associated STAs
> > - */
> > -
> > Hmm.. that seems useful.. yet nothing is using it? What happen here?
>
> It's "used" in wireless-dev in ioctls, but talking to Jouni he seemed to
> have a better idea instead so was OK with removing it.
For the original reason for adding this (making it easier to do some
continuous TX tests), yes, it can be done in other ways. The only thing
I can think of here would be some special frames (like broadcast data
frames as a beacon of some upper layer service) that would be used by
clients that are capable of receiving frames when not associated (e.g.,
they use monitor mode). This is getting a bit speculative here, so
unless someone can point out a real example that depends on this, I
would go ahead and remove it now.
--
Jouni Malinen PGP id EFC895FA