| Message ID | 20200322120408.GA19411@SDF.ORG (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wilc1000: Use crc7 in lib/ rather than a private copy | expand |
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 <lkml@sdf.org> > Cc: Adham Abozaeid <adham.abozaeid@microchip.com> > Cc: Ajay Singh <ajay.kathat@microchip.com> > Cc: linux-wireless@vger.kernel.org > --- > 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, Ajay.Kathat@microchip.com 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, Ajay.Kathat@microchip.com 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
> 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
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;
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 <lkml@sdf.org> Cc: Adham Abozaeid <adham.abozaeid@microchip.com> Cc: Ajay Singh <ajay.kathat@microchip.com> Cc: linux-wireless@vger.kernel.org --- 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(-)