All of lore.kernel.org
 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.