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
Attachment:
pgpHMOkKKpkxQ.pgp
Description: PGP signature
