Thanks for the review, Johannes.
I am addressing a few comments here.
Johannes Berg wrote:
> Only STA support is currently enabled
>
> Thanks for that. The folks at Intel still haven't gotten the message ;)
>
How far along is IBSS ?
> +#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.
>
There are plenty of structures/enums in the erstwhile HAL that have been named in such a manner.
Will fix this.
> +#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.
>
Will do.
> +#define HAL_RXERR_CRC 0x01
>
> Ditto about HAL_*
>
Will fix.
> +enum hal_bool {
> + AH_FALSE = 0,
> + AH_TRUE = 1,
> +};
>
> Eww. What do you need that for? That's gross.
Heh, legacy cruft. Will clean that.
> +#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.
>
No specific reason. But it was a very common occurrence in the original code.
Cleaned up most of the places, guess this was overlooked.
> +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.
>
Ok.
> +enum hal_rx_filter {
> + HAL_RX_FILTER_UCAST = 0x00000001,
>
> BIT()?
>
> +enum hal_int {
> + HAL_INT_RX = 0x00000001,
>
> Dito.
>
Ok.
> +#define CHANNEL_QUARTER 0x08000
> +#define CHANNEL_HT20 0x10000
> +#define CHANNEL_HT40PLUS 0x20000
> +#define CHANNEL_HT40MINUS 0x40000
>
>
> CHANNEL_ seems a bit too generic.
Will change it to something else.
>
> +#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?
>
I'll check and try to make this easier on the eyes.
> +enum {
> + CTRY_DEBUG = 0x1ff,
> + CTRY_DEFAULT = 0
> +};
>
> ??
I don't understand it either. :)
>
> +#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
Will fix.
>
> +#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.
>
Ok.
> +#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? :)
>
I guess. :)
Will fix.
> +#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?
>
I am not sure right now, will check.
> +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?
>
Luis brought this up a while ago.
Anyway, will clean this up.
> +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".
Heh.
>
> +enum hal_freq_band {
> + HAL_FREQ_BAND_5GHZ = 0,
> + HAL_FREQ_BAND_2GHZ = 1,
> +};
>
> You can remove those.
And use mac80211's band macros ?
>
> +enum {
> + HAL_TRUE_CHIP = 1
> +};
>
> And that one most likely as well.
>
Ok.
> +enum phytype {
> + PHY_DS,
> + PHY_FH,
> + PHY_OFDM,
> + PHY_HT,
> + PHY_MAX
>
> What's a MAX PHY? And where do you use that constant?
>
AFAICS, it is not used anywhere. Will clean this.
> +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...
>
Right.
> +#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?
>
Hardware capability.
> +enum hal_int ath9k_hw_set_interrupts(struct ath_hal *ah,
> + enum hal_int ints);
>
> Please use more standard return values.
Ok.
>
> +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.
Ok. :)
> + * 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?
In due time. :)
>
> + 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.
Will fix.
>
> + /* 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.
Right, any help on locking in the driver is welcome. :)
> +#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
Ok.
>
> +static int ath_outdoor = AH_FALSE; /* enable outdoor use */
>
> How is that useful if it cannot be set?
Will remove this.
>
> + * 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?
The comment refers to an old LED module that was removed prior to submission.
Will clean this.
> + * 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.
Ok.
>
> + * reset 802.11 state machine
> + * (sends station deassoc/deauth frames)
>
> what? are you sure you're doing that?
Nope, we are not. Again, cruft from a net80211 specific layer.
>
> + * 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?
Heh, see my previous response.
>
> +/*
> + * 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.
Ok.
>
> + 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?
I did this. Patch on its way.
> + /*
> + * Query the hal about antenna support
>
> There should be no hal.
Right.
>
> + 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.
>
Ok.
> +#ifdef CONFIG_SLOW_ANT_DIV
>
> I haven't seen a Kconfig definition for that
>
I think Luis cooked up a patch 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.
Ok.
>
> +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?
Aggregation information is maintained on a per-STA basis.
All this node stuff will be cleaned up anyway.
> +#define KASSERT(exp, msg) do { \
> + if (unlikely(!(exp))) { \
> + printk msg; \
> + BUG(); \
> + } \
> + } while (0)
>
> what's wrong with BUG_ON?
Will clean this.
>
> +static inline unsigned long get_timestamp(void)
> +{
> + return ((jiffies / HZ) * 1000) + (jiffies % HZ) * (1000 / HZ);
>
> ??
Me neither. :)
>
> +#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?
Yep, the original code had separate debug levels for HAL and the layer above.
Will clean this.
>
> +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.
Ok.
> +/* 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.
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 ?
>
> +/********/
> +/* VAPs */
> +/********/
>
> Please use "virtual interface" terminology instead of VAP since
> that matches what mac80211 says. Not just here, of course!
Ok.
>
> +/* 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.
Ok.
>
> +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?
It is.
>
> +static inline enum hal_status ath9k_hw_init_macaddr(struct ath_hal *ah)
>
> inline? bit large for that
>
Ok, will clean this.
> +static inline enum hal_bool
> +ath9k_hw_get_lower_upper_index(u_int8_t target,
>
> ditto.
Ok.
>
> +static inline void ath9k_hw_ani_setup(struct ath_hal *ah)
>
> etc. Why do you think setup functions need to be inlines?
Heh, the entire codebase was on a massive inline abuse trip.
Will clean this.
>
> +++ b/drivers/net/wireless/ath9k/initvals.h
>
> initvals probably should not be in a header file but rather a C file
>
Ok.
> +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?
>
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.
> +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.
>
Ok, will remove this.
> +/* Until mac80211 includes these fields */
>
> You'll have to tell us why, how, and what for.
>
Will do.
>
> +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.
Bah, I removed this already. Will send out patch soon.
>
> +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?
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.
>
> + 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)
But IBSS is on its way. :)
>
> + 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.
>
Right, will fix this.
> +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?
Will look into this.
>
> +void ath__update_txpow(struct ath_softc *sc,
> + u_int16_t txpowlimit,
> + u_int16_t txpowlevel)
> +{
> +
> +}
>
> Huh?
I think that was removed already.
>
> +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?
I have already removed all this. Patch on its way.
>
> + /* 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.
Ok.
>
> + if ((wMode >= WIRELESS_MODE_MAX) || (type != NORMAL_RATE))
> + return;
>
> What's with all those defines?
Will clean them.
>
> + for (i = 0; i < maxrates; i++) {
> + switch (wMode) {
> + case WIRELESS_MODE_11b:
> + case WIRELESS_MODE_11g:
>
> and those?
>
Will clean them.
> + 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...
>
Ok.
>
> 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.
>
Will address the comments.
Thanks again for the review !
Sujith
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
