diff mbox

NIU driver: Sun x8 Express Quad Gigabit Ethernet Adapter

Message ID ada8wromu0g.fsf@cisco.com
State Rejected, archived
Delegated to: David Miller
Headers show

Commit Message

Roland Dreier Nov. 12, 2008, 10:58 p.m. UTC 
> Just google "C order of evaluation" and you will get hundreds
 > of tables, and all of them will have an entry for "|" (not
 > just "||") which says that operands are evaluated left to
 > right.

You're talking about associativity, which says how an expression like
"a | b | c" is implicitly parenthesized.  The order of evaluation is
undefined -- in fact the C standard I have says:

  Except as specified later (for the function-call (), &&, ||, ?:, and
  comma operators), the order of evaluation of subexpressions and the
  order in which side effects take place are both unspecified.

So there is no rule about which subexpression is evaluated first in an
expression like "a | b".

 > And since these MMIO reads are volatile operations, there is
 > no way the compiler can execute them out of order.

"volatile" just means that accessing a volatile expression is considered
a side effect -- and side effects are only ordered with respect to
sequence points.

So according to my understanding of the C standard, there is no required
on which readl() is done first in an expression like "readl(a) | readl(b)".

 > And the plain truth is that no compiler does, and that is what
 > matters in the end.

I think it's cleaner to avoid relying on undefined behavior (eg gcc 4.5
will probably break things), especially when the fix is so simple --
something the following should work fine:

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index 9acb5d7..1fb0d2f 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -51,8 +51,8 @@  MODULE_VERSION(DRV_MODULE_VERSION);
 #ifndef readq
 static u64 readq(void __iomem *reg)
 {
-	return (((u64)readl(reg + 0x4UL) << 32) |
-		(u64)readl(reg));
+	u64 v = readl(reg);
+	return v | (u64) readl(reg + 0x4UL) << 32;
 }
 
 static void writeq(u64 val, void __iomem *reg)