Hi!

> > Looks nice.. why is ieee->seq_number += 0x10; increased by 0x10?
> 
> Sequence number is stored in bits 4 to 15 of the Sequence Control field
> in 802.11 header. Lower 4 bits are used for fragment number.

Comment would be nice.

> > > separate-from-ethernet.patch
> > 
> > Okay, here it starts to be "interesting". Patches up to here seem
> > quite mergeable to me. This one will rename wifi from eth1 to wlan0,
> > right?
> > 
> > OTOH that should not be a big problem; wifi is not in mainline, yet.
> 
> Hostap uses wlan0 as well.

What other changes are required in userspace after your patches are
applied?

> > +#define IEEE80211_SNAP_IS_BRIDGE_TUNNEL(snap) \
> > +               ((snap)->oui[0] == 0 && (snap)->oui[1] == 0 && 
> > (snap)->oui[2] == 0xf8)
> > 
> > You really need some shorter prefix, and shorter names would be
> > welcome, too. [Maybe these should be inline functions?
> 
> I agree. Don't know who started to use ieee80211_ prefixes but I think
> there wasn't better prefix available (p80211 is the name of another -
> although dead - project, wlan is confusing, etc.). We try to use as
> short names as possible - do you have any idea how to shorten this
> one?

BRIDGE_TUNEL_P :-). Okay WIFI is trademark-encumbered. [Do trademarks
even apply in this context?]


> > +#define IEEE80211_GET_SADDR(hdr) \
> > +               (IEEE80211_FC_GET_FROMDS(hdr) ? \
> > +                       (IEEE80211_FC_GET_TODS(hdr) ? (hdr)->addr4 : 
> > (hdr)->addr3) \
> > +                       : (hdr)->addr2)
> > 
> > ]
> 
> I'm not against turning these macros into inline functions. But I'm
> afraid that will make ieee80211.h longer and yet more unreadable. What
> do you think is better?

I think inline function is going to be more readable than above.

> > > fix-packet_socket-arp.patch
> > 
> > +#if defined(CONFIG_IEEE80211) || defined(CONFIG_IEEE80211_MODULE)
> > [...]
> > +#endif
> > 
> > I believe CONFIG_FOO is set if CONFIG_FOO_MODULE is set; therefore we
> > do not need as ugly ifdefs.
> 
> I'm not sure about this - when I tested it few months ago, it wasn't.
> And it seems that in net/ipv4/arp.c it is used the same way.

Perhaps I do not remember correctly.

> > > fix-definitions-for-xmit.patch
> > 
> > +u8 ieee80211_rate_values[IEEE80211_NUM_RATES] = {
> > +       0x02, 0x04,
> > +       0x0b, 0x16,
> > +       0x2c, 0x42,
> > +       0x0c, 0x12, 0x18, 0x24, 0x30, 0x48, 0x60, 0x6c
> > +};
> > +EXPORT_SYMBOL(ieee80211_rate_values);
> > 
> > Could we get some comment here?
> 
> It is a static table for converting of internally used rate constants (enum
> ieee80211_rate) to (and from) values used in 802.11 management frames.
> It is exported as drivers for some devices with more advanced firmware
> need to convert the values themselves.

Comment in the source code would be nice.

                                                                Pavel
-- 
if you have sharp zaurus hardware you don't need... you know my address
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to