Re: [PATCH] [v15] wireless: Initial driver submission for pureLiFi STA devices

Srinivasan Raju [off-list ref] writes:

This introduces the pureLiFi LiFi driver for LiFi-X, LiFi-XC
and LiFi-XL USB devices.

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.

Signed-off-by: Srinivasan Raju <srini.raju@purelifi.com>

[...]

+int purelifi_set_beacon_interval(struct purelifi_chip *chip, u16 interval,
+				 u8 dtim_period, int type)
+{
+	if (!interval ||
+	    (chip->beacon_set &&
+	     le16_to_cpu(chip->beacon_interval) == interval))
+		return 0;
+
+	chip->beacon_interval = cpu_to_le16(interval);
+	chip->beacon_set = true;
+	return plf_usb_wreq((const u8 *)&chip->beacon_interval,
+			     sizeof(chip->beacon_interval),
+			     USB_REQ_BEACON_INTERVAL_WR);

Can't you make plf_usb_wreq() to a void pointer? Then that ugly "const
u8 *" cast is not needed.

unit_type_t -> unit_type

+struct purelifi_chip {
+	struct purelifi_usb usb;
+	struct mutex mutex; /* lock to protect chip data */
+	enum unit_type_t unit_type;
+	u16 link_led;
+	u8  beacon_set;

Extra space after u8.

+int download_fpga(struct usb_interface *intf)
+{
+#define PLF_VNDR_FPGA_STATE_REQ 0x30
+#define PLF_VNDR_FPGA_SET_REQ 0x33
+#define PLF_VNDR_FPGA_SET_CMD 0x34
+#define PLF_VNDR_FPGA_STATE_CMD 0x35

Please move these to a .h file.

+	kfree(fpga_dmabuff);/* free PLF_FPGA_STATUS_LEN*/

Pointless comment.

+	fpga_dmabuff = NULL;
+	fpga_dmabuff = kmalloc(PLF_FPGA_STATE_LEN, GFP_KERNEL);

No need to assign fpga_dmabuff to NULL.

+int download_xl_firmware(struct usb_interface *intf)
+{
+#define PLF_VNDR_XL_FW_CMD 0x80
+#define PLF_VNDR_XL_DATA_CMD 0x81
+#define PLF_VNDR_XL_FILE_CMD 0x82
+#define PLF_VNDR_XL_EX_CMD 0x83

Please move these to a .h file.

+struct flash_t {
+	unsigned char enabled;
+	unsigned int sector_size;
+	unsigned int sectors;
+	unsigned char ec;
+};

Remove all _t suffixes in names.

+int upload_mac_and_serial(struct usb_interface *intf,
+			  unsigned char *hw_address,
+			  unsigned char *serial_number)
+{
+#define PLF_MAC_VENDOR_REQUEST 0x36
+#define PLF_SERIAL_NUMBER_VENDOR_REQUEST 0x37
+#define PLF_FIRMWARE_VERSION_VENDOR_REQUEST 0x39
+#define PLF_SERIAL_LEN 14
+#define PLF_FW_VER_LEN 8

Move to .h file.

+void purelifi_block_queue(struct purelifi_usb *usb, bool block)
+{
+	if (block)
+		ieee80211_stop_queues(purelifi_usb_to_hw(usb));
+	else
+		ieee80211_wake_queues(purelifi_usb_to_hw(usb));
+}

Looks like a useless abstraction to me, just use ieee80211_stop_queues()
and ieee80211_wake_queues() directly.

+/**
+ * purelifi_mac_tx_status: reports tx status of a packet if required
+ * @hw: a &struct ieee80211_hw pointer
+ * @skb: a sk-buffer
+ * @flags: extra flags to set in the TX status info
+ * @ackssi: ACK signal strength
+ * @success: True for successful transmission of the frame
+ *
+ * This information calls ieee80211_tx_status_irqsafe() if required by the
+ * control information. It copies the control information into the status
+ * information.
+ *
+ */

I'm against this kind of documentation in wireless drivers, they get out
of date so easily.

+	/*check if 32 bit aligned and align data*/

Correct format is:

/* check if 32 bit aligned and align data */

+	/* check if not multiple of 512 and align data*/

/* check if not multiple of 512 and align data */

+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;
+	static unsigned short int min_exp_seq_nmb;

No static variables, "static const" are only exceptions. Otherwise two
devices on the same host won't work correctly.

+	switch (buffer[0]) {
+	unsigned short int seq_nmb;

Please move the variable declaration to the beginning of the function.

+	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");
+		min_exp_seq_nmb = 0;
+		break;
+	case IEEE80211_FTYPE_DATA:
+		seq_nmb = (buffer[23] << 4) | ((buffer[22] & 0xF0) >> 4);

No magic numbers, please.

+		if (seq_nmb < min_exp_seq_nmb &&
+		    ((min_exp_seq_nmb - seq_nmb) < 3000))
+			dev_dbg(purelifi_mac_dev(mac), "seq_nmb < min_exp\n");
+		else
+			min_exp_seq_nmb = (seq_nmb + 1) % 4096;
+		break;

Here are also 3000 and 4096.

+static const struct ieee80211_ops purelifi_ops = {
+	.tx                 = purelifi_op_tx,
+	.start              = purelifi_op_start,
+	.stop               = purelifi_op_stop,
+	.add_interface      = purelifi_op_add_interface,
+	.remove_interface   = purelifi_op_remove_interface,
+	.set_rts_threshold  = purelifi_set_rts_threshold,
+	.config             = purelifi_op_config,
+	.configure_filter   = purelifi_op_configure_filter,
+	.bss_info_changed   = purelifi_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,

AFAICS, purelife is the vendor and plfxlc is the driver, so you should
"plfxlc_" as the prefix here.

+	hw->extra_tx_headroom = sizeof(struct purelifi_ctrlset) + 4;
+	/* 4 for 32 bit alignment if no tailroom */

Move the comment before the actual code.

+#define purelifi_mac_dev(mac) (purelifi_chip_dev(&(mac)->chip))

Isn't the parenthesis unnecessary?

+
+#define PURELIFI_MAC_STATS_BUFFER_SIZE 16
+#define PURELIFI_MAC_MAX_ACK_WAITERS 50
+
+struct purelifi_ctrlset {
+	__be32		id;/*should be usb_req_enum_t*/

Move the comment to a separate line, above this line. And fix the
comment format.

+	__be32		len;
+	u8              modulation;
+	u8              control;
+	u8              service;
+	u8		pad;
+	__le16		packet_length;
+	__le16		current_length;
+	__le16		next_frame_length;
+	__le16		tx_length;
+	__be32		payload_len_nw;
+} __packed;
+
+/*overlay*/

Fix comment format.

+	/* whether to pass frames with CRC errors to stack */
+	unsigned int pass_failed_fcs:1;
+
+	/* whether to pass control frames to stack */
+	unsigned int pass_ctrl:1;
+
+	/* whether we have received a 802.11 ACK that is pending */
+	bool ack_pending;

Inconsistent use of flags/booleans, either use unsigned int or bool.

+static atomic_t data_queue_flag;

No static variables.

+/*Tx retry backoff timer (in milliseconds).*/

I'm going to stop commenting about comment format now, you should know
by now.

+# define TX_RETRY_BACKOFF_MS 10
+# define STA_QUEUE_CLEANUP_MS 5000

No space after #. Applies to all usage in the driver.

+static struct usb_device_id usb_ids[] = {

static const

+	{ USB_DEVICE(PURELIFI_X_VENDOR_ID_0, PURELIFI_X_PRODUCT_ID_0),
+	  .driver_info = DEVICE_LIFI_X },
+	{ USB_DEVICE(PURELIFI_XC_VENDOR_ID_0, PURELIFI_XC_PRODUCT_ID_0),
+	  .driver_info = DEVICE_LIFI_XC },
+	{ USB_DEVICE(PURELIFI_XL_VENDOR_ID_0, PURELIFI_XL_PRODUCT_ID_0),
+	  .driver_info = DEVICE_LIFI_XL },
+	{}
+};
+
+static struct usb_interface *ez_usb_interface;

No static variables.

+#define STATION_FIFO_ALMOST_FULL_MESSAGE     0
+#define STATION_FIFO_ALMOST_FULL_NOT_MESSAGE 1
+#define STATION_CONNECT_MESSAGE              2
+#define STATION_DISCONNECT_MESSAGE           3

Move to a .h file.

+static void rx_urb_complete(struct urb *urb)
+{
+	int r;
+	static u8 retry;

No static variables.

+	struct purelifi_usb *usb;
+	struct purelifi_usb_tx *tx;
+	const u8 *buffer;
+	static u8 fpga_link_connection_f;

Ditto.

+	unsigned int length;
+	u16 status;
+	u8 sidx;
+
+	if (!urb) {
+		dev_err(purelifi_usb_dev(usb), "urb is NULL.\n");
+		return;
+	} else if (!urb->context) {
+		dev_err(purelifi_usb_dev(usb), "urb ctx is NULL.\n");
+		return;
+	}
+	usb = urb->context;
+
+	if (usb->initialized != 1)
+		return;
+
+	switch (urb->status) {
+	case 0:
+		break;
+	case -ESHUTDOWN:
+	case -EINVAL:
+	case -ENODEV:
+	case -ENOENT:
+	case -ECONNRESET:
+	case -EPIPE:
+		dev_dbg(urb_dev(urb), "urb %p error %d\n", urb, urb->status);
+		return;
+	default:
+		dev_dbg(urb_dev(urb), "urb %p error %d\n", urb, urb->status);
+		if (retry++ < PURELIFI_URB_RETRY_MAX) {
+			dev_dbg(urb_dev(urb), "urb %p resubmit %d", urb, retry);
+			goto resubmit;
+		} else {
+			dev_dbg(urb_dev(urb), "urb %p  max resubmits reached", urb);
+			retry = 0;
+			return;
+		}
+	}
+
+	buffer = urb->transfer_buffer;
+	length = (*(u32 *)(buffer + sizeof(struct rx_status)))

This does not look endian safe.

+		 + sizeof(u32);
+
+	tx = &usb->tx;
+
+	if (urb->actual_length != 8) {

Magic value.

+		if (usb->initialized && fpga_link_connection_f)
+			handle_rx_packet(usb, buffer, length);
+		goto resubmit;
+	}
+
+	status = buffer[7];

Magic value. A proper struct for these 8 bytes would be the best way to
document and access it.

+
+	dev_dbg(&usb->intf->dev, "Recv status=%u\n", status);
+	dev_dbg(&usb->intf->dev, "Tx packet MAC=%x:%x:%x:%x:%x:%x\n",
+		buffer[0], buffer[1], buffer[2], buffer[3],
+		buffer[4], buffer[5]);

Ok, so that struct should start with a mac address.

+
+	switch (status) {
+	case STATION_FIFO_ALMOST_FULL_NOT_MESSAGE:
+		dev_dbg(&usb->intf->dev,
+			"FIFO full not packet receipt\n");
+		tx->mac_fifo_full = 1;
+		for (sidx = 0; sidx < MAX_STA_NUM; sidx++)
+			tx->station[sidx].flag |= STATION_FIFO_FULL_FLAG;
+		break;
+	case STATION_FIFO_ALMOST_FULL_MESSAGE:
+		dev_dbg(&usb->intf->dev, "FIFO full packet receipt\n");
+
+		for (sidx = 0; sidx < MAX_STA_NUM; sidx++)
+			tx->station[sidx].flag &= 0xFD;

Magic value.

+
+		purelifi_send_packet_from_data_queue(usb);
+		break;
+	case STATION_CONNECT_MESSAGE:
+		fpga_link_connection_f = 1;

I remember if I said this already, but no "_f" style of naming. What
does it even mean, a flag?

+	for (i = 0; i < RX_URBS_COUNT; i++) {
+		r = usb_submit_urb(urbs[i], GFP_KERNEL);
+		if (r)
+			goto error_submit;
+	}
+
+	return 0; /*no error return*/

A useless comment.

+void purelifi_usb_disable_tx(struct purelifi_usb *usb)
+{
+	struct purelifi_usb_tx *tx = &usb->tx;
+	unsigned long flags;
+
+	atomic_set(&tx->enabled, 0);

I didn't check, but I suspect you are misusing atomic variables here.
Just use SET_BIT() & co.

+void tx_urb_complete(struct urb *urb)
+{
+	struct sk_buff *skb;
+	struct ieee80211_tx_info *info;
+	struct purelifi_usb *usb;
+
+	skb = (struct sk_buff *)urb->context;

urb->context is a void pointer, no need to cast it.

+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 (!atomic_read(&tx->enabled)) {

Yeah, just use TEST_BIT().

+void purelifi_usb_init(struct purelifi_usb *usb, struct ieee80211_hw *hw,
+		       struct usb_interface *intf)
+{
+	memset(usb, 0, sizeof(*usb));
+	usb->intf = usb_get_intf(intf);
+	usb_set_intfdata(usb->intf, hw);
+	hw->conf.chandef.width = NL80211_CHAN_WIDTH_20;

I was expecting to set the width in mac.c.

+static void get_usb_req(struct usb_device *udev, const u8 *buffer,
+			u32 buffer_len, enum usb_req_enum_t usb_req_id,
+			struct usb_req_t *usb_req)
+{
+	u8 *buffer_dst_p = usb_req->buf;

No "_p" naming either. Is that hungarian notation or what? Anyway, we
don't use that in upstream.

+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;
+	static u8 hw_address[ETH_ALEN];
+	static u8 serial_number[PURELIFI_SERIAL_LEN];

No static variables.

+	unsigned int i;
+
+	ez_usb_interface = intf;
+
+	hw = purelifi_mac_alloc_hw(intf);
+
+	if (!hw) {
+		r = -ENOMEM;
+		goto error;
+	}
+
+	chip = &purelifi_hw_mac(hw)->chip;
+	usb = &chip->usb;
+	tx = &usb->tx;
+
+	r = upload_mac_and_serial(intf, hw_address, serial_number);
+
+	if (r) {
+		dev_err(&intf->dev, "MAC and Serial upload failed (%d)\n", r);
+		goto error;
+	}
+	chip->unit_type = STA;
+	dev_dbg(&intf->dev, "unit type is station");
+
+	r = purelifi_mac_preinit_hw(hw, hw_address);
+	if (r) {
+		dev_dbg(&intf->dev, "init mac failed (%d)\n", r);
+		goto error;
+	}
+
+	r = ieee80211_register_hw(hw);
+	if (r) {
+		dev_dbg(&intf->dev, "register device failed (%d)\n", r);
+		goto error;
+	}
+	dev_info(&intf->dev, "%s\n", wiphy_name(hw->wiphy));

Looks like a pointless info message to me, please remove.

+	if ((le16_to_cpu(interface_to_usbdev(intf)->descriptor.idVendor) ==
+				PURELIFI_XL_VENDOR_ID_0) &&
+	    (le16_to_cpu(interface_to_usbdev(intf)->descriptor.idProduct) ==
+				PURELIFI_XL_PRODUCT_ID_0)) {
+		r = download_xl_firmware(intf);
+	} else {
+		r = download_fpga(intf);
+	}
+	if (r != 0) {
+		dev_err(&intf->dev, "FPGA download failed (%d)\n", r);
+		goto error;
+	}
+
+	tx->mac_fifo_full = 0;
+	spin_lock_init(&tx->lock);
+
+	msleep(PLF_MSLEEP_TIME);
+	r = purelifi_usb_init_hw(usb);
+	if (r < 0) {
+		dev_dbg(&intf->dev, "usb_init_hw failed (%d)\n", r);
+		goto error;
+	}
+
+	msleep(PLF_MSLEEP_TIME);
+	r = purelifi_chip_switch_radio(chip, 1); /* Switch ON Radio */

A pointless comment. You can change the integer to an enum
(PLFXLC_RADIO_ON) if you want to document clearly when it's turned on.

+	if (r < 0) {
+		dev_dbg(&intf->dev, "chip_switch_radio_on failed (%d)\n", r);
+		goto error;
+	}
+
+	msleep(PLF_MSLEEP_TIME);
+	r = purelifi_chip_set_rate(chip, 8);
+	if (r < 0) {
+		dev_dbg(&intf->dev, "chip_set_rate failed (%d)\n", r);
+		goto error;
+	}
+
+	msleep(PLF_MSLEEP_TIME);
+	r = plf_usb_wreq(hw_address, ETH_ALEN, USB_REQ_MAC_WR);
+	if (r < 0) {
+		dev_dbg(&intf->dev, "MAC_WR failure (%d)\n", r);
+		goto error;
+	}
+
+	purelifi_chip_enable_rxtx(chip);
+
+	/* Initialise the data plane Tx queue */
+	atomic_set(&data_queue_flag, 1);

Misuse of atomic variables again, SET_BIT().

+static struct workqueue_struct *purelifi_workqueue;

This static variable actually might be ok as it's called only from
usb_init() and usb_exit(). Just rename it to plfxlc_workqueue.

+static int __init usb_init(void)
+{
+	int r;
+
+	purelifi_workqueue = create_singlethread_workqueue(driver.name);
+	if (!purelifi_workqueue) {
+		pr_err("%s couldn't create workqueue\n", driver.name);
+		r = -ENOMEM;
+		goto error;
+	}
+
+	r = usb_register(&driver);
+	if (r) {
+		destroy_workqueue(purelifi_workqueue);
+		pr_err("%s usb_register() failed %d\n", driver.name, r);
+		return r;
+	}
+
+	pr_debug("Driver initialized :%s\n", driver.name);
+	return 0;
+
+error:
+	return r;
+}
+
+static void __exit usb_exit(void)
+{
+	usb_deregister(&driver);
+	destroy_workqueue(purelifi_workqueue);
+	pr_debug("%s %s\n", driver.name, __func__);
+}
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("USB driver for pureLiFi devices");
+MODULE_AUTHOR("pureLiFi");
+MODULE_VERSION("1.0");
+MODULE_FIRMWARE("plfxlc/lifi-x.bin");
+MODULE_DEVICE_TABLE(usb, usb_ids);
+module_init(usb_init);
+module_exit(usb_exit);

I see that you have MODULE_LICENSE() three times:

chip.c.97:MODULE_LICENSE("GPL");
mac.c.841:MODULE_LICENSE("GPL");
usb.c.1006:MODULE_LICENSE("GPL");

But you only have one module. So remove MODULE_LICENSE() from chip.c and
mac.c, and leave it only here.

+/* USB interrupt */
+struct purelifi_usb_interrupt {

A useless comment.

+	spinlock_t lock;/* spin lock for usb interrupt buffer */

Add space before the comment.

+	struct urb *urb;
+	void *buffer;
+	int interval;
+};
+
+#define RX_URBS_COUNT 5
+
+struct purelifi_usb_rx {
+	spinlock_t lock;/* spin lock for rx urb */

Here too.

+	struct mutex setup_mutex; /* mutex lockt for rx urb */
+	u8 fragment[2 * USB_MAX_RX_SIZE];
+	unsigned int fragment_length;
+	unsigned int usb_packet_size;
+	struct urb **urbs;
+	int urbs_count;
+};
+
+struct station_t {
+   //  7...3    |    2      |     1     |     0	    |
+   // Reserved  | Heartbeat | FIFO full | Connected |

No C++ comments, please.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches