Search Linux Wireless

Re: [PATCH,RFC] initial mwl8k driver for marvell topdog wireless

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]
  Powered by Linux