Re: [PATCH 1/4] Add new Atheros IEEE 802.11n ath9k driver

[Johannes Berg <johannes@sipsolutions.net>]

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