Seb & Venu, Thanks for the review of Atheros driver. Our feedback is attached below. Please let us know if you have any additional questions.
Thanks, - Judy
CODE REVIEW COMMENTS FEEDBACK ===================================================== CODE REVIEWER: [EMAIL PROTECTED] WEBREV: http://cr.grommit.com/~meem/wifi-0926 FILES: Atheros driver 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) ===================================================== I haven't reviewed this in detail, but have a quick comment on ath_detach() while I'm thinking of it: Why does ath_detach() not check to see if there is an established connection for the device, and fail the detach operation if there is one? I keep hearing, "you have to plumb an IP interface before connecting the WiFi link in order to keep the driver from unloading". This seems like an arbitrary administrative restriction, especially since the driver has enough information at hand to do the right thing and not unload while there are established connections. It's possible there is a technical reason why the driver needs to allow a detach in this situation, and if so, please educate me. :-) | - DEFER | The problem here is to not stop or detach the driver while | there is an established connection, so that the station can keep | to connect to an AP even when the wifi interface isn't plumbed. | | This administrative requirement is not specific to Atheros driver | but should be common to all wifi dirvers. Then this way wifi drivers, | like ipw, iwi can never be detached. Because these two Centrino | drivers will start to connect in xxx_m_start() routine and cannot | be disconnected until xxx_m_stop() stops the hardware. | | This functionality should be implemented by dladm or wificonfig | rather than by each wifi driver. That means, "dladm connect-wifi" | should keep device open until "dladm disconnect-wifi" runs. | | Actually this topic has been discussed a lot during wificonfig's | putback. I don't know the details. But the final consensus is | that wifi drivers must be implemented as to scan whenever the | interface is plumbed or not. But wificonfig & wifi drivers are | kept to do connect after the interface is plumbed. On a slightly related note, the ath_detach() function seems to do quite a bit of disabling and freeing things prior to calling mac_unregister(). If mac_unregister() fails, this will leave the device in a strange state, as things like interrupts have been disabled some things have been freed. It might make more sense to first call mac_unregister(), and then if that succeeds, to destroy the rest of the device state. | - ACCEPT
CODE REVIEW COMMENTS FEEDBACK ===================================================== CODE REVIEWER: [EMAIL PROTECTED] WEBREV: http://cr.grommit.com/~meem/wifi-0926 FILES: Atheros driver 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) ===================================================== ______________________________________________________________________________ usr/src/uts/common/io/ath/ath_aux.c L93- Is net80211.h a new file that is being delivered? Can't find it in onnv, not in this webrev, but see it in usr/src/uts/common/sys in the workspace. Also, is there a reason these member names in ieee80211com differ from those in ath_ieee80211.h in onnv? Since it still seems to be asc_isc in "struct ath". | - EXPLAIN | net80211.h is delivered with net80211 module that's not included in | this code review but will be sent out for review soon after. | Net80211 module is ported from BSD, the member names of "struct | ieee80211com" are kept the same with BSD to make other BSD wifi | drivers' porting easier. | "asc_isc" is unchanged to avoid unnecessary additional modifications. L183 Add a line after this. | - ACCEPT L232 Why is ieee80211channel changed to ieee80211_channel? | - EXPLAIN | "struct ieee80211_channel" is defined in net80211.h. The same reason | as 'L93', its defined the same as BSD's. L186 Is txq->axq_qnum equal to the value of i'? | - EXPLAIN | Yes, they are equal. L447 Looks like the return value from ath_chan_set gets set in ic_newstate? If so, what does '1' mean? | - ACCEPT | Changed back to "EIO" L-502-507 Given the change here, is the comment still the same? | - ACCEPT | Modified comments as below: | /* | * Setup the number of consecutive beacons to miss | * before taking a BMISS interrupt. | * Note that we clamp the result to at most 10 beacons. | */ L555 Typo uniacst -> unicast | - ACCEPT L581 If it should not happen, should we assert? | - DISCUSS | The "if {}" clause is executed when WPA/WPA2 is enabled. Since | current ath doesn't support WPA/WPA2. the "if {}" clause is deleted. L563,584,591,595 Given that it returns only 1 or 0 and it is actually used a a boolean, i.e: if (!DEV_KEY_ALLOC(ic, key, &keyix, &rxkeyix)) why doesn't cs_key_alloc return a boolean? | - EXPLAIN | The data structure defined in net80211 is ported from BSD and I'd | like to keep the interface unchanged. So that later wifi drivers' | porting will be easier. L565 Is the continuation spaced correcly? Looks like a tab. 614 | - ACCEPT | Changed to indent by 4 spaces. L599 This always returns 1, so could cs_key_delete be a void instead of a int? | - EXPLAIN | The same reason as L563, I'd like to keep the interface of net80211 | the same as BSD's. L601 I guess we don't really need asc to be defined, we could have obtained asc_ah directly from ic in L602. L650-651 | - ACCEPT L633 The 'and' at the end doesn't seem right. | - ACCEPT | 'and' is deleted. L641-642 Use "{}" for else. | - ACCEPT L648 Add a comment for this. | - ACCEPT | Added comments as below: | /* | * Enable/Disable short slot timing | */ L696 Since this always returns 0, could ic_reset return void? Or could a reset fail? | - EXPLAIN | 'ic_reset' is defined as this to keep the interface of net80211 the | same as BSD's. usr/src/uts/common/io/ath/ath_aux.h usr/src/uts/common/io/ath/ath_hal.h no comments. usr/src/uts/common/io/ath/ath_impl.h L306-310 Why are these macros? | - EXPLAIN | Atheros driver is ported from OpenSource MadWiFi project. These macros | are defined in latest MadWiFi. They make code more readable then I | ported these macros. usr/src/uts/common/io/ath/ath_main.c General: So, is this being nemo-ised? IF so does 54-63 still hold? | - ACCEPT | Since the introduction of GLDv3 WiFi support, the architecture of | Atheros driver has changed. The whole block of comments is modified | as below: |/* | * Driver for the Atheros Wireless LAN controller. | * | * The Atheros driver calls into net80211 module for IEEE80211 protocol | * management functionalities. The driver includes a LLD(Low Level Driver) | * part to implement H/W related operations. | * The following is the high level structure of ath driver. | * (The arrows between modules indicate function call direction.) | * | * | * | | * | GLD thread | * V | * ================== ========================================= | * | | |[1] | | * | | | GLDv3 Callback functions registered | | * | Net80211 | ========================= by | | * | module | | | driver | | * | | V | | | * | |======================== | | | * | Functions exported by net80211 | | | | * | | | | | * ========================================== ================= | * | | | * V | | * +----------------------------------+ | | * |[2] | | | * | Net80211 Callback functions | | | * | registered by LLD | | | * +----------------------------------+ | | * | | | * V v | * +-----------------------------------------------------------+ | * |[3] | | * | LLD Internal functions | | * | | | * +-----------------------------------------------------------+ | * ^ | * | Software interrupt thread | * | | * | * The short description of each module is as below: | * Module 1: GLD callback functions, which are intercepting the calls from | * GLD to LLD. | * Module 2: Net80211 callback functions registered by LLD, which | * calls into LLD for H/W related functions needed by net80211. | * Module 3: LLD Internal functions, which are responsible for allocing | * descriptor/buffer, handling interrupt and other H/W | * operations. | * | * All functions are running in 3 types of thread: | * 1. GLD callbacks threads, such as ioctl, intr, etc. | * 2. Clock interruptt thread which is responsible for scan, rate control and | * calibration. | * 3. Software Interrupt thread originated in LLD. | * | * The lock strategy is as below: | * There have 4 queues for tx, each queue has one asc_txqlock[i] to | * prevent conflicts access to queue resource from different thread. | * | * All the transmit buffers are contained in asc_txbuf which are | * protected by asc_txbuflock. | * | * Each receive buffers are contained in asc_rxbuf which are protected | * by asc_rxbuflock. | * | * In ath struct, asc_genlock is a general lock, protecting most other | * operational data in ath_softc struct and HAL accesses. | * It is acquired by the interupt handler and most "mode-ctrl" routines. | * | * Any of the locks can be acquired singly, but where multiple | * locks are acquired, they *must* be in the order: | * asc_genlock >> asc_txqlock[i] >> asc_txbuflock >> asc_rxbuflock | */ L236 Where does 150 come from? | - EXPLAIN | ath_dwelltime defines the scan interval. The way ath do scan is by | sending out a PROBE_REQUEST frame and wait for PROBE_RESPONSE/BEACON | frame from APs. Since most APs define its beacon interval as 100ms | by default, to make sure the station can receive the PROBE_RESPONSE/ | BEACON frame before scan the next channel, the scan interval of station | should be longer than 100ms. | But longer scan interval means longer total time spent on scan. | So at last 150ms is chosen to make a balance. | A short comment is added here as below: | /* scan interval, ms */ L257 I suppose we can have a NULL here for getcapab? | - ACCEPT L577-594 (old code) Is this now taken care in net80211.c:ieee80211_input(), L3174-? | - EXPLAIN | Yes. L637 So is the header now filled via ... mac_wifi_header_cook()..-> mac_wifi_header()? Does the comment in L645-646 still hold? | - ACCEPT | No and the comment is modified as below: |/* | * The input parameter mp has following assumption: | * For data packets, GLDv3 mac_wifi plugin allocates and fills the | * ieee80211 header. For management packets, net80211 allocates and | * fills the ieee80211 header. In both cases, enough spaces in the | * header are left for encryption option. | */ L674 (old code) iswep could have been a boolean since it used as one. | - EXPLAIN | Since IEEE80211_FC1_WEP is defined as 0x40, so 'iswep' is defined as an int. | Modified L677 as: | if (iswep != 0) { L859-862 Does this comment still hold? | - ACCEPT | L859-864 are deleted. | Modified ATH_HAL_SETUPTXDESC(...) to use an->an_tx_antenna directly. L928 Is cstyle OK with the space before "++"? | - ACCEPT | Deleted the space. L948 Why is error int32_t instead of just int? | - ACCEPT | Changed to 'int'. L967-975 Does this mean data is not freed, but control is if there is no xmit buffer? | - EXPLAIN | Yes. Data frames come from upper GLD, it's not freed for further | processing by upper layer on error. | Management frames coming from net80211 module are just freed on error. L1024-1025 When condition spans a line, use {} | - ACCEPT L1025 If error != 0, the caller gets the mp? | - EXPLAIN | Yes, for data frames, it's handled by ath_main.c:ath_m_tx():L1055-1056. | Management frames are always freed. L1054 Since ath_xmit() doesn't return a boolean, use != 0. | - ACCEPT L1216 So, will the scan be set if the state is not IEEE80211_S_SCAN? Of is the state likely to change after starting a timer? | - EXPLAIN | The state can change after starting a scan timer. For example, after | all channels have been scaned, the state will change to AUTH. L1253 Is cstyle OK with the space after '!', L1417 L1458 L1577 | - ACCEPT | Deleted the space. L1444 use ntimer != 0 | - ACCEPT L1435 It is OK to invoke callback with the lock held? | - EXPLAIN | Yes. The callback doesn't hold the lock. L1577 There is a macro for this, right? | - ACCEPT | Changed to use macro L1574 This lock released by the caller? | - EXPLAIN | This function 'ath_stop_locked()' is called with caller holding | the lock. The lock is released before calling ieee80211_new_state() | and then the lock is reacquired after ieee80211_stop_watchdog(). L1580-1581 Use {} for else | - ACCEPT L1705 !add should have been enough | - ACCEPT L1747 Is this the else for L1738? | - ACCEPT | Changed as below: | - if (err == ERESTART) { | + else if (err == ERESTART) { L1791 WIFI_STAT_TX_FAILED could have been added @ L1776? | - EXPLAIN | These two have different meaning but the same value for ath driver. L2019 63 could be a macro. | - ACCEPT | Macro is defined for this constant as below: | #define ATH_MAX_RSSI 63 /* max rssi */ usr/src/uts/common/io/ath/ath_osdep.c L159 ath_halfix[i].p != NULL | - ACCEPT usr/src/uts/common/io/ath/ath_rate.c L191 I suppose this isn't needed, (ath_t *)arg in L193 should do. | - ACCEPT usr/src/uts/common/io/ath/ath_rate.h
_______________________________________________ networking-discuss mailing list [email protected]
