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

Milton Miller
2008-11-18T16:24:46Z

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