From: Al Viro <viro@ZenIV•linux.org.uk>
To: "Tayar, Tomer" <Tomer.Tayar@cavium•com>
Cc: "netdev@vger•kernel.org" <netdev@vger•kernel.org>,
"Elior, Ariel" <Ariel.Elior@cavium•com>,
David Miller <davem@davemloft•net>
Subject: Re: [RFC] endianness issues in drivers/net/ethernet/qlogic/qed
Date: Sun, 24 Sep 2017 16:30:18 +0100 [thread overview]
Message-ID: <20170924153018.GT32076@ZenIV.linux.org.uk> (raw)
In-Reply-To: <BN3PR0701MB15918FECAEB8503920850A7580650@BN3PR0701MB1591.namprd07.prod.outlook.com>
On Sun, Sep 24, 2017 at 02:34:19PM +0000, Tayar, Tomer wrote:
>
> > "qed: Utilize FW 8.10.3.0" has attempted some endianness annotations
> > in that driver; unfortunately, either annotations are BS or the driver is genuinely
> > broken on big-endian hosts.
> [...]
> > Is that driver intended to be used on big-endian hosts at all?
>
> Thanks for taking the time to review our driver and pointing out these mistakes.
> Support for BE machines is planned to be added but currently it is not available.
> However, the structures which are used to abstract the HW carry endianity annotations.
> Obviously, there are some misses and some annotations were added when not required.
> We will prepare a patch that fixes the issues you pointed out and similar ones.
OK... sparse is pretty good at spotting the problems; if you have any questions - just
ask. A bit of random braindump concerning that kind of work:
* bitfields and fixed-endian data do not mix. It's much better to have just
__le32 (or __le64, etc.) in the structure and use GET_FIELD/SET_FIELD or similar for
accesses. Another safe technics is something like
if ((foo->bar & cpu_to_le32(BAR_MASK)) == cpu_to_le32(BAR_THIS << BAR_SHIFT))
instead of
if (get_bar(foo) == BAR_THIS))
since that keeps shift and endianness conversion on the constant size. The same goes
for
if ((foo->bar ^ baz->bar) & cpu_to_le32(BAR_MASK))
instead of
if (get_bar(foo) != get_bar(baz))
If would be nice if compiler had recognized that kind of stuff and transformed the
latter into the former on its own, but...
* swab... is a Bloody Bad Idea(tm) in almost all situations. Keeping track of
whether given data is little-endian or host-endian is much easier than keeping track of
how many times have we flipped it.
* don't mix little-endian and host-endian in the same variable. See the previous
point for the reasons - static typing is much safer and easier to reason about. Code
doing
n = cpu_to_le32(n);
is asking for trouble. For local variables it's not even an optimization - compiler
is generally pretty good at spotting two local variables that are never live at the
same point and reusing memory. And for anything non-local you are introducing a hidden
piece of state - "is that field in this structure little-endian or host-endian at the
moment?", making it very easy to screw up a few months down the road. Brittle and
hell to debug...
* one very common source of noise is cpu_to_le32() when le32_to_cpu() was
intended. Sure, they do the same transformation on anything even remotely plausible
(something like V0 V2 V3 V1 is not a byte order likely to happen on any hardware),
but the choice documents what kind of values do you have before and after the
conversion. Both the human readers and automated typechecking (sparse) have much
easier life if those are accurate. Again, see the point re keeping track of the
number of flips vs. keeping track of what's host-endian and what's little-endian.
The latter is local, the former takes reasoning about control flow.
* for situations like "use this le32 value as search key in binary tree",
where you are really OK with having differently-shaped trees on l-e and b-e hosts,
use something like
if ((__force __u32) key > node->key)
preferably with a comment explaining why treating this value that way is OK.
prev parent reply other threads:[~2017-09-24 15:30 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-18 17:49 [RFC] endianness issues in drivers/net/ethernet/qlogic/qed Al Viro
2017-09-24 14:34 ` Tayar, Tomer
2017-09-24 15:30 ` Al Viro [this message]
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=20170924153018.GT32076@ZenIV.linux.org.uk \
--to=viro@zeniv$(echo .)linux.org.uk \
--cc=Ariel.Elior@cavium$(echo .)com \
--cc=Tomer.Tayar@cavium$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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