Re: [PATCH] staging: rtl8188eu: Replace a custom function with crc32_le()

[Fabio M. De Francesco]

On Tuesday, July 13, 2021 2:50:47 PM CEST Greg Kroah-Hartman wrote:
> On Thu, Jul 01, 2021 at 03:38:09PM +0200, Fabio M. De Francesco wrote:
> > Use crc32_le in place of the custom getcrc32. This change makes GCC
> > to warn about incorrect castings to the restricted type __le32, but
> > they can be safely ignored because crc32_le calculates bitwise
> > little-endian Ethernet AUTODIN II CRC32.
> > 
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>
> > ---
> > 
> >  drivers/staging/rtl8188eu/core/rtw_security.c | 22 ++++---------------
> >  1 file changed, 4 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8188eu/core/rtw_security.c
> > b/drivers/staging/rtl8188eu/core/rtw_security.c index 
1b2cb6196463..5f010cb66970
> > 100644
> > --- a/drivers/staging/rtl8188eu/core/rtw_security.c
> > +++ b/drivers/staging/rtl8188eu/core/rtw_security.c
> > @@ -111,21 +111,6 @@ static void crc32_init(void)
> > 
[...]
> 
> Why are you casting a native endian return value to a little endian
> pointer?  
>
Actually, I was not sure whether or not crc32_le() returns native or little 
endian values: 

(1) That "_le" in crc32_le() made me think it calculates little endian values 
and the documentation of the function is a little misleading where it affirms 
that "[crc32_le()] calculates *bitwise
little-endian* Ethernet AUTODIN II CRC32".

(2) Furthermore, I took commit b97fad10de387 as a model for my patch. You 
applied that commit and it makes exactly the same change that I make in my 
patch. I still cannot understand why you took that. Maybe I'm missing 
something that I can't still see. What is it?
>
> Are you _SURE_ that is correct?
> 
To summarize, I thought that crc32_le is some way returning an LE value in an 
array of four u8 (aka an unsigned integer).

Now I suppose I have to change "*((__le32 *)crc) = ~crc32_le(~0, payload, 
length);" to "*((__le32 *)crc) = cpu_to_le32(~crc32_le(~0, payload, length)".

Thanks,

Fabio
> We can not just ignore warnings, they are there for a reason.  Or if
> not, then fix the code up to not have the warnings, but I can't take
> this as-is, sorry.
> 
> thanks,
> 
> greg k-h