linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: David Mosberger-Tang <davidm@egauge.net>
To: linux-wireless@vger.kernel.org
Cc: Ajay Singh <ajay.kathat@microchip.com>,
	Claudiu Beznea <claudiu.beznea@microchip.com>,
	davidm@egauge.net
Subject: [PATCH 4/4] wilc1000: Add support for enabling CRC
Date: Wed, 24 Feb 2021 05:52:00 +0000 (UTC)	[thread overview]
Message-ID: <20210224055135.1509200-4-davidm@egauge.net> (raw)
In-Reply-To: <20210224055135.1509200-1-davidm@egauge.net>

The driver so far has always disabled CRC protection.  This means any
data corruption that occurred during the SPI transfers could
potentially go unnoticed.  This patch adds the macros ENABLE_CRC7 and
ENABLE_CRC16 to allow compile-time selection of whether or not CRC7
and CRC16, respectively, should be enabled.

The default configuration remains unchanged, with both CRC7 and CRC16
off.

Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
---
 .../net/wireless/microchip/wilc1000/Kconfig   |   1 +
 drivers/net/wireless/microchip/wilc1000/spi.c | 151 +++++++++++++-----
 2 files changed, 108 insertions(+), 44 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/Kconfig b/drivers/net/wireless/microchip/wilc1000/Kconfig
index 7f15e42602dd..62cfcdc9aacc 100644
--- a/drivers/net/wireless/microchip/wilc1000/Kconfig
+++ b/drivers/net/wireless/microchip/wilc1000/Kconfig
@@ -27,6 +27,7 @@ config WILC1000_SPI
 	depends on CFG80211 && INET && SPI
 	select WILC1000
 	select CRC7
+	select CRC_ITU_T
 	help
 	  This module adds support for the SPI interface of adapters using
 	  WILC1000 chipset. The Atmel WILC1000 has a Serial Peripheral
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index b0e096a03a28..c745a440d273 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -7,10 +7,23 @@
 #include <linux/clk.h>
 #include <linux/spi/spi.h>
 #include <linux/crc7.h>
+#include <linux/crc-itu-t.h>
 
 #include "netdev.h"
 #include "cfg80211.h"
 
+/**
+ * Establish the driver's desired CRC configuration.  CRC7 is used for
+ * command transfers which have no other protection against corruption
+ * during the SPI transfer.  Commands are short so CRC7 is relatively
+ * cheap.  CRC16 is used for data transfers, including network packet
+ * transfers.  Since those transfers can be large, CRC16 is relatively
+ * expensive.  CRC16 is also often redundant as network packets
+ * typically are protected by their own, higher-level checksum.
+ */
+#define ENABLE_CRC7	0	/* set to 1 to protect SPI commands with CRC7 */
+#define ENABLE_CRC16	0	/* set to 1 to protect SPI data with CRC16 */
+
 /*
  * For CMD_SINGLE_READ and CMD_INTERNAL_READ, WILC may insert one or
  * more zero bytes between the command response and the DATA Start tag
@@ -22,7 +35,8 @@
 #define WILC_SPI_RSP_HDR_EXTRA_DATA	8
 
 struct wilc_spi {
-	int crc_off;
+	bool crc7_enabled;
+	bool crc16_enabled;
 };
 
 static const struct wilc_hif_func wilc_hif_spi;
@@ -123,6 +137,10 @@ static int wilc_bus_probe(struct spi_device *spi)
 	if (!spi_priv)
 		return -ENOMEM;
 
+	/* WILC chip resets to both CRCs enabled: */
+	spi_priv->crc7_enabled = true;
+	spi_priv->crc16_enabled = true;
+
 	ret = wilc_cfg80211_init(&wilc, &spi->dev, WILC_HIF_SPI, &wilc_hif_spi);
 	if (ret) {
 		kfree(spi_priv);
@@ -303,7 +321,8 @@ static int spi_data_write(struct wilc *wilc, u8 *b, u32 sz)
 	struct wilc_spi *spi_priv = wilc->bus_data;
 	int ix, nbytes;
 	int result = 0;
-	u8 cmd, order, crc[2] = {0};
+	u8 cmd, order, crc[2];
+	u16 crc_calc;
 
 	/*
 	 * Data
@@ -345,9 +364,12 @@ static int spi_data_write(struct wilc *wilc, u8 *b, u32 sz)
 		}
 
 		/*
-		 * Write Crc
+		 * Write CRC
 		 */
-		if (!spi_priv->crc_off) {
+		if (spi_priv->crc16_enabled) {
+			crc_calc = crc_itu_t(0xffff, &b[ix], nbytes);
+			crc[0] = crc_calc >> 8;
+			crc[1] = crc_calc;
 			if (wilc_spi_tx(wilc, crc, 2)) {
 				dev_err(&spi->dev, "Failed data block crc write, bus error...\n");
 				result = -EINVAL;
@@ -381,11 +403,11 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
 	struct spi_device *spi = to_spi_device(wilc->dev);
 	struct wilc_spi *spi_priv = wilc->bus_data;
 	u8 wb[32], rb[32];
-	u8 crc[2];
 	int cmd_len, resp_len, i;
+	u16 crc_calc, crc_recv;
 	struct wilc_spi_cmd *c;
-	struct wilc_spi_read_rsp_data *r_data;
 	struct wilc_spi_rsp_data *r;
+	struct wilc_spi_read_rsp_data *r_data;
 
 	memset(wb, 0x0, sizeof(wb));
 	memset(rb, 0x0, sizeof(rb));
@@ -409,7 +431,7 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
 	cmd_len = offsetof(struct wilc_spi_cmd, u.simple_cmd.crc);
 	resp_len = sizeof(*r) + sizeof(*r_data) + WILC_SPI_RSP_HDR_EXTRA_DATA;
 
-	if (!spi_priv->crc_off) {
+	if (spi_priv->crc7_enabled) {
 		c->u.simple_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
 		cmd_len += 1;
 		resp_len += 2;
@@ -455,8 +477,16 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
 	if (b)
 		memcpy(b, r_data->data, 4);
 
-	if (!spi_priv->crc_off)
-		memcpy(crc, r_data->crc, 2);
+	if (!clockless && spi_priv->crc16_enabled) {
+		crc_recv = (r_data->crc[0] << 8) | r_data->crc[1];
+		crc_calc = crc_itu_t(0xffff, r_data->data, 4);
+		if (crc_recv != crc_calc) {
+			dev_err(&spi->dev, "%s: bad CRC 0x%04x "
+				"(calculated 0x%04x)\n", __func__,
+				crc_recv, crc_calc);
+			return -EINVAL;
+		}
+	}
 
 	return 0;
 }
@@ -483,7 +513,7 @@ static int wilc_spi_write_cmd(struct wilc *wilc, u8 cmd, u32 adr, u32 data,
 		c->u.internal_w_cmd.addr[1] = adr;
 		c->u.internal_w_cmd.data = cpu_to_be32(data);
 		cmd_len = offsetof(struct wilc_spi_cmd, u.internal_w_cmd.crc);
-		if (!spi_priv->crc_off)
+		if (spi_priv->crc7_enabled)
 			c->u.internal_w_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
 	} else if (cmd == CMD_SINGLE_WRITE) {
 		c->u.w_cmd.addr[0] = adr >> 16;
@@ -491,14 +521,14 @@ static int wilc_spi_write_cmd(struct wilc *wilc, u8 cmd, u32 adr, u32 data,
 		c->u.w_cmd.addr[2] = adr;
 		c->u.w_cmd.data = cpu_to_be32(data);
 		cmd_len = offsetof(struct wilc_spi_cmd, u.w_cmd.crc);
-		if (!spi_priv->crc_off)
+		if (spi_priv->crc7_enabled)
 			c->u.w_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
 	} else {
 		dev_err(&spi->dev, "write cmd [%x] not supported\n", cmd);
 		return -EINVAL;
 	}
 
-	if (!spi_priv->crc_off)
+	if (spi_priv->crc7_enabled)
 		cmd_len += 1;
 
 	resp_len = sizeof(*r);
@@ -536,6 +566,7 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
 {
 	struct spi_device *spi = to_spi_device(wilc->dev);
 	struct wilc_spi *spi_priv = wilc->bus_data;
+	u16 crc_recv, crc_calc;
 	u8 wb[32], rb[32];
 	int cmd_len, resp_len;
 	int retry, ix = 0;
@@ -554,7 +585,7 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
 		c->u.dma_cmd.size[0] = sz >> 8;
 		c->u.dma_cmd.size[1] = sz;
 		cmd_len = offsetof(struct wilc_spi_cmd, u.dma_cmd.crc);
-		if (!spi_priv->crc_off)
+		if (spi_priv->crc7_enabled)
 			c->u.dma_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
 	} else if (cmd == CMD_DMA_EXT_WRITE || cmd == CMD_DMA_EXT_READ) {
 		c->u.dma_cmd_ext.addr[0] = adr >> 16;
@@ -564,14 +595,14 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
 		c->u.dma_cmd_ext.size[1] = sz >> 8;
 		c->u.dma_cmd_ext.size[2] = sz;
 		cmd_len = offsetof(struct wilc_spi_cmd, u.dma_cmd_ext.crc);
-		if (!spi_priv->crc_off)
+		if (spi_priv->crc7_enabled)
 			c->u.dma_cmd_ext.crc[0] = wilc_get_crc7(wb, cmd_len);
 	} else {
 		dev_err(&spi->dev, "dma read write cmd [%x] not supported\n",
 			cmd);
 		return -EINVAL;
 	}
-	if (!spi_priv->crc_off)
+	if (spi_priv->crc7_enabled)
 		cmd_len += 1;
 
 	resp_len = sizeof(*r);
@@ -637,12 +668,35 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
 		}
 
 		/*
-		 * Read Crc
+		 * Read CRC
 		 */
-		if (!spi_priv->crc_off && wilc_spi_rx(wilc, crc, 2)) {
-			dev_err(&spi->dev,
-				"Failed block crc read, bus err\n");
-			return -EINVAL;
+		if (spi_priv->crc16_enabled) {
+			if (wilc_spi_rx(wilc, crc, 2)) {
+				dev_err(&spi->dev,
+					"Failed block crc read, bus err\n");
+				return -EINVAL;
+			}
+			crc_recv = (crc[0] << 8) | crc[1];
+			crc_calc = crc_itu_t(0xffff, &b[ix], nbytes);
+			if (crc_recv != crc_calc) {
+				dev_err(&spi->dev, "%s: bad CRC 0x%04x "
+					"(calculated 0x%04x)\n", __func__,
+					crc_recv, crc_calc);
+
+				{
+					u8 byte;
+					int i;
+
+					for (i = 0; i < 16384; ++i) {
+						byte = 0;
+						wilc_spi_rx(wilc, &byte, 1);
+						if (!byte)
+							break;
+					}
+				}
+
+				return -EINVAL;
+			}
 		}
 
 		ix += nbytes;
@@ -871,43 +925,52 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
 	ret = spi_internal_read(wilc, WILC_SPI_PROTOCOL_OFFSET, &reg);
 	if (ret) {
 		/*
-		 * Read failed. Try with CRC off. This might happen when module
-		 * is removed but chip isn't reset
+		 * Read failed. Try with CRC7 off. This might happen
+		 * when module is removed but chip isn't reset.
 		 */
-		spi_priv->crc_off = 1;
+		spi_priv->crc7_enabled = false;
 		dev_err(&spi->dev,
-			"Failed read with CRC on, retrying with CRC off\n");
+			"Failed read with CRC7 on, retrying with CRC7 off\n");
 		ret = spi_internal_read(wilc, WILC_SPI_PROTOCOL_OFFSET, &reg);
 		if (ret) {
 			/*
-			 * Read failed with both CRC on and off,
+			 * Read failed with both CRC7 on and off,
 			 * something went bad
 			 */
 			dev_err(&spi->dev, "Failed internal read protocol\n");
 			return ret;
 		}
 	}
-	if (spi_priv->crc_off == 0) {
-		/* disable crc checking: */
-		reg &= ~(PROTOCOL_REG_CRC7_MASK | PROTOCOL_REG_CRC16_MASK);
-
-		/* set the data packet size: */
-		BUILD_BUG_ON(DATA_PKT_LOG_SZ < DATA_PKT_LOG_SZ_MIN
-			     || DATA_PKT_LOG_SZ > DATA_PKT_LOG_SZ_MAX);
-		reg &= ~PROTOCOL_REG_PKT_SZ_MASK;
-		reg |= FIELD_PREP(PROTOCOL_REG_PKT_SZ_MASK,
-				  DATA_PKT_LOG_SZ - DATA_PKT_LOG_SZ_MIN);
-
-		ret = spi_internal_write(wilc, WILC_SPI_PROTOCOL_OFFSET, reg);
-		if (ret) {
-			dev_err(&spi->dev,
-				"[wilc spi %d]: Failed internal write reg\n",
-				__LINE__);
-			return ret;
-		}
-		spi_priv->crc_off = 1;
+
+	/* set up the desired CRC configuration: */
+	reg &= ~(PROTOCOL_REG_CRC7_MASK | PROTOCOL_REG_CRC16_MASK);
+#if ENABLE_CRC7
+	reg |= PROTOCOL_REG_CRC7_MASK;
+#endif
+#if ENABLE_CRC16
+	reg |= PROTOCOL_REG_CRC16_MASK;
+#endif
+
+	/* set up the data packet size: */
+	BUILD_BUG_ON(DATA_PKT_LOG_SZ < DATA_PKT_LOG_SZ_MIN
+		     || DATA_PKT_LOG_SZ > DATA_PKT_LOG_SZ_MAX);
+	reg &= ~PROTOCOL_REG_PKT_SZ_MASK;
+	reg |= FIELD_PREP(PROTOCOL_REG_PKT_SZ_MASK,
+			  DATA_PKT_LOG_SZ - DATA_PKT_LOG_SZ_MIN);
+
+	/* establish the new setup: */
+	ret = spi_internal_write(wilc, WILC_SPI_PROTOCOL_OFFSET, reg);
+	if (ret) {
+		dev_err(&spi->dev,
+			"[wilc spi %d]: Failed internal write reg\n",
+			__LINE__);
+		return ret;
 	}
 
+	/* now that new set up is established, update our state to match: */
+	spi_priv->crc7_enabled = ENABLE_CRC7;
+	spi_priv->crc16_enabled = ENABLE_CRC16;
+
 	/*
 	 * make sure can read back chip id correctly
 	 */
-- 
2.25.1


  parent reply	other threads:[~2021-02-24  5:53 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-24  5:51 [PATCH 1/4] wilc1000: Make SPI transfers work at 48MHz David Mosberger-Tang
2021-02-24  5:51 ` [PATCH 2/4] wilc1000: Introduce symbolic names for SPI protocol register David Mosberger-Tang
2021-02-24  5:51 ` [PATCH 3/4] wilc1000: Check for errors at end of DMA write David Mosberger-Tang
2021-02-25  8:27   ` Kalle Valo
2021-02-25 18:10     ` David Mosberger-Tang
2021-02-24  5:52 ` David Mosberger-Tang [this message]
2021-02-24 10:01   ` [PATCH 4/4] wilc1000: Add support for enabling CRC Ajay.Kathat
2021-02-24 16:25     ` David Mosberger-Tang
2021-02-25  4:50       ` Ajay.Kathat
2021-02-24 13:35   ` Ajay.Kathat
2021-02-24 15:47     ` David Mosberger-Tang
2021-02-25  4:58       ` Ajay.Kathat
2021-02-25  8:25         ` Kalle Valo
2021-02-24 21:19   ` Julian Calaby
2021-02-24 23:36     ` David Mosberger-Tang
2021-02-25  5:56       ` Ajay.Kathat
2021-02-25  8:22         ` Kalle Valo
2021-02-25 11:03           ` Ajay.Kathat
2021-02-26  7:09             ` Kalle Valo
2021-02-27 17:29 ` [PATCH v2 1/4] wilc1000: Make SPI transfers work at 48MHz David Mosberger-Tang
2021-04-17 17:48   ` Kalle Valo
2021-02-27 17:29 ` [PATCH v2 2/4] wilc1000: Introduce symbolic names for SPI protocol register David Mosberger-Tang
2021-02-27 17:29 ` [PATCH v2 3/4] wilc1000: Check for errors at end of DMA write David Mosberger-Tang
2021-02-27 17:31 ` [PATCH v2 4/4] wilc1000: Add support for enabling CRC David Mosberger-Tang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210224055135.1509200-4-davidm@egauge.net \
    --to=davidm@egauge.net \
    --cc=ajay.kathat@microchip.com \
    --cc=claudiu.beznea@microchip.com \
    --cc=linux-wireless@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).