On Tue, 2008-12-16 at 03:55 +0100, Lennert Buytenhek wrote: > +static DEFINE_PCI_DEVICE_TABLE(mwl8k_table) = { > + { PCI_VDEVICE(MARVELL, 0x2a2b) }, /* 88w8687 */ > + { PCI_VDEVICE(MARVELL, 0x2a30) }, /* 88w8687 */ > + { } > +}; > +/* Translate PCI device ID to device number */ > +static u32 mwl8k_get_partnum(u16 pci_id) > +{ > + switch (pci_id) { > + case 0x2a2b: > + case 0x2a30: > + return 8687; > + default: > + return 0; > + } > +} It might make sense to do this with pci driver data in the device table instead? > +/* HT control fields for firmware */ > +struct ewc_ht_info { > + __u16 control1; > + __u16 control2; > + __u16 control3; > +} __attribute__((packed)); Does the firmware support endian swapping? Or should this be annotated? > +struct peer_capability_info { > + /* Peer type - AP vs. STA. */ > + __u8 peer_type; > + > + /* Basic 802.11 capabilities from assoc resp. */ > + __u16 basic_caps; > + > + /* Set if peer supports 802.11n high throughput (HT). */ > + __u8 ht_support; > + > + /* Valid if HT is supported. */ > + __u16 ht_caps; > + __u8 extended_ht_caps; > + struct ewc_ht_info ewc_info; > + > + /* Legacy rate table. Intersection of our rates and peer rates. */ > + __u8 legacy_rates[MWL8K_IEEE_LEGACY_DATA_RATES]; > + > + /* HT rate table. Intersection of our rates and peer rates. */ > + __u8 ht_rates[MWL8K_MCS_BITMAP_SIZE]; > + __u8 pad[pad_size]; > + > + /* If set, interoperability mode, no proprietary extensions. */ > + __u8 interop; > + __u8 pad2; > + __u8 station_id; > + __u16 amsdu_enabled; > +} __attribute__((packed)); Same here, packed but no endian annotations? > +static inline int mwl8k_get_frametype(__le16 fc) > +{ > + if (ieee80211_is_data(fc)) > + return IEEE80211_FTYPE_DATA; > + > + if (ieee80211_is_mgmt(fc)) > + return IEEE80211_FTYPE_MGMT; > + > + if (ieee80211_is_ctl(fc)) > + return IEEE80211_FTYPE_CTL; > + > + return 0; > +} That seems a little odd? > +/* Inline functions to manipulate QoS field in data descriptor. */ > +static inline u16 mwl8k_qos_setbit_tid(u16 qos, u8 tid) > +static inline u16 mwl8k_qos_setbit_eosp(u16 qos) > +static inline u16 mwl8k_qos_setbit_ack(u16 qos, u8 ack_policy) > +static inline u16 mwl8k_qos_setbit_amsdu(u16 qos) > +static inline u16 mwl8k_qos_setbit_qlen(u16 qos, u8 len) Why do you need to work with the QoS header? > +/* DMA header used by firmware and hardware. */ > +struct mwl8k_dma_data { > + __u16 fwlen; > + struct ieee80211_hdr wh; > +} __attribute__((packed)); endianness? Ok, I'm starting to suspect you program the endianness into the firmware. Is that really a good idea? Firmware bugs might lurk that way, and are harder to fix this way? > + /* > + * Skb data is changed when DMA header is added. > + * Unshare/Unclone skb. > + */ > + if (skb_cloned(skb)) > + skb = skb_unshare(skb, GFP_ATOMIC); > + else if (skb_shared(skb)) > + skb = skb_share_check(skb, GFP_ATOMIC); That won't ever happen with mac80211 since it has modified the header already anyway. > + /* > + * Copy up/down the 802.11 header; the firmware requires > + * we present a 2-byte payload length followed by a > + * 4-address header (w/o QoS), followed (optionally) by > + * any WEP/ExtIV header (but only filled in for CCMP). > + */ > + if (hdrlen < sizeof(struct mwl8k_dma_data)) { That comparison seems slightly odd to me > + const int space = sizeof(struct mwl8k_dma_data) - hdrlen; > + u32 headroom = skb_headroom(skb); > + > + if (headroom < space) { > + /* NB: should never happen */ > + printk(KERN_ERR "Not enough headroom, need %d, " > + "found %d, len %d\n", space, > + headroom, skb->len); > + return NULL; > + } > + skb_push(skb, space); maybe just remove the check then, it'll be done in skb_push and panic() > + /* Clear addr4 */ > + memset(tr->wh.addr4, 0, IEEE80211_ADDR_LEN); ? > +/* > + * Scan a list of BSSIDs to process for finalize join. > + * Allows for extension to process multiple BSSIDs. > + */ Can you explain why you need to do this much BSSID stuff? Maybe we can extend mac80211 a little instead? > + if (rx_desc->rx_status & > + MWL8K_RX_CTRL_DECRYPT_ERROR) > + status.flag |= RX_FLAG_MMIC_ERROR; This seems inappropriate for, say, ccmp. Unless the hw flag is misnamed? > +/* Function sleeps, cannot be called from atomic context. */ > +static int mwl8k_tx_wait_empty(struct ieee80211_hw *hw, u32 delay_ms) > +{ just stick in might_sleep() instead? > + printk(KERN_ERR > + "%s: Error removing DMA header from tx skb 0x%p.\n", > + priv->name, skb); That looks a bit odd, and I think you're allowed to go > 80 cols for printks now. > + /* Convert firmware status stuff into tx_status */ > + if (MWL8K_TXD_SUCCESS(status)) { > + /* Transmit OK */ > + info->flags |= IEEE80211_TX_STAT_ACK; > + } else if (MWL8K_TXD_FAIL_RETRY(status)) { > + info->status.excessive_retries = 1; > + } Against which kernel is this? :) > + if (force) > + ieee80211_tx_status(hw, skb); > + else > + ieee80211_tx_status_irqsafe(hw, skb); That's a little odd. You should only ever use one of them to avoid ordering issues. > + /* > + * Tell firmware to not send EAPOL pkts in an > + * aggregate. Verify against mac80211 tx path. If > + * stack turns off AMPDU for an EAPOL frame this > + * check will be removed. > + */ Can you elaborate how aggregation will work? Not relevant for this patch, but for mac80211 ampdu work I am going to do. > + struct mwl8k_priv *priv = hw->priv; > + > + if (hw == NULL || hw->priv == NULL) > + return -EINVAL; That'll give coverity some fodder ;) > +#define MWL8K_SUPPORTED_IF_FLAGS FIF_BCN_PRBRESP_PROMISC Not more? That's not very nice as a monitor then :) > +mwl8k_configure_filter_exit: > + return rc; Generally, I think your labels are a tad too verbose. But I can't say I care much. > +static int mwl8k_set_rts_threshold(struct ieee80211_hw *hw, u32 value) > +{ > + int rc; > + struct mwl8k_set_rts_threshold_worker *worker; > + struct mwl8k_priv *priv = hw->priv; > + > + worker = kzalloc(sizeof(*worker), GFP_KERNEL); > + if (worker == NULL) > + return -ENOMEM; > + > + worker->value = value; > + > + rc = mwl8k_queue_work(hw, &worker->header, > + priv->config_wq, > + mwl8k_set_rts_threshold_wt); > + kfree(worker); > + > + if (rc == -ETIMEDOUT) { > + printk(KERN_ERR "%s() timed out\n", __func__); > + rc = -EINVAL; > + } > + > + return rc; > +} Does that really need to be atomic? That seems odd and easy to change. > +static void > +mwl8k_sta_notify(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > + enum sta_notify_cmd cmd, const u8 *addr) > +{ > +} No need to, just leave the function pointer NULL. > +static int mwl8k_conf_tx(struct ieee80211_hw *hw, u16 queue, > + const struct ieee80211_tx_queue_params *params) > +{ > + int rc; > + struct mwl8k_conf_tx_worker *worker; > + struct mwl8k_priv *priv = hw->priv; > + > + worker = kzalloc(sizeof(*worker), GFP_KERNEL); > + if (worker == NULL) > + return -ENOMEM; > + > + worker->queue = queue; > + worker->params = params; > + rc = mwl8k_queue_work(hw, &worker->header, > + priv->config_wq, mwl8k_conf_tx_wt); > + kfree(worker); > + if (rc == -ETIMEDOUT) { > + printk(KERN_ERR "%s() timed out\n", __func__); > + rc = -EINVAL; > + } > + return rc; > +} Are you sure this needs to be deferred to the wq? I'm not exactly sure but I thought conf_tx wasn't under a spinlock? > +static int mwl8k_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd, > + const u8 *local_address, const u8 *address, > + struct ieee80211_key_conf *key) > +{ > + int rc; > + struct mwl8k_set_key_worker *worker; > + struct mwl8k_priv *priv = hw->priv; > + > + worker = kzalloc(sizeof(*worker), GFP_KERNEL); > + if (worker == NULL) > + return -ENOMEM; > + > + worker->cmd = cmd; > + worker->local_address = local_address; > + worker->address = address; > + worker->key = key; > + rc = mwl8k_queue_work(hw, &worker->header, > + priv->config_wq, mwl8k_set_key_wt); > + kfree(worker); > + if (rc == -ETIMEDOUT) { > + printk(KERN_ERR "%s() timed out\n", __func__); > + rc = -EINVAL; > + } > + return rc; > +} set_key can sleep for sure, no need to queue it off to the wq. Can you rebase onto wireless-testing with the API changes there? johannes
Attachment:
signature.asc
Description: This is a digitally signed message part