Breakage caused by unreviewed patch in x86 tree

I'm pissed off because it broke a class of voyager machines: those which

This patch:

commit 6371b495991debfd1417b17c2bc4f7d7bae05739

Author: Ingo Molnar <[email protected]>
Date:   Wed Jan 30 13:33:40 2008 +0100

x86: change ioremap() to default to uncached

As far as I can tell went blindly into the x86 tree without being shared on any mailing list at all. How can something that completely alters the semantics of ioremap on x86 platforms go in without any review.

Quoted 6 lines · James Bottomley

This patch:

commit 6371b495991debfd1417b17c2bc4f7d7bae05739

Author: Ingo Molnar <[email protected]>
Date:   Wed Jan 30 13:33:40 2008 +0100

Quoted 6 lines

x86: change ioremap() to default to uncached

As far as I can tell went blindly into the x86 tree without being shared on any mailing list at all. How can something that completely alters the semantics of ioremap on x86 platforms go in without any review.

what happened before was that on x86 ioremap() was "lax" about the PTE cachability and just said "writeback cached". That was utterly false for most of the real ioremap()s done by drivers, but it happened to work out fine due to the courtesy of BIOSes setting up UC MTRRs that forced those areas into uncachable.

with the PAT changes, what used to be this default and careless ioremap() behavior by x86 turned into a hard cache aliasing conflict. So we pretty much _have to_ default to uncached - like all other sane architectures already did forever. This is a direct consequence of the PAT changes which were discussed on lkml.

But with PAT we take over from the MTRRs on x86 and using a cacheable ioremap() would give us aliasing conflicts and trouble all around the place.

In the Voyager case we should use ioremap_cache(), and thanks for pointing out that dependency of the QIC. Does the patch below fix it for you?

Ingo

--------------->
Subject: x86, voyager: fix ioremap_nocache()
From: Ingo Molnar <[email protected]>
Date: Sun Apr 27 23:21:03 CEST 2008

James Bottomley reported that the following PAT related commit:

| commit 6371b495991debfd1417b17c2bc4f7d7bae05739
| Author: Ingo Molnar <[email protected]>
| Date:   Wed Jan 30 13:33:40 2008 +0100
|
|     x86: change ioremap() to default to uncached

broke Voyager.

James says:

" it broke a class of voyager machines: those which
rely on the quad interrupt controller (QIC). The precis of why they broke is because the QIC does IPIs (or CPIs in its terminology) via cache line interference: you interrupt a processor by moving a designated memory area to write exclusive in the cache (by simply writing to the line) and the CPU acks the interrupt by moving it back to read shared (by reading from it). That area, is, of course, mapped by ioremap, so reversing the ioremap semantics and adding the uncached bit completely breaks the QIC. "

Sorry about that!

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/mach-voyager/voyager_cat.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-x86.q/arch/x86/mach-voyager/voyager_cat.c
===================================================================

arch/x86/mach-voyager/voyager_cat.c void __init voyager_cat_init(void) · line 605

Quoted 10 lines

Index: linux-x86.q/arch/x86/mach-voyager/voyager_cat.c
===================================================================

arch/x86/mach-voyager/voyager_cat.c +1 -1

@@ -602,7 +602,7 @@ void __init voyager_cat_init(void)
 
 		request_resource(&iomem_resource, &res);
 		voyager_SUS = (struct voyager_SUS *)
-		    ioremap(addr, 0x400);
+		    ioremap_cache(addr, 0x400);

Actually, no ... wrong ioremap. That one is the SUS clickmap remap, which is irrelevant whether it's cached or not. The QIC cacheline area is the next one below it.

I've already got the actual patch (with a nice explanation) in the voyager tree. I'll send them all out to the mailing list shortly; I'm just trying to chase down a problem with TSCs before the voyager tree is ready.

arch/x86/mach-voyager/voyager_cat.c void __init voyager_cat_init(void) · line 605

Quoted 4 lines

arch/x86/mach-voyager/voyager_cat.c +1 -1

 		request_resource(&iomem_resource, &res);
 		voyager_SUS = (struct voyager_SUS *)
-		    ioremap(addr, 0x400);
+		    ioremap_cache(addr, 0x400);

Actually, no ... wrong ioremap. That one is the SUS clickmap remap, which is irrelevant whether it's cached or not. The QIC cacheline area is the next one below it.

arch/x86/mach-voyager/voyager_cat.c void __init voyager_cat_init(void) · line 880

Quoted 2 lines

arch/x86/mach-voyager/voyager_cat.c +1 -1

-               qic_addr = (unsigned long)ioremap(qic_addr, 0x400);
+               qic_addr = (unsigned long)ioremap_cache(qic_addr, 0x400);

Quoted 6 lines

with the PAT changes, what used to be this default and careless ioremap() behavior by x86 turned into a hard cache aliasing conflict. So we pretty much _have to_ default to uncached - like all other sane architectures already did forever. This is a direct consequence of the PAT changes which were discussed on lkml.

Quoted 3 lines

But with PAT we take over from the MTRRs on x86 and using a cacheable ioremap() would give us aliasing conflicts and trouble all around the place.

I'm not saying the patch is wrong ... or that just because it broke voyager it shouldn't be done. [...]

[...] What I'm saying is that it shouldn't have been put into the x86 tree without mailing list review.

But, we'd not mind at all posting 1000 x86.git patches to lkml (or another list) every 3 months (or more frequently), if people request that.

You can post whatever patches you like a million times to lkml. That's not the problem.

It's that the patches don't get reviewed, posting them more or to a different place doesn't help that.

Quoted 5 lines · On Sun, 27 Apr 2008 16:31:06 -0700 (PDT) David Miller <[email protected]>…

You can post whatever patches you like a million times to lkml. That's not the problem.

It's that the patches don't get reviewed, posting them more or to a different place doesn't help that.

If you really want to enforce this, I bet it could be automated with scripts around git.

Simply refuse to apply a patch that does not have at least two Signed-off-by/Reviewed-by/Acked-by lines and refuse to apply a "git pull" if there is a changeset like that in the tree.

are you going to tell me that this had been reviewed? Note these ntohs() applied to 8bit values in there. Two signed-off-by, including Greg "two s-o-b are enough to guarantee review"...

You can post whatever patches you like a million times to lkml. That's not the problem.

It's that the patches don't get reviewed, posting them more or to a different place doesn't help that.

Quoted 5 lines

You can post whatever patches you like a million times to lkml. That's not the problem.

It's that the patches don't get reviewed, posting them more or to a different place doesn't help that.

So review them. Your comments strike me as the pot calling the kettle black given the way the network people like to live on their own mailing list.

From: Alan Cox <[email protected]>
Date: Mon, 28 Apr 2008 10:01:54 +0100

Quoted 9 lines

Quoted 5 lines

You can post whatever patches you like a million times to lkml. That's not the problem.

It's that the patches don't get reviewed, posting them more or to a different place doesn't help that.

So review them. Your comments strike me as the pot calling the kettle black given the way the network people like to live on their own mailing list.

Oh contraire. Because we networking folks use a seperate mailing list with a lower signal to noise ratio than lkml, and as a result more specialization, more patches get more review by more specialists.

It's the point I'm trying to make every time the "post everything to lkml" argument gets fronted.
...

Quoted 3 lines

So review them. Your comments strike me as the pot calling the kettle black given the way the network people like to live on their own mailing list.

Oh contraire. Because we networking folks use a seperate mailing list with a lower signal to noise ratio than lkml, and as a result more specialization, more patches get more review by more specialists.

You might want to know that linux-next mainly exists because of how much of this has been going on over the past half year or so.

... but the PAT patchset was all around lkml. The ioremap change was trivial, went into v2.6.25 and was discussed on lkml:

http://lkml.org/lkml/2008/1/25/287

Quoted 3 lines · Ingo Molnar

... but the PAT patchset was all around lkml. The ioremap change was trivial, went into v2.6.25 and was discussed on lkml:

http://lkml.org/lkml/2008/1/25/287

lkml is a hell-hole with a signal/noise ratio worse than slashdot. Any chance you could create a linux-x86 list for x86 specific changes so that a focussed review and discussion can happen?

Here's another piece of the x86 API that's designed to be cached. The dma_declare_coherent_memory() usually represents behind bridge memory that's fully participatory in the coherence model.

Making it uncached damages the utility of this memory because doing cacheline sized burst cycles when needed to it is far faster than individual byte/word/quad writes.

Signed-off-by James Bottomley <[email protected]>

---

arch/x86/kernel/pci-dma.c +0 -0

index 388b113..df83ffd 100644

Isn't there Yet More Breakage in lib/iomap.c, given these new semantics?

        if (flags & IORESOURCE_MEM) {
                if (flags & IORESOURCE_CACHEABLE)
                        return ioremap(start, len);
                return ioremap_nocache(start, len);
        }

Any driver using pci_iomap() (libata, and others) is affected.

I disagree with this semantics change. A number of code places _and drivers_ GET IT RIGHT, and these are all broken now?

I disagree with this semantics change. A number of code places _and drivers_ GET IT RIGHT, and these are all broken now?

From: Jeff Garzik <[email protected]>
Date: Sun, 27 Apr 2008 18:39:24 -0400

Quoted 1 line

I disagree with this semantics change. A number of code places _and drivers_ GET IT RIGHT, and these are all broken now?

[ Note, James's patch that you quoted is about mapping DMA
memory, in dma_declare_coherent_memory(), rather than devices. But I know what you are trying to talk about Jeff. :-) ]

Wrt. ioremap() semanics, it is important to realize that if the implementation of this on x86 has been giving non-cached I/O mappings out up until recently, you can expect that there are hundreds of drivers that might now be broken.

That's the sad fact of the ubiquity of x86, and it doesn't matter how we defined the API is some document.

Anyways, my point is that this angle should be strongly considered in any discussion about ioremap() behavior.

Arguably, the right thing is to not even have ioremap() anymore, just ioremap_{cache,nocache,wc} and consider any unconverted ioremap() as a flag to audit that particular piece of code.

I don't think that can be done "instantly", though, so defaulting ioremap() to uncached (as it apparently has been on other arches already?) is the conservative option in the meantime.

I think this is a reasonable course of action.

That leaves one of Jeff's concerns, what to do with pci_iomap(). That was designed to give mappings with caching enabled, and as a result we probably should make it behave that way.

Quoted 5 lines · David Miller

I think this is a reasonable course of action.

That leaves one of Jeff's concerns, what to do with pci_iomap(). That was designed to give mappings with caching enabled, and as a result we probably should make it behave that way.

Yes, it should. Just be clear that *that* is a semantic change over what the code currently does; it would appear that that is what the code is *trying* to do.

I believe on x86 it will still get clamped by the MTRRs, at least initially (I don't think we have flipped the default MTRR type to WB yet) but that would immediately change the behaviour on non-x86 architectures.

Arguably, the right thing is to not even have ioremap() anymore, just ioremap_{cache,nocache,wc} and consider any unconverted ioremap() as a flag to audit that particular piece of code.

Quoted 1 line · H. Peter Anvin

Arguably, the right thing is to not even have ioremap() anymore, just ioremap_{cache,nocache,wc} and consider any unconverted ioremap() as a flag to audit that particular piece of code.

Seems like a sane plan... last handful might be tough, though.

From: Jeff Garzik <[email protected]>
Date: Sun, 27 Apr 2008 18:39:24 -0400

Quoted 1 line

I disagree with this semantics change. A number of code places _and drivers_ GET IT RIGHT, and these are all broken now?

[ Note, James's patch that you quoted is about mapping DMA
memory, in dma_declare_coherent_memory(), rather than devices. But I know what you are trying to talk about Jeff. :-) ]

Wrt. ioremap() semanics, it is important to realize that if the implementation of this on x86 has been giving non-cached I/O mappings out up until recently, you can expect that there are hundreds of drivers that might now be broken.

That's the sad fact of the ubiquity of x86, and it doesn't matter how we defined the API is some document.

Anyways, my point is that this angle should be strongly considered in any discussion about ioremap() behavior.

Understood.

I guess I am more annoyed that this stealth semantics change appears to have broken everything that depends on pci_iomap(), including 90%+ of all libata drivers, unless I am missing something.

That one piece of code (pci_iomap) was correct under the old semantics, on x86 and elsewhere. It's tested and working nicely, and depended upon by many drivers.

That one piece of code has had no effective change. Under both the old and the new code, both branches functionally because ioremap_nocache(), in one case because of MTRR and in one case because of PAT.

Quoted 7 lines · Jeff Garzik

Understood.

I guess I am more annoyed that this stealth semantics change appears to have broken everything that depends on pci_iomap(), including 90%+ of all libata drivers, unless I am missing something.

That one piece of code (pci_iomap) was correct under the old semantics, on x86 and elsewhere. It's tested and working nicely, and depended upon by many drivers.

That one piece of code has had no effective change. Under both the old and the new code, both branches functionally because ioremap_nocache(), in one case because of MTRR and in one case because of PAT.

Quoted 14 lines · H. Peter Anvin

Quoted 12 lines · Jeff Garzik

Understood.

I guess I am more annoyed that this stealth semantics change appears to have broken everything that depends on pci_iomap(), including 90%+ of all libata drivers, unless I am missing something.

That one piece of code (pci_iomap) was correct under the old semantics, on x86 and elsewhere. It's tested and working nicely, and depended upon by many drivers.

Quoted 4 lines

That one piece of code has had no effective change. Under both the old and the new code, both branches functionally because ioremap_nocache(), in one case because of MTRR and in one case because of PAT.

OK good... libata uses it for controller registers exclusively, so that should be fine from an operational standpoint.

at the very least I
could have pointed out that lib/iomap.c wanted an update, and the 2.5 yo discussion could have resurfaced.

From: Jeff Garzik <[email protected]>
Date: Sun, 27 Apr 2008 18:39:24 -0400

Quoted 2 lines

I disagree with this semantics change. A number of code places _and drivers_ GET IT RIGHT, and these are all broken now?

[ Note, James's patch that you quoted is about mapping DMA
memory, in dma_declare_coherent_memory(), rather than devices. But I know what you are trying to talk about Jeff. :-) ]

Wrt. ioremap() semanics, it is important to realize that if the implementation of this on x86 has been giving non-cached I/O mappings out up until recently, you can expect that there are hundreds of drivers that might now be broken.

Quoted 6 lines · On Sun, 27 Apr 2008 15:46:20 -0700 (PDT) David Miller <[email protected]>…

From: Jeff Garzik <[email protected]>
Date: Sun, 27 Apr 2008 18:39:24 -0400

Quoted 2 lines

I disagree with this semantics change. A number of code places _and drivers_ GET IT RIGHT, and these are all broken now?

Quoted 8 lines

[ Note, James's patch that you quoted is about mapping DMA
memory, in dma_declare_coherent_memory(), rather than devices. But I know what you are trying to talk about Jeff. :-) ]

Wrt. ioremap() semanics, it is important to realize that if the implementation of this on x86 has been giving non-cached I/O mappings out up until recently, you can expect that there are hundreds of drivers that might now be broken.

it's even worse than that.
99% of the time it gave out uncached memory, but if the bios was iffy, it would suddenly give out something else.

The changes to ioremap() recently turned that into "100% uncached". For me, that's the right (because safe) behavior, exactly for the reason you mentioned: it's what happened in practice, and driver writers would assume that to happen.

Anything except uncached would just be insanity as default. (well the old "whatever mood the bios writer was in, usually uncached" is effectively uncached except for weird behaving boxes)

OK, but look; this is why you broke me.

The former meaning of ioremap() was give me whatever caching the mtrrs set up (assuming you actually have mtrrs. For voyager, we don't so it always meant give me cached memory).

The change broke the QIC ioremap area because it absolutely *requires*
cached memory. It drops performance on the Q720 SCSI cards because
caching was used essentially like write combining to reduce the unbursted traffic across the MCA bus.

When the Voyagers were designed, the only way to get them better performace with the slow bus technology was to do a massive amount of caching, so they're optimised to the point where every element on the voyager bus (sort of like the intel frontside bus) is a primary participant in the caching algorithm, and this includes the MCA controllers. So for me, I want every invocation of ioremap to mean ioremap_cached() otherwise I don't benefit from the added caches.

I can fully understand that bus technology has got worse in terms of caching since the voyager heydays. However, I'd rather not be penalised for everyone else's hardware failings. Since the old ioremap is only used in the IORESOURCE_CACHABLE case for PCI, and since that used to defer to the mtrr settings anyway, it probably does make sense to keep it as ioremap_cache() as long as that still defers to the mtrr settings.

I suppose I could think about an ioremap platform override for voyager since, by and large, ioremap_nocache() is the wrong thing on the platform.

Isn't there Yet More Breakage in lib/iomap.c, given these new semantics?

        if (flags & IORESOURCE_MEM) {
                if (flags & IORESOURCE_CACHEABLE)
                        return ioremap(start, len);
                return ioremap_nocache(start, len);
        }

Quoted 13 lines · James Bottomley

Here's another piece of the x86 API that's designed to be cached. The dma_declare_coherent_memory() usually represents behind bridge memory that's fully participatory in the coherence model.

Making it uncached damages the utility of this memory because doing cacheline sized burst cycles when needed to it is far faster than individual byte/word/quad writes.

Signed-off-by: James Bottomley
<[email protected]>

---

arch/x86/kernel/pci-dma.c int dma_declare_coherent_memory(struct device *dev, dma_addr_t bus_addr, · line 219

Quoted 12 lines

arch/x86/kernel/pci-dma.c +0 -0

index 388b113..df83ffd 100644
@@ -214,7 +214,7 @@ int dma_declare_coherent_memory(struct device
 *dev, dma_addr_t bus_addr, 
 	/* FIXME: this routine just ignores

DMA_MEMORY_INCLUDES_CHILDREN */

arch/x86/kernel/pci-dma.c +1 -1

-	mem_base = ioremap(bus_addr, size);
+	mem_base = ioremap_cache(bus_addr, size);
 	if (!mem_base)
 		goto out;

I would rather change drivers to use ioremap_nocache(), and leave the API as-is.

Isn't there Yet More Breakage in lib/iomap.c, given these new semantics?

         if (flags & IORESOURCE_MEM) {
                 if (flags & IORESOURCE_CACHEABLE)
                         return ioremap(start, len);
                 return ioremap_nocache(start, len);
         }

Any driver using pci_iomap() (libata, and others) is affected.

I disagree with this semantics change. A number of code places _and drivers_ GET IT RIGHT, and these are all broken now?

Quoted 14 lines · On Sun, 27 Apr 2008 18:39:24 -0400 Jeff Garzik <[email protected]> wrote:

Quoted 13 lines · James Bottomley

Here's another piece of the x86 API that's designed to be cached. The dma_declare_coherent_memory() usually represents behind bridge memory that's fully participatory in the coherence model.

Making it uncached damages the utility of this memory because doing cacheline sized burst cycles when needed to it is far faster than individual byte/word/quad writes.

Signed-off-by: James Bottomley
<[email protected]>

---

Quoted 4 lines

Quoted 4 lines

arch/x86/kernel/pci-dma.c +0 -0

index 388b113..df83ffd 100644

Quoted 8 lines

arch/x86/kernel/pci-dma.c int dma_declare_coherent_memory(struct device *dev, dma_addr_t bus_addr, · line 219

Quoted 8 lines

arch/x86/kernel/pci-dma.c +0 -0

@@ -214,7 +214,7 @@ int dma_declare_coherent_memory(struct device
 *dev, dma_addr_t bus_addr, 
 	/* FIXME: this routine just ignores

DMA_MEMORY_INCLUDES_CHILDREN */

arch/x86/kernel/pci-dma.c +1 -1

-	mem_base = ioremap(bus_addr, size);
+	mem_base = ioremap_cache(bus_addr, size);
 	if (!mem_base)
 		goto out;

this patch patch is likely broken on x86; or rather, anyone who uses it is... thinking you can find cache coherent memory on a PCI or similar bus that is actually cachable... keep dreaming. (for now; there's talk about extending PCI)

Quoted 27 lines · Arjan van de Ven

Quoted 24 lines · On Sun, 27 Apr 2008 18:39:24 -0400 Jeff Garzik <[email protected]> wrote:

Quoted 23 lines · James Bottomley

Here's another piece of the x86 API that's designed to be cached. The dma_declare_coherent_memory() usually represents behind bridge memory that's fully participatory in the coherence model.

Making it uncached damages the utility of this memory because doing cacheline sized burst cycles when needed to it is far faster than individual byte/word/quad writes.

Signed-off-by: James Bottomley
<[email protected]>

---

(quoted diff) +0 -0

b/arch/x86/kernel/pci-dma.c index 388b113..df83ffd 100644

arch/x86/kernel/pci-dma.c +0 -0

@@ -214,7 +214,7 @@ int dma_declare_coherent_memory(struct
 device *dev, dma_addr_t bus_addr, 
 	/* FIXME: this routine just ignores

DMA_MEMORY_INCLUDES_CHILDREN */

arch/x86/kernel/pci-dma.c +0 -1

-	mem_base = ioremap(bus_addr, size);

Quoted 8 lines

Quoted 3 lines

arch/x86/kernel/pci-dma.c int dma_declare_coherent_memory(struct device *dev, dma_addr_t bus_addr, · line 219

Quoted 3 lines

arch/x86/kernel/pci-dma.c +1 -0

+	mem_base = ioremap_cache(bus_addr, size);
 	if (!mem_base)
 		goto out;

this patch patch is likely broken on x86; or rather, anyone who uses it is... thinking you can find cache coherent memory on a PCI or similar bus that is actually cachable... keep dreaming. (for now; there's talk about extending PCI)

No ... it works for me, and caching is a performance advantage for me too. The only current consumer of this API is the NCR_Q720 SCSI card which keeps a bunch of cacheable memory remote across the MCA bus.

If you think about it logically, most busses are second citizens in the caching hierarchy: they really only get to force a flush and invalidate of the CPU cache line rather than being fully participatory in the coherence protocol.

However, even being second
class is enough of a speed up on slow busses because it allows bursting of the cache line for the bus transfers.

The other consumers are SoC embedded ... so yes, perhaps I should ask about this on linux-arch.

Cached means that the cpu, at any time, can do a speculative read to the memory.
It also means that the cpu can then write the speculated cacheline back at any time later, if some speculation was going to write to the cacheline but didn't actually happen. (before you think this is bogus, at least AMD cpus do this and I can't vouch for Intel cpus never doing this).
If the on-the-bus hardware *ever* writes to the memory without being part of the full cache coherence protocol it's in trouble. Big time.
Even if it sends an invalidate first (which PCI and others just don't allow, not sure about MCA though), it's not enough because the cpu can just read it right back... one needs a "take for ownership" not an "invalidate" for this to work, and that means being part of the full protocol.

I might add that the intel SAPIC functions
in roughly the same manner, so this might break more than just voyager.

Quoted 2 lines · James Bottomley

I might add that the intel SAPIC functions
in roughly the same manner, so this might break more than just voyager.

Are you referring to the IA64 SAPIC here, or something else? The only mention of SAPIC in the x86 tree appear to be naming of fields in ACPI tables.

Quoted 4 lines · H. Peter Anvin

Quoted 2 lines · James Bottomley

I might add that the intel SAPIC functions
in roughly the same manner, so this might break more than just voyager.

Are you referring to the IA64 SAPIC here, or something else? The only mention of SAPIC in the x86 tree appear to be naming of fields in ACPI tables.

Yes, that's the one ... but I believe a class of the xAPICs also used a similar principle.

Quoted 8 lines · James Bottomley

Quoted 6 lines · H. Peter Anvin

Quoted 2 lines · James Bottomley

I might add that the intel SAPIC functions
in roughly the same manner, so this might break more than just voyager.

Are you referring to the IA64 SAPIC here, or something else? The only mention of SAPIC in the x86 tree appear to be naming of fields in ACPI tables.

Quoted 2 lines

Yes, that's the one ... but I believe a class of the xAPICs also used a similar principle.

I certainly have never seen a system on which the APIC has been mapped cacheable. I would be very interested in the details, so if you could elaborate that would be extremely useful.

Quoted 3 lines

Quoted 2 lines

Yes, that's the one ... but I believe a class of the xAPICs also used a similar principle.

I certainly have never seen a system on which the APIC has been mapped cacheable. I would be very interested in the details, so if you could elaborate that would be extremely useful.

Not really ... I just remember when the SAPIC and later the xAPIC details were published as novel nearly a decade ago, I remember saying that some of the voyager interrupt controllers had been using a similar method for years.

This patch:

commit 6371b495991debfd1417b17c2bc4f7d7bae05739

Author: Ingo Molnar <[email protected]>
Date:   Wed Jan 30 13:33:40 2008 +0100

x86: change ioremap() to default to uncached

As far as I can tell went blindly into the x86 tree without being shared on any mailing list at all. How can something that completely alters the semantics of ioremap on x86 platforms go in without any review.

I might add that the intel
SAPIC functions in roughly the same manner, so this might break more than just voyager.

Quoted 6 lines · On Sun, 27 Apr 2008 16:51:25 -0400 James Bottomley…

This patch:

commit 6371b495991debfd1417b17c2bc4f7d7bae05739

Author: Ingo Molnar <[email protected]>
Date:   Wed Jan 30 13:33:40 2008 +0100

Quoted 6 lines

x86: change ioremap() to default to uncached

As far as I can tell went blindly into the x86 tree without being shared on any mailing list at all. How can something that completely alters the semantics of ioremap on x86 platforms go in without any review.

it changed from "whatever coinflip you got" to "predictable outcome".

From: Arjan van de Ven <[email protected]>
Date: Sun, 27 Apr 2008 15:58:03 -0700

Quoted 16 lines

Quoted 12 lines · On Sun, 27 Apr 2008 16:51:25 -0400 James Bottomley…

This patch:

commit 6371b495991debfd1417b17c2bc4f7d7bae05739

Author: Ingo Molnar <[email protected]>
Date:   Wed Jan 30 13:33:40 2008 +0100

x86: change ioremap() to default to uncached

As far as I can tell went blindly into the x86 tree without being shared on any mailing list at all. How can something that completely alters the semantics of ioremap on x86 platforms go in without any review.

Quoted 2 lines

it changed from "whatever coinflip you got" to "predictable outcome".

You're making technical responses to a question about process.

Quoted 6 lines · On Sun, 27 Apr 2008 16:51:25 -0400 James Bottomley…

This patch:

commit 6371b495991debfd1417b17c2bc4f7d7bae05739

Author: Ingo Molnar <[email protected]>
Date:   Wed Jan 30 13:33:40 2008 +0100

Quoted 6 lines

x86: change ioremap() to default to uncached

As far as I can tell went blindly into the x86 tree without being shared on any mailing list at all. How can something that completely alters the semantics of ioremap on x86 platforms go in without any review.

it changed from "whatever coinflip you got" to "predictable outcome".
What you got before was uncached (most of the time), or if the bios was creative, write combining. Or if the bios was broken in how it set up MTRR's, you could suddenly get "cached".

When you're mapping device memory, uncached is the safe default.

With the switch to PAT (and phasing out of MTRR), the kernel needs to pick one of the three (cached, writecombining, uncached) since you can no longer really depend on MTRRs saving your bacon there.
Drivers in general, with VERY few exceptions, want uncached. Any other choice would have been deadly...

I'd like to ask you which one you would pick... you maintain a whole bunch of drivers as scsi maintainer, what would you have picked?
The answer "whatever the MTRR set up" no longer holds ;(

Quoted 3 lines · Arjan van de Ven

On Sun, 27 Apr 2008 16:51:25 -0400
James Bottomley <[email protected]> wrote:
When you're mapping device memory, uncached is the safe default.

Well, for certain device mailboxes, uncached does mean less performant. The voyager breakage was exceptional ... I expect other problems just to result in a loss of performance that caching gave by improving the bursting. If we're lucky, the PCI bridge cache might hide a lot of this.

My major complaint is the lack of review and notice, not the actual patch.

I wouldn't have picked ... I'd have asked for input.