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;
MIME-Version: 1.0
References: <20210224055135.1509200-1-davidm@egauge.net> <20210224055135.1509200-4-davidm@egauge.net>
In-Reply-To: <20210224055135.1509200-4-davidm@egauge.net>
From:   Julian Calaby <julian.calaby@gmail.com>
Date:   Thu, 25 Feb 2021 08:19:21 +1100
Message-ID: <CAGRGNgUR7wHgprWcqz5orJNuhcxeLMSE1C3WCi1_UH_MCwJ6Cg@mail.gmail.com>
Subject: Re: [PATCH 4/4] wilc1000: Add support for enabling CRC
To:     David Mosberger-Tang <davidm@egauge.net>
Cc:     linux-wireless@vger.kernel.org,
        Ajay Singh <ajay.kathat@microchip.com>,
        Claudiu Beznea <claudiu.beznea@microchip.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi David,

On Wed, Feb 24, 2021 at 6:56 PM David Mosberger-Tang <davidm@egauge.net> wrote:
>
> 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 */

Should these be Kconfig variables instead?

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/