=====================================================
CODE REVIEWER: [EMAIL PROTECTED]
WEBREV: http://cr.grommit.com/~meem/wifi-1018
FILES: net80211 core
NOTES: Description of feedback:
ACCEPT Request accepted
REJECT Request rejected
EXPLAIN Explanation given
DISCUSS Request requires further discussion to resolve
DEFER Request deferred (e.g. because work is
out-of-scope)
=====================================================
Venu, thanks for the review. Below is our response to your comments. Please
let us know if you have any additional questions.
______________________________________________________________________________
usr/src/uts/common/io/net80211/net80211.c
General comment. Quite a few functions don't have a lot of comments,
some none. Consider adding a block comment giving some details for
those that don't.
| ACCEPT
If it is possible, try break this file functionally into smaller ones.
Else, it will keep growing and may become difficult to maintain.
| ACCEPT
| To keep consistent with FreeBSD file schema, split net80211.c back
| to 5 files as below:
| ieee80211_input.c - process received frames
| ieee80211_output.c - conpose and send out frames
| ieee80211_node.c - node management functions
| ieee80211_proto.c - protocol management, mainly manage state
transition
| ieee80211.c - generic functions
L38 - Collapse deltas (I suppose you plan to do this, so won't
mention for others).
| EXPLAIN
| Yes. This is to track changes during development. Before putback,
| 'wx redelget -m' will be ran
L92-95 - Can probably prefix these fns with ieee80211_
L110,L1750
| ACCEPT
| Added prefix 'ieee80211_'
L92-126 - Fn declarations are not consistent, either name the
arguments
for all or leave them out.
| ACCEPT
| Deleted all the arguments names
L128 - Prefix macro with IEEE80211_ and also a comment on why '2'
will be helpful.
| ACCEPT
| Added prefix 'IEEE80211_'
| Added comments as below:
|/*
| * association failures before ignored
| * The failure may be caused by the response frame is lost for
| * environmental reason. So Try associate more than once before
| * ignore the node
| */
L129 - Name seems very generic, check if you can use one of the
existing ABS macro or re-name to something more specific.
| ACCEPT
| Replaced it with system ABS macro.
L147-149 - Consider using ieee80211_err() by passing a flag to
distinguish between CE_CONT and CE_WARN rather than
duplicating.
| ACCEPT
| To remove the duplicating, a Macro IEEE80211_DPRINT is defined
| and changed ieee80211_err() & ieee80211_dbg() to call it with
| different severity level
L160 - Doesn't seem to be indented by 4 spaces. Is the code cstyled?
L168,L307,L438,L620,L663-664,L758,L764,L839,L1033,L1101,L1125
L1109,L1136,L1156,L1207,L1224-1228,L1337,L1496,L1525,L1559-1562,
L1607 L1609-1611,L1618,L1656,L1741-1745,L1754,L1758-1759 etc.
| ACCEPT
| Changed to indent by 4 spaces
L159,L167 - Why are these functions, seems to be called only
at one place.
| EXPLAIN
| These're OS specific functions. In FreeBSD, they're called more
| than once.
L159 - why pass in newassoc when it is not being used?
| ACCEPT
| Deleted argument 'newassoc'
L175 - mbuf -> mblk?
| ACCEPT
L180,183 - Any reason it is uint32_t instead of, say, uint_t.
| ACCEPT
| Changed to 'int'
L207 - Why '8'? Define as macro and use or add comments.
| ACCEPT
| Added comments as below:
| /*
| * Calculate ic_tim_bitmap size in bytes
| * IEEE80211_AID_MAX defines maximum bits in ic_tim_bitmap
| */
L222,225 - Suppose this could just have been
return (kmem_zalloc(sizeof (ieee80211_node_t), KM_SLEEP));
| ACCEPT
L237 - Is in_rxfrag always guaranteed to be a block, i.e. b_cont is
L250 always NULL?
| ACCEPT
| Fixed to call freemsg() instead of freeb()
L292 - Use {} for else
L563,
L1194,L3068,L3070,L3718
| ACCEPT
| Added '{}'
L366-368 - Use copy_bss()
L1053-1055
| ACCEPT
| Changed to use ieee80211_copy_bss(). (Here copy_bss() is renamed to be
| ieee80211_copy_bss() by previous comment)
L544 - Could include this comment in the previous line too. Is this
for ieee80211_fakeup_adhoc_node()? If, so move it down.
| ACCEPT
| Yes, this comment is for ieee80211_fakeup_adhoc_node(). So moved
| it down and included it in the comments before
ieee80211_fakeup_adhoc_node()
L572 - Prefix this MACRO appropriately.
L608
| ACCEPT
| Added prefix 'IEEE80211_'
L590 - Use {} when condition spans multiple lines.
L626, L629,L646, L649,L1249,L1443,L1893,L2213,L2342,L2460,L2636,L2700,
L3416,L3420,L3814,L3916-3923
| ACCEPT
| Added '{}'
L601-602 - Does this need to be done within the lock?
| ACCEPT
| No. So moved IEEE80211_NODE_UNLOCK() right above L601.
L609 - Is cstyle OK when there isn't space around the operator?
L2274
| ACCEPT
| Added spaces before and after binary operators
L622-623 - Not sure what the coding standard says, but I prefer one
L869 defn/line.
L1240-1242,L1384,L1643,L2161,L2445,L2683-2684,2687,L3028,L3310,L3655,
L4015
| DISCUSS
| Changed to be one defn/line.
L627,L629 - !((b->in_capinfo & IEEE80211_CAPINFO_PRIVACY) instead of
checking it with 0.
| ACCEPT
L613 - Any reason you could't actually return the node instead of
> 0 then a? Since the way it is used:
if (ieee80211_node_compare(ic, in, selbs) > 0)
selbs = in;
clould just as well be:
selbs = ieee80211_node_compare(ic, in, selbs);
| ACCEPT
| Changed ieee80211_node_compare() to return selected node.
L639 - Add a comment for '5', better still define it as macro.
| ACCEPT
| Defined a macro as below:
| /*
| * The RSSI values of two node are taken as almost the same when
| * the difference between these two node's RSSI values is within
| * IEEE80211_RSSI_CMP_THRESHOLD
| */
| #define IEEE80211_RSSI_CMP_THRESHOLD 5
L669 - inact is %d.
| ACCEPT
| Changed from '%u' to '%d'
L703 - Is ieee80211_node_table_reset used?
| EXPLAIN
| In FreeBSD, it's called by iwi driver.
L728-731 - This comment is not very clear to me.
| ACCEPT
| The comment doesn't make sense here so deleted it.
L754 - use boolean_t instead.
| ACCEPT
L823 - So, assuming we don't send any management frame, we will
always start from the head whenever we timeout a node?
| EXPLAIN
| Before calling ieee80211_node_leave(), the node table lock is
| released (Or there will be recursive mutex lock), Then during
| this period, it's possible that the node next to current one
| disassociates and thus is removed from the node table. That
| makes it impossible to know which node should be the next. So here
| it always re-starts from the head. And the in_scangen will prevent
| the node from being processed again.
L854 - ic_private will always be non-null? (and in some other places
as well).
| EXPLAIN
| Yes. ic_private is allocated in ieee80211_attach(). So it must not
| be NULL
L861 - Is ieee80211_node_unauthorize() used?
| ACCEPT
| Fixed to call ieee80211_node_unauthorize() from ieee80211_send_mgmt()
L872 - Why this assert given that ieee80211_alloc_node() seems to
check for null allocation?
| EXPLAIN
| Allocate bss node fails may cause system crash. So here ASSERT
| is used.
L972 - Add an empty line.
L1087,L1217,L1687,L2060,L2632
| ACCEPT
L978-979 - Is there a case it will not exist or are we here just
for this case (i.e switiching mastership).
| EXPLAIN
| ic_sta is initialized in ieee80211_attach(). So actually
| ic_sta is always not NULL.
| The sentence is confusing here, thus removed "it will ..." from
| the comment.
L963,1012 - Why isn't this a void? The only places I see this used
either ignore the return or, check for 0 (which will never
happen). ieee80211_ibss_merge() returns this value, but
it could just return 1, anyways.
| ACCEPT
| Changed to return void
L1037 - Define as boolean_t.
| ACCEPT
L1165 - Could the function somehow result in removing the node?
| EXPLAIN
| Yes, it's possible.
| Before calling the callback function, the node reference count is
| increased, and then node lock is released.
| During execution of the callback function, it's possible that
| another thread tries to remove the node. Since the reference count
| isn't 0, the node will be kept in the node table.
| Then after the execution of the callback function, ieee80211_free_node()
| is called to decrease node reference and free the node if the
| reference count is 0.
L1235 - Please add some comments for this.
| ACCEPT
| Added comments as below:
|/*
| * Adjust/Fix the specified node's rate table
| *
| * in node
| * flag IEEE80211_F_DOSORT : sort the node's rate table
| * IEEE80211_F_DONEGO : mark a rate as basic rate if it is
| * a device's basic rate
| * IEEE80211_F_DODEL : delete rates not supported by the device
| * IEEE80211_F_DOFRATE: check if the fixed rate is supported by
| * the device
| *
| * The highest bit of returned rate value is set to 1 on failure.
| */
L1258-1268 - This just sorts i, right? Not the entire list.
| EXPLAIN
| Yes
L1296 - Does it matter what the value of ignore is, i.e. could it
just be a boolean_t?
L1303 - use (ignore > 0)
| ACCEPT
| Changed ignore to be of type boolean_t.
L1320 - Why isn't this in L1255?
| EXPLAIN
| When the flag is IEEE80211_F_DODEL and nrs->ir_rates[i] should be
| deleted, rates of index [i+1]-[nrs->ir_nrates-1] will move ahead
| and become [i]-[nrs->ir_nrates-2]. 'i++' isn't needed here. So
| 'i++' isn't put in L1255 'for ()'
L1303-1307 - Don't know how often this happens. Is array the most
effective option in such cases?
| DISCUSS
| At least AFAIK, yes.
L1472 - declare onoff as boolean_t
| ACCEPT
L1546 - typo (channe)
| ACCEPT
| Changed to 'channel'
L1580-1583 - just wondering if this is enforced somehow.. or what
results if this is not true.
| EXPLAIN
| Generally it's required for devices which do scan with software.
| If such driver doesn't either call ieee80211_begin_scan() or initialize
| properly, no probe request frame will be sent when it call
| ieee80211_next_scan(). But since it could still receive beacon
| frames from AP. The result is apparently inconsistent scan results.
L1586,1621,1627 - from what I can see none of the callers seem
interested in the return value.
| ACCEPT
| Changed return value to be void
L1725 - Return boolean
L1749
| ACCEPT
L1910 - Should this be an assert, then?
| DISCUSS
| ieee80211_auth_open() can also be called when the device functions
| as an AP. But the feature isn't supported yet now. Instead of an
| assert, I'd like to remove this comment.
L1982 - use (timer > 0)
| ACCEPT
L1967,1990 - why is it returning anything? ieee80211_send_mgmt()
seem to be using the return, but I suppose it could
just as well return 0. (Why not return ic_xmit,
instead of ignoring it? - for some others too)
| ACCEPT
| Changed to return ic_xmit()
L2100 - Why int32_t? and not just int?
| ACCEPT
| Changed to int
L2122 - I suppose we can just add optielen, assuming it will be 0
if optie == NULL (similarly for L2327)
| ACCEPT
| Changed to add optielen directly
L2189-2191 - Use of Macros are better.
L2225,L2289
| ACCEPT
| Added Macros as below:
|/* Length of management frame variable-length components in bytes */
|#define IEEE80211_NWID_LEN 32 /* SSID */
|#define IEEE80211_FH_LEN 5 /* FH parameters */
|#define IEEE80211_DS_LEN 1 /* DS parameters */
|#define IEEE80211_IBSS_LEN 4 /* IBSS parameters */
|#define IEEE80211_ERP_LEN 1 /* ERP information */
L2161 - use boolean_t where appropriate
| ACCEPT
| Changed type of has_challenge, is_shared_key to boolean_t
L2119-2121, - Consider using a macro if this is a recurring thing.
2187-2188,2192
| DISCUSS
| Each infomation element has its own definition and may be included
| in different management frames. But puting these 3 information
| elements together doesn't make any sense. So I'd like to keep
| them seperated. But a Macro is defined for
| (IEEE80211_RATE_MAXSIZE - IEEE80211_RATE_SIZE)
| as below:
|#define IEEE80211_XRATE_SIZE (IEEE80211_RATE_MAXSIZE -
IEEE80211_RATE_SIZE)
| /* size of extended supported rates */
L2197 - We allocate it regardless of whether it is filled in
or not?
| ACCEPT
| Changed as below:
| (ic->ic_flags & IEEE80211_F_WME ?
| sizeof (struct ieee80211_wme_param) : 0)); /* WME */
L2202-2203 - Is the timestamp filled someplace else?
| ACCEPT
| Added comments as below
| /* timestamp is set by hardware/driver */
L2291,2293 - Remove space before ++
| ACCEPT
L2461 - in_associd is not a boolean
| ACCEPT
| Changed to 'in->in_associd != 0'
L2479 - What is 12?
| ACCEPT
| Added Macro as below:
|/*
| * minimal length of beacon/probe response frame elements
| * time stamp[8] + beacon interval[2] + capability[2]
| */
|#define IEEE80211_BEACON_ELEM_MIN 12
L2494-2495 - Why continue?
| ACCEPT
| Changed to use 'break'
L2557 - '2' gets us to the next paramter?
| ACCEPT
| Yes. And to make it clear, added Macro as below:
|/*
| * Length of management frame information elements containing
| * a variable-length component is:
| * element_id(1 byte) + length(1 byte) + component(variable bytes)
| */
|#define IEEE80211_ELEM_LEN(complen) (2 + (complen))
|
| And changed code as below:
| /* frm[1] - component length */
| frm += IEEE80211_ELEM_LEN(frm[1]);
L2603 - !(ic->ic_flags & IEEE80211_F_SCAN)
| ACCEPT
L2738 - allocbs = B_FALSE.
| ACCEPT
L2742 - allocbs = B_TRUE
| ACCEPT
L2764 - think cstyle requires the operator to be in the prev
line.
| ACCEPT
L2753 - don't we alloc a node here too?
| EXPLAIN
| Yes. But this node will be used later, it cannot be freed. So
| allocbs isn't set.
L2763-2764 - how do we come up with these flags, a comment might
L2884-2885 help.
| ACCEPT & EXPLAIN
| Each Macro has comments followed. Comment has been added to
| ieee80211_fix_rate() which used these flags.
| Here added comments as below:
| For L2763-2764
| /*
| * Adjust and check station's rate list with device's
| * supported rate. Send back response if there is at
| * least one rate or the fixed rate(if being set) is
| * supported by both station and the device
| */
| For L2884-2885
| /*
| * Adjust and check AP's rate list with device's
| * supported rate. Re-start scan if no rate is or the
| * fixed rate(if being set) cannot be supported by
| * either AP or the device.
| */
L2792 - use macro instead of '6'.
L2836
| ACCEPT
| Added Macro as below:
|/*
| * Minimal length of authentication frame elements
| * algorithm[2] + sequence[2] + status[2]
| */
|#define IEEE80211_AUTH_ELEM_MIN 6
|/*
| * Minimal length of association response frame elements
| * capability[2] + status[2] + association ID[2]
| */
|#define IEEE80211_ASSOC_RESP_ELEM_MIN 6
L2867 - continue? could be break, right?
| ACCEPT
| Changed to use break
L3001-3004 - We don't need to check the mp itself here? The
caller is responsible?
| ACCEPT
| Added ASSERT as below:
| ASSERT(mp != NULL && MBLKL(mp) >= sizeof (struct ieee80211_frame));
L3059 - !(ic->ic_flags & IEEE80211_F_SCAN)
(similarly in L3118,L3400,L3403,L3407,L3714)
| ACCEPT
L3119,3122,3133 - tid is always 0?
| EXPLAIN
| tid is non-zero when QoS(802.11e) is enabled. Currently QoS is not
| supported and it is always 0.
L3190 - ieee80211_crypto_demic - typo??
| EXPLAIN
| ieee80211_crypto_demic() is to support WPA/WPA2. Currently it's
| not ported. Later it will be added.
L3223 - Didn't see any stats in ieee80211_defrag() to
reflect this..
| EXPLAIN
| In ieee80211_defrag(), L3364-3367 implies frame is dropped.
| L3377-3383 implies frame is not complete yet.
L3268 - ic->ic_stats.is_wep_errors++??
| ACCEPT
| You're right. Added ic->ic_stats.is_wep_errors++
L3310 - fragno & more_frag used as boolean_t
| EXPLAIN
| Both are not boolean_t but uint8_t
L3352 - cstyle requires space around operator
| ACCEPT
L3351 - what does this comment mean?
| ACCEPT
| Added more comment as below:
| * Sequence control field contains 12-bit sequence no
| * and 4-bit fragment number. For fragemnts, the
| * sequence no is not changed.
L3359 - so, if the incoming sequence is not the next one
(L3352) we discard the existing fragment (mfrag)?
| EXPLAIN
| Yes. This can be either a corrupted fragment or a new fragment.
| Discard the corrupted one. For a new one, since currently we only
| support one fragments. The old one will be discarded.
L3390 - Even though fail seems to be a bit mask, it doesn't
seem to be used that way (L1696 & L 1735)
| EXPLAIN
| It's useful for debugging
L3492 - ieee80211_begin_scan() takes boolean_t as 2nd arg,
so use B_FALSE or B_TRUE.
| ACCEPT
L3649 - Is ieee80211_beacon_alloc() used?
| EXPLAIN
| It's called by wifi drivers.
L3671-3672 - these are optional?
| EXPLAIN
| The features are not supported yet.
L3699-3709 - Can this be pulled out, looks like there a couple
of places this is duplicated?
| ACCEPT
| Added new static function ieee80211_get_capinfo()
L3774 - Are these being used?
L3800
| EXPLAIN
| These are used by wifi drivers.
L3777 - Remove
| ACCEPT
L3849 - ic->ic_watchdog != NULL
L3862
| ACCEPT
L3879,3906 - suppose this can be a boolean_t
| EXPLAIN
| The variable need_inact_timer is not boolean_t. But its name may
| cause confusion. So re-named it to inact_timer.
L3869-3871 - Can we add some more comments here, particulary
about L3892-3902, is there a reason for this
sequence given nt_inact_timer used in both and
nt_inact_timer is decremented in L3893 (I mean
if that affects L3898).
| EXPLAIN
| L3892-3902 won't affect L3898. L3892-3902 runs for node table ic_scan
| while L3898-3902 are for node table ic_sta.
L3947-3948 - If this assumption changes in the future, what is
the impact?
| EXPLAIN
| It's true according to current 802.11 protocol physical layer
| definition. It's unlikely to be changed. But in case it's changed,
| both WiFi driver and net80211 should change accordingly.
L3969-3985 - Where do these numbers come from, specifying these
in the comments will be helpful.
| ACCEPT
| Added comment as below:
| * 802.11b 2GHz: 14 channels, each 5 MHz wide. Channel 1 is placed
| * at 2.412 GHz, channel 2 at 2.417 GHz, and so on up to channel 13
| * at 2.472 GHz. Channel 14 was defined especially for operation in
| * Japan, and has a center frequency 2.484 GHz.
| * 802.11g 2GHz: adopts the frequency plan of 802.11b. Japan only
| * allows 802.11g operation in channels 1-13
| * 802.11a 5GHz: starting every 5 MHz
| * 802.11b/g channels 15-24 (2512-2692) are used by some implementation
| * (Atheros etc.)
L4015 - where do numbers like 100 & 25 come from? Do we
check that these are not exceeded?
| EXPLAIN
| 100 is a roundup of maximum output message buffer size. 25 is a
| roundup of maximum temporary message buffer size. Since snprintf()
| and strncat() are used instead of sprintf() & strcat(), it won't
| exceed.
L4024 - use snprintf insead of sprintg, strncat instead of
strcat
| ACCEPT
| Changed to use snprintf & strncat
L4134 - Do we need to assert that val is non-null?
| ACCEPT
______________________________________________________________________________
usr/src/uts/common/io/net80211/net80211_impl.h
L76-77 - We should probably explicitly include
L236 net80211_proto.h
| ACCEPT
| Include <sys/net80211_proto.h> explicitly
L145-152 - I suppose "!= 0" isn't needed.
| ACCEPT
L156-158 - Could possibly use some spaces in the defs.
| ACCEPT
| Added spaces before and after each binary operator.
L164 - Why not just define it as IEEE80211_FC1_PWR_MGT?
| DISCUSS
| IEEE80211_FC1_PWR_MGT is checked by i_fc[1] ('struct ieee80211_frame'),
| while this one (IEEE80211_NODE_PWR_MGT) is used by in_flags
| (struct ieee80211_node). The comments here doesn't make sense. So
| removed this comment.
L254-292 - Check that all the args in the macros have '()'
as appropriate.
| ACCEPT
| Fixed to add '()'
L286 - Do we have to check for null '_in'?
| ACCEPT
| Since _in must not bu NULL, added ASSERT() as below:
| + ASSERT((_in) != NULL); \
L295,299 - Don't we have any equivalent currently? If not, can
we have this in some common location for others to
use?
| ACCEPT
| LE_READ_4() is not used and thus removed. Replaced LE_READ_2() with
| LE_16() and thus also removed LE_READ_2.
L309 - Use ! instead of == 0.
| ACCEPT
L342 - Is ieee80211_log() defined/used? If not, remove.
| ACCEPT
| Removed ieee80211_log().
______________________________________________________________________________
usr/src/uts/common/sys/net80211.h
L61 - Any reason 0x00000010 isn't used?
| ACCEPT & EXPLAIN
| The reason is that IEEE80211_C_XXX is used to describe hardware
| capability. While corresponding to some IEEE80211_C_XXX, there
| is a IEEE80211_F_XXX which is used to describe whether the
| capability is enabled and is defined the same as IEEE80211_C_XXX.
| For examle,
| #define IEEE80211_C_WPA1 0x00800000
| #define IEEE80211_F_WPA1 0x00800000
| Then since IEEE80211_F_PRIVACY is defined to be 0x10,
| IEEE80211_C_CKIP is defined as 0x20 to skip 0x10.
|
| But it's also OK to define IEEE80211_F_PRIVACY and one of
| IEEE80211_C_XXX the same, because IEEE80211_F_XXX is used by ic_flags,
| and IEEE80211_C_XXX is used by ic_caps.
|
| Then re-defined as below:
| #define IEEE80211_C_CKIP 0x00000010 /* CAPABILITY:
CKIP available */
L154-157 - I suppose != 0 isn't required?
| DISCUSS
| This is from previous comments on coding style that if 'x' is a
| numeric variable, then use 'if (x != 0) { ... }'. Here ich_flags
| is of type 'uint16_t'. Then I'd like to keep these unchanged.
L268-269 - Whatis '17'?
| ACCEPT
| 802.11e defines the TID to be 4 bytes (0~15). But here index 0 is
| reserved for the case that QoS is not enabled.
| Added comments as below:
| /*
| * Tx/Rx sequence number.
| * index 0 is used when QoS is not enabled. index 1-16 is used
| * when QoS is enabled. 1-16 corresponds to TID 0-15.
| */
L377 - To be consistent with others, remove 'type'.
| ACCEPT
| Removed 'type'
L432 - Suppose ieee80211_node_dectestref is a typo?
| ACCEPT
| It should be ieee80211_node_decref_nv, Changed as below
| * ieee80211_node_decref_nv remove a reference and return new value
L458 - Whatis this for?
| EXPLAIN
| The pointer is set with NULL to prevent later usage.
L477-481,489 - If used should these be static in net80211.c?
L493,497
| EXPLAIN
| In FreeBSD, these functions are also called by WiFi drivers. So I'd
| like to keep them global.
L483 - To be consistent with others remove 'ic' and 'reset'.
| ACCEPT
| Removed
L488 - Should this be static in net80211.c?
L492
| DISCUSS
| In FreeBSD, these functions are called by WiFi drivers. So I'd like
| to keep them global.
______________________________________________________________________________
usr/src/uts/common/sys/net80211_proto.h
L247-254 - prefix these, if possible.
| ACCEPT
| Replaced general prefix 'param_' with 'wme_'
L332-334 - Are these used?
L400-404
| ACCEPT
| Not used and thus removed
L439 - Why the gap (i.e. 10 not used)
| ACCEPT
| Originally 10 is a reserved reason code in 802.11 protocol. Later
| it's used by 802.11h for invalid power capability element. So added
| codes as below:
| IEEE80211_REASON_INVALID_POWER = 10,
L449 - and the reason for this gap (i.e. 2-9)?
| EXPLAIN
| These're reserved status code.
| To make it clear, added comments as below:
|/*
| * Status codes
| *
| * Unlisted codes are reserved and unused
| */
L482 - just curious is this the absolute max. even for jumbo
frames?
| EXPLAIN
| Jumbo frame is an extension to ethernet. 802.11 MAC has no such
definition.
L487 - what is 2300?
| ACCEPT
| 2300 shouldn't be used here. Re-defined as below:
|#define IEEE80211_MAX_LEN \
| (sizeof (struct ieee80211_frame_addr4) + \
| (IEEE80211_WEP_IVLEN + IEEE80211_WEP_KIDLEN + IEEE80211_WEP_CRCLEN) + \
| IEEE80211_MTU_MAX + IEEE80211_CRC_LEN)
L535-548 - There seems to be some duplication from llc1.h. Why?
L555-569 - Are these used? If so, net80211_proto.h doesn't seem
right for these..
| ACCEPT
| All of these LLC_XXX are either duplicated or not used. So removed
| all LLC_XXX (L532-569)
_______________________________________________
networking-discuss mailing list
[email protected]
