Meem:
Find attached comments for the core files. -venu On Fri, 27 Oct 2006, Peter Memishian wrote:
> Second, the other half of WiFi for GLDv3 (Nemo) is now ready for code > review. This part consists of the net80211 kernel module, and an updated > version of the ath driver so that it uses the latest HAL[1]. > > webrev: http://cr.grommit.com/~meem/wifi-1018 > gate: /net/azariah.prc/builds/meem/wifi-1018 > cscope: /net/azariah.prc/builds/meem/wifi-1018/usr/src > /net/azariah.prc/builds/meem/wifi-1018/usr/src/uts Folks, I accidentally left three files out of the original webrev: usr/src/uts/common/sys/net80211.h usr/src/uts/common/sys/net80211_proto.h usr/src/uts/common/sys/net80211_crypto.h The webrev has been updated to include these (the first two are part of "core"; the second is part of "crypto"). Apologies for the omission and please let us know if you need more time. -- meem _______________________________________________ networking-discuss mailing list [email protected]
From http://cr.grommit.com/~meem/wifi-1018 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. If it is possible, try break this file functionally into smaller ones. Else, it will keep growing and may become difficult to maintain. L38 - Collapse deltas (I suppose you plan to do this, so won't mention for others). L92-95 - Can probably prefix these fns with ieee80211_ L110,L1750 L92-126 - Fn declarations are not consistent, either name the arguments for all or leave them out. L128 - Prefix macro with IEEE80211_ and also a comment on why '2' will be helpful. L129 - Name seems very generic, check if you can use one of the existing ABS macro or re-name to something more specific. L147-149 - Consider using ieee80211_err() by passing a flag to distinguish between CE_CONT and CE_WARN rather than duplicating. 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. L159,L167 - Why are these functions, seems to be called only at one place. Also, at L159 why pass in newassoc when it is not being used? L175 - mbuf -> mblk? L180,183 - Any reason it is uint32_t instead of, say, uint_t. L207 - Why '8'? Define as macro and use or add comments. L222,225 - Suppose this could just have been return (kmem_zalloc(sizeof (ieee80211_node_t), KM_SLEEP)); L237 - Is in_rxfrag always guaranteed to be a block, i.e. b_cont is L250 always NULL? L292 - Use {} for else L563, L1194,L3068,L3070,L3718 L366-368 - Use copy_bss() L1053-1055 L544 - Could include this comment in the previous line too. Is this for ieee80211_fakeup_adhoc_node()? If, so move it down. L572 - Prefix this MACRO appropriately. L608 L590 - Use {} when condition spans multiple lines. L626, L629,L646, L649,L1249,L1443,L1893,L2213,L2342,L2460,L2636,L2700, L3416,L3420,L3814,L3916-3923 L601-602 - Does this need to be done within the lock? L609 - Is cstyle OK when there isn't space around the operator? L2274 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 L627,L629 - !((b->in_capinfo & IEEE80211_CAPINFO_PRIVACY) instead of checking it with 0. 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); L639 - Add a comment for '5', better still define it as macro. L669 - inact is %d. L703 - Is ieee80211_node_table_reset used? L728-731 - This comment is not very clear to me. L754 - use boolean_t instead. L823 - So, assuming we don't send any management frame, we will always start from the head whenever we timeout a node? L854 - ic_private will always be non-null? (and in some other places as well). L861 - Is ieee80211_node_unauthorize() used? L872 - Why this assert given that ieee80211_alloc_node() seems to check for null allocation? L972 - Add an empty line. L1087,L1217,L1687,L2060,L2632 L978-979 - Is there a case it will not exist or are we here just for this case (i.e switiching mastership). 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. L1037 - Define as boolean_t. L1165 - Could the function somehow result in removing the node? L1235 - Please add some comments for this. L1258-1268 - This just sorts i, right? Not the entire list. L1296 - Does it matter what the value of ignore is, i.e. could it just be a boolean_t? L1303 - use (ignore > 0) L1320 - Why isn't this in L1255? L1303-1307 - Don't know how often this happens. Is array the most effective option in such cases? L1472 - declare onoff as boolean_t L1546 - typo (channe) L1580-1583 - just wondering if this is enforced somehow.. or what results if this is not true. L1586,1621,1627 - from what I can see none of the callers seem interested in the return value. L1725 - Return boolean L1749 L1910 - Should this be an assert, then? L1982 - use (timer > 0) 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) L2100 - Why int32_t? and not just int? L2122 - I suppose we can just add optielen, assuming it will be 0 if optie == NULL (similarly for L2327) L2189-2191 - Use of Macros are better. L2225,L2289 L2161 - use boolean_t where appropriate L2119-2121, - Consider using a macro if this is a recurring thing. 2187-2188,2192 L2197 - We allocate it regardless of whether it is filled in or not? L2202-2203 - Is the timestamp filled someplace else? L2291,2293 - Remove space before ++ L2461 - in_associd is not a boolean L2479 - What is 12? L2494-2495 - Why continue? L2557 - '2' gets us to the next paramter? L2603 - !(ic->ic_flags & IEEE80211_F_SCAN) L2738 - allocbs = B_FALSE. L2742 - allocbs = B_TRUE L2764 - think cstyle requires the operator to be in the prev line. L2753 - don't we alloc a node here too? L2763-2764 - how do we come up with these flags, a comment might L2884-2885 help. L2792 - use macro instead of '6'. L2836 L2867 - continue? could be break, right? L3001-3004 - We don't need to check the mp itself here? The caller is responsible? L3059 - !(ic->ic_flags & IEEE80211_F_SCAN) (similarly in L3118,L3400,L3403,L3407,L3714) L3119,3122,3133 - tid is always 0? L3190 - ieee80211_crypto_demic - typo?? L3223 - Didn't see any stats in ieee80211_defrag() to reflect this.. L3268 - ic->ic_stats.is_wep_errors++?? L3310 - fragno & more_frag used as boolean_t L3352 - cstyle requires space around operator L3351 - what does this comment mean? L3359 - so, if the incoming sequence is not the next one (L3352) we discard the existing fragment (mfrag)? L3390 - Even though fail seems to be a bit mask, it doesn't seem to be used that way (L1696 & L 1735) L3492 - ieee80211_begin_scan() takes boolean_t as 2nd arg, so use B_FALSE or B_TRUE. L3649 - Is ieee80211_beacon_alloc() used? L3671-3672 - these are optional? L3699-3709 - Can this be pulled out, looks like there a couple of places this is duplicated? L3774 - Are these being used? L3800 L3777 - Remove L3849 - ic->ic_watchdog != NULL L3862 L3879,3906 - suppose this can be a boolean_t 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). L3947-3948 - If this assumption changes in the future, what is the impact? L3969-3985 - Where do these numbers come from, specifying these in the comments will be helpful. L4015 - where do numbers like 100 & 25 come from? Do we check that these are not exceeded? L4024 - use snprintf insead of sprintg, strncat instead of strcat L4134 - Do we need to assert that val is non-null? usr/src/uts/common/io/net80211/net80211_impl.h L76-77 - We should probably explicitly include L236 net80211_proto.h L145-152 - I suppose "!= 0" isn't needed. L156-158 - Could possibly use some spaces in the defs. L164 - Why not just define it as IEEE80211_FC1_PWR_MGT? L254-292 - Check that all the args in the macros have '()' as appropriate. L286 - Do we have to check for null '_in'? L295,299 - Don't we have any equivalent currently? If not, can we have this in some common location for others to use? L309 - Use ! instead of == 0. L342 - Is ieee80211_log() defined/used? If not, remove. usr/src/uts/common/sys/net80211.h L61 - Any reason 0x00000010 isn't used? L154-157 - I suppose != 0 isn't required? L268-269 - Whatis '17'? L377 - To be consistent with others, remove 'type'. L432 - Suppose ieee80211_node_dectestref is a typo? L458 - Whatis this for? L477-481,489 - If used should these be static in net80211.c? L493,497 L483 - To be consistent with others remove 'ic' and 'reset'. L488 - Should this be static in net80211.c? L492 usr/src/uts/common/sys/net80211_proto.h L247-254 - prefix these, if possible. L332-334 - Are these used? L400-404 L439 - Why the gap (i.e. 10 not used) L449 - and the reason for this gap (i.e. 2-9)? L482 - just curious is this the absolute max. even for jumbo frames? L487 - what is 2300? 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..
_______________________________________________ networking-discuss mailing list [email protected]
