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
