linux-staging.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: David Laight <David.Laight@ACULAB.COM>
To: "'Fabio M. De Francesco'" <fmdefrancesco@gmail.com>,
	Larry Finger <Larry.Finger@lwfinger.net>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-staging@lists.linux.dev" <linux-staging@lists.linux.dev>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v3 1/2] staging: rtl8188eu: Replace a custom function with crc32_le()
Date: Sat, 24 Jul 2021 11:04:28 +0000	[thread overview]
Message-ID: <9385be072a3e4d29ad55bb1b27b7ae03@AcuMS.aculab.com> (raw)
In-Reply-To: <3650881.QlJdx9khu8@localhost.localdomain>

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)

  reply	other threads:[~2021-07-24 11:04 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-21 11:00 [PATCH v3 0/2] " Fabio M. De Francesco
2021-07-21 11:00 ` [PATCH v3 1/2] " Fabio M. De Francesco
2021-07-22 15:30   ` David Laight
2021-07-23 17:40     ` Fabio M. De Francesco
2021-07-24 11:04       ` David Laight [this message]
2021-07-21 11:00 ` [PATCH v3 2/2] staging: rtl8188eu: Remove no more used functions and variables Fabio M. De Francesco

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9385be072a3e4d29ad55bb1b27b7ae03@AcuMS.aculab.com \
    --to=david.laight@aculab.com \
    --cc=Larry.Finger@lwfinger.net \
    --cc=fmdefrancesco@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --subject='RE: [PATCH v3 1/2] staging: rtl8188eu: Replace a custom function with crc32_le()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox