Re: [PATCH v2] wilc1000: Use crc7 in lib/ rather than a private copy

[<Ajay.Kathat@microchip.com>]

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