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?
drivers/staging/wilc1000/Kconfig | 1 +
drivers/staging/wilc1000/spi.c | 56 ++------------------------------
2 files changed, 3 insertions(+), 54 deletions(-)
diff --git a/drivers/staging/wilc1000/Kconfig b/drivers/staging/wilc1000/Kconfig
index 59e58550d1397..09242cc7c04b7 100644
--- a/drivers/staging/wilc1000/Kconfig
+++ b/drivers/staging/wilc1000/Kconfig
@@ -22,6 +22,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
diff --git a/drivers/staging/wilc1000/spi.c b/drivers/staging/wilc1000/spi.c
index 55f8757325f0f..431aee27e7212 100644
--- a/drivers/staging/wilc1000/spi.c
+++ b/drivers/staging/wilc1000/spi.c
@@ -6,6 +6,7 @@
#include <linux/clk.h>
#include <linux/spi/spi.h>
+#include <linux/crc7.h>
#include "netdev.h"
#include "cfg80211.h"
@@ -18,59 +19,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;
-}
-
/********************************************
*
* Spi protocol Function
@@ -396,7 +344,7 @@ static int spi_cmd_complete(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz,
return result;
if (!spi_priv->crc_off)
- wb[len - 1] = (crc7(0x7f, (const u8 *)&wb[0], len - 1)) << 1;
+ wb[len - 1] = crc7_be(0xfe, wb, len - 1);
else
len -= 1;
--
2.26.0.rc2
Hi George,
On 22/03/20 5:34 pm, George Spelvin wrote:
>
> 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.
>
> 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?
>
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.
Regards,
Ajay
On Mon, Mar 23, 2020 at 05:03:02AM +0000, [email protected] wrote:
> On 22/03/20 5:34 pm, George Spelvin wrote:
>> 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.
> 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.
Ah, thanks. Sorry for the noise, then. The rcr7 patch still applies,
however.
On 23/03/20 12:15 pm, George Spelvin wrote:
>
> On Mon, Mar 23, 2020 at 05:03:02AM +0000, [email protected] wrote:
>> On 22/03/20 5:34 pm, George Spelvin wrote:
>>> 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.
>
You are right both implementation compute same results.
My bad, I didn't use correct data length value while verifying this
patch with latest driver. 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'.
Could you please modify and submit the patch using the latest staging
code so it could be applied to staging.
Thanks again for submitting the patch.
Regards,
Ajay
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(-)
diff --git a/drivers/staging/wilc1000/Kconfig b/drivers/staging/wilc1000/Kconfig
index 59e58550d1397..80c92e8bf8a59 100644
--- a/drivers/staging/wilc1000/Kconfig
+++ b/drivers/staging/wilc1000/Kconfig
@@ -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
diff --git a/drivers/staging/wilc1000/spi.c b/drivers/staging/wilc1000/spi.c
index 8d4b8c219c2fc..3f19e3f38a397 100644
--- a/drivers/staging/wilc1000/spi.c
+++ b/drivers/staging/wilc1000/spi.c
@@ -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
Thanks George.
On 23/03/20 7:44 pm, George Spelvin wrote:
>
> 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]>
The changes looks fine to me.
Reviewed-by: Ajay Singh <[email protected]>
Regards,
Ajay
> 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(-)
>
> diff --git a/drivers/staging/wilc1000/Kconfig b/drivers/staging/wilc1000/Kconfig
> index 59e58550d1397..80c92e8bf8a59 100644
> --- a/drivers/staging/wilc1000/Kconfig
> +++ b/drivers/staging/wilc1000/Kconfig
> @@ -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
> diff --git a/drivers/staging/wilc1000/spi.c b/drivers/staging/wilc1000/spi.c
> index 8d4b8c219c2fc..3f19e3f38a397 100644
> --- a/drivers/staging/wilc1000/spi.c
> +++ b/drivers/staging/wilc1000/spi.c
> @@ -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
>
> 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.
> 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())?
Hi George,
On 23/03/20 10:35 pm, George Spelvin wrote:
>
>> 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.
>
Sounds good. I will try to add these comments in a separate patch for
'linux/crc7.h'.
>> 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.
>
Over multiple code reviews and contribution from community has helped to
bring this driver to follow kernel recommendations. I hope the driver
will be soon out of staging.
> 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())?
Actually, the cookie is used to match the management action frame
requests with its response status received from wilc1000 device.
The driver assign the cookie value for transmit management frame
received from user space and later while publishing status back it uses
the same cookie value. It's validation is taken care in user space e.g
wpa_supplicant.
Even though the application expects u64-bit value but passing upscaled
u32-bit random value(prandom_u32() alone) should be enough for this case.
Regards,
Ajay
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.
Regards,
Ajay
On 23/03/20 7:44 pm, George Spelvin wrote:
>
> 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(-)
>
> diff --git a/drivers/staging/wilc1000/Kconfig b/drivers/staging/wilc1000/Kconfig
> index 59e58550d1397..80c92e8bf8a59 100644
> --- a/drivers/staging/wilc1000/Kconfig
> +++ b/drivers/staging/wilc1000/Kconfig
> @@ -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
> diff --git a/drivers/staging/wilc1000/spi.c b/drivers/staging/wilc1000/spi.c
> index 8d4b8c219c2fc..3f19e3f38a397 100644
> --- a/drivers/staging/wilc1000/spi.c
> +++ b/drivers/staging/wilc1000/spi.c
> @@ -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
>
On Thu, Mar 26, 2020 at 02:54:41PM +0000, [email protected] wrote:
> 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.
That's a very good reason for me to have never seen it, how else would I
have known to take it? :)
> Please confirm if new version for patch should be submitted to apply in
> staging.
My staging queue is long-empty, please resend.
thanks,
greg k-h