On Thu, 2008-10-30 at 11:37 +0000, Dave wrote: > Johannes Berg wrote: > > On Wed, 2008-10-29 at 15:27 +0000, Dave wrote: > > > >>> -static inline u8 *orinoco_get_ie(u8 *data, size_t len, > >>> - enum ieee80211_mfie eid) > >>> +static inline u8 *orinoco_get_ie(u8 *data, size_t len, u8 eid) > >> Would it be better to change to enum ieee80211_eid here? > I don't expect orinoco to be doing anything non-standard with IE's, so > anything we want should be available from the enum. And I like the extra > type checking. So I'd go with the enum if you don't mind. Ok, sure. I think I deleted the patch locally, but I'll import my own email and edit it :) > >>> - if ( (new_mtu + ENCAPS_OVERHEAD + IEEE80211_HLEN) > > >>> + /* MTU + encapsulation + header length */ > >>> + if ( (new_mtu + ENCAPS_OVERHEAD + 24) > > >> I think that constant should be 30. I'd prefer it if we didn't use a > >> magic number here. How about sizeof(ieee80211_hdr)? > > > > I wanted to use sizeof, but then I checked and realised the driver > > doesn't support WDS mode, so it never needs a 4-addr header format, so > > 24 is the right header size. > > I'm not sure how this was originally set, and what the MTU is all > about... so I'll defer to you on this. However it might make sense to do > the change in value in a separate commit. Well IEEE82011_HLEN is 30 == sizeof(ieee80211_hdr) (I think) But when I saw this, I noticed that since it doesn't support WDS mode it'll never actually need 30 bytes of header. The check here is to verify that all packets passed down from the networking stack actually fit into 802.11 packets, so using 30 obviously won't matter since it means a smaller MTU is supposed, and since ethernet tends to have a 1500 byte MTU with wireless being much larger, that is typically not an issue. I don't know anybody who sets their wireless MTU larger than the default of 1500. > I had a quick reread for sanity: in orinoco_xmit we always write at > least 46 (HERMES_802_3_OFFSET) bytes of header before the payload. > Shouldn't our max MTU key off of that? It looks fishy to me. That I don't understand, the value 46 doesn't tell me anything. > > Oh. I wasn't aware the constants differed. What's this used for? > > I think this just allocates the hardware buffer we copy our frames into > for transmission. It looks like the hw buffer can go up to 4k (except on > buggy Symbol firmware). Ok. I'll change the HLEN and element ID things and resend, thanks for the review. johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
