wilc1000: Use crc7 in lib/ rather than a private copy

The code in lib/ is the desired polynomial, and even includes the 1-bit left shift in the table rather than needing to code it explicitly.

Signed-off-by George Spelvin <[email protected]>

Cc Adham Abozaeid <[email protected]>

Cc Ajay Singh <[email protected]>

Cc [email protected]

---
Just a drive-by fix I happened to spot. Another possible bug I found in the code is in wilc_wfi_cfg_tx_vendor_spec:

                        get_random_bytes(&priv->p2p.local_random, 1);
                        priv->p2p.local_random++;

What is the point of the increment? Since local_random is an 8-bit variable, the result is 8 random bits in the range [0..255], the same thing that was there before the increment.

Also, I was thinking it could be replaced by prandom_u32(); does this application call for crypto-grade randomness?

Quoted 3 lines · George Spelvin

The code in lib/ is the desired polynomial, and even includes the 1-bit left shift in the table rather than needing to code it explicitly.

These changes will break functionality. The crc7 used in 'wilc' is based on the previous revision(commit# ad241528c491). The new changes can not be adopted from 'wilc' device side because the crc calculation is done on hardware IP and it expects the value based the older implementation.

It seems you are using an old version of 'wilc' driver. This logic is already changed in the latest code. We have remove custom behavior to decide p2p role (P2P_Go/P2P_Client) between 2 wilc devices based on 'local_random' value and now relies on 'intent' value received from 'wpa_s'. To submit changes for wilc, please use 'staging-next' branch of
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git repo.

Quoted 9 lines · On Mon, Mar 23, 2020 at 05:03:02AM +0000, [email protected] wrote:

Quoted 3 lines · George Spelvin

The code in lib/ is the desired polynomial, and even includes the 1-bit left shift in the table rather than needing to code it explicitly.

These changes will break functionality. The crc7 used in 'wilc' is based on the previous revision(commit# ad241528c491). The new changes can not be adopted from 'wilc' device side because the crc calculation is done on hardware IP and it expects the value based the older implementation.

I'm confused. Both crc7 functions compute the exact same value. I put them in a test harness and checked that they produce identical output before submitting.

The only difference is that the implementation I deleted does

        crc = 0x7f;
        while (len--)
                crc = crc_cyndrome_table[(crc << 1) ^ *byte++];
        return crc << 1;

while the lib/crc7.c code maintains its "crc" state value already shifted left 1 bit, so it can use the simpler loop:

        crc = 0xfe;     /* 0x7f << 1 */
        while (len--)
                crc = crc_cyndrome_table2[crc ^ *byte++];
        return crc;

It's not a different CRC-7, it's the *exact same* CRC-7. You can see that the syndrome tables are identical, just shifted one bit over.

The code in lib/ is the desired polynomial, and even includes the 1-bit left shift in the table rather than needing to code it explicitly.

While I'm in Kconfig, add a description of what a WILC1000 is. Kconfig questions that require me to look up a data sheet to find out that I probably don't have one are a pet peeve.

Signed-off-by George Spelvin <[email protected]>

Cc Ajay Singh <[email protected]>

Cc Adham Abozaeid <[email protected]>

Cc [email protected]

---
v2: Rebase on staging-next tree

 drivers/staging/wilc1000/Kconfig |  5 +++
 drivers/staging/wilc1000/spi.c   | 64 +++-----------------------------
 2 files changed, 11 insertions(+), 58 deletions(-)

drivers/staging/wilc1000/Kconfig +5 -0

index 59e58550d1397..80c92e8bf8a59 100644
@@ -2,6 +2,10 @@
 config WILC1000
        tristate
        help
+         Add support for the Atmel WILC1000 802.11 b/g/n SoC.
+         This provides Wi-FI over an SDIO or SPI interface, and
+         is usually found in IoT devices.
+
          This module only support IEEE 802.11n WiFi.
 
 config WILC1000_SDIO

@@ -22,6 +26,7 @@ config WILC1000_SPI
        tristate "Atmel WILC1000 SPI (WiFi only)"
        depends on CFG80211 && INET && SPI
        select WILC1000
+       select CRC7
        help
          This module adds support for the SPI interface of adapters using
          WILC1000 chipset. The Atmel WILC1000 has a Serial Peripheral

drivers/staging/wilc1000/spi.c +6 -58

index 8d4b8c219c2fc..3f19e3f38a397 100644
@@ -6,6 +6,7 @@
 
 #include <linux/clk.h>
 #include <linux/spi/spi.h>
+#include <linux/crc7.h>
 
 #include "netdev.h"
 #include "cfg80211.h"

@@ -16,64 +17,6 @@ struct wilc_spi {
 
 static const struct wilc_hif_func wilc_hif_spi;
 
-/********************************************
- *
- *      Crc7
- *
- ********************************************/
-
-static const u8 crc7_syndrome_table[256] = {
-       0x00, 0x09, 0x12, 0x1b, 0x24, 0x2d, 0x36, 0x3f,
-       0x48, 0x41, 0x5a, 0x53, 0x6c, 0x65, 0x7e, 0x77,
-       0x19, 0x10, 0x0b, 0x02, 0x3d, 0x34, 0x2f, 0x26,
-       0x51, 0x58, 0x43, 0x4a, 0x75, 0x7c, 0x67, 0x6e,
-       0x32, 0x3b, 0x20, 0x29, 0x16, 0x1f, 0x04, 0x0d,
-       0x7a, 0x73, 0x68, 0x61, 0x5e, 0x57, 0x4c, 0x45,
-       0x2b, 0x22, 0x39, 0x30, 0x0f, 0x06, 0x1d, 0x14,
-       0x63, 0x6a, 0x71, 0x78, 0x47, 0x4e, 0x55, 0x5c,
-       0x64, 0x6d, 0x76, 0x7f, 0x40, 0x49, 0x52, 0x5b,
-       0x2c, 0x25, 0x3e, 0x37, 0x08, 0x01, 0x1a, 0x13,
-       0x7d, 0x74, 0x6f, 0x66, 0x59, 0x50, 0x4b, 0x42,
-       0x35, 0x3c, 0x27, 0x2e, 0x11, 0x18, 0x03, 0x0a,
-       0x56, 0x5f, 0x44, 0x4d, 0x72, 0x7b, 0x60, 0x69,
-       0x1e, 0x17, 0x0c, 0x05, 0x3a, 0x33, 0x28, 0x21,
-       0x4f, 0x46, 0x5d, 0x54, 0x6b, 0x62, 0x79, 0x70,
-       0x07, 0x0e, 0x15, 0x1c, 0x23, 0x2a, 0x31, 0x38,
-       0x41, 0x48, 0x53, 0x5a, 0x65, 0x6c, 0x77, 0x7e,
-       0x09, 0x00, 0x1b, 0x12, 0x2d, 0x24, 0x3f, 0x36,
-       0x58, 0x51, 0x4a, 0x43, 0x7c, 0x75, 0x6e, 0x67,
-       0x10, 0x19, 0x02, 0x0b, 0x34, 0x3d, 0x26, 0x2f,
-       0x73, 0x7a, 0x61, 0x68, 0x57, 0x5e, 0x45, 0x4c,
-       0x3b, 0x32, 0x29, 0x20, 0x1f, 0x16, 0x0d, 0x04,
-       0x6a, 0x63, 0x78, 0x71, 0x4e, 0x47, 0x5c, 0x55,
-       0x22, 0x2b, 0x30, 0x39, 0x06, 0x0f, 0x14, 0x1d,
-       0x25, 0x2c, 0x37, 0x3e, 0x01, 0x08, 0x13, 0x1a,
-       0x6d, 0x64, 0x7f, 0x76, 0x49, 0x40, 0x5b, 0x52,
-       0x3c, 0x35, 0x2e, 0x27, 0x18, 0x11, 0x0a, 0x03,
-       0x74, 0x7d, 0x66, 0x6f, 0x50, 0x59, 0x42, 0x4b,
-       0x17, 0x1e, 0x05, 0x0c, 0x33, 0x3a, 0x21, 0x28,
-       0x5f, 0x56, 0x4d, 0x44, 0x7b, 0x72, 0x69, 0x60,
-       0x0e, 0x07, 0x1c, 0x15, 0x2a, 0x23, 0x38, 0x31,
-       0x46, 0x4f, 0x54, 0x5d, 0x62, 0x6b, 0x70, 0x79
-};
-
-static u8 crc7_byte(u8 crc, u8 data)
-{
-       return crc7_syndrome_table[(crc << 1) ^ data];
-}
-
-static u8 crc7(u8 crc, const u8 *buffer, u32 len)
-{
-       while (len--)
-               crc = crc7_byte(crc, *buffer++);
-       return crc;
-}
-
-static u8 wilc_get_crc7(u8 *buffer, u32 len)
-{
-       return crc7(0x7f, (const u8 *)buffer, len) << 1;
-}
-
 /********************************************
  *
  *      Spi protocol Function

@@ -403,6 +346,11 @@ static int spi_data_write(struct wilc *wilc, u8 *b, u32 sz)
  *      Spi Internal Read/Write Function
  *
  ********************************************/
+static u8 wilc_get_crc7(u8 *buffer, u32 len)
+{
+       return crc7_be(0xfe, buffer, len);
+}
+
 static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
                                u8 clockless)
 {

base-commit: 3017e587e36819f87e53d3c8751afdf987c1f542 --
2.26.0.rc2

The code in lib/ is the desired polynomial, and even includes the 1-bit left shift in the table rather than needing to code it explicitly.

While I'm in Kconfig, add a description of what a WILC1000 is. Kconfig questions that require me to look up a data sheet to find out that I probably don't have one are a pet peeve.

Signed-off-by George Spelvin <[email protected]>

Cc Ajay Singh <[email protected]>

Cc Adham Abozaeid <[email protected]>

Cc [email protected]

---
v2: Rebase on staging-next tree

 drivers/staging/wilc1000/Kconfig |  5 +++
 drivers/staging/wilc1000/spi.c   | 64 +++-----------------------------
 2 files changed, 11 insertions(+), 58 deletions(-)

drivers/staging/wilc1000/Kconfig +5 -0

index 59e58550d1397..80c92e8bf8a59 100644
@@ -2,6 +2,10 @@
 config WILC1000
        tristate
        help
+         Add support for the Atmel WILC1000 802.11 b/g/n SoC.
+         This provides Wi-FI over an SDIO or SPI interface, and
+         is usually found in IoT devices.
+
          This module only support IEEE 802.11n WiFi.
 
 config WILC1000_SDIO

@@ -22,6 +26,7 @@ config WILC1000_SPI
        tristate "Atmel WILC1000 SPI (WiFi only)"
        depends on CFG80211 && INET && SPI
        select WILC1000
+       select CRC7
        help
          This module adds support for the SPI interface of adapters using
          WILC1000 chipset. The Atmel WILC1000 has a Serial Peripheral

drivers/staging/wilc1000/spi.c +6 -58

index 8d4b8c219c2fc..3f19e3f38a397 100644
@@ -6,6 +6,7 @@
 
 #include <linux/clk.h>
 #include <linux/spi/spi.h>
+#include <linux/crc7.h>
 
 #include "netdev.h"
 #include "cfg80211.h"

@@ -16,64 +17,6 @@ struct wilc_spi {
 
 static const struct wilc_hif_func wilc_hif_spi;
 
-/********************************************
- *
- *      Crc7
- *
- ********************************************/
-
-static const u8 crc7_syndrome_table[256] = {
-       0x00, 0x09, 0x12, 0x1b, 0x24, 0x2d, 0x36, 0x3f,
-       0x48, 0x41, 0x5a, 0x53, 0x6c, 0x65, 0x7e, 0x77,
-       0x19, 0x10, 0x0b, 0x02, 0x3d, 0x34, 0x2f, 0x26,
-       0x51, 0x58, 0x43, 0x4a, 0x75, 0x7c, 0x67, 0x6e,
-       0x32, 0x3b, 0x20, 0x29, 0x16, 0x1f, 0x04, 0x0d,
-       0x7a, 0x73, 0x68, 0x61, 0x5e, 0x57, 0x4c, 0x45,
-       0x2b, 0x22, 0x39, 0x30, 0x0f, 0x06, 0x1d, 0x14,
-       0x63, 0x6a, 0x71, 0x78, 0x47, 0x4e, 0x55, 0x5c,
-       0x64, 0x6d, 0x76, 0x7f, 0x40, 0x49, 0x52, 0x5b,
-       0x2c, 0x25, 0x3e, 0x37, 0x08, 0x01, 0x1a, 0x13,
-       0x7d, 0x74, 0x6f, 0x66, 0x59, 0x50, 0x4b, 0x42,
-       0x35, 0x3c, 0x27, 0x2e, 0x11, 0x18, 0x03, 0x0a,
-       0x56, 0x5f, 0x44, 0x4d, 0x72, 0x7b, 0x60, 0x69,
-       0x1e, 0x17, 0x0c, 0x05, 0x3a, 0x33, 0x28, 0x21,
-       0x4f, 0x46, 0x5d, 0x54, 0x6b, 0x62, 0x79, 0x70,
-       0x07, 0x0e, 0x15, 0x1c, 0x23, 0x2a, 0x31, 0x38,
-       0x41, 0x48, 0x53, 0x5a, 0x65, 0x6c, 0x77, 0x7e,
-       0x09, 0x00, 0x1b, 0x12, 0x2d, 0x24, 0x3f, 0x36,
-       0x58, 0x51, 0x4a, 0x43, 0x7c, 0x75, 0x6e, 0x67,
-       0x10, 0x19, 0x02, 0x0b, 0x34, 0x3d, 0x26, 0x2f,
-       0x73, 0x7a, 0x61, 0x68, 0x57, 0x5e, 0x45, 0x4c,
-       0x3b, 0x32, 0x29, 0x20, 0x1f, 0x16, 0x0d, 0x04,
-       0x6a, 0x63, 0x78, 0x71, 0x4e, 0x47, 0x5c, 0x55,
-       0x22, 0x2b, 0x30, 0x39, 0x06, 0x0f, 0x14, 0x1d,
-       0x25, 0x2c, 0x37, 0x3e, 0x01, 0x08, 0x13, 0x1a,
-       0x6d, 0x64, 0x7f, 0x76, 0x49, 0x40, 0x5b, 0x52,
-       0x3c, 0x35, 0x2e, 0x27, 0x18, 0x11, 0x0a, 0x03,
-       0x74, 0x7d, 0x66, 0x6f, 0x50, 0x59, 0x42, 0x4b,
-       0x17, 0x1e, 0x05, 0x0c, 0x33, 0x3a, 0x21, 0x28,
-       0x5f, 0x56, 0x4d, 0x44, 0x7b, 0x72, 0x69, 0x60,
-       0x0e, 0x07, 0x1c, 0x15, 0x2a, 0x23, 0x38, 0x31,
-       0x46, 0x4f, 0x54, 0x5d, 0x62, 0x6b, 0x70, 0x79
-};
-
-static u8 crc7_byte(u8 crc, u8 data)
-{
-       return crc7_syndrome_table[(crc << 1) ^ data];
-}
-
-static u8 crc7(u8 crc, const u8 *buffer, u32 len)
-{
-       while (len--)
-               crc = crc7_byte(crc, *buffer++);
-       return crc;
-}
-
-static u8 wilc_get_crc7(u8 *buffer, u32 len)
-{
-       return crc7(0x7f, (const u8 *)buffer, len) << 1;
-}
-
 /********************************************
  *
  *      Spi protocol Function

@@ -403,6 +346,11 @@ static int spi_data_write(struct wilc *wilc, u8 *b, u32 sz)
  *      Spi Internal Read/Write Function
  *
  ********************************************/
+static u8 wilc_get_crc7(u8 *buffer, u32 len)
+{
+       return crc7_be(0xfe, buffer, len);
+}
+
 static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
                                u8 clockless)
 {

base-commit: 3017e587e36819f87e53d3c8751afdf987c1f542 --
2.26.0.rc2

Hi Greg,

I just noticed this patch is not applied to staging. I suspect, its not picked because not sent to [email protected] and 'staging' is missing from subject.

Please confirm if new version for patch should be submitted to apply in staging.

Earlier, I also tried to replace crc7 by using existing library but it gave different results with 'crc7_be()' because I didn't modify '0x7f' to '0xfe'.

Thanks again for submitting the patch.

Quoted 4 lines

Earlier, I also tried to replace crc7 by using existing library but it gave different results with 'crc7_be()' because I didn't modify '0x7f' to '0xfe'.

I had an afterthought that maybe documenting this in <linux/crc7.h> would be useful, since you're unlikely to be the last person to make this mistake.

Something like:

/*
* Generate a CRC with the polynomial x^7 + x^3 + 1 and big-endian
* bit order. (Thus, the polynomial is 0x89.) The result is in the
* most-significant 7 bits of the crc variable.
*
* This is where most protocols want the CRC (the lsbit is past the
* end of CRC coverage and is used for something else), but differs
* from most example code, which computes the CRC in the lsbits and
* does an explicit 1-bit shift at the end.
*
* Because the state is maintained left-aligned, the common "preset
* to all ones" CRC variant requires the crc be preset to 0xfe.
* (Right-aligned example code will show a preset to 0x7f.) */

Feel free to add that to the patch (preserving my S-o-b) if you like.

Quoted 1 line

Thanks again for submitting the patch.

Thank you for writing the whole driver! I know it can be a real PITA; Linux kernel developers Really Really Want drivers in a common style and using existing kernel facilities as much as possible, but you're usually starting from some internal driver that has its own, very different, support library.

BTW, one thing I noticed at cfg80211.c:1132:

        *cookie = prandom_u32();
        priv->tx_cookie = *cookie;

I don't know what the cookie is for, but I notice that *cookie and priv->tx_cookie are both 64-bit data types.

Should that be "(u64)prandom_u32() << 32 | prandom_u32()" (since there is no prandom_u64())?