Subject: Re: WiFi for GLDv3 code review, part 2 (due 10/31) - msg#00143

Previous Message by Date: click to view message preview

Re: WiFi for GLDv3 code review, part 2 (due 10/31)

===================================================== CODE REVIEWER: kacheong.poon-xsfywfwIY+M@xxxxxxxxxxxxxxxx WEBREV: http://cr.grommit.com/~meem/wifi-1018 FILES: net80211 crypto + ioctl NOTES: Description of feedbacks: 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) ===================================================== Kacheong, thanks for the review and sorry for the late reply. Below is our response to your comments. Please let us know if you have any additional questions. ______________________________________________________________________________ net80211_impl.h: 46: Is net80211.h a new file? Is it reviewed? | EXPLAIN | Yes. The code review request has been updated with this header file | and other two files: | sys/net80211_proto.h | sys/net80211_crypto.h | The updated code review request is at: | http://www.opensolaris.org/jive/thread.jspa?messageID=67839&#67839 | The updated webrev is at: | http://cr.grommit.com/~meem/wifi-1018 95-100: It is unclear from reading the comment why those values need to be divided by IEEE80211_INACT_WAIT. Please explain this in the comment also. | ACCEPT | Added comments as below to explain why '/IEEE80211_INACT_WAIT': | | * IEEE80211_INACT_WAIT defines node table's inactivity interval in | * seconds. On timeout, node table's registered nt_timeout callback | * function is executed. Each node in the node table has a timeout | * set in its in_inact field with IEEE80211_INACT_<state>. In | * nt_timeout function, node table is iterated and each node's | * in_inact is decremented. So IEEE80211_INACT_<state> is defined in | * the form [inact_sec]/IEEE80211_INACT_WAIT. 97-98: Nit. It is not intuitive to have "IEEE80211_INACT_AUTH" represent "associated but not authorized." Maybe IEEE80211_INACT_ASSOC? Or IEEE80211_INACT_WAIT_AUTH? | ACCEPT | Renamed this macro to IEEE80211_INACT_ASSOC. 206: Please move this macro up, after IEEE80211_MSG_CONFIG. | ACCEPT ______________________________________________________________________________ net80211_ioctl.c: 50: Should we add a DDI strnlen() instead? | DEFER | To track this issue, | CR6490662 add strnlen() function to DDI | has been filed and it will be fixed after this putback. 53: Please add an empty line. | ACCEPT 75: Do we need to NULL terminate ow_essid->wl_essid_essid? And do we need to make sure that the wldp_buf is big enough before copying? If not, please add a comment explaining that. | ACCEPT & EXPLAIN | No need to NULL terminate ow_essid->wl_essid_essid. Because the | maximum length of essid is 32 chars, while wl_essid_essid is | defined as: | char wl_essid_essid[34]; /* wifi_ioctl.h */ | And the output buffer is zeroed when it's been allcated. Then | actually ow_essid->wl_essid_essid is NULL terminated. | | As to the size of wldp_buf, after the changes made to accept | comments on line 963, each wifi_cfg_xx() will allocate its | own output buffer, so apparently the wldp_buf is big enough now. 91: Do we really need to zero out the whole buffer? Can we just NULL terminate the string after copy? | ACCEPT | Modified as below | - bzero(ic->ic_des_essid, IEEE80211_NWID_LEN); | if (ic->ic_des_esslen != 0) | bcopy(essid, ic->ic_des_essid, ic->ic_des_esslen); | + if (ic->ic_des_esslen < IEEE80211_NWID_LEN) | + ic->ic_des_essid[ic->ic_des_esslen] = 0; 94: So if the wl_essid_length is 0 and the ic_des_essid is zero'ed out, we won't see the debug message? I think the debug message is useful when the ic_des_essid is actually changed. | ACCEPT | Moved ieee80211_dbg() out of the 'if' clause. 98, 130: It is quite strange that the ioctl succeeds but err is set. I know that in ieee80211_ioctl(), this particular error value is checked. But is there a better to communicate this? Or is it necessary to return this value in this function? If the desired ESSID is changed, should the driver do something about it? It seems that the ENETRESET "error" is just ignored. I that these questions also apply to the other wifi_cfg_XX(). | EXPLAIN | ENETRESET, as the return value of ieee80211_ioctl(), is checked | and used by wifi drivers. For example, if the desired ESSID is | changed, ENETRESET is returned by ieee80211_ioctl(), wifi driver | then is supposed to re-start connecting to an AP with the specified | ESSID. | ENETRESET is ignored when ieee80211_ioctl() sending back ioctl ACK | message to config tools (dladm or wificonfig). 124: Do we need to make sure that outp->wldp_buf is big enough? If not, please add a comment explaining that. | EXPLAIN | After the changes made by accepting comments on L963, each | wifi_cfg_xxx() allocates its own output message buffer. So | apparently wldp_buf is big enough now. 143-189: This function is almost identical to wifi_cfg_essid(). So the above questions also apply to this function. | ACCEPT | 1. Moved ieee80211_dbg() out of the 'if' clause. | 2. Removed bzero() as below: | - bzero(ic->ic_nickname, IEEE80211_NWID_LEN); | if (len > 0) | bcopy(iw_name->wl_nodename_name, ic->ic_nickname, len); | + if (len < IEEE80211_NWID_LEN) | + ic-ic_nickname[len] = 0; 206, 212, 219, 226: Please add an empty line. | ACCEPT 301: Do we need to make sure that the passed in inp->wldp_buf has enough room for MAX_NWEPKEYS? If not, please add a comment explaining that. | ACCEPT | Added below codes: | if (inp ->wldp_length < sizeof (wl_wep_key_tab_t)) { | ieee80211_err("wifi_cfg_wepkey: " | "parameter too short, %d, expected %d\n", | inp->wldp_length, sizeof (wl_wep_key_tab_t)); | outp->wldp_result = WL_NOTSUPPORTED; | return (EINVAL); | } 576: Please add a comment explaining that each "rate" is a uint8_t. This is why the memory allocation is done that way. | ACCEPT | Added comments: | /* type of rate value is char */ 595-608: I suspect that the code can be simplified. Firstly, I think it does not need to allocate srates. Just use the destination buffer wl_rates_rates as the scratch. Go over ic_sup_rates and check if the rate is already in the scratch. If not, add it in sequence. If yet, skip. Then the loop 584-591 can be avoided. | ACCEPT | The changes looks like: | case WLAN_GET_PARAM: | /* all rates supported by the device */ | ow_rates->wl_rates_num = 0; | drates = (uint8_t *)ow_rates->wl_rates_rates; | for (i = 0; i < IEEE80211_MODE_MAX; i++) { | srs = &ic->ic_sup_rates[i]; | if (srs->ir_nrates == 0) | continue; | | for (j = 0; j < srs->ir_nrates; j++) { | srates = IEEE80211_RV(srs->ir_rates[j]); | /* sort and skip duplicated rates */ | for (k = 0; k < ow_rates->wl_rates_num; k++) { | if (srates <= drates[k]) | break; | } | if (srates == drates[k]) | continue; /* duplicate, skip */ | /* sort */ | for (l = ow_rates->wl_rates_num; l > k; l--) | drates[l] = drates[l-1]; | drates[k] = srates; | ow_rates->wl_rates_num++; | } | } BTW, does the code need to make sure that wldp_buf can hold all the rates? If not, please explain. | EXPLAIN | After the changes made by accepting comments on L963, each | wifi_cfg_xxx() allocates its own output message buffer. So | apparently wldp_buf is big enough now. 672: So if drate is not supported in the negotiated rate set (in_rates) and ic->ic_state == IEEE80211_S_RUN, the code will continue in 676. And if it happens that the drate is in the supported rate set by the driver, the ioctl will return with no error. Is this the correct behavior? The driver cannot really use the rate being set. | EXPLAIN | Yes, it's the desired behavior. The desired rate is only for data | transfer. So usually the desired rate is checked and used when the | driver changes to RUN state. | If the driver is SCAN, the desired rate is set but driver don't have | to do anything. | If the driver is AUTH or ASSOC, the drate is checked against rates | supported by current ESS, nothing will be done if the drate is | supported, and re-connecting is required to check if another ESS | with specified drate can be found when the drate is not supported | by the current ESS. 681: Can found be a boolean_t? | ACCEPT | Changed type of found to be boolean_t and modified code accordingly. 721: I guess a comment is needed to explain what the code is trying to do here. Why rescale the RSSI value to [0, MAX_RSSI]? | ACCEPT & EXPLAIN | Each wifi device has its own range of RSSI value. The wifi driver | IOCTLs defines the range of printed RSSI value between 0 and 15 | (MAX_RSSI). So the device's RSSI value is rescaled. | Added comments to function wifi_getrssi() as below: |/* | * Rescale device's RSSI value to (0, 15) as required by WiFi | * driver IOCTLs (PSARC/2003/722) | */ 749: Will WAIT_SCAN_MAX be better to be a global so that it can be changed in case of a problem in production? | DISCUSS | Please refer to explaination and modifications made below (L760). 760: Should 200000 be configurable also? And why can't cv_timedwait_sig() be used for this? Please add some comment explaining the reason. | ACCEPT | Changed to use cv_timedwait_sig(), thus 200000 is removed. And | WAIT_SCAN_MAX is re-defined to be maximum total scan wait time | as below. | /* | * maximum scan wait time in second. | * Time spent on scaning one channel is usually 100~200ms. The maximum | * number of channels defined in wifi_ioctl.h is 99 (MAX_CHANNEL_NUM). | * As a result the maximum total scan time is defined to be | * (200ms * 100 = 20s) | */ | #define WAIT_SCAN_MAX 20 | Actually the maximum channel number 99 is bigger than number of | standard 802.11 channels. And since this value is big enough, I'd | prefer to keep it staticly defined. 760: And is it better for wifi_wait_scan() to take an argument which specifies the max time to wait? | DISCUSS | wifi_wait_scan() is a static function called internally by net80211 | ioctl code. It's not supposed to be called by other module or wifi | drivers, and the wait time has been made long enough (as is explained | previously). So I'd prefer to keep it as it is now. 768: Please add a comment explaining that this function is the call back for ieee80211_iterate_nodes() used in wifi_cfg_esslist() to get all the nodes. The comment should also explain the argument passed in. | ACCEPT | Added comments as below: |/* | * Callback function used by ieee80211_iterate_nodes() in | * wifi_cfg_esslist() to get info of each node in a node table | * arg output buffer, pointer to wl_ess_list_t | * in each node in the node table | */ 777: Please add a comment saying that it is assumed the passed in buffer is MAX_BUF_LEN large. | EXPLAIN | After the changes made by accepting comments on L963, each | wifi_cfg_xxx() allocates its own output message buffer. So | apparently wldp_buf is big enough now. 810, 818, 831: Please add an empty line. | ACCEPT 887: Please add an empty line. | ACCEPT 887: The code does nothing to change the state if the current state is not IEEE80211_S_INIT. Is it correct? So if the current state is IEEE80211_S_RUN, should the code just return? Or should the code continue to initiate a scan? Please add some comment explaining what the code tried to do. | ACCEPT & EXPLAIN | If current state is RUN, the code just returns for a known bug. | 6212098 Ath driver can not scan out the new network name when | it is already connect with a wlan | Added comments and codes as below: | /* Return when current state is RUN for CR6212098 */ | if (ostate == IEEE80211_S_RUN) | return (0); | | And fixed to start SCAN in other states by removing first | 'if (ostate == IEEE80211_S_INIT) {' 895: Please explain why the state needs to be changed back to IEEE80211_S_INIT. | EXPLAIN | The state is changed back to INIT only when original state is | also INIT. The reason is that at the end of SCAN stage, if an | AP is selected, the station will consequently connect to that | AP. Then it looks unreasonable that for a disconnected device, | a SCAN command causes it connected. So the state is changed back | to INIT. 963, 1030-1032: Why can't an mblk be allocated instead and pass in each wifi_cfg_XX()? Then there is no need to kmem_zalloc() and them kmem_free(). If this means that a MAX_BUF_LEN mblk needs to be allocated, why can't each wifi_cfg_XX() allocate its own mblk and pass back to the wifi_cfg_getset()? I think mi_mpprintf_putc() should not be used here. It is intended to add one more character to an mblk, but not to be used as a copy function. | ACCEPT | Removed kmem_zalloc(), kmem_free() and mi_mpprintf_putc(), instead | each wifi_cfg_xxx() allocates its own output mblk and pass back to | wifi_cfg_getset(). 1082: Can secpolicy_net_config be NULL? | ACCEPT | Removed 'if (secpolicy_net_config != NULL)' and 'else ...' ______________________________________________________________________________ net80211_crypto.h: 56-58: I don't quite understand this note. Why will these IEEE80211_CIPHER_XXX conflict with IEEE80211_F_PRIVACY? Should they be in different name spaces? Isn't IEEE80211_F_XXX supposed to be used in ic_flags? | ACCEPT & EXPLAIN | The reason for this comment is that corresponding to each | IEEE80211_CIPHER_XXX, there is a IEEE80211_C_XXX where | IEEE80211_C_XXX = 1 << IEEE80211_CIPHER_XXX | IEEE80211_C_XXX is used to describe hardware capability. | Consequently, 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 example, | #define IEEE80211_CIPHER_WEP 0 | #define IEEE80211_C_WEP 0x00000001 /* 1 << 0 */ | and | #define IEEE80211_C_WPA1 0x00800000 | #define IEEE80211_F_WPA1 0x00800000 | Then since IEEE80211_F_PRIVACY is defined to be 0x10, | IEEE80211_PRIVACY_XXX is defined to skip 4. | | 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. The comment here seems | confusing. | | Then changed the comments to: | /* | * NB: these values are ordered carefully; there are lots of | * of implications in any reordering. | */ | | And re-define as below: |------- usr/src/uts/common/sys/net80211_crypto.h ------- | #define IEEE80211_CIPHER_CKIP 4 | #define IEEE80211_CIPHER_NONE 5 /* pseudo value */ | |------- usr/src/uts/common/sys/net80211.h ------- | #define IEEE80211_C_CKIP 0x00000010 /* CAPABILITY: CKIP available */ 67: The max number of ciphers available is hard coded. Why? So when there is a new cipher module, does it mean that we need to re-compile the 802.11 crypto code to accommodate that? This does not sound extensible. | DISCUSS | Originally in BSD, each cipher is made a seperate module. But | the codes are not so flexible, as you've pointed out - max number | is hard coded thus when a new cipher is added, re-compiling of | net80211 is always required. During porting, the cipher is made | part of net80211 instead of a seperate module since currently | 802.11/802.11i standardized ciphers are limited (only WEP/TKIP/CCMP). 69: I suppose the size is in bytes. Maybe a comment? Will we ever have a key larger than 16 bytes? Is it limited by the 802.11 standard? A comment? | ACCEPT | Added comments as below: |/* | * Maxmium length of key in bytes | * WEP key length present in the 802.11 standard is 40-bit. | * Many implementations also support 104-bit WEP keys. | * 802.11i standardize TKIP/CCMP use 128-bit key | */ 82: Please move this #define below the typedef of ieee80211_keyix for clarity. | ACCEPT | Moved #define below typedef. 105: Please add a comment to explain the reason to have a null cipher always available. | ACCEPT | The comment isn't correct and is changed to: |/* | * Template for a supported cipher. Ciphers register with the | * crypto code. | */ 106: Is refcnt needed or not? We should remove all XXX in production code. | ACCEPT | Removed the XXX comments 114-123: Is there a document explaining what these functions are? If not, please add some comments. | ACCEPT | Added comments as below: | | * ic_attach - Initialize cipher. The return value is set to wk_private | * ic_detach - Destruct a cipher. | * ic_setkey - Validate key contents | * ic_encap - Encrypt the 802.11 MAC payload | * ic_decap - Decrypt the 802.11 MAC payload | * ic_enmic - Add MIC | * ic_demic - Check and remove MIC | */ 138, 139: Is it a problem to have separate buffers for TX mic and RX mic? Then we don't have to do the #define. If it is a problem, please comment. | DISCUSS | The structure definition is ported from BSD. I'd prefer to | keep it unchanged to ease later wifi drivers' porting. 144: XXX... | ACCEPT | Removed XXX comments 147: Why is WEP crypto #define IEEE80211_WEP_NKID used in a general structure for all 802.11 ciphers? | ACCEPT & EXPLAIN | 'struct ieee80211_crypto_state' is common for WEP|TKIP|CCMP. While | TKIP|CCMP use one key, WEP uses 4 keys. So 4 (IEEE80211_WEP_NKID) | is used as the maximum number of keys. The macro here is confusing. | So added a new macro and changed the definition as below: | /* Maximum number of keys */ | #define IEEE80211_KEY_MAX IEEE80211_WEP_NKID | | struct ieee80211_crypto_state{ | struct ieee80211_key cs_nw_keys[IEEE80211_KEY_MAX]; | ... | } 164: XXX... | ACCEPT | Removed XXX comments ______________________________________________________________________________ net80211_crypto.c: It seems that a lot of the functions return (0) and (1). Can boolean_t be used? | DISCUSS | These are registered callback functions to 'struct ieee80211_crypto_state'. | I'd prefer to keep the interface definition the same as BSD's to | ease later WiFi drivers' porting. 170: Can a key be used by two different ciphers? I ask because of this check (key->wk_cipher != cip). | EXPLAIN | Yes, cs_nw_keys[i] can be used with different ciphers. When the | device changes to use another cipher. The selected key's | key->wk_cipher is checked, old cipher is detached and new cipher | is attached if necessary. 226: Should the key be detached from the cipher on failure? | EXPLAIN | When re-initialize a key, the cipher is checked and replaced | with the new cipher if necessary. So no need to detach the cipher | on failure here. ______________________________________________________________________________ net80211_crypto_none.c: 43-67: Just a sanity check. Does compiling this file require so many header files? | ACCEPT | Modified to only include: | #include "net80211_impl.h" ______________________________________________________________________________ net80211_crypto_wep.c: 44-67: Just a sanity check. Does compiling this file require so many header files? | ACCEPT | Modified to only include: | #include <sys/byteorder.h> | #include <sys/crypto/common.h> | #include <sys/crypto/api.h> | #include "net80211_impl.h" 124: Can random_get_pseudo_bytes() be used here? If not, please add a comment. | ACCEPT | Changed as below: | (void) random_get_pseudo_bytes((unsigned char *)&ctx->wc_iv, | sizeof (uint32_t)); 140: Is 40/NBBY minimum allowed key length? How about max? Please add a comment. | ACCEPT | Added comments and changed as below: | /* | * WEP key length is standardized to 40-bit. Many | * implementations support 104-bit WEP kwys. | */ | return (k->wk_keylen == 40/NBBY || k->wk_keylen == 128/NBBY); 182: What is ATH_HOST_BIG_ENDIAN? If it should not be define, please remove it from the source. | ACCEPT | Removed. 259-294: Can those macros in <sys/crc32.h> be used? If not, why not? | ACCEPT | Re-defined crc_table as below: | static uint32_t crc_table[] = { CRC32_TABLE }; | Removed crc_init(). Replaced crc_update() with CRC32(). 390-486: Can the KCF be used for RC4? If not, why not? | EXPLAIN | We're using KCF.

Next Message by Date: click to view message preview

Re: WiFi for GLDv3 code review, part 2 (due 10/31)

===================================================== CODE REVIEWER: venu.iyer-xsfywfwIY+M@xxxxxxxxxxxxxxxx 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)

Previous Message by Thread: click to view message preview

Re: WiFi for GLDv3 code review, part 2 (due 10/31)

Kacheong, Thanks for your quick response. Please see my answer below. Kacheong Poon On 11/15/06 00:05 wrote: | EXPLAIN | ENETRESET, as the return value of ieee80211_ioctl(), is checked | and used by wifi drivers. For example, if the desired ESSID is | changed, ENETRESET is returned by ieee80211_ioctl(), wifi driver | then is supposed to re-start connecting to an AP with the specified | ESSID. | ENETRESET is ignored when ieee80211_ioctl() sending back ioctl ACK | message to config tools (dladm or wificonfig). I c. Could you please add a comment in ieee80211_ioctl() before calling miocack() explaining this? OK. Added comments for function ieee80211_ioctl() as below: * Return value should be checked by WiFi drivers. Return 0 * on success. Otherwise, return non-zero value to indicate * the error. Driver should operate as below when the return * error is: * ENETRESET Reset wireless network and re-start to join a * WLAN. ENETRESET is returned when a configuration * parameter has been changed. * When acknowledge a M_IOCTL message, thie error * is ignored. Also added comments before miocack() as below: /* ignore ENETRESET when acknowledge the M_IOCTL message */ Here since the error ERESTART is not used. So removed '|| err == ERESTART' from L1113. | EXPLAIN | Yes, it's the desired behavior. The desired rate is only for data | transfer. So usually the desired rate is checked and used when the | driver changes to RUN state. | If the driver is SCAN, the desired rate is set but driver don't have | to do anything. | If the driver is AUTH or ASSOC, the drate is checked against rates | supported by current ESS, nothing will be done if the drate is | supported, and re-connecting is required to check if another ESS | with specified drate can be found when the drate is not supported | by the current ESS. Could you please add a comment explaining the above? OK. Added comments before L672 as below: /* * Set desired rate. the desired rate is for data transfer * and usually is checked and used when driver changes to * RUN state. * If the driver is in AUTH | ASSOC | RUN state, desired * rate is checked against rates supported by current ESS. * If it's supported and current state is AUTH|ASSOC, nothing * needs to be doen by driver since the desired rate will * be enabled when the device changes to RUN state. And * when current state is RUN, Re-associate with the ESS to * enable the desired rate. */ 721: I guess a comment is needed to explain what the code is trying to do here. Why rescale the RSSI value to [0, MAX_RSSI]? | ACCEPT & EXPLAIN | Each wifi device has its own range of RSSI value. The wifi driver | IOCTLs defines the range of printed RSSI value between 0 and 15 | (MAX_RSSI). So the device's RSSI value is rescaled. | Added comments to function wifi_getrssi() as below: |/* | * Rescale device's RSSI value to (0, 15) as required by WiFi | * driver IOCTLs (PSARC/2003/722) | */ Could you also explain why the range is [0, 15], as defined in the PSARC case? Is this required by some third party code? I am just curious why 15, not 10, not 100 :-) Here is the explain from its designer: In 802.11 protocol, RSSI for the frequency-Hopping(FH) PHY is defined as 0-15. So the MAX_RSSI is defined to be 15. While for DSSS PHY, RSSI is a 8-bit value, then they need to be recaled to fit into this range. And they also admit that a percentage might make more sense. But at that time, they choose to define it this way. | ACCEPT | Changed to use cv_timedwait_sig(), thus 200000 is removed. And | WAIT_SCAN_MAX is re-defined to be maximum total scan wait time | as below. | /* | * maximum scan wait time in second. | * Time spent on scaning one channel is usually 100~200ms. The maximum | * number of channels defined in wifi_ioctl.h is 99 (MAX_CHANNEL_NUM). | * As a result the maximum total scan time is defined to be | * (200ms * 100 = 20s) | */ | #define WAIT_SCAN_MAX 20 | Actually the maximum channel number 99 is bigger than number of | standard 802.11 channels. And since this value is big enough, I'd | prefer to keep it staticly defined. OK. Then maybe it should be defined as something like #define WAIT_SCAN_MAX (200 * MAX_CHANNEL_NUM) OK. I'll change it. | ACCEPT & EXPLAIN | If current state is RUN, the code just returns for a known bug. | 6212098 Ath driver can not scan out the new network name when | it is already connect with a wlan | Added comments and codes as below: | /* Return when current state is RUN for CR6212098 */ I guess it is better to put in the problem in a comment instead of using the CR number. OK. Replaced this comment with below: /* * Do not scan when current state is RUN. The reason is * when connected, STA is on the same channel as AP. But * to do scan, STA have to switch to each available channel, * send probe request and wait certian time for probe * response/beacon. Then when the STA switches to a channel * different than AP's, as a result it cannot send/receive * data packets to/from the connected WLAN. This eventually * will cause data loss. */ | EXPLAIN | The state is changed back to INIT only when original state is | also INIT. The reason is that at the end of SCAN stage, if an | AP is selected, the station will consequently connect to that | AP. Then it looks unreasonable that for a disconnected device, | a SCAN command causes it connected. So the state is changed back | to INIT. Could you please put the above as a comment in the code? OK. Above explain is added as comment. Thanks, Judy

Next Message by Thread: click to view message preview

Re: WiFi for GLDv3 code review, part 2 (due 10/31)

On Tue, Nov 14, 2006 at 06:30:20PM +0800, Judy Chen wrote: > Renee, > Thanks for the review and sorry for the late reply. Below is our response to > your comments. Please let us know if you have any additional questions. Thanks for the responses; these all sound fine. A few replies on specific items are in-line. -renee > ------------------------------------------------------------------------------- > FEEDBACK ON TECHNICAL DOCUMENTATION/CODE > ------------------------------------------------------------------------------- > > Reviewer Name: Renee Danson > > Document/Module Title: wifi-1018-diff > > Document/Module Version/Date: 11/10/06 > > No comments: > usr/src/uts/common/io/ath/ath_impl.h > > ------------------------------------------------------------------------------- > usr/src/uts/common/io/ath/ath_aux.c > ------------------------------------------------------------------------------- > RD-17 289-290 Cos You should be able to merge these two lines. > > | DISCUSS > | Lines 289-290 are: > | > | uint16_t flags; > | ix = ath_hal_mhz2ieee(ah, c->channel, c->channelFlags); > | > | In what way should the be merged? Heh, good question. I'm sorry, I typed the wrong line numbers; I was referring to lines 275 and 276. > RD-18 298-299 Com I'm puzzled by the if conditional and the > comment; they don't seem to match. And I can't > find the implementation of ath_hal_mhz2ieee to > figure out what ix really is. This is probably > okay, I'm just confused; but I'd like to > understand it! > > | EXPLAIN > | Function ath_hal_mhz2ieee() is defined in ath HAL, which is provided > | by opensource MadWifi project in binary only form, thus no source > | code for it. It's used to convert a frequence value into IEEE > | channel number. For example, frequency 2412MHz is IEEE802.11b/g > | channel 1. So here 'ix' means channel number, which is used to > | index into array ic_sup_channels[]. > | For the comments > | /* can't handle stuff <2400 right now */ > | As explain previously, 2412MHz is IEEE802.11b/g channel 1. But > | some chipsets supports frequency less than 2412 MHz, like 2372MHz. > | In this case, channel number is returned as negative value. Here > | we ignored such frequencies. > | > | To make it clear, modified comments as below > | /* > | * can't handle frequency <2400MHz (negative > | * channels) right now > | */ Great, thanks for the explanation. The modified comments are helpful, too. > RD-22 279-280 Func Just to be sure: it's intentional that the > first two items in the enum are set to 0x0001? > > | EXPLAIN > | Yes. These two definitions are the same as the ones in opensource > | MadWifi project and FreeBSD. > | Added comments to explain this as below: > | * When 0x0001 is set, both TXQ_TXOKINT and TXQ_TXERRINT > * will be enabled. Thanks for the explanation. -renee