Atheros IEEE 802.11n ath9k driver

This series of patches adds the ath9k driver, which adds support for all Atheors IEEE 802.11n chipsets along some new linked list helpers. We are hosting this driver temporarily in a git tree until it is merged into wireless-testing.

More information about this driver is available at:

http://wireless.kernel.org/en/users/Drivers/ath9k

Our mailing list for this driver is:

https://lists.ath9k.org/mailman/listinfo/ath9k-devel

The temporary git URL to build this driver separately is:

git://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/ath9k.git

If you would like to wget these patches individually you can get them here:

http://www.kernel.org/pub/linux/kernel/people/mcgrof/patches/ath9k/

We realize there is a lot of work ahead and look forward to any comments on the driver. As it stands this has been mainly tested only on STA mode. We have a few known issues but are working hard on resolving them.

We are eager to talk to others about this driver further at the Wireless summit.

Note: kernel.org is rsyncing right now, so in the meantime you can fetch the patches and source from the git tree from:

git://w1.fi/srv/git/ath9k.git

http://w1.fi/ath9k/

Luis

This adds the new mac80211 ath9k Atheros driver. Only STA support is currently enabled and tested. This patch is close to 1 MB so please download from:

http://www.kernel.org/pub/linux/kernel/people/mcgrof/patches/ath9k/add-ath9k-01/0001-Add-new-Atheros-IEEE-802.11n-ath9k-driver.patch

Note: for now kernel.org is rsyncing so until this is done you can get this from:

http://w1.fi/ath9k/0001-Add-new-Atheros-IEEE-802.11n-ath9k-driver.patch

Remove Atheros 11n devices from being claimed by ath5k as its now handled by ath9k.

---
 drivers/net/wireless/ath5k/base.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

index 217d506..99f0cd3 100644
@@ -93,8 +93,6 @@ static struct pci_device_id ath5k_pci_id_table[] __devinitdata = {
 	{ PCI_VDEVICE(ATHEROS, 0x001a), .driver_data = AR5K_AR5212 }, /* 2413 Griffin-lite */
 	{ PCI_VDEVICE(ATHEROS, 0x001b), .driver_data = AR5K_AR5212 }, /* 5413 Eagle */
 	{ PCI_VDEVICE(ATHEROS, 0x001c), .driver_data = AR5K_AR5212 }, /* 5424 Condor (PCI-E)*/
-	{ PCI_VDEVICE(ATHEROS, 0x0023), .driver_data = AR5K_AR5212 }, /* 5416 */
-	{ PCI_VDEVICE(ATHEROS, 0x0024), .driver_data = AR5K_AR5212 }, /* 5418 */
 	{ 0 }
 };
 MODULE_DEVICE_TABLE(pci, ath5k_pci_id_table);

1.5.4.3

If you are using linked lists for queus list_splice() will not do what you would expect even if you use the elements passed reversed. We need to handle these differently. We add list_splice_tail() and list_splice_tail_init()

---
 drivers/net/wireless/ath9k/core.h |   42 ----------------------------------
 include/linux/list.h              |   45 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 43 deletions(-)

index 1499f6e..807dcdf 100644
@@ -52,19 +52,6 @@ struct ath_node;
 /* An attempt will be made to merge these link list helpers upstream
  * instead */
 
-static inline void __list_splice_tail(const struct list_head *list,
-				 struct list_head *head)
-{
-	struct list_head *first = list->next;
-	struct list_head *last = list->prev;
-	struct list_head *current_tail = head->prev;
-
-	current_tail->next = first;
-	last->next = head;
-	head->prev = last;
-	first->prev = current_tail;
-}
-
 static inline void __list_cut_position(struct list_head *list,
 		struct list_head *head, struct list_head *entry)
 {

@@ -79,35 +66,6 @@ static inline void __list_cut_position(struct list_head *list,
 }
 
 /**
- * list_splice_tail - join two lists, each list being a queue
- * @list: the new list to add.
- * @head: the place to add it in the first list.
- */
-static inline void list_splice_tail(const struct list_head *list,
-				struct list_head *head)
-{
-	if (!list_empty(list))
-		__list_splice_tail(list, head);
-}
-
-/**
- * list_splice_tail_init - join two lists, each list being a queue, and
- * 	reinitialise the emptied list.
- * @list: the new list to add.
- * @head: the place to add it in the first list.
- *
- * The list at @list is reinitialised
- */
-static inline void list_splice_tail_init(struct list_head *list,
-				    struct list_head *head)
-{
-	if (!list_empty(list)) {
-		__list_splice_tail(list, head);
-		INIT_LIST_HEAD(list);
-	}
-}
-
-/**
  * list_cut_position - cut a list into two
  * @list: a new list to add all removed entries
  * @head: a list with entries

1.5.4.3

This adds list_cut_position() which lets you cut a list into two lists given a pivot in in the list.

---
 drivers/net/wireless/ath9k/core.h |   35 -----------------------------------
 include/linux/list.h              |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 35 deletions(-)

index 807dcdf..65f8c50 100644
@@ -49,41 +49,6 @@ struct ath_node;
 /* Utility macros */
 /******************/
 
-/* An attempt will be made to merge these link list helpers upstream
- * instead */
-
-static inline void __list_cut_position(struct list_head *list,
-		struct list_head *head, struct list_head *entry)
-{
-	struct list_head *new_first =
-		(entry->next != head) ? entry->next : head;
-	list->next = head->next;
-	list->next->prev = list;
-	list->prev = entry;
-	entry->next = list;
-	head->next = new_first;
-	new_first->prev = head;
-}
-
-/**
- * list_cut_position - cut a list into two
- * @list: a new list to add all removed entries
- * @head: a list with entries
- * @entry: an entry within head, could be the head itself
- *	and if so we won't won't cut the list
- */
-static inline void list_cut_position(struct list_head *list,
-		struct list_head *head, struct list_head *entry)
-{
-	BUG_ON(list_empty(head));
-	if (list_is_singular(head))
-		BUG_ON(head->next != entry && head != entry);
-	if (entry == head)
-		INIT_LIST_HEAD(list);
-	else
-		__list_cut_position(list, head, entry);
-}
-
 /* Macro to expand scalars to 64-bit objects */
 
 #define	ito64(x) (sizeof(x) == 8) ? \

index fc7b700..8766d33 100644
@@ -326,6 +326,38 @@ static inline int list_is_singular(const struct list_head *head)
 	return !list_empty(head) && (head->next == head->prev);
 }
 
+static inline void __list_cut_position(struct list_head *list,
+		struct list_head *head, struct list_head *entry)
+{
+	struct list_head *new_first =
+		(entry->next != head) ? entry->next : head;
+	list->next = head->next;
+	list->next->prev = list;
+	list->prev = entry;
+	entry->next = list;
+	head->next = new_first;
+	new_first->prev = head;
+}
+
+/**
+ * list_cut_position - cut a list into two
+ * @list: a new list to add all removed entries
+ * @head: a list with entries
+ * @entry: an entry within head, could be the head itself
+ *	and if so we won't won't cut the list
+ */
+static inline void list_cut_position(struct list_head *list,
+		struct list_head *head, struct list_head *entry)
+{
+	BUG_ON(list_empty(head));
+	if (list_is_singular(head))
+		BUG_ON(head->next != entry && head != entry);
+	if (entry == head)
+		INIT_LIST_HEAD(list);
+	else
+		__list_cut_position(list, head, entry);
+}
+
 static inline void __list_splice(const struct list_head *list,
 				 struct list_head *head)
 {

1.5.4.3

Hi.

I must admit that I can't tell it myself due to a lack of in-depth knowledge about both drivers, hence the question: why a new driver, rather than adding 11n support to ath5k and have a single driver for all Atheros chipsets?

Bye, Mike

Working on a separate driver was the way we could guarantee the *best possible support and quality* for our drivers for users. There are some similarities in the drivers but the architecture varies a bit. Both drivers can use help from each other in terms of design but right now the focus is to get users supported. We expect to talk more about this particular topic at the summit.

Luis

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@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

Or just open-code the ioread/writes

;-)

Indeed.

Consider using s64/u64 if you really need 64-bits.

Also, for anything shared with the hardware in a fixed endianness, le16/32/64 and be16/32/64 are your friends. To reiterate what Johannes mentioned elsewhere.

Cheers,

Harvey

Thanks for the review, Johannes.
I am addressing a few comments here.

How far along is IBSS ?

There are plenty of structures/enums in the erstwhile HAL that have been named in such a manner. Will fix this.

Will do.

Will fix.

Heh, legacy cruft. Will clean that.

No specific reason. But it was a very common occurrence in the original code. Cleaned up most of the places, guess this was overlooked.

Ok.

Ok.

Will change it to something else.

I'll check and try to make this easier on the eyes.

I don't understand it either. :)

Will fix.

Ok.

I guess. :)
Will fix.

I am not sure right now, will check.

Luis brought this up a while ago.
Anyway, will clean this up.

Heh.

And use mac80211's band macros ?

Ok.

AFAICS, it is not used anywhere. Will clean this.

Right.

:)

Hardware capability.

Ok.

Ok. :)

In due time. :)

Will fix.

Right, any help on locking in the driver is welcome. :)

Ok.

Will remove this.

The comment refers to an old LED module that was removed prior to submission. Will clean this.

:)

Ok.

Nope, we are not. Again, cruft from a net80211 specific layer.

Heh, see my previous response.

Ok.

I did this. Patch on its way.

Right.

Ok.

I think Luis cooked up a patch for that.

Ok.

Aggregation information is maintained on a per-STA basis. All this node stuff will be cleaned up anyway.

Will clean this.

Me neither. :)

Yep, the original code had separate debug levels for HAL and the layer above. Will clean this.

Ok.

Can sta_info be accessed directly from the driver ?
Don't we need a ieee80211_local ptr to retrieve sta_info ? And if so, is that ok ?
Or will you be providing a private driver area embedded in sta_info ?

Ok.

Ok.

It is.

Ok, will clean this.

Ok.

Heh, the entire codebase was on a massive inline abuse trip. Will clean this.

Ok.

This was added to somehow deal with the fact that the HAL expects a mode when doing a hw reset. But yeah, we'll look into this.

Ok, will remove this.

Will do.

Bah, I removed this already. Will send out patch soon.

No it doesn't, for some reason IBSS has a value 0 for its opmode. But if opmode happens to be invalid, -EINVAL is returned. And yep, it shouldn't be passed twice.

But IBSS is on its way. :)

Right, will fix this.

Will look into this.

I think that was removed already.

I have already removed all this. Patch on its way.

Ok.

Will clean them.

Will clean them.

Ok.

Will address the comments.
Thanks again for the review !

Sujith

I guess the question really is what you need them for, but yeah, if you really need them you should be using the mac80211 stuff to avoid having to translate the values into each other all the time.

Ok cool. As I just discussed with the Intel folks, we can give you a private area in the sta_info struct that we pass down all the time so you can look in there instead of implementing your own hashtable/list/...

No, you cannot look at sta_info, but it shouldn't be hard to make part of the structure public, embed a private area into it and pass that down, like the "vif" pointer in a few places.

Ok so of course I think you should use the mac80211 interface type instead of opmode all the way through, and you should be checking the interface types you support here because right now it looks like if I try to bring up an AP or mesh interface I'll get IBSS?

johannes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
Url : http://lists.ath9k.org/pipermail/ath9k-devel/attachments/20080724/c69fba94/attachment.pgp