Return-path: Received: from xc.sipsolutions.net ([83.246.72.84]:48711 "EHLO sipsolutions.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753577AbYGWToN (ORCPT ); Wed, 23 Jul 2008 15:44:13 -0400 Subject: Re: [PATCH 1/4] Add new Atheros IEEE 802.11n ath9k driver From: Johannes Berg To: "Luis R. Rodriguez" Cc: linville@tuxdriver.com, linux-wireless@vger.kernel.org, ath9k-devel@lists.ath9k.org, Senthil Balasubramanian , Jouni Malinen , Sujith Manoharan , Vasanthakumar Thiagarajan In-Reply-To: <20080720024500.GR17936@ruslug.rutgers.edu> (sfid-20080720_044515_228476_FA83BA3F) References: <20080720024341.GQ17936@ruslug.rutgers.edu> <20080720024500.GR17936@ruslug.rutgers.edu> (sfid-20080720_044515_228476_FA83BA3F) Content-Type: text/plain Date: Wed, 23 Jul 2008 21:43:58 +0200 Message-Id: <1216842238.13587.31.camel@johannes.berg> (sfid-20080723_214416_538145_42C49384) Mime-Version: 1.0 Sender: linux-wireless-owner@vger.kernel.org List-ID: Only STA support is currently enabled Thanks for that. The folks at Intel still haven't gotten the message ;) +#define HAL_TXERR_XRETRY 0x01 +#define HAL_TXERR_FILT 0x02 +#define HAL_TXERR_FIFO 0x04 +#define HAL_TXERR_XTXOP 0x08 +#define HAL_TXERR_TIMER_EXPIRED 0x10 + +#define HAL_TX_BA 0x01 +#define HAL_TX_PWRMGMT 0x02 +#define HAL_TX_DESC_CFG_ERR 0x04 +#define HAL_TX_DATA_UNDERRUN 0x08 +#define HAL_TX_DELIM_UNDERRUN 0x10 +#define HAL_TX_SW_ABORTED 0x40 +#define HAL_TX_SW_FILTERED 0x80 Do you really have to have HAL_ names? :) Kinda reminiscent of the old days... And it makes little sense in a driver that hopefully (I'm not that far yet) doesn't have a HAL. I think those should probably be called ATH9K_ instead. +#ifndef howmany +#define howmany(x, y) (((x)+((y)-1))/(y)) +#endif The #ifndef and the name of the macro make little sense. You should change the name and get rid of the ifndef. +struct ath_tx_status { + u_int32_t ts_tstamp; + u_int16_t ts_seqnum; + u_int8_t ts_status; That looks suspiciously like a hardware shared struct, should it be declared with explicit endianness? And I think it should be declared packed just in case some architectures don't have the alignment properties you think this struct needs. Also, commonly that would be u32, u16 etc. +struct ath_rx_status { + u_int32_t rs_tstamp; Ditto. +#define HAL_RXERR_CRC 0x01 Ditto about HAL_* +enum hal_bool { + AH_FALSE = 0, + AH_TRUE = 1, +}; Eww. What do you need that for? That's gross. +struct ath_desc { + u_int32_t ds_link; ... +} __packed; Aha. So that one is packed. +#define ds_txstat ds_us.tx +#define ds_rxstat ds_us.rx +#define ds_stat ds_us.stats I personally don't like that, but I know the kernel does it in a few places. Is there a specific reason? Usually it's mostly done when fields are moved into structs or so. +enum hal_capability_type { + HAL_CAP_CIPHER = 0, kernel-doc? And should they be defined explicitly so that nobody tries to add numbers or so, these look like they're shared with hardware. +struct hal_capabilities { + u_int halChanSpreadSupport:1, + halChapTuningSupport:1, bitfield suck. And what's u_int? +struct hal_tx_queue_info { + u_int32_t tqi_ver; If you didn't say hal_ everywhere it could possible also be much clearer which part is hardware dependent and which part is just for the driver. +enum hal_rx_filter { + HAL_RX_FILTER_UCAST = 0x00000001, BIT()? +enum hal_int { + HAL_INT_RX = 0x00000001, Dito. +#define CHANNEL_QUARTER 0x08000 +#define CHANNEL_HT20 0x10000 +#define CHANNEL_HT40PLUS 0x20000 +#define CHANNEL_HT40MINUS 0x40000 CHANNEL_ seems a bit too generic. +#define IS_CHAN_A(_c) ((((_c)->channelFlags & CHANNEL_A) == CHANNEL_A) || \ + (((_c)->channelFlags & CHANNEL_A_HT20) == CHANNEL_A_HT20) || \ + (((_c)->channelFlags & CHANNEL_A_HT40PLUS) == CHANNEL_A_HT40PLUS) || \ + (((_c)->channelFlags & CHANNEL_A_HT40MINUS) == CHANNEL_A_HT40MINUS)) Do you really have to build the structs this way? I mean, couldn't you use a separate A/B/G flag in your structs? Or is this shared with hw? +enum { + CTRY_DEBUG = 0x1ff, + CTRY_DEFAULT = 0 +}; ?? +#define HAL_DBG_MALLOC 0x00400000 +#define HAL_DBG_POWER_OVERRIDE 0x01000000 +#define HAL_DBG_SPUR_MITIGATE 0x02000000 +#define HAL_DBG_UNMASKABLE 0xFFFFFFFF whitespace mixup tabs and spaces +#define REG_WRITE(_ah, _reg, _val) iowrite32(_val, _ah->ah_sh + _reg) +#define REG_READ(_ah, _reg) ioread32(_ah->ah_sh + _reg) Do you really need macros for this? Static inlines that do type-checking are so much nicer. +#define SM(_v, _f) (((_v) << _f##_S) & _f) +#define MS(_v, _f) (((_v) & _f) >> _f##_S) +#define OS_REG_RMW(_a, _r, _set, _clr) \ + REG_WRITE(_a, _r, (REG_READ(_a, _r) & ~(_clr)) | (_set)) +#define OS_REG_RMW_FIELD(_a, _r, _f, _v) \ + REG_WRITE(_a, _r, \ + (REG_READ(_a, _r) & ~_f) | (((_v) << _f##_S) & _f)) +#define OS_REG_SET_BIT(_a, _r, _f) \ + REG_WRITE(_a, _r, REG_READ(_a, _r) | _f) +#define OS_REG_CLR_BIT(_a, _r, _f) \ + REG_WRITE(_a, _r, REG_READ(_a, _r) & ~_f) +#define OS_REG_ath9k_regd_is_bit_set(_a, _r, _f) \ + ((REG_READ(_a, _r) & _f) != 0) Uh what? OS_?? Did you forget to resolve the os-indep layer from your windows driver completely? :) +#define IEEE80211_WEP_IVLEN 3 +#define IEEE80211_WEP_KIDLEN 1 +#define IEEE80211_WEP_CRCLEN 4 +#define IEEE80211_MAX_MPDU_LEN (3840 + FCS_LEN + \ + (IEEE80211_WEP_IVLEN + \ + IEEE80211_WEP_KIDLEN + \ + IEEE80211_WEP_CRCLEN)) +#define IEEE80211_MAX_LEN (2300 + FCS_LEN + \ + (IEEE80211_WEP_IVLEN + \ + IEEE80211_WEP_KIDLEN + \ + IEEE80211_WEP_CRCLEN)) Those seems like they should be in linux/ieee80211.h already, and if not be added there. or is that hw dependent, in which case it should be renamed? +#define LE_READ_2(p) \ + ((u_int16_t) \ + ((((const u_int8_t *)(p))[0]) | \ + (((const u_int8_t *)(p))[1] << 8))) + +#define LE_READ_4(p) \ + ((u_int32_t) \ + ((((const u_int8_t *)(p))[0]) | \ + (((const u_int8_t *)(p))[1] << 8) | \ + (((const u_int8_t *)(p))[2] << 16) | \ + (((const u_int8_t *)(p))[3] << 24))) Harvey will flame you for that. Rightfully. +enum hal_status { + HAL_OK = 0, + HAL_ENXIO, Ok, so what do you use these for? What's wrong with negative error codes like 99% of the kernel uses, and using pre-defined numbers we already have? +enum { + HAL_SLOT_TIME_6 = 6, + HAL_SLOT_TIME_9 = 9, + HAL_SLOT_TIME_20 = 20, +}; That's great, new names for "6", "9" and "20". +enum hal_freq_band { + HAL_FREQ_BAND_5GHZ = 0, + HAL_FREQ_BAND_2GHZ = 1, +}; You can remove those. +enum { + HAL_TRUE_CHIP = 1 +}; And that one most likely as well. +enum phytype { + PHY_DS, + PHY_FH, + PHY_OFDM, + PHY_HT, + PHY_MAX What's a MAX PHY? And where do you use that constant? +struct ath_hal { + u_int32_t ah_magic; + u_int16_t ah_devid; + u_int16_t ah_subvendorid; + void *ah_sc; A void * pointer? This isn't a hal where you need to hide stuff behind void * pointers because the users aren't supposed to know what it is... +#ifndef ATH_NF_PER_CHAN ?? Is it on or not? Should it be configurable or not? +#define HDPRINTF(_ah, _m, _fmt, ...) do { \ + if (((_ah) == NULL && _m == HAL_DBG_UNMASKABLE) || \ + (((struct ath_hal *)(_ah))->ah_config.ath_hal_debug & _m)) \ + printk(KERN_DEBUG _fmt , ##__VA_ARGS__); \ + } while (0) Ugh. +enum hal_status ath_hal_getcapability(struct ath_hal *ah, + enum hal_capability_type type, + u_int32_t capability, + u_int32_t *result); HAL or hardware capability? +enum hal_int ath9k_hw_set_interrupts(struct ath_hal *ah, + enum hal_int ints); Please use more standard return values. +enum hal_bool ath9k_hw_reset(struct ath_hal *ah, enum hal_opmode opmode, And there's a perfectly fine "bool" type. It even comes with "true" and "false" values. + if (!ath9k_hw_settxqueueprops(ah, sc->sc_bhalq, &qi)) { + DPRINTF(sc, ATH_DEBUG_FATAL, + "%s: unable to update h/w beacon queue parameters\n", + __func__); + return 0; + } else { + ath9k_hw_resettxqueue(ah, sc->sc_bhalq); /* push to h/w */ + return 1; + } You could move the else stuff into the outer level. +/* + * Setup the beacon frame for transmit. + * + * Associates the beacon frame buffer with a transmit descriptor. Will set + * up all required antenna switch parameters, rate codes, and channel flags. + * Beacons are always sent out at the lowest rate, and are not retried. +*/ Why not use kerneldoc notation? + ath9k_hw_set11n_txdesc(ah, ds + , skb->len + FCS_LEN /* frame length */ + , HAL_PKT_TYPE_BEACON /* Atheros packet type */ + , avp->av_btxctl.txpower /* txpower XXX */ + , HAL_TXKEYIX_INVALID /* no encryption */ + , HAL_KEY_TYPE_CLEAR /* no encryption */ + , flags /* no ack, veol for beacons */ + ); That's some pretty weird layout. + /* XXX: spin_lock_bh should not be used here, but sparse bitches + * otherwise. We should fix sparse :) */ + spin_lock_bh(&mcastq->axq_lock); First and foremost you should write correct code regardless of whether sparse complains about it or not. Then let others review it and possibly submit a bug report to sparse. This is the wrong way around. + /* XXX: doth needs the chanchange IE countdown decremented. + * We should consider adding a mac80211 call to indicate + * a beacon miss so appropriate action could be taken + * (in that layer). And what would mac80211 do? +#define TSF_TO_TU(_h,_l) \ + ((((u_int32_t)(_h)) << 22) | (((u_int32_t)(_l)) >> 10)) You keep defining that in each function. It should probably be in linux/ieee80211.h +static int ath_outdoor = AH_FALSE; /* enable outdoor use */ How is that useful if it cannot be set? + * This function initializes and fills the rate table in the ATH object based + * on the operating mode. The blink rates are also set up here, although + * they have been superceeded by the ath_led module. +*/ ath_led module? stuff that is superceeded? + /* XXX beware of overlow */ heh. + * This routine will provide the enumerated WIRELESSS_MODE value based + * on the settings of the channel flags. If ho valid set of flags ho ho ho + /* NB: should not get here */ + return WIRELESS_MODE_11b; WARN instead. + * reset 802.11 state machine + * (sends station deassoc/deauth frames) what? are you sure you're doing that? + * Scan End + * + * This routine is called by the upper layer when the scan is completed. This + * will set the filters back to normal operating mode, set the BSSID to the + * correct value, and restore the power save state. Uh huh? What upper layer are you talking about? +/* + * Set the current channel + * + * Set/change channels. If the channel is really being changed, it's done + * by reseting the chip. To accomplish this we must first cleanup any pending + * DMA, then restart stuff after a la ath_init. +*/ You should indent the */ by one space too, happens in tons of places. + if (!ath9k_hw_intrpend(ah)) { /* shared irq, not for us */ + return ATH_ISR_NOTMINE; + } Ok, why is this not using irqreturn_t to start with? + /* + * Whether we should enable h/w TKIP MIC. + * XXX: if we don't support WME TKIP MIC, then we wouldn't + * report WMM capable, so it's always safe to turn on + * TKIP MIC in this case. + */ ?? + /* + * Query the hal about antenna support There should be no hal. + sc->sc_hasveol = ah->ah_caps.halVEOLSupport; And stuff like that is particularly ugly. You can test the relevant feature bit wherever necessary, no need to copy feature bits around. +#ifdef CONFIG_SLOW_ANT_DIV I haven't seen a Kconfig definition for that +bad2: + /* cleanup tx queues */ + for (i = 0; i < HAL_NUM_TX_QUEUES; i++) + if (ATH_TXQ_SETUP(sc, i)) + ath_tx_cleanupq(sc, &sc->sc_txq[i]); +bad: + if (ah) + ath9k_hw_detach(ah); + return error; The labels could be named better. +struct ath_node *ath_node_get(struct ath_softc *sc, u8 *addr) + * Setup driver-specific state for a newly associated node. This routine + * really only applies if compression or XR are enabled, there is no code + * covering any other cases. What sort of state do you need? Anything mac80211 could cover? +/* Macro to expand scalars to 64-bit objects */ + +#define ito64(x) (sizeof(x) == 8) ? \ + (((unsigned long long int)(x)) & (0xff)) : \ + (sizeof(x) == 16) ? \ + (((unsigned long long int)(x)) & 0xffff) : \ + ((sizeof(x) == 32) ? \ + (((unsigned long long int)(x)) & 0xffffffff) : \ + (unsigned long long int)(x)) Eww. +#define KASSERT(exp, msg) do { \ + if (unlikely(!(exp))) { \ + printk msg; \ + BUG(); \ + } \ + } while (0) what's wrong with BUG_ON? +static inline unsigned long get_timestamp(void) +{ + return ((jiffies / HZ) * 1000) + (jiffies % HZ) * (1000 / HZ); ?? +#define DPRINTF(sc, _m, _fmt, ...) do { \ + if (sc->sc_debug & (_m)) \ + printk(_fmt , ##__VA_ARGS__); \ + } while (0) We already had that. Are you duplicating things in the driver because that's how the HAL was built? Ahh. HDPRINTF was HAL DPRINTF? +struct ath_buf_state { + int bfs_nframes; /* # frames in aggregate */ + u_int16_t bfs_al; /* length of aggregate */ + u_int16_t bfs_frmlen; /* length of frame */ + int bfs_seqno; /* sequence number */ + int bfs_tidno; /* tid of this frame */ + int bfs_retries; /* current retries */ + struct ath_rc_series bfs_rcs[4]; /* rate series */ + u8 bfs_isdata:1; /* is a data frame/aggregate */ bitfields. +/* return whether a bit at index _n in bitmap _bm is set + * _sz is the size of the bitmap */ +#define ATH_BA_ISSET(_bm, _n) (((_n) < (WME_BA_BMP_SIZE)) && \ + ((_bm)[(_n) >> 5] & (1 << ((_n) & 31)))) __test_bit and use normal bitmat stuff? Or is that 802.11 specific and should not be here? +/* driver-specific node state */ +struct ath_node { + struct list_head list; + struct ath_softc *an_sc; /* back pointer */ + atomic_t an_refcnt; + struct ath_chainmask_sel an_chainmask_sel; + struct ath_node_aggr an_aggr; /* A-MPDU aggregation state */ + u_int8_t an_smmode; /* SM Power save mode */ + u_int8_t an_flags; + u8 an_addr[ETH_ALEN]; What sort of node state do you really need? Why do you need PS state? Can we help you here with mac80211? Get some insight into a part of sta_info and have that passed to the driver to avoid having a list/hash table in the driver etc. +/********/ +/* VAPs */ +/********/ Please use "virtual interface" terminology instead of VAP since that matches what mac80211 says. Not just here, of course! +/* driver-specific vap state */ +struct ath_vap { + struct ieee80211_vif *av_if_data; /* interface(vap) + instance from 802.11 protocal layer */ + enum hal_opmode av_opmode; /* VAP operational mode */ + struct ath_buf *av_bcbuf; /* beacon buffer */ + struct ath_beacon_offset av_boff; /* dynamic update state */ + struct ath_tx_control av_btxctl; /* tx control information + for beacon */ + int av_bslot; /* beacon slot index */ + struct ath_txq av_mcastq; /* multicast + transmit queue */ + struct ath_vap_config av_config; /* vap configuration + parameters from 802.11 protocol layer*/ You should be embedding this struct in the private part of the vif structure instead of linking the vif structure off of this, and then use container_of or so to get at it. +struct ath_softc { + struct ieee80211_hw *hw; /* mac80211 instance */ + struct pci_dev *pdev; /* Bus handle */ + void __iomem *mem; /* address of the device */ + struct tasklet_struct intr_tq; /* General tasklet */ + struct tasklet_struct bcon_tasklet; /* Beacon tasklet */ I think this should be embedded in the hw struct private area. Or is hung off the driver data in the pci dev? +static inline enum hal_status ath9k_hw_init_macaddr(struct ath_hal *ah) inline? bit large for that +static inline enum hal_bool +ath9k_hw_get_lower_upper_index(u_int8_t target, ditto. +static inline void ath9k_hw_ani_setup(struct ath_hal *ah) etc. Why do you think setup functions need to be inlines? +++ b/drivers/net/wireless/ath9k/initvals.h initvals probably should not be in a header file but rather a C file +static int ath_check_chanflags(struct ieee80211_channel *chan, + u_int32_t mode, + struct ath_softc *sc) and what exactly does this actually do with flags? +static void ath_key_delete(struct ath_softc *sc, struct ieee80211_key_conf *key) +{ +#define ATH_MAX_NUM_KEYS 4 That's one of the most useless defines I've ever seen, keyidx is always 0..3 (or 5 for 11w). And nothing in that is hw specific. +/* Until mac80211 includes these fields */ You'll have to tell us why, how, and what for. + /* + * NB:mac80211 validates rx rate index against the supported legacy rate + * index only (should be done against ht rates also), return the highest + * legacy rate index for rx rate which does not match any one of the + * supported basic and extended rates to make mac80211 happy. + * The following hack will be cleaned up once the issue with + * the rx rate index validation in mac80211 is fixed. + */ patches welcome to make mac80211 aware of MCS rate. +static int ath9k_beacon_update(struct ieee80211_hw *hw, + struct sk_buff *skb) + +{ + struct ath_softc *sc = hw->priv; + + DPRINTF(sc, ATH_DEBUG_BEACON, "%s: Update Beacon\n", __func__); + return ath9k_tx(hw, skb); Didn't I just remove beacon_update? Confused. +static int ath9k_add_interface(struct ieee80211_hw *hw, + struct ieee80211_if_init_conf *conf) +{ + struct ath_softc *sc = hw->priv; + int error, ic_opmode = 0; + + /* Support only vap for now */ + + if (sc->sc_nvaps) + return -1; + + switch (conf->type) { + case IEEE80211_IF_TYPE_STA: + ic_opmode = HAL_M_STA; + default: + break; + } + + DPRINTF(sc, ATH_DEBUG_CONFIG, "%s: Attach a VAP of type: %d\n", + __func__, + ic_opmode); + + error = ath_vap_attach(sc, 0, conf->vif, ic_opmode, ic_opmode, 0); + if (error) { + DPRINTF(sc, ATH_DEBUG_FATAL, + "%s: Unable to attach vap, error: %d\n", + __func__, error); + goto bad; Does that reject the call if ic_opmode is 0? Why is ic_opmode passed twice? + switch (vif->type) { + case IEEE80211_IF_TYPE_STA: + /* XXX: Handle (conf->changed & IEEE80211_IFCC_SSID) */ You might not need to if you don't care about the SSID (since you have no IBSS yet) + if (changed_flags & FIF_BCN_PRBRESP_PROMISC) { + if (*total_flags & FIF_BCN_PRBRESP_PROMISC) + ath_scan_start(sc); + else + ath_scan_end(sc); That's extremly wrong. What do you need scan start/end for? Pick up Dan's patch to add callbacks for this. +static int ath9k_ampdu_action(struct ieee80211_hw *hw, + enum ieee80211_ampdu_mlme_action action, + const u8 *addr, + u16 tid, + u16 *ssn) How do you handle the SSN part? Now that mac80211 does the SSN assignment I think we should drop that argument. +u_int32_t ath_chan2flags(struct ieee80211_channel *chan, + struct ath_softc *sc) +{ + struct ieee80211_hw *hw = sc->hw; + struct ath_ht_info *ht_info = &sc->sc_ht_info; + + if (sc->sc_scanning) { + if (chan->band == IEEE80211_BAND_5GHZ) { + if (ath_check_chanflags(chan, CHANNEL_A_HT20, sc)) + return CHANNEL_A_HT20; + else + return CHANNEL_A; Why can't you use cfg80211's flags, adding required ones? +void ath__update_txpow(struct ath_softc *sc, + u_int16_t txpowlimit, + u_int16_t txpowlevel) +{ + +} Huh? +struct sk_buff *ath_get_beacon(struct ath_softc *sc, + int if_id, + struct ath_beacon_offset *bo, + struct ath_tx_control *txctl) +{ + return NULL; +} + +int ath_update_beacon(struct ath_softc *sc, + int if_id, + struct ath_beacon_offset *bo, + struct sk_buff *skb, + int mcast) +{ + return 0; +} Huh? + /* remove FCS before passing up to protocol stack */ + skb_trim(skb, (skb->len - FCS_LEN)); No, please don't, just tell us it's still there. + if ((wMode >= WIRELESS_MODE_MAX) || (type != NORMAL_RATE)) + return; What's with all those defines? + for (i = 0; i < maxrates; i++) { + switch (wMode) { + case WIRELESS_MODE_11b: + case WIRELESS_MODE_11g: and those? + error = ieee80211_register_hw(hw); + if (error != 0) { + ath_rate_control_unregister(); + goto bad; + } + + /* initialize tx/rx engine */ + + error = ath_tx_init(sc, ATH_TXBUF); + if (error != 0) + goto bad1; + + error = ath_rx_init(sc, ATH_RXBUF); + if (error != 0) + goto bad1; You should probably do that the other way around... So some more comments. You need to dissolve the hal layer better; get rid of all the HAL constants. Rename all the ATH_ constants that aren't hw but 802.11 and put them into ieee80211.h. Tell us what sort of node information you need, and let's put it into sta_info and/or have some sort of private sta_info area, instead of implementing a linked station list yourself. johannes