Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 27 Oct 2021 15:38:39 +0300
From:   Kari Argillander <kari.argillander@gmail.com>
To:     Srinivasan Raju <srini.raju@purelifi.com>
Cc:     "mostafa.afgani@purelifi.com Kalle Valo" <kvalo@codeaurora.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Johannes Berg <johannes@sipsolutions.net>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:NETWORKING DRIVERS (WIRELESS)" 
        <linux-wireless@vger.kernel.org>,
        "open list:NETWORKING DRIVERS" <netdev@vger.kernel.org>
Subject: Re: [PATCH v20 2/2] wireless: Initial driver submission for pureLiFi
 STA devices
Message-ID: <20211027123839.6h3rgxsgk6p4ydg3@kari-VirtualBox>
References: <20200928102008.32568-1-srini.raju@purelifi.com>
 <20211018100143.7565-1-srini.raju@purelifi.com>
 <20211018100143.7565-3-srini.raju@purelifi.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211018100143.7565-3-srini.raju@purelifi.com>
Precedence: bulk

On Mon, Oct 18, 2021 at 11:00:55AM +0100, Srinivasan Raju wrote:
> This driver implementation has been based on the zd1211rw driver
> 
> Driver is based on 802.11 softMAC Architecture and uses
> native 802.11 for configuration and management
> 
> The driver is compiled and tested in ARM, x86 architectures and
> compiled in powerpc architecture

Just small style issues in this review.

> Signed-off-by: Srinivasan Raju <srini.raju@purelifi.com>
> ---
>  MAINTAINERS                                   |   6 +
>  drivers/net/wireless/Kconfig                  |   1 +
>  drivers/net/wireless/Makefile                 |   1 +
>  drivers/net/wireless/purelifi/Kconfig         |  17 +
>  drivers/net/wireless/purelifi/Makefile        |   2 +
>  drivers/net/wireless/purelifi/plfxlc/Kconfig  |  14 +
>  drivers/net/wireless/purelifi/plfxlc/Makefile |   3 +
>  drivers/net/wireless/purelifi/plfxlc/chip.c   |  95 ++
>  drivers/net/wireless/purelifi/plfxlc/chip.h   |  89 ++
>  .../net/wireless/purelifi/plfxlc/firmware.c   | 275 +++++
>  drivers/net/wireless/purelifi/plfxlc/intf.h   |  52 +
>  drivers/net/wireless/purelifi/plfxlc/mac.c    | 770 ++++++++++++++
>  drivers/net/wireless/purelifi/plfxlc/mac.h    | 190 ++++
>  drivers/net/wireless/purelifi/plfxlc/usb.c    | 975 ++++++++++++++++++
>  drivers/net/wireless/purelifi/plfxlc/usb.h    | 196 ++++
>  15 files changed, 2686 insertions(+)
>  create mode 100644 drivers/net/wireless/purelifi/Kconfig
>  create mode 100644 drivers/net/wireless/purelifi/Makefile
>  create mode 100644 drivers/net/wireless/purelifi/plfxlc/Kconfig
>  create mode 100644 drivers/net/wireless/purelifi/plfxlc/Makefile
>  create mode 100644 drivers/net/wireless/purelifi/plfxlc/chip.c
>  create mode 100644 drivers/net/wireless/purelifi/plfxlc/chip.h
>  create mode 100644 drivers/net/wireless/purelifi/plfxlc/firmware.c
>  create mode 100644 drivers/net/wireless/purelifi/plfxlc/intf.h
>  create mode 100644 drivers/net/wireless/purelifi/plfxlc/mac.c
>  create mode 100644 drivers/net/wireless/purelifi/plfxlc/mac.h
>  create mode 100644 drivers/net/wireless/purelifi/plfxlc/usb.c
>  create mode 100644 drivers/net/wireless/purelifi/plfxlc/usb.h


> diff --git a/drivers/net/wireless/purelifi/plfxlc/chip.h b/drivers/net/wireless/purelifi/plfxlc/chip.h
> new file mode 100644
> index 000000000000..167e9b9bd7a7
> --- /dev/null
> +++ b/drivers/net/wireless/purelifi/plfxlc/chip.h
> @@ -0,0 +1,89 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2021 pureLiFi
> + */
> +
> +#ifndef _LF_X_CHIP_H
> +#define _LF_X_CHIP_H
> +
> +#include <net/mac80211.h>
> +
> +#include "usb.h"
> +
> +enum unit_type {
> +	STA = 0,
> +	AP = 1,
> +};
> +
> +enum {
> +	PLFXLC_RADIO_OFF = 0,
> +	PLFXLC_RADIO_ON = 1,
> +};
> +
> +struct purelifi_chip {
> +	struct purelifi_usb usb;
> +	struct mutex mutex; /* lock to protect chip data */
> +	enum unit_type unit_type;
> +	u16 link_led;
> +	u8 beacon_set;
> +	__le16 beacon_interval;

Does seem little wierd that one variable is __le16. You could change
this u16 because there will not be anymore conversion either way.

> +};

> +#endif /* _LF_X_CHIP_H */
> diff --git a/drivers/net/wireless/purelifi/plfxlc/firmware.c b/drivers/net/wireless/purelifi/plfxlc/firmware.c
> new file mode 100644
> index 000000000000..30a8a63e13b6
> --- /dev/null
> +++ b/drivers/net/wireless/purelifi/plfxlc/firmware.c

> +
> +int download_fpga(struct usb_interface *intf)
> +{
> +	int r, actual_length;
> +	int fw_data_i, blk_tran_len = PLF_BULK_TLEN;
> +	const char *fw_name;
> +	unsigned char *fpga_dmabuff = NULL;
> +	unsigned char *fw_data;
> +	const struct firmware *fw = NULL;
> +	struct usb_device *udev = interface_to_usbdev(intf);

Can you please make something for function variables. They seems totally
random. If I am correct net/ and drivers/net want's reverse christmas
tree format. 

> +int download_xl_firmware(struct usb_interface *intf)
> +{
> +	const struct firmware *fwp = NULL;
> +	struct firmware_file file = {0};
> +	int s, r;
> +	u8 *buf;
> +	u32 i;
> +	const char *fw_pack;
> +	struct usb_device *udev = interface_to_usbdev(intf);
> +
> +	r = send_vendor_command(udev, PLF_VNDR_XL_FW_CMD, NULL, 0);
> +	msleep(PLF_MSLEEP_TIME);
> +
> +	if (r) {
> +		dev_err(&intf->dev, "vendor command failed (%d)\n", r);
> +		return -EINVAL;
> +	}
> +	/* Code for single pack file download */
> +
> +	fw_pack = "plfxlc/lifi-xl.bin";
> +
> +	r = request_firmware(&fwp, fw_pack, &intf->dev);
> +	if (r) {
> +		dev_err(&intf->dev, "Request_firmware failed (%d)\n", r);
> +		return -EINVAL;
> +	}
> +	file.total_files = get_unaligned_le32(&fwp->data[0]);
> +	file.total_size = get_unaligned_le32(&fwp->size);
> +
> +	dev_dbg(&intf->dev, "XL Firmware (%d, %d)\n",
> +		file.total_files, file.total_size);
> +
> +	buf = kzalloc(PLF_XL_BUF_LEN, GFP_KERNEL);
> +	if (!buf) {
> +		release_firmware(fwp);
> +		return -ENOMEM;
> +	}
> +
> +	if (file.total_files > 10) {
> +		dev_err(&intf->dev, "Too many files (%d)\n", file.total_files);
> +		release_firmware(fwp);
> +		kfree(buf);
> +		return -EINVAL;
> +	}
> +
> +	/* Download firmware files in multiple steps */
> +	for (s = 0; s < file.total_files; s++) {
> +		buf[0] = s;
> +		r = send_vendor_command(udev, PLF_VNDR_XL_FILE_CMD, buf,
> +					PLF_XL_BUF_LEN);
> +
> +		if (s < file.total_files - 1)
> +			file.size = get_unaligned_le32(&fwp->data[4 + ((s + 1) * 4)])
> +				    - get_unaligned_le32(&fwp->data[4 + (s) * 4]);
> +		else
> +			file.size = file.total_size -
> +				    get_unaligned_le32(&fwp->data[4 + (s) * 4]);
> +
> +		if (file.size > file.total_size || file.size > 60000) {
> +			dev_err(&intf->dev, "File size is too large (%d)\n", file.size);
> +			break;
> +		}
> +
> +		file.start_addr = get_unaligned_le32(&fwp->data[4 + (s * 4)]);
> +
> +		if (file.size % PLF_XL_BUF_LEN && s < 2)
> +			file.size += PLF_XL_BUF_LEN - file.size % PLF_XL_BUF_LEN;
> +
> +		file.control_packets = file.size / PLF_XL_BUF_LEN;
> +
> +		for (i = 0; i < file.control_packets; i++) {
> +			memcpy(buf,
> +			       &fwp->data[file.start_addr + (i * PLF_XL_BUF_LEN)],
> +			       PLF_XL_BUF_LEN);
> +			r = send_vendor_command(udev, PLF_VNDR_XL_DATA_CMD, buf,
> +						PLF_XL_BUF_LEN);
> +		}
> +		dev_dbg(&intf->dev, "fw-dw step=%d,r=%d size=%d\n", s, r,
> +			file.size);
> +	}
> +	release_firmware(fwp);
> +	kfree(buf);
> +
> +	/* Code for single pack file download ends fw download finish*/

space after finish

> +
> +	r = send_vendor_command(udev, PLF_VNDR_XL_EX_CMD, NULL, 0);
> +	dev_dbg(&intf->dev, "Download fpga (4) (%d)\n", r);
> +
> +	return 0;
> +}

> diff --git a/drivers/net/wireless/purelifi/plfxlc/mac.c b/drivers/net/wireless/purelifi/plfxlc/mac.c
> new file mode 100644
> index 000000000000..6e403d5ac84c
> --- /dev/null
> +++ b/drivers/net/wireless/purelifi/plfxlc/mac.c


> +int purelifi_restore_settings(struct purelifi_mac *mac)
> +{
> +	struct sk_buff *beacon;
> +	int beacon_interval, beacon_period;
> +
> +	spin_lock_irq(&mac->lock);
> +	beacon_interval = mac->beacon.interval;
> +	beacon_period = mac->beacon.period;
> +	spin_unlock_irq(&mac->lock);
> +
> +	if (mac->type != NL80211_IFTYPE_ADHOC)
> +		return 0;
> +
> +	if (mac->vif) {
> +		beacon = ieee80211_beacon_get(mac->hw, mac->vif);
> +		if (beacon) {
> +			purelifi_mac_config_beacon(mac->hw, beacon);
> +			kfree_skb(beacon);
> +			/* Returned skb is used only once and lowlevel
> +			 *  driver is responsible for freeing it.

after * there is extre space.

> +			 */
> +		}
> +	}
> +
> +	purelifi_set_beacon_interval(&mac->chip, beacon_interval,
> +				     beacon_period, mac->type);
> +
> +	spin_lock_irq(&mac->lock);
> +	mac->beacon.last_update = jiffies;
> +	spin_unlock_irq(&mac->lock);
> +
> +	return 0;
> +}

> +void purelifi_mac_tx_to_dev(struct sk_buff *skb, int error)
> +{
> +	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> +	struct ieee80211_hw *hw = info->rate_driver_data[0];
> +	struct purelifi_mac *mac = purelifi_hw_mac(hw);
> +
> +	ieee80211_tx_info_clear_status(info);
> +	skb_pull(skb, sizeof(struct purelifi_ctrlset));
> +
> +	if (unlikely(error ||
> +		     (info->flags & IEEE80211_TX_CTL_NO_ACK))) {
> +		ieee80211_tx_status_irqsafe(hw, skb);

If this is unlikely make this  return;

> +	} else {

and delete this else so you get lower indention level.

> +		struct sk_buff_head *q = &mac->ack_wait_queue;
> +
> +		skb_queue_tail(q, skb);
> +		while (skb_queue_len(q)/* > PURELIFI_MAC_MAX_ACK_WAITERS*/) {
> +			purelifi_mac_tx_status(hw, skb_dequeue(q),
> +					       mac->ack_pending ?
> +					       mac->ack_signal : 0,
> +					       NULL);
> +			mac->ack_pending = 0;
> +		}
> +	}
> +}
> +
> +static int purelifi_mac_config_beacon(struct ieee80211_hw *hw,
> +				      struct sk_buff *beacon)
> +{
> +	return plf_usb_wreq(beacon->data, beacon->len,
> +			USB_REQ_BEACON_WR);
> +}
> +
> +static int fill_ctrlset(struct purelifi_mac *mac, struct sk_buff *skb)

Should this also have prefix purelifi.

> +{
> +	unsigned int frag_len = skb->len;
> +	unsigned int tmp;
> +	u32 temp_len = 0;
> +	u32 temp_payload_len = 0;
> +	struct purelifi_ctrlset *cs;
> +
> +	if (skb_headroom(skb) < sizeof(struct purelifi_ctrlset)) {
> +		dev_dbg(purelifi_mac_dev(mac), "Not enough hroom(1)\n");
> +		return 1;
> +	}
> +
> +	cs = (void *)skb_push(skb, sizeof(struct purelifi_ctrlset));
> +	temp_payload_len = frag_len;
> +	temp_len = temp_payload_len +
> +		  sizeof(struct purelifi_ctrlset) -
> +		  sizeof(cs->id) - sizeof(cs->len);
> +
> +	/* data packet lengths must be multiple of four bytes
> +	 * and must not be a multiple of 512
> +	 * bytes. First, it is attempted to append the
> +	 * data packet in the tailroom of the skb. In rare
> +	 * ocasions, the tailroom is too small. In this case,
> +	 * the content of the packet is shifted into
> +	 * the headroom of the skb by memcpy. Headroom is allocated
> +	 * at startup (below in this file). Therefore,
> +	 * there will be always enough headroom. The call skb_headroom
> +	 * is an additional safety which might be
> +	 * dropped.
> +	 */

This is not wrapped correctly. Should be like:

	/* Data packet lengths must be multiple of four bytes and must
	 * not be a multiple of 512 bytes. First, it is attempted to
	 * append the data packet in the tailroom of the skb. In rare
	 * ocasions, the tailroom is too small. In this case, the
	 * content of the packet is shifted into the headroom of the skb
	 * by memcpy. Headroom is allocated at startup (below in this
	 * file). Therefore, there will be always enough headroom. The
	 * call skb_headroom is an additional safety which might be
	 * dropped.
 	 */

> +
> +	/* check if 32 bit aligned and align data */
> +	tmp = skb->len & 3;
> +	if (tmp) {
> +		if (skb_tailroom(skb) < (3 - tmp)) {
> +			if (skb_headroom(skb) >= 4 - tmp) {
> +				u8 len;
> +				u8 *src_pt;
> +				u8 *dest_pt;
> +
> +				len = skb->len;
> +				src_pt = skb->data;
> +				dest_pt = skb_push(skb, 4 - tmp);
> +				memmove(dest_pt, src_pt, len);
> +			} else {
> +				return -ENOBUFS;
> +			}
> +		} else {
> +			skb_put(skb, 4 - tmp);
> +		}
> +		temp_len += 4 - tmp;
> +	}
> +
> +	/* check if not multiple of 512 and align data */
> +	tmp = skb->len & 0x1ff;
> +	if (!tmp) {
> +		if (skb_tailroom(skb) < 4) {
> +			if (skb_headroom(skb) >= 4) {
> +				u8 len = skb->len;
> +				u8 *src_pt = skb->data;
> +				u8 *dest_pt = skb_push(skb, 4);
> +
> +				memcpy(dest_pt, src_pt, len);

I notice that Joe has said that you should use memmove in first version.
Just asking if this can overlap. If you are not sure memmove is safer.

> +			} else {
> +				/* should never happen because
> +				 * sufficient headroom was reserved
> +				 */
> +				return -ENOBUFS;
> +			}
> +		} else {
> +			skb_put(skb, 4);
> +		}
> +		temp_len += 4;
> +	}

You use almoust same thing two times row and it is quite long. Maybe
do new inline function.

> +
> +	cs->id = cpu_to_be32(USB_REQ_DATA_TX);
> +	cs->len = cpu_to_be32(temp_len);
> +	cs->payload_len_nw = cpu_to_be32(temp_payload_len);
> +
> +	return 0;
> +}
> +
> +static void plfxlc_op_tx(struct ieee80211_hw *hw,
> +			 struct ieee80211_tx_control *control,
> +			 struct sk_buff *skb)
> +{
> +	struct purelifi_mac *mac = purelifi_hw_mac(hw);
> +	struct purelifi_usb *usb = &mac->chip.usb;
> +	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> +	struct purelifi_header *plhdr = (void *)skb->data;
> +	unsigned long flags;
> +	int r;
> +
> +	r = fill_ctrlset(mac, skb);
> +	if (r)
> +		goto fail;
> +
> +	info->rate_driver_data[0] = hw;
> +
> +	if (plhdr->frametype  == IEEE80211_FTYPE_DATA) {
> +		u8 *dst_mac = plhdr->dmac;
> +		u8 sidx;
> +		bool found = false;
> +		struct purelifi_usb_tx *tx = &usb->tx;
> +
> +		for (sidx = 0; sidx < MAX_STA_NUM; sidx++) {
> +			if (!(tx->station[sidx].flag & STATION_CONNECTED_FLAG))
> +				continue;
> +			if (!memcmp(tx->station[sidx].mac, dst_mac, ETH_ALEN)) {
> +				found = true;
> +				break;
> +			}

			if (memcmp(tx->station[sidx].mac, dst_mac, ETH_ALEN))
 				continue;

			found = true;
			break;

> +		}
> +
> +		/* Default to broadcast address for unknown MACs */
> +		if (!found)
> +			sidx = STA_BROADCAST_INDEX;
> +
> +		/* Stop OS from sending packets, if the queue is half full */
> +		if (skb_queue_len(&tx->station[sidx].data_list) > 60)
> +			ieee80211_stop_queues(purelifi_usb_to_hw(usb));
> +
> +		/* Schedule packet for transmission if queue is not full */
> +		if (skb_queue_len(&tx->station[sidx].data_list) < 256) {
> +			skb_queue_tail(&tx->station[sidx].data_list, skb);
> +			purelifi_send_packet_from_data_queue(usb);
> +		} else {
> +			goto fail;
> +		}

Reverse if statment and goto from there. Then no need for else.

> +	} else {
> +		spin_lock_irqsave(&usb->tx.lock, flags);
> +		r = plf_usb_wreq_async(&mac->chip.usb, skb->data, skb->len,
> +				       USB_REQ_DATA_TX, tx_urb_complete, skb);
> +		spin_unlock_irqrestore(&usb->tx.lock, flags);
> +		if (r)
> +			goto fail;
> +	}
> +	return;
> +
> +fail:
> +	dev_kfree_skb(skb);
> +}
> +
> +static int purelifi_filter_ack(struct ieee80211_hw *hw, struct ieee80211_hdr *rx_hdr,
> +			       struct ieee80211_rx_status *stats)
> +{
> +	struct purelifi_mac *mac = purelifi_hw_mac(hw);
> +	struct sk_buff *skb;
> +	struct sk_buff_head *q;
> +	unsigned long flags;
> +	bool found = 0;

bool is nicer if used with true/false.

> +	int i, position = 0;
> +
> +	if (!ieee80211_is_ack(rx_hdr->frame_control))
> +		return 0;
> +
> +	dev_dbg(purelifi_mac_dev(mac), "ACK Received\n");
> +
> +	/* code based on zy driver, this logic may need fix */
> +	q = &mac->ack_wait_queue;
> +	spin_lock_irqsave(&q->lock, flags);
> +
> +	skb_queue_walk(q, skb) {
> +		struct ieee80211_hdr *tx_hdr;
> +
> +		position++;
> +
> +		if (mac->ack_pending && skb_queue_is_first(q, skb))
> +			continue;
> +		else if (mac->ack_pending == 0)

if is enogh

> +			break;
> +
> +		tx_hdr = (struct ieee80211_hdr *)skb->data;
> +		if (likely(ether_addr_equal(tx_hdr->addr2, rx_hdr->addr1))) {
> +			found = 1;
> +			break;
> +		}
> +	}
> +
> +	if (found) {
> +		for (i = 1; i < position; i++)
> +			skb = __skb_dequeue(q);
> +		if (i == position) {
> +			purelifi_mac_tx_status(hw, skb,
> +					       mac->ack_pending ?
> +					       mac->ack_signal : 0,
> +					       NULL);
> +			mac->ack_pending = 0;
> +		}
> +
> +		mac->ack_pending = skb_queue_len(q) ? 1 : 0;
> +		mac->ack_signal = stats->signal;
> +	}
> +
> +	spin_unlock_irqrestore(&q->lock, flags);
> +	return 1;
> +}
> +
> +int purelifi_mac_rx(struct ieee80211_hw *hw, const u8 *buffer,
> +		    unsigned int length)
> +{
> +	struct purelifi_mac *mac = purelifi_hw_mac(hw);
> +	struct ieee80211_rx_status stats;
> +	const struct rx_status *status;
> +	struct sk_buff *skb;
> +	int bad_frame = 0;
> +	__le16 fc;
> +	int need_padding;
> +	unsigned int payload_length;
> +	int sidx;
> +	struct purelifi_usb_tx *tx;

Add space

> +	/* Packet blockade during disabled interface. */
> +	if (!mac->vif)
> +		return 0;
> +
> +	memset(&stats, 0, sizeof(stats));
> +	status = (struct rx_status *)buffer;
> +
> +	stats.flag     = 0;
> +	stats.freq     = 2412;
> +	stats.band     = NL80211_BAND_LC;
> +	mac->rssi      = -15 * be16_to_cpu(status->rssi) / 10;
> +
> +	stats.signal   = mac->rssi;
> +
> +	if (status->rate_idx > 7)
> +		stats.rate_idx = 0;
> +	else
> +		stats.rate_idx = status->rate_idx;
> +
> +	mac->crc_errors = be64_to_cpu(status->crc_error_count);
> +
> +	if (!bad_frame &&
> +	    purelifi_filter_ack(hw, (struct ieee80211_hdr *)buffer, &stats) &&
> +	    !mac->pass_ctrl)
> +		return 0;
> +
> +	buffer += sizeof(struct rx_status);
> +	payload_length = get_unaligned_be32(buffer);
> +
> +	/* MTU = 1500, MAC header = 36, CRC = 4, sum = 1540 */

Seems little confusing that you write sum = 1540 and next line you use
1560.

> +	if (payload_length > 1560) {
> +		dev_err(purelifi_mac_dev(mac), " > MTU %u\n", payload_length);
> +		return 0;
> +	}
> +	buffer += sizeof(u32);
> +
> +	fc = get_unaligned((__le16 *)buffer);
> +	need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);
> +
> +	tx = &mac->chip.usb.tx;
> +
> +	for (sidx = 0; sidx < MAX_STA_NUM - 1; sidx++) {
> +		if (memcmp(&buffer[10], tx->station[sidx].mac, ETH_ALEN))
> +			continue;
> +		if (tx->station[sidx].flag & STATION_CONNECTED_FLAG) {
> +			tx->station[sidx].flag |= STATION_HEARTBEAT_FLAG;
> +			break;
> +		}
> +	}
> +
> +	if (sidx == MAX_STA_NUM - 1) {
> +		for (sidx = 0; sidx < MAX_STA_NUM - 1; sidx++) {
> +			if (tx->station[sidx].flag & STATION_CONNECTED_FLAG)
> +				continue;
> +			memcpy(tx->station[sidx].mac, &buffer[10], ETH_ALEN);
> +			tx->station[sidx].flag |= STATION_CONNECTED_FLAG;
> +			tx->station[sidx].flag |= STATION_HEARTBEAT_FLAG;
> +			break;
> +		}
> +	}
> +
> +	switch (buffer[0]) {
> +	case IEEE80211_STYPE_PROBE_REQ:
> +		dev_dbg(purelifi_mac_dev(mac), "Probe request\n");
> +		break;
> +	case IEEE80211_STYPE_ASSOC_REQ:
> +		dev_dbg(purelifi_mac_dev(mac), "Association request\n");
> +		break;
> +	case IEEE80211_STYPE_AUTH:
> +		dev_dbg(purelifi_mac_dev(mac), "Authentication req\n");
> +		break;
> +	case IEEE80211_FTYPE_DATA:
> +		dev_dbg(purelifi_mac_dev(mac), "802.11 data frame\n");
> +		break;
> +	}
> +
> +	skb = dev_alloc_skb(payload_length + (need_padding ? 2 : 0));
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	if (need_padding)
> +		/* Make sure that the payload data is 4 byte aligned. */
> +		skb_reserve(skb, 2);
> +
> +	skb_put_data(skb, buffer, payload_length);
> +	memcpy(IEEE80211_SKB_RXCB(skb), &stats, sizeof(stats));
> +	ieee80211_rx_irqsafe(hw, skb);
> +	return 0;
> +}
> +
> +static int plfxlc_op_add_interface(struct ieee80211_hw *hw,
> +				   struct ieee80211_vif *vif)
> +{
> +	struct purelifi_mac *mac = purelifi_hw_mac(hw);
> +	static const char * const iftype80211[] = {
> +		[NL80211_IFTYPE_STATION]	= "Station",
> +		[NL80211_IFTYPE_ADHOC]		= "Adhoc"
> +	};
> +
> +	if (mac->type != NL80211_IFTYPE_UNSPECIFIED)
> +		return -EOPNOTSUPP;
> +
> +	if (vif->type == NL80211_IFTYPE_ADHOC ||
> +	    vif->type == NL80211_IFTYPE_STATION) {
> +		dev_dbg(purelifi_mac_dev(mac), "%s %s\n", __func__,
> +			iftype80211[vif->type]);
> +		mac->type = vif->type;
> +		mac->vif = vif;
> +	} else {
> +		dev_dbg(purelifi_mac_dev(mac), "unsupported iftype\n");
> +		return -EOPNOTSUPP;
> +	}

Reverse if so no else needed.

> +
> +	return 0;
> +}

> +static void plfxlc_op_bss_info_changed(struct ieee80211_hw *hw,
> +				       struct ieee80211_vif *vif,
> +				       struct ieee80211_bss_conf *bss_conf,
> +				       u32 changes)
> +{
> +	struct purelifi_mac *mac = purelifi_hw_mac(hw);
> +	int associated;
> +
> +	dev_dbg(purelifi_mac_dev(mac), "changes: %x\n", changes);
> +
> +	if (mac->type != NL80211_IFTYPE_ADHOC) { /* for STATION */
> +		associated = is_valid_ether_addr(bss_conf->bssid);
> +		goto exit_all;
> +	}
> +	/* for ADHOC */
> +	associated = true;
> +	if (changes & BSS_CHANGED_BEACON) {
> +		struct sk_buff *beacon = ieee80211_beacon_get(hw, vif);
> +
> +		if (beacon) {
> +			purelifi_mac_config_beacon(hw, beacon);
> +			kfree_skb(beacon);
> +			/*Returned skb is used only once and

space after *

> +			 * low-level driver is
> +			 * responsible for freeing it.
> +			 */
> +		}
> +	}
> +
> +	if (changes & BSS_CHANGED_BEACON_ENABLED) {
> +		u16 interval = 0;
> +		u8 period = 0;
> +
> +		if (bss_conf->enable_beacon) {
> +			period = bss_conf->dtim_period;
> +			interval = bss_conf->beacon_int;
> +		}
> +
> +		spin_lock_irq(&mac->lock);
> +		mac->beacon.period = period;
> +		mac->beacon.interval = interval;
> +		mac->beacon.last_update = jiffies;
> +		spin_unlock_irq(&mac->lock);
> +
> +		purelifi_set_beacon_interval(&mac->chip, interval,
> +					     period, mac->type);
> +	}
> +exit_all:
> +	spin_lock_irq(&mac->lock);
> +	mac->associated = associated;
> +	spin_unlock_irq(&mac->lock);
> +}

> +static const struct ieee80211_ops plfxlc_ops = {
> +	.tx                 = plfxlc_op_tx,
> +	.start              = plfxlc_op_start,
> +	.stop               = plfxlc_op_stop,
> +	.add_interface      = plfxlc_op_add_interface,
> +	.remove_interface   = plfxlc_op_remove_interface,
> +	.set_rts_threshold  = purelifi_set_rts_threshold,
> +	.config             = plfxlc_op_config,
> +	.configure_filter   = plfxlc_op_configure_filter,
> +	.bss_info_changed   = plfxlc_op_bss_info_changed,
> +	.get_stats          = purelifi_get_stats,
> +	.get_et_sset_count  = purelifi_get_et_sset_count,
> +	.get_et_stats       = purelifi_get_et_stats,
> +	.get_et_strings     = purelifi_get_et_strings,
> +};

Just asking why some prefixes are purelifi and some are plfxlc?

> diff --git a/drivers/net/wireless/purelifi/plfxlc/mac.h b/drivers/net/wireless/purelifi/plfxlc/mac.h
> new file mode 100644
> index 000000000000..3852d7343a40
> --- /dev/null
> +++ b/drivers/net/wireless/purelifi/plfxlc/mac.h
> @@ -0,0 +1,190 @@

> +struct purelifi_mac {
> +	struct purelifi_chip chip;
> +	spinlock_t lock; /* lock for mac data */
> +	struct ieee80211_hw *hw;
> +	struct ieee80211_vif *vif;
> +	struct beacon beacon;
> +	struct work_struct set_rts_cts_work;
> +	struct work_struct process_intr;
> +	struct purelifi_mc_hash multicast_hash;
> +	u8 intr_buffer[USB_MAX_EP_INT_BUFFER];
> +	u8 regdomain;
> +	u8 default_regdomain;
> +	u8 channel;
> +	int type;
> +	int associated;
> +	unsigned long flags;
> +	struct sk_buff_head ack_wait_queue;
> +	struct ieee80211_channel channels[14];
> +	struct ieee80211_rate rates[12];
> +	struct ieee80211_supported_band band;

Maybe move these below other ieee8011_* structs. Will be muh more
readable. Right now this struct is just big mess to read.

> +
> +	/* whether to pass frames with CRC errors to stack */
> +	bool pass_failed_fcs;
> +
> +	/* whether to pass control frames to stack */
> +	bool pass_ctrl;
> +
> +	/* whether we have received a 802.11 ACK that is pending */
> +	bool ack_pending;
> +
> +	/* signal strength of the last 802.11 ACK received */
> +	int ack_signal;
> +
> +	unsigned char hw_address[ETH_ALEN];
> +	char serial_number[PURELIFI_SERIAL_LEN];
> +	u64 crc_errors;
> +	u64 rssi;
> +};

> diff --git a/drivers/net/wireless/purelifi/plfxlc/usb.c b/drivers/net/wireless/purelifi/plfxlc/usb.c
> new file mode 100644
> index 000000000000..8cd9f223025e
> --- /dev/null
> +++ b/drivers/net/wireless/purelifi/plfxlc/usb.c

> +void purelifi_send_packet_from_data_queue(struct purelifi_usb *usb)
> +{
> +	struct sk_buff *skb = NULL;
> +	unsigned long flags;
> +	static u8 sidx;

Static here, but I might already tell that in preview message, but here
for safety.

> +	u8 last_served_sidx;
> +	struct purelifi_usb_tx *tx = &usb->tx;
> +
> +	spin_lock_irqsave(&tx->lock, flags);
> +	last_served_sidx = sidx;
> +	do {
> +		sidx = (sidx + 1) % MAX_STA_NUM;
> +		if (!tx->station[sidx].flag & STATION_CONNECTED_FLAG)
> +			continue;
> +		if (!(tx->station[sidx].flag & STATION_FIFO_FULL_FLAG))
> +			skb = skb_peek(&tx->station[sidx].data_list);
> +	} while ((sidx != last_served_sidx) && (!skb));
> +
> +	if (skb) {
> +		skb = skb_dequeue(&tx->station[sidx].data_list);
> +		plf_usb_wreq_async(usb, skb->data, skb->len, USB_REQ_DATA_TX,
> +				   tx_urb_complete, skb);
> +		if (skb_queue_len(&tx->station[sidx].data_list) <= 60)
> +			ieee80211_wake_queues(purelifi_usb_to_hw(usb));
> +	}
> +	spin_unlock_irqrestore(&tx->lock, flags);
> +}

> +static void rx_urb_complete(struct urb *urb)
> +{
> +	int r;
> +	struct purelifi_usb *usb;
> +	struct purelifi_usb_tx *tx;
> +	const u8 *buffer;
> +	unsigned int length;
> +	u16 status;
> +	u8 sidx;
> +
> +	if (!urb) {
> +		pr_err("urb is NULL\n");
> +		return;
> +	} else if (!urb->context) {

Another if is as good as this.

> +		pr_err("urb ctx is NULL\n");
> +		return;
> +	}

> +int purelifi_usb_enable_rx(struct purelifi_usb *usb)
> +{
> +	int r;
> +	struct purelifi_usb_rx *rx = &usb->rx;
> +
> +	mutex_lock(&rx->setup_mutex);
> +	r = __lf_x_usb_enable_rx(usb);
> +	if (!r)
> +		usb->rx_usb_enabled = 1;

Use true.

> +
> +	mutex_unlock(&rx->setup_mutex);
> +
> +	return r;
> +}

> +int purelifi_usb_tx(struct purelifi_usb *usb, struct sk_buff *skb)
> +{
> +	int r;
> +	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> +	struct usb_device *udev = purelifi_usb_to_usbdev(usb);
> +	struct urb *urb;
> +	struct purelifi_usb_tx *tx = &usb->tx;
> +
> +	if (!test_bit(PLF_BIT_ENABLED, &tx->enabled)) {
> +		r = -ENOENT;
> +		goto out;
> +	}
> +
> +	urb = usb_alloc_urb(0, GFP_ATOMIC);
> +	if (!urb) {
> +		r = -ENOMEM;
> +		goto out;
> +	}
> +
> +	usb_fill_bulk_urb(urb, udev, usb_sndbulkpipe(udev, EP_DATA_OUT),
> +			  skb->data, skb->len, tx_urb_complete, skb);
> +
> +	info->rate_driver_data[1] = (void *)jiffies;
> +	skb_queue_tail(&tx->submitted_skbs, skb);
> +	usb_anchor_urb(urb, &tx->submitted);
> +
> +	r = usb_submit_urb(urb, GFP_ATOMIC);
> +	if (r) {
> +		dev_dbg(purelifi_usb_dev(usb), "urb %p submit failed (%d)\n",
> +			urb, r);
> +		usb_unanchor_urb(urb);
> +		skb_unlink(skb, &tx->submitted_skbs);
> +		goto error;
> +	}
> +	return 0;
> +error:
> +	usb_free_urb(urb);
> +out:
> +	return r;

Do need to error paths in this function. Just return when goto out and
you use error just in one place.

> +}

> +int plf_usb_wreq_async(struct purelifi_usb *usb, const u8 *buffer,
> +		       int buffer_len, enum plf_usb_req_enum usb_req_id,
> +		       usb_complete_t complete_fn,
> +		       void *context)
> +{
> +	int r;
> +	struct usb_device *udev = interface_to_usbdev(ez_usb_interface);
> +	struct urb *urb = usb_alloc_urb(0, GFP_ATOMIC);
> +
> +	usb_fill_bulk_urb(urb, udev, usb_sndbulkpipe(udev, EP_DATA_OUT),
> +			  (void *)buffer, buffer_len, complete_fn, context);
> +
> +	r = usb_submit_urb(urb, GFP_ATOMIC);
> +

Remove newline.

> +	if (r)
> +		dev_err(&udev->dev, "Async write submit failed (%d)\n", r);
> +
> +	return r;
> +}
> +
> +int plf_usb_wreq(void *buffer, int buffer_len,
> +		 enum plf_usb_req_enum usb_req_id)
> +{
> +	int r;
> +	int actual_length;
> +	int usb_bulk_msg_len;
> +	unsigned char *dma_buffer = NULL;
> +	struct usb_device *udev = interface_to_usbdev(ez_usb_interface);
> +	struct plf_usb_req usb_req;
> +
> +	get_usb_req(udev, buffer, buffer_len, usb_req_id, &usb_req);
> +	usb_bulk_msg_len = sizeof(__le32) + sizeof(__le32) +
> +			   be32_to_cpu(usb_req.len);
> +
> +	dma_buffer = kmemdup(&usb_req, usb_bulk_msg_len, GFP_KERNEL);
> +
> +	if (!dma_buffer) {
> +		r = -ENOMEM;
> +		goto error;
> +	}
> +
> +	r = usb_bulk_msg(udev,
> +			 usb_sndbulkpipe(udev, EP_DATA_OUT),
> +			 dma_buffer, usb_bulk_msg_len,
> +			 &actual_length, USB_BULK_MSG_TIMEOUT_MS);
> +	kfree(dma_buffer);
> +error:
> +	if (r)
> +		dev_err(&udev->dev, "usb_bulk_msg failed (%d)\n", r);

Can also be enomem. So maybe return -ENOMEM in upper.

> +
> +	return r;
> +}

> +static int probe(struct usb_interface *intf,
> +		 const struct usb_device_id *id)
> +{
> +	int r = 0;
> +	struct purelifi_chip *chip;
> +	struct purelifi_usb *usb;
> +	struct purelifi_usb_tx *tx;
> +	struct ieee80211_hw *hw = NULL;
> +	u8 hw_address[ETH_ALEN];
> +	u8 serial_number[PURELIFI_SERIAL_LEN];
> +	unsigned int i;
> +
> +	ez_usb_interface = intf;
> +
> +	hw = purelifi_mac_alloc_hw(intf);
> +

Remove newline

> +	if (!hw) {
> +		r = -ENOMEM;
> +		goto error;

Just return straight away. This should not need message.

> +	}
> +
> +	chip = &purelifi_hw_mac(hw)->chip;
> +	usb = &chip->usb;
> +	tx = &usb->tx;
> +
> +	r = upload_mac_and_serial(intf, hw_address, serial_number);
> +

Remove newline.

<snip>

> +	timer_setup(&usb->sta_queue_cleanup,
> +		    sta_queue_cleanup_timer_callb, 0);
> +	usb->sta_queue_cleanup.expires = jiffies + STA_QUEUE_CLEANUP_JIFF;
> +	add_timer(&usb->sta_queue_cleanup);
> +
> +	usb->initialized = 1;

true

> +	return 0;
> +error:
> +	if (hw) {
> +		purelifi_mac_release(purelifi_hw_mac(hw));
> +		ieee80211_unregister_hw(hw);
> +		ieee80211_free_hw(hw);
> +	}
> +	dev_err(&intf->dev, "pureLifi:Device error");
> +	return r;
> +}
> +
> +static void disconnect(struct usb_interface *intf)
> +{
> +	struct ieee80211_hw *hw = purelifi_intf_to_hw(intf);
> +	struct purelifi_mac *mac;
> +	struct purelifi_usb *usb;
> +
> +	/* Either something really bad happened, or
> +	 * we're just dealing with
> +	 * a DEVICE_INSTALLER.
> +	 */

wrapping

> +	if (!hw)
> +		return;
> +
> +	mac = purelifi_hw_mac(hw);
> +	usb = &mac->chip.usb;
> +
> +	del_timer_sync(&usb->tx.tx_retry_timer);
> +	del_timer_sync(&usb->sta_queue_cleanup);
> +
> +	ieee80211_unregister_hw(hw);
> +
> +	purelifi_usb_disable_tx(usb);
> +	purelifi_usb_disable_rx(usb);
> +
> +	/* If the disconnect has been caused by a removal of the
> +	 * driver module, the reset allows reloading of the driver. If the
> +	 * reset will not be executed here,
> +	 * the upload of the firmware in the
> +	 * probe function caused by the reloading of the driver will fail.
> +	 */

ditto

> +	usb_reset_device(interface_to_usbdev(intf));
> +
> +	purelifi_mac_release(mac);
> +	ieee80211_free_hw(hw);
> +}

> +static struct purelifi_usb *get_purelifi_usb(struct usb_interface *intf)
> +{
> +	struct ieee80211_hw *hw = purelifi_intf_to_hw(intf);
> +	struct purelifi_mac *mac;
> +
> +	/* Either something really bad happened, or
> +	 * we're just dealing with
> +	 * a DEVICE_INSTALLER.
> +	 */

wrapping

  Argillander