Re: [PATCH 4/4] wilc1000: Add support for enabling CRC

[<Ajay.Kathat@microchip.com>]

On 24/02/21 11:22 am, David Mosberger-Tang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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);

One more observation.
I am not clear if the below block is really needed. Have you faced any
issue here and did the below logic of skipping data helped to come out
of it. Also checking the limit of 16384(2*8KB) byte looks odd when the
max limit for data packet is around 8KB. Am I missing something here.

> +
> +                               {
> +                                       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
>