RE: [PATCH v3 1/2] staging: rtl8188eu: Replace a custom function with crc32_le()

[David Laight <David.Laight@ACULAB.COM>]

From: Fabio M. De Francesco
> Sent: 23 July 2021 18:41
> 
> Hi David,
> 
> This driver is going to be replaced by a "better" version, so I'm not sure
> whether or not this patch is still needed.
> 
> However, I see that we have similar problems in rtl8723bs and perhaps also in
> other drivers. Therefore, I'd like to solve this problem, whatever will happen
> to the "worse" rti8188eu, and change the code where else it needs to be
> changed.
> 
> Now I have a few questions...

Your mailer mail a right pigs breakfast of this quoted text.
Even outlook (which work makes me use) isn't that bad
- it is pretty shitty though.

...
> > Change crc to be __le32, kill the casts and pass &crc in the last call.
> >
> 
> I could do it, but the last call (that to arcfour_encrypt() takes a pointer to
> u8 type as the third parameter. How can I use a __le32 for that?

Try a cast :-)
arcfour_encrypt() takes a 'buffer' - so it should be 'const void *'.
That will let the call pass what they want.

...
> > > @@ -682,7 +669,7 @@ u32 rtw_tkip_decrypt(struct adapter *padapter, struct
> recv_frame
> > > *precvframe)>
> > >  			arcfour_init(&mycontext, rc4key, 16);
> > >  			arcfour_encrypt(&mycontext, payload,
> payload, length);
> > >
> > > -			*((__le32 *)crc) = getcrc32(payload, length
> - 4);
> > > +			*((__le32 *)crc) = cpu_to_le32(~crc32_le(~0,
> payload, length - 4));
> > >
> > >  			if (crc[3] != payload[length - 1] ||
> > >
> > >  			    crc[2] != payload[length - 2] ||
> >
> > You could to the same here, or make crc u32, remove the cpu_to_le32()
> > and use get_unaligned_u32(payload + length - 4) (or whatever it is called).
> >
> 
> Sorry, I can't understand this line. Can you please elaborate it a bit more?
> 
> > But it is much better to do:
> > 	crc = crc32_le(~0, payload, length);
> > 	if (crc != VALID_CRC32)
> > 		res = _FAIL;
> >
> 
> Why "crc = crc32_le(~0, payload, length);"? Shouldn't it be "crc =
> cpu_to_le32(~crc32_le(~0, patload, length);"?
> 
> Why did you drop both the cpu_to_le32() call and the '~' operator?

Because they aren't needed.
Think about what happens when the CRC is processed bit by bit with
a hardware shift register.
When 'length - 4' bytes have been processed the receivers CRC register
matches that the transmitter had at the same point.
The transmitter inverts each crc bit before sending it.
So when the receiver XOR's a received bit with a CRC bit (to feed back
into the shift register) it always gets a '1' bits
(either the crc bit or the data is a 1).
Once the receiver has processed 'length' bytes the CRC register always
contains the crc of four 0xff bytes - regardless of the input frame.

If you don't invert the crc before sending (some early frame formats
didn't) then the last data bits and crc bits cancel each other out
and the crc is always 0 for a good frame.

It is also worth realising that a crc is a linear function.
This means that if you 'exclusive or' two frames with valid CRC
the resultant buffer also has a valid crc (align the ends of
the frames).
Initialising the crc to ~0 has the effect of including the frame
length in the crc - particularly useful if the frame starts with
zero bytes.

Since the crc algorithm is reversable, if you get a CRC error
you can 'wind back' the error bits until only a small number
of bits are affected - flip the matching bits of the receive
data and so correct a single short error burst (the most
likely errors) to get the original data.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)