Hi David, all,
I'm not really in the chain here, but I figured I'd review your driver
nonetheless. I'll want to take a closer look at some things, but for now
here's a bit of a review just of the code.
Can you perhaps explain how the STA/AP firmware separation works? I've
not really managed to figure that out - admittedly with not all that
much effort though.
On Thu, 2015-06-04 at 04:57 +0000, David Lin wrote:
> drivers/net/wireless/mwlwifi/Kconfig | 17 +
Does this driver has any relation to mwifiex? Perhaps the same
maintainer team, etc.? Could consider moving all the Marvell drivers
into one directory, but not really necessary I guess.
Out of curiosity - why "mwlwifi" and not just "mwl" or similar? :)
Also - consider adding a MAINTAINERS entry for this driver.
> drivers/net/wireless/mwlwifi/mwl_debug.c | 207 ++
The mwl_ prefix doesn't really do much for this driver (especially since
it's used for all files) -- I'd consider removing it.
> @@ -284,5 +284,6 @@ source "drivers/net/wireless/zd1211rw/Kconfig"
> source "drivers/net/wireless/mwifiex/Kconfig"
> source "drivers/net/wireless/cw1200/Kconfig"
> source "drivers/net/wireless/rsi/Kconfig"
> +source "drivers/net/wireless/mwlwifi/Kconfig"
Perhaps, just like with the directory structure, we should also consider
having per-vendor Kconfig structure, like Ethernet drivers have now.
Then again, unless we decide we want to do this for all drivers and all
devices it doesn't really do much.
> + depends on PCI && MAC80211
> + select FW_LOADER
> + select OF
That's interesting, why does a PCI(e) driver need OF?
> +/* CONSTANTS AND MACROS
> +*/
that one-line comment style (also in other places below, and perhaps
other files) is a but strange
> +#define PRT_8BYTES "0x%02x 0x%02x 0x%02x 0x%02x 0x%02x 0x%02x 0x%02x 0x%02x\n"
Do you really need the "0x" in here? Otherwise it'd be much nicer to use
%ph instead of macros.
> +void mwl_debug_prt(u32 classlevel, const char *func, const char *format, ...)
> +{
> + debug_string = kmalloc(1024, GFP_ATOMIC);
> +
> + if (!debug_string)
> + return;
That seems a bit questionable - when memory allocations start failing is
one of those places where you really want debug output ...
> + switch (class) {
> + case DBG_CLASS_ENTER:
> + pr_debug("Enter %s() ...\n", func);
> + break;
> + case DBG_CLASS_EXIT:
> + pr_debug("... Exit %s()\n", func);
> + break;
I think you should not have enter/exit logging at all but use function
tracing instead in the unlikely event this becomes necessary.
> + if (strlen(debug_string) > 0) {
strlen() > 0 seems pretty expensive - perhaps the compiler can optimise
it, but still.
> + if (debug_string[strlen(debug_string) - 1] == '\n')
> + debug_string[strlen(debug_string) - 1] = '\0';
> + pr_debug("%s(): %s\n", func, debug_string);
> + }
You should also be able to just pass the original arguments to pr_debug,
perhaps with %pV, and get rid of the allocation entirely.
> + for (curr_byte = 0; curr_byte < len; curr_byte = curr_byte + 8) {
This really is pretty icky - get rid of it and use %ph in the caller. If
it's longer than what's supported there you probably shouldn't have it
at all and add tracing events to your driver instead.
> +void mwl_debug_dumpdata(const void *data, int len, char *marker)
ditto.
> +#define DBG_LEVEL_0 BIT(0) /* mwl_main.c */
> +#define DBG_LEVEL_1 BIT(1) /* mwl_fwdl.c */
> +#define DBG_LEVEL_2 BIT(2) /* mwl_fwcmd.c */
> +#define DBG_LEVEL_3 BIT(3) /* mwl_tx.c */
> +#define DBG_LEVEL_4 BIT(4) /* mwl_rx.c */
> +#define DBG_LEVEL_5 BIT(5) /* mwl_mac80211.c */
This seems very questionable.
> +#define DBG_CLASS_7 BIT(23)
and those are hopefully not used at all - remove them?
Consider adding __printf(x,y) annotation to the functions to check the
validity of the format strings.
> +#define WLDBG_DUMP_DATA(classlevel, data, len)
> +#define WLDBG_ENTER(classlevel)
> +#define WLDBG_ENTER_INFO(classlevel, ...)
> +#define WLDBG_EXIT(classlevel)
> +#define WLDBG_EXIT_INFO(classlevel, ...)
> +#define WLDBG_INFO(classlevel, ...)
> +#define WLDBG_WARNING(classlevel, ...)
> +#define WLDBG_ERROR(classlevel, ...)
> +#define WLDBG_PANIC(classlevel, ...)
Consider still expanding them to some inline that gets the arguments and
also has __printf(x,y) annotation so you get compile-time checking even
when you don't have debug enabled.
> +/* Map to 0x80000000 (Bus control) on BAR0
> +*/
should be single-line comment style - not going to comment on that any
more
> +#define MACREG_A2HRIC_BIT_TX_DONE 0x00000001 /* bit 0 */
> +#define MACREG_A2HRIC_BIT_RX_RDY 0x00000002 /* bit 1 */
> +#define MACREG_A2HRIC_BIT_OPC_DONE 0x00000004 /* bit 2 */
> +#define MACREG_A2HRIC_BIT_MAC_EVENT 0x00000008 /* bit 3 */
> +#define MACREG_A2HRIC_BIT_RX_PROBLEM 0x00000010 /* bit 4 */
> +#define MACREG_A2HRIC_BIT_RADIO_OFF 0x00000020 /* bit 5 */
> +#define MACREG_A2HRIC_BIT_RADIO_ON 0x00000040 /* bit 6 */
> +#define MACREG_A2HRIC_BIT_RADAR_DETECT 0x00000080 /* bit 7 */
> +#define MACREG_A2HRIC_BIT_ICV_ERROR 0x00000100 /* bit 8 */
> +#define MACREG_A2HRIC_BIT_WEAKIV_ERROR 0x00000200 /* bit 9 */
Those comments are clearly useless - just use BIT(N) as below?!
> +#define MACREG_A2HRIC_BIT_QUEUE_EMPTY BIT(10)
> +#define MACREG_A2HRIC_BIT_CHAN_SWITCH BIT(12) /* IEEE80211_DH */
What's that supposed to mean?
> +#define MACREG_A2HRIC_BA_WATCHDOG BIT(14)
> +#define MACREG_A2HRIC_BIT_SSU_DONE BIT(16)
> +#define MACREG_A2HRIC_CONSEC_TXFAIL BIT(17) /* 15 taken by ISR_TXACK */
That comment seems misplaced? maybe it should be between 14 and 16?
> +#define ISR_SRC_BITS ((MACREG_A2HRIC_BIT_RX_RDY) | \
> + (MACREG_A2HRIC_BIT_TX_DONE) | \
> + (MACREG_A2HRIC_BIT_OPC_DONE) | \
Lots of useless parentheses.
> +#define MACREG_H2ARIC_BIT_PPA_READY 0x00000001 /* bit 0 */
see above
> +#define WL_SEC_SLEEP(num_secs) mdelay(num_secs * 1000)
> +#define WL_MSEC_SLEEP(num_milli_secs) mdelay(num_milli_secs)
> +
> +#define ENDIAN_SWAP32(_val) (cpu_to_le32(_val))
> +#define ENDIAN_SWAP16(_val) (cpu_to_le16(_val))
> +
> +#define DECLARE_LOCK(l) spinlock_t l
> +#define SPIN_LOCK_INIT(l) spin_lock_init(l)
> +#define SPIN_LOCK(l) spin_lock(l)
> +#define SPIN_UNLOCK(l) spin_unlock(l)
> +#define SPIN_LOCK_IRQSAVE(l, f) spin_lock_irqsave(l, f)
> +#define SPIN_UNLOCK_IRQRESTORE(l, f) spin_unlock_irqrestore(l, f)
Nope, get rid of all these.
> +/* vif and station
> +*/
> +#define MAX_WEP_KEY_LEN 13
Use WLAN_KEY_LEN_WEP104?
> +#define MWL_VIF(_vif) ((struct mwl_vif *)&((_vif)->drv_priv))
> +#define IEEE80211_KEY_CONF(_u8) ((struct ieee80211_key_conf *)(_u8))
> +#define MWL_STA(_sta) ((struct mwl_sta *)&((_sta)->drv_priv))
convert these to static inline functions
> +enum {
> + IEEE_TYPE_MANAGEMENT = 0,
> + IEEE_TYPE_CONTROL,
> + IEEE_TYPE_DATA
> +};
This will be rather confusing since they don't match the spec - get rid
of them and use IEEE80211_FTYPE_* with the correct shift/mask.
> +struct mwl_rate_info {
> + u32 format:2; /* 0 = Legacy, 1 = 11n, 2 = 11ac */
> + u32 stbc:1;
> + u32 rsvd1:1;
> + u32 bandwidth:2; /* 0 = 20 MHz, 1 = 40 MHz, 2 = 80 MHz */
> + u32 short_gi:1; /* 0 = standard guard interval, 1 = short */
> + u32 rsvd2:1;
> + u32 rate_id_mcs:7;
> + u32 preamble_type:1; /* Preambletype 0 = Long, 1 = Short; */
> + u32 power_id:6;
> + u32 adv_coding:1; /* ldpc */
> + u32 bf:1;
> + u32 ant_select:8; /* Bitmap to select one of the transmit antenna */
> +} __packed;
This being __packed seems to indicate you want to send it to the
hardware - you should probably put such structures into a separate
header file.
Also, using bitfields in a structure sent to hardware cannot possibly be
endian correct - simply don't do it and use constants/shifts/masks/etc.
instead.
Then again, after seeing the next struct - perhaps this simply shouldn't
be packed, or have a comment as to why it must be?
> +struct mwl_tx_desc {
> + u8 data_rate;
> + u8 tx_priority;
> + u16 qos_ctrl;
> + u32 pkt_ptr;
> + u16 pkt_len;
> + u8 dest_addr[ETH_ALEN];
> + u32 pphys_next;
> + u32 sap_pkt_info;
> + struct mwl_rate_info rate_info;
> + u8 type;
> + u8 xmit_control; /* bit 0: use rateinfo, bit 1: disable ampdu */
> + u16 reserved;
> + u32 tcpack_sn;
> + u32 tcpack_src_dst;
> + struct sk_buff *psk_buff;
> + struct mwl_tx_desc *pnext;
> + u8 reserved1[2];
> + u8 packet_info;
> + u8 packet_id;
> + u16 packet_len_and_retry;
> + u16 packet_rate_info;
> + u8 *sta_info;
> + u32 status;
> +} __packed;
See above - clearly this one cannot be sent to the hardware.
> +struct mwl_hw_rssi_info {
> + u32 rssi_a:8;
> + u32 rssi_b:8;
> + u32 rssi_c:8;
> + u32 rssi_d:8;
> +} __packed;
Err, what's with the bitfields?!
> +struct mwl_hw_noise_floor_info {
> + u32 noise_floor_a:8;
> + u32 noise_floor_b:8;
> + u32 noise_floor_c:8;
> + u32 noise_floor_d:8;
> +} __packed;
Ditto.
> +struct mwl_rx_desc {
> + u16 pkt_len; /* total length of received data */
> + struct mwl_rxrate_info rate; /* receive rate information */
> + u32 pphys_buff_data; /* physical address of payload data */
> + u32 pphys_next; /* physical address of next RX desc */
> + u16 qos_ctrl; /* received QosCtrl field variable */
> + u16 ht_sig2; /* like name states */
> + struct mwl_hw_rssi_info hw_rssi_info;
> + struct mwl_hw_noise_floor_info hw_noise_floor_info;
> + u8 noise_floor;
> + u8 reserved[3];
> + u8 rssi; /* received signal strengt indication */
> + u8 status; /* status field containing USED bit */
> + u8 channel; /* channel this pkt was received on */
> + u8 rx_control; /* the control element of the desc */
> + /* above are 32bits aligned and is same as FW, RxControl put at end
> + * for sync
> + */
> + struct sk_buff *psk_buff; /* associated sk_buff for Linux */
> + void *pbuff_data; /* virtual address of payload data */
> + struct mwl_rx_desc *pnext; /* virtual address of next RX desc */
> +} __packed;
This is really really strange - the first few fields *are* used with the
firmware, and the latter aren't? But how come then they can be "u32"
rather than "__le32"? Very odd.
> +struct mwl_desc_data {
> + dma_addr_t pphys_tx_ring; /* ptr to first TX desc (phys.) */
> + struct mwl_tx_desc *ptx_ring; /* ptr to first TX desc (virt.) */
> + struct mwl_tx_desc *pnext_tx_desc; /* next TX desc that can be used */
> + struct mwl_tx_desc *pstale_tx_desc;/* the staled TX descriptor */
> + dma_addr_t pphys_rx_ring; /* ptr to first RX desc (phys.) */
> + struct mwl_rx_desc *prx_ring; /* ptr to first RX desc (virt.) */
> + struct mwl_rx_desc *pnext_rx_desc; /* next RX desc that can be used */
> + unsigned int wcb_base; /* FW base offset for registers */
> + unsigned int rx_desc_write; /* FW descriptor write position */
> + unsigned int rx_desc_read; /* FW descriptor read position */
> + unsigned int rx_buf_size; /* length of the RX buffers */
> +} __packed;
Clearly shouldn't be packed.
> +struct mwl_locks {
> + DECLARE_LOCK(xmit_lock); /* used to protect TX actions */
> + DECLARE_LOCK(fwcmd_lock); /* used to protect FW commands */
> + DECLARE_LOCK(stream_lock); /* used to protect stream */
> +};
Whaaa?
No no ... first of all, putting locks with the data they protect seems
much better, then the macros, ...
> +struct beacon_info {
> + bool valid;
> + u16 cap_info;
> + u8 b_rate_set[SYSADPT_MAX_DATA_RATES_G];
> + u8 op_rate_set[SYSADPT_MAX_DATA_RATES_G];
> + u8 ie_wmm_len; /* Keep WMM IE */
> + u8 *ie_wmm_ptr;
I'm not sure why you'd need such a struct with mac80211, but even if you
do it'd be far more efficient to not alternate u8 and u8 * fields since
that basically wastes 7 bytes for alignment each time you do it.
> + u16 iv16;
> + u32 iv32;
> + s8 keyidx;
> +};
Hmm. Are you sure you need an iv16/iv32 in a *vif* struct? That seems
very suspicious.
> +struct mwl_sta {
> + u8 is_ampdu_allowed;
> + struct mwl_tx_info tx_stats[MWL_MAX_TID];
> + u16 iv16;
> + u32 iv32;
> +};
Here also - you can still have multiple keys, even TKIP.
> +struct mwl_tx_ctrl {
> + u8 tx_priority;
> + u16 qos_ctrl;
> + u8 type;
> + u8 xmit_control;
> + u8 *sta_info;
> + bool ccmp;
> +} __packed;
Again, don't do packed for host structures, it just kills your
performance on many architectures. Instead, reorder the structure so
it's natively packed!
> +/* Genenral error */
typo
> +/* Key type is WEP */
> +#define KEY_TYPE_ID_WEP 0x00
> +/* Key type is TKIP */
> +#define KEY_TYPE_ID_TKIP 0x01
> +/* Key type is AES-CCMP */
> +#define KEY_TYPE_ID_AES 0x02
Those comments are pretty useless.
> +/* Group key for TX */
> +#define ENCR_KEY_FLAG_TXGROUPKEY 0x00000004
> +/* pairwise */
> +#define ENCR_KEY_FLAG_PAIRWISE 0x00000008
same here etc...
> +/* Misc
> +*/
> +#define MWL_SPIN_LOCK(X) SPIN_LOCK_IRQSAVE(X, flags)
> +#define MWL_SPIN_UNLOCK(X) SPIN_UNLOCK_IRQRESTORE(X, flags)
Umm, nope.
> +struct hostcmd_header {
> + u16 cmd;
> + u16 len;
> + u8 seq_num;
> + u8 macid;
> + u16 result;
> +} __packed;
Now this really looks like a struct you exchange with the device, so it
should probably be using __le16 instead of u16.
> +struct hostcmd_cmd_get_hw_spec {
> + struct hostcmd_header cmd_hdr;
> + u8 version; /* version of the HW */
> + u8 host_if; /* host interface */
> + u16 num_wcb; /* Max. number of WCB FW can handle */
> + u16 num_mcast_addr; /* MaxNbr of MC addresses FW can handle */
ditto, and throughout this file.
Unless the firmware can somehow run in "host endian"? Seems rather
unlikely to me since dealing with wifi is much easier in little endian
firmware.
> + /* Indicates supported AMPDU type
> + * (1 = implicit, 0 = explicit (default))
> + */
> + u32 implicit_ampdu_ba:1;
> + /* indicates mbss features disable in FW */
> + u32 disablembss:1;
> + u32 host_form_beacon:1;
> + u32 host_form_probe_response:1;
> + u32 host_power_save:1;
> + u32 host_encr_decr_mgt:1;
> + u32 host_intra_bss_offload:1;
> + u32 host_iv_offload:1;
> + u32 host_encr_decr_frame:1;
> + u32 reserved: 21; /* Reserved */
> + u32 tx_wcb_num_per_queue;
> + u32 total_rx_wcb;
> +} __packed;
And here for real - don't use bitfields in data exchanged with the
device.
> +struct hostcmd_cmd_broadcast_ssid_enable {
> + struct hostcmd_header cmd_hdr;
> + u32 enable;
> +} __packed;
Maybe, btw, you should consider having the header somehow separate from
the structs since it's always present. No idea how you use it though, so
perhaps not something you want to do.
> +struct rsn_ie {
> + u8 elem_id;
> + u8 len;
> + u8 oui_type[4]; /* 00:50:f2:01 */
> + u8 ver[2];
> + u8 grp_key_cipher[4];
> + u8 pws_key_cnt[2];
> + u8 pws_key_cipher_list[4];
> + u8 auth_key_cnt[2];
> + u8 auth_key_list[4];
> +} __packed;
> +
> +struct rsn48_ie {
> + u8 elem_id;
> + u8 len;
> + u8 ver[2];
I'm getting a feeling that your hardware is too smart for a mac80211
driver ... ;-)
> +struct hostcmd_cmd_update_encryptoin {
typo - encryption
> +static bool mwl_fwcmd_chk_adapter(struct mwl_priv *priv);
Personally, I'd never mix host function declarations with device command
structure declarations in a header file so they're both more clearly
identifiable.
Actually, this is in a C file - you should avoid static forward
declarations anyway, reorder the code instead.
> +void mwl_fwcmd_reset(struct ieee80211_hw *hw)
> + BUG_ON(!hw);
> + priv = hw->priv;
> + BUG_ON(!priv);
Avoid BUG_ON(), it just makes life easier, if only for the poor
developer who happened to pass NULL for some stupid reason ...
Also, if hw is valid then hw->priv really can't be NULL ... think of how
the memory layout is done.
> + if (mwl_fwcmd_chk_adapter(priv)) {
> + writel(ISR_RESET,
> + priv->iobase1 + MACREG_REG_H2A_INTERRUPT_EVENTS);
> + }
No need for braces.
> + WLDBG_EXIT(DBG_LEVEL_2);
I think you should get rid of all of these.
And of course this is going to be a theme since all of your functions
are written this way :)
> + pcmd->cmd_hdr.cmd = ENDIAN_SWAP16(HOSTCMD_CMD_GET_HW_SPEC);
> + pcmd->cmd_hdr.len = ENDIAN_SWAP16(sizeof(*pcmd));
> + pcmd->fw_awake_cookie = ENDIAN_SWAP32(priv->pphys_cmd_buf + 2048);
So I really think you need to add the proper __le16 annotations, make
sparse run on your code - add
ccflags-y += -D__CHECK_ENDIAN__
to your Makefile to do that, get rid of the macros and fix all the
sparse warnings by using the proper leXX_to_cpu and cpu_to_leXX in all
places.
> + while (mwl_fwcmd_exec_cmd(priv, HOSTCMD_CMD_GET_HW_SPEC)) {
> + WLDBG_PRINT("failed execution");
> + WL_MSEC_SLEEP(1000);
> + WLDBG_PRINT("repeat command = %x", (unsigned int)pcmd);
> + }
An infinite loop doesn't seem like a great idea.
> + if ((conf->chandef.width == NL80211_CHAN_WIDTH_20_NOHT) ||
> + (conf->chandef.width == NL80211_CHAN_WIDTH_20)) {
> + width = CH_20_MHZ_WIDTH;
> + sub_ch = NO_EXT_CHANNEL;
> +
> + } else if (conf->chandef.width == NL80211_CHAN_WIDTH_40) {
extra blank line
> + if ((priv->powinited & 1) == 0) {
> + mwl_fwcmd_get_tx_powers(priv, priv->target_powers,
> + channel->hw_value, band, width, sub_ch);
> + priv->powinited |= 1;
> + }
That "1" should probably be a constant that's defined somewhere?
> + if ((priv->powinited & 2) == 0) {
> + mwl_fwcmd_get_tx_powers(priv, priv->max_tx_pow,
> + channel->hw_value, band, width, sub_ch);
> +
> + priv->powinited |= 2;
> + }
Ditto for the "2"?
> + if ((conf->chandef.width == NL80211_CHAN_WIDTH_20_NOHT) ||
> + (conf->chandef.width == NL80211_CHAN_WIDTH_20)) {
> + pcmd->chnl_flags.chnl_width = CH_20_MHZ_WIDTH;
> + pcmd->chnl_flags.act_primary = ACT_PRIMARY_CHAN_0;
> + } else if (conf->chandef.width == NL80211_CHAN_WIDTH_40) {
> + pcmd->chnl_flags.chnl_width = CH_40_MHZ_WIDTH;
> + if (conf->chandef.center_freq1 > channel->center_freq)
> + pcmd->chnl_flags.act_primary = ACT_PRIMARY_CHAN_0;
> + else
> + pcmd->chnl_flags.act_primary = ACT_PRIMARY_CHAN_1;
> + } else if (conf->chandef.width == NL80211_CHAN_WIDTH_80) {
> + pcmd->chnl_flags.chnl_width = CH_80_MHZ_WIDTH;
> + pcmd->chnl_flags.act_primary =
> + mwl_fwcmd_get_80m_pri_chnl_offset(pcmd->curr_chnl);
> + }
Btw, might be worth rewriting these as switch statements.
[skipping the rest of this file for this round]
> +/* Define OpMode for SoftAP/Station mode
> + *
> + * The following mode signature has to be written to PCI scratch register#0
> + * right after successfully downloading the last block of firmware and
> + * before waiting for firmware ready signature
> + */
> +
> +#define HOSTCMD_STA_MODE 0x5A
> +#define HOSTCMD_SOFTAP_MODE 0xA5
This is a bit strange - how are you advertising the capabilities for
this?
Also, no P2P which requires combinations?
> +static void mwl_mac80211_tx(struct ieee80211_hw *hw,
> + struct ieee80211_tx_control *control,
> + struct sk_buff *skb);
again, try to avoid static forward declarations
> +static irqreturn_t (*mwl_mac80211_isr)(int irq, void *dev_id);
> +
> +static const struct ieee80211_rate mwl_rates_24[] = {
> + { .bitrate = 10, .hw_value = 2, },
> + { .bitrate = 20, .hw_value = 4, },
> + { .bitrate = 55, .hw_value = 11, },
> + { .bitrate = 110, .hw_value = 22, },
> + { .bitrate = 220, .hw_value = 44, },
Interesting - 22 MBps support is very rare.
> +struct ieee80211_ops *mwl_mac80211_get_ops(void)
> +{
> + return &mwl_mac80211_ops;
> +}
This looks strange, why should it be necessary rather than simply
exposing the structure in the header file??
> +void mwl_mac80211_set_isr(irqreturn_t (*isr)(int irq, void *dev_id))
> +{
> + WLDBG_ENTER(DBG_LEVEL_5);
> +
> + mwl_mac80211_isr = isr;
This doesn't seem right - you should avoid global state, but store state
per device.
> + index = skb_get_queue_mapping(skb);
> +
> + mwl_tx_xmit(hw, index, control->sta, skb);
why pass the index and station separately, since they're both in the
skb?
> + rc = mwl_fwcmd_radio_enable(hw);
> +
> + if (!rc)
> + rc = mwl_fwcmd_set_rate_adapt_mode(hw, 0);
> +
> + if (!rc)
> + rc = mwl_fwcmd_set_wmm_mode(hw, true);
> +
> + if (!rc)
> + rc = mwl_fwcmd_set_dwds_stamode(hw, true);
> +
> + if (!rc)
> + rc = mwl_fwcmd_set_fw_flush_timer(hw, 0);
This is pretty non-standard error handling code for the kernel, better
rewrite with goto.
> + /* Setup driver private area.
> + */
> + mwl_vif = MWL_VIF(vif);
> + memset(mwl_vif, 0, sizeof(*mwl_vif));
The area should already be memset to 0, unless the interface had been
added before. Not sure you want to preserve or not, but you should be
aware.
> + priv->macids_used |= 1 << mwl_vif->macid;
The mac-ID handling would appear to be missing locking? Unless you
really never ever access it from outside the add/remove context.
> + rc = mwl_fwcmd_set_rf_channel(hw, conf);
> +
> + if (rc)
> + goto out;
I'd remove those extra blank lines after the assignments, and I note
that here you used more customary style with gotos.
> +static int mwl_mac80211_sta_add(struct ieee80211_hw *hw,
> + struct ieee80211_vif *vif,
> + struct ieee80211_sta *sta)
Please consider implementing sta_state, I hope we can perhaps one day
get rid of sta_add/sta_remove.
> + if (mwl_vif->is_sta)
> + mwl_fwcmd_set_new_stn_del(hw, vif, sta->addr);
That seems suspicious, it shouldn't be needed to delete before you add?
> +static int mwl_mac80211_ampdu_action(struct ieee80211_hw *hw,
> + struct ieee80211_vif *vif,
> + enum ieee80211_ampdu_mlme_action action,
> + struct ieee80211_sta *sta,
> + u16 tid, u16 *ssn, u8 buf_size)
> +{
> + int i, rc = 0;
> + struct mwl_priv *priv = hw->priv;
> + struct mwl_ampdu_stream *stream;
> + u8 *addr = sta->addr, idx;
> + struct mwl_sta *sta_info = MWL_STA(sta);
> +
> + WLDBG_ENTER(DBG_LEVEL_5);
> +
> + if (!(hw->flags & IEEE80211_HW_AMPDU_AGGREGATION)) {
> + WLDBG_EXIT_INFO(DBG_LEVEL_5, "no HW AMPDU");
> + return -ENOTSUPP;
> + }
I'm almost certain mac80211 checks this already :)
> + case IEEE80211_AMPDU_TX_START:
> + /* By the time we get here the hw queues may contain outgoing
> + * packets for this RA/TID that are not part of this BA
> + * session. The hw will assign sequence numbers to these
> + * packets as they go out. So if we query the hw for its next
> + * sequence number and use that for the SSN here, it may end up
> + * being wrong, which will lead to sequence number mismatch at
> + * the recipient. To avoid this, we reset the sequence number
> + * to O for the first MPDU in this BA stream.
> + */
> + *ssn = 0;
I'm not sure that does what you think it does? "ssn" is just what
mac80211 tells the remote side, which really shouldn't be 0 nor should
it be reset since otherwise the peer will expect something else:
Imagine you're at seqno 0xf00 now, with ssn=0 the peer would still
expect 0x100 packets before aggregation starts. That's probably not so
bad though, but imagine you're at 0 at this moment for wrapping
reasons ... seems that would get confused.
> + /* Release the lock before we do the time consuming stuff
> + */
> + SPIN_UNLOCK(&priv->locks.stream_lock);
> +
> + for (i = 0; i < MAX_AMPDU_ATTEMPTS; i++) {
> + /* Check if link is still valid
> + */
> + if (!sta_info->is_ampdu_allowed) {
> + SPIN_LOCK(&priv->locks.stream_lock);
> + mwl_fwcmd_remove_stream(hw, stream);
> + SPIN_UNLOCK(&priv->locks.stream_lock);
> + WLDBG_EXIT_INFO(DBG_LEVEL_5,
> + "link is no valid now");
> + return -EBUSY;
> + }
> +
> + rc = mwl_fwcmd_check_ba(hw, stream, vif);
> +
> + if (!rc)
> + break;
> +
> + WL_MSEC_SLEEP(1000);
> + }
That seems FAR too long to do in the context of mac80211's work queue,
it'll stall a lot of other processing. You should do this
asynchronously.
> + if (changed & BSS_CHANGED_ERP_PREAMBLE) {
> + mwl_fwcmd_set_radio_preamble(hw,
> + vif->bss_conf.use_short_preamble);
> + }
> +
> + if ((changed & BSS_CHANGED_ASSOC) && vif->bss_conf.assoc) {
> + mwl_fwcmd_set_aid(hw, vif, (u8 *)vif->bss_conf.bssid,
> + vif->bss_conf.aid);
> + }
No need for braces.
> + if ((info->ssid[0] != '\0') &&
> + (info->ssid_len != 0) &&
> + (!info->hidden_ssid))
> + mwl_fwcmd_broadcast_ssid_enable(hw, vif, true);
> + else
> + mwl_fwcmd_broadcast_ssid_enable(hw, vif, false);
> + }
Why does the firmware need to know about this? What part does it offload
that makes this necessary? This seems a bit like a hack since we already
have this in struct cfg80211_ap_settings, but perhaps just are missing
to pass it through to the driver? Unless you have some offloads though
it shouldn't be needed?
> + band->vht_cap.cap |= IEEE80211_VHT_CAP_SU_BEAMFORMER_CAPABLE;
> + band->vht_cap.cap |= IEEE80211_VHT_CAP_SU_BEAMFORMEE_CAPABLE;
> + band->vht_cap.cap |= IEEE80211_VHT_CAP_MU_BEAMFORMER_CAPABLE;
> + band->vht_cap.cap |= IEEE80211_VHT_CAP_MU_BEAMFORMEE_CAPABLE;
You should probably not yet advertise MU_BEAMFORMEE since mac80211
doesn't yet have the logic to properly split that per virtual
interface ... We're working on that. How come you haven't thought about
group collisions? I thought this driver supported >1 virtual client
interface.
> + /* Extra headroom is the size of the required DMA header
> + * minus the size of the smallest 802.11 frame (CTS frame).
> + */
> + hw->extra_tx_headroom =
> + sizeof(struct mwl_dma_data) - sizeof(struct ieee80211_cts);
If you have an S/G DMA engine, consider avoiding this, it's often not
very efficient.
> + /* Queue ADDBA request in the respective data queue. While setting up
> + * the ampdu stream, mac80211 queues further packets for that
> + * particular ra/tid pair. However, packets piled up in the hardware
> + * for that ra/tid pair will still go out. ADDBA request and the
> + * related data packets going out from different queues asynchronously
> + * will cause a shift in the receiver window which might result in
> + * ampdu packets getting dropped at the receiver after the stream has
> + * been setup.
> + */
That's why you're supposed to set the SSN correctly ... :)
> + for (num = 0; num < SYSADPT_NUM_OF_DESC_DATA; num++) {
> + while (STALE_TXD(num) && (STALE_TXD(num)->status &
> + ENDIAN_SWAP32(EAGLE_TXD_STATUS_OK)) &&
> + (!(STALE_TXD(num)->status &
> + ENDIAN_SWAP32(EAGLE_TXD_STATUS_FW_OWNED)))) {
> + pci_unmap_single(priv->pdev,
> + ENDIAN_SWAP32(STALE_TXD(num)->pkt_ptr),
> + STALE_TXD(num)->psk_buff->len,
> + PCI_DMA_TODEVICE);
> + done_skb = STALE_TXD(num)->psk_buff;
> + rate_info = STALE_TXD(num)->rate_info;
> + STALE_TXD(num)->pkt_len = 0;
> + STALE_TXD(num)->psk_buff = NULL;
> + STALE_TXD(num)->status =
> + ENDIAN_SWAP32(EAGLE_TXD_STATUS_IDLE);
> + priv->fw_desc_cnt[num]--;
> + STALE_TXD(num) = STALE_TXD(num)->pnext;
> + wmb(); /* memory barrier */
That's not a very good comment. Are you sure that's correct btw? It
doesn't make much sense to me here.
> + CURR_TXD(num).status =
> + ENDIAN_SWAP32(EAGLE_TXD_STATUS_IDLE);
> + CURR_TXD(num).psk_buff = NULL;
> + CURR_TXD(num).pkt_ptr = 0;
> + CURR_TXD(num).pkt_len = 0;
That's a bit macro-heavy for my taste - why not just have a local
variable curr_txd?
> +static inline void mwl_tx_insert_ccmp_hdr(u8 *pccmp_hdr,
> + u8 key_id, u16 iv16, u32 iv32)
> +{
> + *((u16 *)pccmp_hdr) = iv16;
> + pccmp_hdr[2] = 0;
> + pccmp_hdr[3] = EXT_IV | (key_id << 6);
> + *((u32 *)&pccmp_hdr[4]) = iv32;
> +}
Why don't you let mac80211 do this?
johannes
--
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
