Re: [PATCH] niu: bitwise or does not imply ordering

Re: [PATCH] niu: bitwise or does not imply ordering

[Jesper Dangaard Brouer]

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 <hawk@comx.dk>
Tested-by: Jesper Dangaard Brouer <hawk@comx.dk>


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 <miltonm@bga.com>
> Acked-by: Segher Boessenkool <segher@kernel.crashing.org>
> ---
> 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

Re: [PATCH] niu: bitwise or does not imply ordering

[David Miller]

From: Milton Miller <miltonm@bga.com>
Date: Sun, 16 Nov 2008 15:43:18 -0600

> 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.

I'm not applying this patch, the problem is in your
imagination only.

Re: [PATCH] niu: bitwise or does not imply ordering

[David Miller]

From: Jesper Dangaard Brouer <jdb@comx.dk>
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.

[SPARSE REQUEST] was [PATCH] niu: bitwise or does not imply ordering

[Milton Miller]

On Nov 16, 2008, at 2:49 PM, David Miller wrote:
> From: Jesper Dangaard Brouer <jdb@comx.dk>
> 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