linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Lars Svensson <Lars1.Svensson@sonymobile.com>
To: Larry Finger <Larry.Finger@lwfinger.net>
Cc: "Jes.Sorensen@redhat.com" <Jes.Sorensen@redhat.com>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"m.v.b@runbox.com" <m.v.b@runbox.com>,
	"gdonald@gmail.com" <gdonald@gmail.com>,
	"joe@perches.com" <joe@perches.com>,
	"ruchandani.tina@gmail.com" <ruchandani.tina@gmail.com>,
	"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
	"devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] staging: rtl8723au: Mark type casts to __le32 as intentional
Date: Wed, 23 Sep 2015 12:11:09 +0200	[thread overview]
Message-ID: <20150923101109.GA1107@sonymobile.com> (raw)
In-Reply-To: <5601C88E.4010807@lwfinger.net>

On Tue, Sep 22, 2015 at 11:30:54PM +0200, Larry Finger wrote:
> 
> You may have silenced the Sparse warnings, but the code was not wrong. Your 
> version is also correct; however, you end up with some really ugly casts.
> 
> Here is my analysis of these two, identical sections:
> 
> The output of getcrc32() is an unsigned, 4-byte quantity that has the endianess 
> of the cpu. Therefore, the le32_to_cpu() conversion is suspect for a big-endian 
> machine. Those statements should be be a simple "actual_crc = getcrc32(....).
> 
> The expected crc comes from a byte string that is in little-endian order. For 
> that reason, it needs to be converted on big-endian machines, which is exactly 
> what get_unaligned_le32() does. Thus, the second statement in each block becomes 
> "expected_crc = get_unaligned_le32(....)".
> 
> Both the original code and your patch byte swap both quantities, thus they get 
> the correct result. at least if all you are doing is to compare the two results.

I agree, it seems obvious when You spell it out like this. I was more
focused on producing an identical result while removing the sparse warnings. Your
suggestion is much nicer, I will change the patch accordingly.

> 
> The above compiles with no Sparse warnings, and I think it would work on both LE 
> and BE architectures; however, it has only been compile tested.

I have no big endian machine to try it on, but it seems reasonable to assume
that at least the compare will work. The printk in case of a mismatch im not
sure about, though. Assuming the analysis is correct, I think the printed values
would be wrong on a BE machine before this change?

Thanks,
//Lars

  reply	other threads:[~2015-09-23 10:21 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-09-22  7:24 [PATCH] staging: rtl8723au: Mark type casts to __le32 as intentional Lars Svensson
2015-09-22 21:30 ` Larry Finger
2015-09-23 10:11   ` Lars Svensson [this message]
2015-09-24  7:11     ` [PATCH V2] staging: rtl8723au: Remove unneeded endianness conversions Lars Svensson
2015-09-26 17:54       ` Larry Finger

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=20150923101109.GA1107@sonymobile.com \
    --to=lars1.svensson@sonymobile.com \
    --cc=Jes.Sorensen@redhat.com \
    --cc=Larry.Finger@lwfinger.net \
    --cc=devel@driverdev.osuosl.org \
    --cc=gdonald@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=joe@perches.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=m.v.b@runbox.com \
    --cc=ruchandani.tina@gmail.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).