> On Saturday, June 06, 2015 9:43 PM, Johannes Berg wrote:
>
> 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.
>
Hi Johannes,
Questions:
1. That's interesting, why does a PCI(e) driver need OF?
This driver will accept parameters in DTS file for band control, antenna setting and power table, to cater for different boards combination inside a system.
2. Can you perhaps explain how the STA/AP firmware separation works?
For this driver, the AP/STA mode will use the same single firmware binary, so there is no “separation” per se. The firmware will support AP/STA mode.
3. Does this driver has any relation to mwifiex?
Mwifiex is driver for Firmware-based MLME. It interfaces with firmware with 802.3 packets. Mwlwifi is the Host Soft AP/STA driver that works with mac80211.
I will be working on the following changes as you suggested:
1. Remove mwl from filename (mwl_xxx.x -> xxx.x).
2. Remove comment for the purpose of the code block and take off static function pre-declaration.
3. Remove file debug.c and debug.h.
4. Typo error, unnecessary comment, prefer comment style and use BIT MACRO instead of constant.
5. Move isr related functions to isr.c and isr.h.
6. Remove MACRO for SPIN LOCK and let spin lock be attached to related data structure.
7. For firmware related structure, use __le16 for u16 and __le32 for u32. Add compiler control “-D__CHECK_ENDIAN__” to help to check endian problem.
8. For firmware related structure, take off bit field.
We recommend that the followings should not be changed at current stage:
1. Directory name “mwlwifi”.
This is to be consistent with some predecessor. For example, Intel uses iwlwifi, realtek uses rtlwifi. We do not see a need to change it. This will make sure we keep the original project mwlwifi on openwrt folder remain intact, while we continue to maintain them the same way/pace.
2. Interface with F/W.
F/W used by this driver is also used by other marvell’s drivers.
3. AMPDU related code.
It has been well tested and leveraged from mwl8k. We may enhance it in future, but please accept the current code status for now.
After modification is done, I will come out [PATCH v2] for you to review again.
Thanks,
David
>
> 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
��.n��������+%������w��{.n�����{���zW����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
