I have tested it on the actual hardware, it works...
I actually agree that we should make it explicit, eventhough DaveM seems
to disagree on the netdev list.
Signed-off-by: Jesper Dangaard Brouer <[email protected]>
Tested-by: Jesper Dangaard Brouer <[email protected]>
On Sun, 2008-11-16 at 15:43 -0600, Milton Miller wrote:
> commit e23a59e1ca6d177a57a7791b3629db93ff1d9813 (niu: Fix readq
> implementation when architecture does not provide one.) reordered the
> arguments to a bitwise or to change the emitted code. However, C does
> not guarantee the evaluation order.
>
> Split the line into two statements. While there, reduce some parens
> by using two variables.
>
> Signed-off-By: Milton Miller <[email protected]>
> Acked-by: Segher Boessenkool <[email protected]>
> ---
> complie tested on ppc6xx_defconfig, I have no hardware.
> making the vars u64 saves the later cast with no signficiant codegen difference
>
> Index: work.git/drivers/net/niu.c
> ===================================================================
> --- work.git.orig/drivers/net/niu.c 2008-11-16 01:34:44.000000000 -0600
> +++ work.git/drivers/net/niu.c 2008-11-16 02:59:06.000000000 -0600
> @@ -51,7 +51,18 @@ MODULE_VERSION(DRV_MODULE_VERSION);
> #ifndef readq
> static u64 readq(void __iomem *reg)
> {
> - return ((u64) readl(reg)) | (((u64) readl(reg + 4UL)) << 32);
> + u64 l, u;
> + /*
> + * The TX_CS register has counters in the upper 32-bits and state
> + * bits in the lower 32-bits. A read clears the state bits.
> + *
> + * Therefore this driver must read the lower word then the upper one
> + * when the architecture doesn't have an atomic readq.
> + */
> + l = readl(reg);
> + u = readl(reg + 4UL);
> +
> + return l | (u << 32);
> }
>
> static void writeq(u64 val, void __iomem *reg)
>
--
Med venlig hilsen / Best regards
Jesper Brouer
ComX Networks A/S
Linux Network developer
Cand. Scient Datalog / MSc.
Author of http://adsl-optimizer.dk
LinkedIn: http://www.linkedin.com/in/brouer
From: Jesper Dangaard Brouer <[email protected]>
Date: Sun, 16 Nov 2008 16:32:05 +0100
>
> I have tested it on the actual hardware, it works...
>
> I actually agree that we should make it explicit, eventhough DaveM seems
> to disagree on the netdev list.
I'm also not applying this patch for another reason.
This is a knee-jerk reaction patch, purely. This person
saw the commit and wants to fix only _THIS_ case.
Well guess what? If you really CARED, you'd go change this
across the whole tree. This exact construct exists ALL
OVER the kernel. In fact there are sequences that match
this new NIU code exactly.
Did these people complaining look for those? No.
On Nov 16, 2008, at 2:49 PM, David Miller wrote:
> From: Jesper Dangaard Brouer <[email protected]>
> Date: Sun, 16 Nov 2008 16:32:05 +0100
[restored context]
>> Milton Miller wrote:
>>> commit e23a59e1ca6d177a57a7791b3629db93ff1d9813 (niu: Fix readq
>>> implementation when architecture does not provide one.) reordered the
>>> arguments to a bitwise or to change the emitted code. However, C
>>> does
>>> not guarantee the evaluation order.
[end restored context]
>>
>> I have tested it on the actual hardware, it works...
>>
>> I actually agree that we should make it explicit, eventhough DaveM
>> seems
>> to disagree on the netdev list.
>
> I'm also not applying this patch for another reason.
>
> This is a knee-jerk reaction patch, purely. This person
> saw the commit and wants to fix only _THIS_ case.
Yes, I saw the commit that said *for this driver* the order of the
operations matter. I saw a change that relied on implementation
behavior that I doubt even the current compiler would make any future
guarantees.
> Well guess what? If you really CARED, you'd go change this
> across the whole tree. This exact construct exists ALL
> OVER the kernel. In fact there are sequences that match
> this new NIU code exactly.
But does the hardware require the two reads occur in order, or does the
order not matter to that hardware?
By the same token, I don't care, as I don't use the hardware. I was
trying to save you future debug.
But thinking about it further, I think you only changed the size of the
window, and the underlying problem still exists. What prevents
hardware from setting (additional) bits between the read to the lower
portion and the upper? You reduced the window to the few cycles
between the first and second read. but the window is still there.
> Did these people complaining look for those? No.
You are right, I didn't look. But I don't think the tool to use to
look for this is grep. I think its either sparse or one of the
semantic parsers. Gcc has -Wsequence-point in -Wall, although I am
told it will only complain when it can prove a multiple reference and
store. What the kernel checker should be checking is (1) volatile
dereference or (2) barrier (volatile asm?) (or any combination) on both
sides of a sequence point. Because the reference might be hidden
out-of-line, it was suggested we annotate things like readl/writel as
(has_side_effects). The way to remove such errors is to choose one
order (preferably the one some version of gcc uses) and create local
variables with the partial results. I would expect this option should
get its own kconfig to enable/disable the warning like the warn
deprecated stuff while the points are identified.
Since I'm not a toolchain hacker, I'm hoping someone on linux-kernel
will see this request and act on it. And I would guess that most of
the cc list doesn't' care to watch it, so consider your cc list.
thanks
milton