* arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars @ 2009-04-19 19:45 Robert P. J. Day 2009-04-19 21:12 ` Roland Dreier 0 siblings, 1 reply; 50+ messages in thread From: Robert P. J. Day @ 2009-04-19 19:45 UTC (permalink / raw) To: Linux Kernel Mailing List from arch/x86/Kconfig: ... select HAVE_READQ select HAVE_WRITEQ ... yet there are no such defined Kconfig vars anywhere. thoughts? rday - ======================================================================== Robert P. J. Day Waterloo, Ontario, CANADA Linux Consulting, Training and Annoying Kernel Pedantry. Web page: http://crashcourse.ca Linked In: http://www.linkedin.com/in/rpjday Twitter: http://twitter.com/rpjday ======================================================================== ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-19 19:45 arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars Robert P. J. Day @ 2009-04-19 21:12 ` Roland Dreier 2009-04-19 21:46 ` Ingo Molnar 0 siblings, 1 reply; 50+ messages in thread From: Roland Dreier @ 2009-04-19 21:12 UTC (permalink / raw) To: Robert P. J. Day; +Cc: Hitoshi Mitake, Ingo Molnar, Linux Kernel Mailing List > from arch/x86/Kconfig: > ... > select HAVE_READQ > select HAVE_WRITEQ > ... > > yet there are no such defined Kconfig vars anywhere. thoughts? git blame shows that this came in from 2c5643b1 ("x86: provide readq()/writeq() on 32-bit too"). And that commit looks very dubious indeed to me -- it defines readq() and writeq() in a way that is not atomic and probably won't generate single 64-bit bus cycles. Now, many drivers do "#ifndef readq <define my own implementation> #endif" but exactly what is required is very hardware-dependent, and accessing 32-bit halves in the wrong order may lead to very subtle bugs. For example, the changelog for e23a59e1 ("niu: Fix readq implementation when architecture does not provide one.") says: In particular one of the issues is whether the top 32-bits or the bottom 32-bits of the 64-bit register should be read first. There could be side effects, and in fact that is exactly the problem here. By coincidence, the 32-bit x86 implementation is actually OK for niu, but I didn't audit every similar driver, and I don't think any implementation of readq()/writeq() that generates multiple bus cycles is suitable in general -- it doesn't meet the requirements of the API. So I would strongly suggest reverting 2c5643b1 since as far as I can tell it just sets a trap for subtle bugs that only show up on 32-bit x86 -- any portable driver still needs to provide readq()/writeq() for other 32-bit architectures, so it doesn't really help anyone. - R. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-19 21:12 ` Roland Dreier @ 2009-04-19 21:46 ` Ingo Molnar 2009-04-19 22:02 ` H. Peter Anvin 2009-04-20 0:53 ` Roland Dreier 0 siblings, 2 replies; 50+ messages in thread From: Ingo Molnar @ 2009-04-19 21:46 UTC (permalink / raw) To: Roland Dreier, H. Peter Anvin, Thomas Gleixner Cc: Robert P. J. Day, Hitoshi Mitake, Linux Kernel Mailing List * Roland Dreier <rdreier@cisco.com> wrote: > > from arch/x86/Kconfig: > > ... > > select HAVE_READQ > > select HAVE_WRITEQ > > ... > > > > yet there are no such defined Kconfig vars anywhere. thoughts? > > git blame shows that this came in from 2c5643b1 ("x86: provide > readq()/writeq() on 32-bit too"). And that commit looks very > dubious indeed to me -- it defines readq() and writeq() in a way > that is not atomic and probably won't generate single 64-bit bus > cycles. Look at the drivers that define their own wrappers: #ifndef readq static inline unsigned long long readq(void __iomem *addr) { return readl(addr) | (((unsigned long long)readl(addr + 4)) << 32LL); } #endif ... it's the obvious 32-bit semantics for reading a 64-bit value from an mmio address. We made that available on 32-bit too. It's being used ... and has been in use for some time. Where's the problem? readl is serializing on all default-ioremap mmio targets on x86 so there's no ambiguity in ordering. > Now, many drivers do "#ifndef readq <define my own implementation> > #endif" but exactly what is required is very hardware-dependent, > and accessing 32-bit halves in the wrong order may lead to very > subtle bugs. For example, the changelog for e23a59e1 ("niu: Fix > readq implementation when architecture does not provide one.") > says: > > In particular one of the issues is whether the top 32-bits > or the bottom 32-bits of the 64-bit register should be read > first. There could be side effects, and in fact that is > exactly the problem here. > > By coincidence, the 32-bit x86 implementation is actually OK for > niu, but I didn't audit every similar driver, and I don't think > any implementation of readq()/writeq() that generates multiple bus > cycles is suitable in general -- it doesn't meet the requirements > of the API. > > So I would strongly suggest reverting 2c5643b1 since as far as I > can tell it just sets a trap for subtle bugs that only show up on > 32-bit x86 [...] Heh. It "only" shows up on the platform that ~80% of all our kernel testers use? ;-) > [...] -- any portable driver still needs to provide > readq()/writeq() for other 32-bit architectures, so it doesn't > really help anyone. So, are you arguing for a per driver definition of readq/writeq? If so then that does not make much technical sense. If not ... then what is your technical point? Your complains are unfounded i think: Firstly, any such bug would have shown up already. Secondly, currently there's about 4 drivers in mainline that define readq/writeq methods: one of them is a very popular x86 driver and works fine, the other one is niu.c that you just mentioned is fine - the other two are for PARISC. Thirdly, a driver simply has no business defining such a low-level hardware API that just happens not to be implemented on 32-bit (but is implemented on 64-bit). Unless you can point to a real breakage that happened in the past ~6 months since this commit has been upstream, i'm not sure what the fuss is about. Drivers found a hole in our APIs and filled it in an ad-hoc way - we plugged the hole on x86. Pretty much the only valid argument to make here is that it should be filled in on all the other platforms as well - but you cant blame the x86 commit for that, can you? So the only beef i have with that commit at the moment is that the HAVE_READQ / HAVE_WRITEQ Kconfig symbols are still orphan - this stuff should have been implemented in a tree-wide way long ago. Note, there's ongoing work regarding that in -mm - i saw related threads about that, recently. Obviously it take a lot of time to propagate something across a space of 20 architectures. Ingo ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-19 21:46 ` Ingo Molnar @ 2009-04-19 22:02 ` H. Peter Anvin 2009-04-19 22:35 ` Ingo Molnar 2009-04-20 0:53 ` Roland Dreier 1 sibling, 1 reply; 50+ messages in thread From: H. Peter Anvin @ 2009-04-19 22:02 UTC (permalink / raw) To: Ingo Molnar Cc: Roland Dreier, Thomas Gleixner, Robert P. J. Day, Hitoshi Mitake, Linux Kernel Mailing List Ingo Molnar wrote: > > Look at the drivers that define their own wrappers: > > #ifndef readq > static inline unsigned long long readq(void __iomem *addr) > { > return readl(addr) | (((unsigned long long)readl(addr + 4)) << 32LL); > } > #endif > > ... it's the obvious 32-bit semantics for reading a 64-bit value > from an mmio address. We made that available on 32-bit too. > > It's being used ... and has been in use for some time. Where's the > problem? readl is serializing on all default-ioremap mmio targets on > x86 so there's no ambiguity in ordering. > I think his point is that they're not atomic. For what it's worth, atomic readq()/writeq() *are* possible with any x86-32 CPU which supports MMX, but it is very costly to do in the kernel since it involves touching the FPU state. For the vast number of users, a non-atomic primitive which is available for both 32- and 64-bit x86 is a win. For a small number of users, it'll be confusing, and for a very small minority it's going to be desirable to have the atomic primitive. The reason the non-atomic is generally fine is because most device drivers are inherently single-threaded on a per-hardware device basis. -hpa -- H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-19 22:02 ` H. Peter Anvin @ 2009-04-19 22:35 ` Ingo Molnar 2009-04-20 0:56 ` Roland Dreier 0 siblings, 1 reply; 50+ messages in thread From: Ingo Molnar @ 2009-04-19 22:35 UTC (permalink / raw) To: H. Peter Anvin Cc: Roland Dreier, Thomas Gleixner, Robert P. J. Day, Hitoshi Mitake, Linux Kernel Mailing List * H. Peter Anvin <hpa@zytor.com> wrote: > Ingo Molnar wrote: > > > > Look at the drivers that define their own wrappers: > > > > #ifndef readq > > static inline unsigned long long readq(void __iomem *addr) > > { > > return readl(addr) | (((unsigned long long)readl(addr + 4)) << 32LL); > > } > > #endif > > > > ... it's the obvious 32-bit semantics for reading a 64-bit value > > from an mmio address. We made that available on 32-bit too. > > > > It's being used ... and has been in use for some time. Where's > > the problem? readl is serializing on all default-ioremap mmio > > targets on x86 so there's no ambiguity in ordering. > > I think his point is that they're not atomic. [...] Ok - i didnt really consider atomicity, because that's not really feasible for a number of reasons anyway: > [...] For what it's worth, atomic readq()/writeq() *are* possible > with any x86-32 CPU which supports MMX, but it is very costly to > do in the kernel since it involves touching the FPU state. > > For the vast number of users, a non-atomic primitive which is > available for both 32- and 64-bit x86 is a win. For a small > number of users, it'll be confusing, and for a very small minority > it's going to be desirable to have the atomic primitive. > > The reason the non-atomic is generally fine is because most device > drivers are inherently single-threaded on a per-hardware device > basis. Also, atomicity might not be possible to guarantee on the bus level: say the device sits on a 32-bit PCI bus. (No matter what instruction the CPU gets, a readq/writeq there has to be done as two 32-bit bus accesses.) (Also, even a genuine 64-bit device might be bridged over 32-bit pathways so a driver cannot really assume atomicity on that level.) Ingo ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-19 22:35 ` Ingo Molnar @ 2009-04-20 0:56 ` Roland Dreier 2009-04-20 2:08 ` Robert Hancock 0 siblings, 1 reply; 50+ messages in thread From: Roland Dreier @ 2009-04-20 0:56 UTC (permalink / raw) To: Ingo Molnar Cc: H. Peter Anvin, Thomas Gleixner, Robert P. J. Day, Hitoshi Mitake, Linux Kernel Mailing List > Also, atomicity might not be possible to guarantee on the bus level: > say the device sits on a 32-bit PCI bus. (No matter what instruction > the CPU gets, a readq/writeq there has to be done as two 32-bit bus > accesses.) Well, the conventional PCI devices I know of with 64-bit registers were PCI-X cards, keyed so they would only fit into a 64-bit slot. And of course there is no such thing as 32-bit PCI Express. > (Also, even a genuine 64-bit device might be bridged over 32-bit > pathways so a driver cannot really assume atomicity on that level.) I have never even heard of a system with a 64-bit PCI slot that went through a 32-bit pathway -- in fact I'm not sure how one could build that. But yes, for example on 32-bit PowerPC I don't think it's possible to generate a 64-bit bus transaction in general. So if a device requires such a cycle then it simply can't work on such a system. But there is also the case where racing accesses to other registers must be avoided (the mthca example I gave in my previous example) where the current 32-bit x86 definition is broken, but it could be fixed in a driver-specific version that used a spinlock. - R. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-20 0:56 ` Roland Dreier @ 2009-04-20 2:08 ` Robert Hancock 0 siblings, 0 replies; 50+ messages in thread From: Robert Hancock @ 2009-04-20 2:08 UTC (permalink / raw) To: Roland Dreier Cc: Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Robert P. J. Day, Hitoshi Mitake, Linux Kernel Mailing List Roland Dreier wrote: > > Also, atomicity might not be possible to guarantee on the bus level: > > say the device sits on a 32-bit PCI bus. (No matter what instruction > > the CPU gets, a readq/writeq there has to be done as two 32-bit bus > > accesses.) > > Well, the conventional PCI devices I know of with 64-bit registers were > PCI-X cards, keyed so they would only fit into a 64-bit slot. And of > course there is no such thing as 32-bit PCI Express. It's quite possible to use most 64-bit PCI-X cards in a 32-bit slot, with the 64-bit part of the card not plugged into anything. It's supposed to work (as long as the card can handle the voltage the slot uses). ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-19 21:46 ` Ingo Molnar 2009-04-19 22:02 ` H. Peter Anvin @ 2009-04-20 0:53 ` Roland Dreier 2009-04-20 1:20 ` H. Peter Anvin 1 sibling, 1 reply; 50+ messages in thread From: Roland Dreier @ 2009-04-20 0:53 UTC (permalink / raw) To: Ingo Molnar Cc: H. Peter Anvin, Thomas Gleixner, Robert P. J. Day, Hitoshi Mitake, Linux Kernel Mailing List > Look at the drivers that define their own wrappers: > > #ifndef readq > static inline unsigned long long readq(void __iomem *addr) > { > return readl(addr) | (((unsigned long long)readl(addr + 4)) << 32LL); > } > #endif > > ... it's the obvious 32-bit semantics for reading a 64-bit value > from an mmio address. We made that available on 32-bit too. But look at, say, drivers/infiniband/hw/amso100/c2.h: #ifndef readq static inline u64 readq(const void __iomem * addr) { u64 ret = readl(addr + 4); ret <<= 32; ret |= readl(addr); return ret; } #endif Notice that it reads from addr+4 *before* it reads from addr, rather than after as in your example (and in fact your example depends on undefined compiler semantics, since there is no sequence point between the two operands of the | operator). Now, I don't know that hardware, so I don't know if it makes a difference, but the niu example I gave in my original email shows that given hardware with clear-on-read registers, the order does very much matter. In a similar vein, drivers/infiniband/hw/mthca (which I wrote) deals with hardware that has 64-bit registers, where we can write in two 32-bit chunks, as long as we have the right order and no other writes to the same page of registers come in between. So on 32-bit architectures, the driver must use a spinlock around the pair of 32-bit writes (see drivers/infiniband/hw/mthca/mthca_doorbell.h for the code). And the simple fact is that if that driver used "#ifdef writeq" (instead of "#if BITS_PER_LONG == 64" as it actually does) then it would be broken on 32-bit x86 right now. > > So I would strongly suggest reverting 2c5643b1 since as far as I > > can tell it just sets a trap for subtle bugs that only show up on > > 32-bit x86 [...] > Heh. It "only" shows up on the platform that ~80% of all our kernel > testers use? ;-) Well, most of the drivers using readq()/writeq() are probably driving "high-end" hardware (InfiniBand, 10G ethernet, "enterprise" SCSI) that is much more tilted to 64-bit architectures. But yes, such bugs would probably be seen quickly -- but the effort to debug "works on x86-64, fails on x86-32 under high load" bugs is pretty big, given that the symptoms of non-atomic access to a 64-bit register are probably pretty mysterious (you can read about how the niu bug I mentioned was fixed -- it took a while to zero in on the root cause). > So, are you arguing for a per driver definition of readq/writeq? If > so then that does not make much technical sense. If not ... then > what is your technical point? Yes, I am arguing for exactly that, because dealing with the semantics of non-atomic access to 64-bit registers involved low-level knowledge of the specific hardware being driven. As it stands 32-bit x86 has readq()/writeq() that are subtly different subtly different from all other 64-bit architectures, in a way that sets a booby trap for any driver that uses them. So yes I stick to my original point that the commit that added them for 32-bit x86 should be reverted. - R. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-20 0:53 ` Roland Dreier @ 2009-04-20 1:20 ` H. Peter Anvin 2009-04-20 10:53 ` Ingo Molnar 0 siblings, 1 reply; 50+ messages in thread From: H. Peter Anvin @ 2009-04-20 1:20 UTC (permalink / raw) To: Roland Dreier Cc: Ingo Molnar, Thomas Gleixner, Robert P. J. Day, Hitoshi Mitake, Linux Kernel Mailing List Roland Dreier wrote: > > Notice that it reads from addr+4 *before* it reads from addr, rather > than after as in your example (and in fact your example depends on > undefined compiler semantics, since there is no sequence point between > the two operands of the | operator). Now, I don't know that hardware, > so I don't know if it makes a difference, but the niu example I gave in > my original email shows that given hardware with clear-on-read > registers, the order does very much matter. > At least for x86, the order should be low-high, because that is the order that those two transactions would be seen on a 32-bit bus downstream from the CPU if the CPU issued a 64-bit transaction. The only sane way to handle this as something other than per-driver hacks would be something like: #include <linux/io64.h> /* Any 64-bit I/O OK */ #include <linux/io64lh.h> /* Low-high splitting OK */ #include <linux/io64hl.h> /* High-low splitting OK */ #include <linux/io64atomic.h> /* 64-bit I/O must be atomic */ ... i.e. letting the driver choose what fallback method it will accept. -hpa -- H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-20 1:20 ` H. Peter Anvin @ 2009-04-20 10:53 ` Ingo Molnar 2009-04-20 14:47 ` Hitoshi Mitake 0 siblings, 1 reply; 50+ messages in thread From: Ingo Molnar @ 2009-04-20 10:53 UTC (permalink / raw) To: H. Peter Anvin Cc: Roland Dreier, Thomas Gleixner, Robert P. J. Day, Hitoshi Mitake, Linux Kernel Mailing List * H. Peter Anvin <hpa@zytor.com> wrote: > Roland Dreier wrote: > > > > Notice that it reads from addr+4 *before* it reads from addr, rather > > than after as in your example (and in fact your example depends on > > undefined compiler semantics, since there is no sequence point between > > the two operands of the | operator). Now, I don't know that hardware, > > so I don't know if it makes a difference, but the niu example I gave in > > my original email shows that given hardware with clear-on-read > > registers, the order does very much matter. > > > > At least for x86, the order should be low-high, because that is the > order that those two transactions would be seen on a 32-bit bus > downstream from the CPU if the CPU issued a 64-bit transaction. > > The only sane way to handle this as something other than per-driver > hacks would be something like: > > #include <linux/io64.h> /* Any 64-bit I/O OK */ > > #include <linux/io64lh.h> /* Low-high splitting OK */ > > #include <linux/io64hl.h> /* High-low splitting OK */ > > #include <linux/io64atomic.h> /* 64-bit I/O must be atomic */ > > ... i.e. letting the driver choose what fallback method it will accept. Yeah - with the default being the natural low-high order. The other argument is that if a driver really wants some rare, oddly different order it should better define its own method that is not named in the same (or in a similar) way as an existing generic API. Otherwise, confusion will ensue. Ingo ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-20 10:53 ` Ingo Molnar @ 2009-04-20 14:47 ` Hitoshi Mitake 2009-04-20 16:03 ` Ingo Molnar 0 siblings, 1 reply; 50+ messages in thread From: Hitoshi Mitake @ 2009-04-20 14:47 UTC (permalink / raw) To: Ingo Molnar Cc: H. Peter Anvin, Roland Dreier, Thomas Gleixner, Robert P. J. Day, Linux Kernel Mailing List On Mon, Apr 20, 2009 at 19:53, Ingo Molnar <mingo@elte.hu> wrote: > > * H. Peter Anvin <hpa@zytor.com> wrote: > >> Roland Dreier wrote: >> > >> > Notice that it reads from addr+4 *before* it reads from addr, rather >> > than after as in your example (and in fact your example depends on >> > undefined compiler semantics, since there is no sequence point between >> > the two operands of the | operator). Now, I don't know that hardware, >> > so I don't know if it makes a difference, but the niu example I gave in >> > my original email shows that given hardware with clear-on-read >> > registers, the order does very much matter. >> > >> >> At least for x86, the order should be low-high, because that is the >> order that those two transactions would be seen on a 32-bit bus >> downstream from the CPU if the CPU issued a 64-bit transaction. >> >> The only sane way to handle this as something other than per-driver >> hacks would be something like: >> >> #include <linux/io64.h> /* Any 64-bit I/O OK */ >> >> #include <linux/io64lh.h> /* Low-high splitting OK */ >> >> #include <linux/io64hl.h> /* High-low splitting OK */ >> >> #include <linux/io64atomic.h> /* 64-bit I/O must be atomic */ >> >> ... i.e. letting the driver choose what fallback method it will accept. > > Yeah - with the default being the natural low-high order. > > The other argument is that if a driver really wants some rare, oddly > different order it should better define its own method that is not > named in the same (or in a similar) way as an existing generic API. > Otherwise, confusion will ensue. I think this is a good way. readq/writeq are already in Linus's tree, removing these is not a good idea. And I've sent the patch to fix a little problem of Kconfig about readq/writeq to you. http://marc.info/?l=linux-kernel&m=123521109218008&w=2 Did you notice? Adding cautions about accessing order or non-atomic to Kconfig's help part may be benefit. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-20 14:47 ` Hitoshi Mitake @ 2009-04-20 16:03 ` Ingo Molnar 2009-04-21 8:33 ` Hitoshi Mitake 0 siblings, 1 reply; 50+ messages in thread From: Ingo Molnar @ 2009-04-20 16:03 UTC (permalink / raw) To: Hitoshi Mitake Cc: H. Peter Anvin, Roland Dreier, Thomas Gleixner, Robert P. J. Day, Linux Kernel Mailing List * Hitoshi Mitake <h.mitake@gmail.com> wrote: > On Mon, Apr 20, 2009 at 19:53, Ingo Molnar <mingo@elte.hu> wrote: > > > > * H. Peter Anvin <hpa@zytor.com> wrote: > > > >> Roland Dreier wrote: > >> > > >> > Notice that it reads from addr+4 *before* it reads from addr, rather > >> > than after as in your example (and in fact your example depends on > >> > undefined compiler semantics, since there is no sequence point between > >> > the two operands of the | operator). Now, I don't know that hardware, > >> > so I don't know if it makes a difference, but the niu example I gave in > >> > my original email shows that given hardware with clear-on-read > >> > registers, the order does very much matter. > >> > > >> > >> At least for x86, the order should be low-high, because that is the > >> order that those two transactions would be seen on a 32-bit bus > >> downstream from the CPU if the CPU issued a 64-bit transaction. > >> > >> The only sane way to handle this as something other than per-driver > >> hacks would be something like: > >> > >> #include <linux/io64.h> /* Any 64-bit I/O OK */ > >> > >> #include <linux/io64lh.h> /* Low-high splitting OK */ > >> > >> #include <linux/io64hl.h> /* High-low splitting OK */ > >> > >> #include <linux/io64atomic.h> /* 64-bit I/O must be atomic */ > >> > >> ... i.e. letting the driver choose what fallback method it will accept. > > > > Yeah - with the default being the natural low-high order. > > > > The other argument is that if a driver really wants some rare, oddly > > different order it should better define its own method that is not > > named in the same (or in a similar) way as an existing generic API. > > Otherwise, confusion will ensue. > I think this is a good way. > readq/writeq are already in Linus's tree, removing these is not a good idea. > > And I've sent the patch to fix a little problem of Kconfig about > readq/writeq to you. > http://marc.info/?l=linux-kernel&m=123521109218008&w=2 > Did you notice? > > Adding cautions about accessing order or non-atomic to Kconfig's help > part may be benefit. It's better to add add such non-interactive help text as Makefile comments: # # This option ... # and they should be invisible in make menuconfig. This is a facility provided by architectures. Note, the whole patchset is still incomplete - readq/writeq wrappers should be provided on all 32-bit architectures. Are those in the works? Ingo ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-20 16:03 ` Ingo Molnar @ 2009-04-21 8:33 ` Hitoshi Mitake 2009-04-21 8:45 ` Ingo Molnar 2009-04-21 15:44 ` H. Peter Anvin 0 siblings, 2 replies; 50+ messages in thread From: Hitoshi Mitake @ 2009-04-21 8:33 UTC (permalink / raw) To: Ingo Molnar Cc: H. Peter Anvin, Roland Dreier, Thomas Gleixner, Robert P. J. Day, Linux Kernel Mailing List On Tue, Apr 21, 2009 at 01:03, Ingo Molnar <mingo@elte.hu> wrote: > > * Hitoshi Mitake <h.mitake@gmail.com> wrote: > >> On Mon, Apr 20, 2009 at 19:53, Ingo Molnar <mingo@elte.hu> wrote: >> > >> > * H. Peter Anvin <hpa@zytor.com> wrote: >> > >> >> Roland Dreier wrote: >> >> > >> >> > Notice that it reads from addr+4 *before* it reads from addr, rather >> >> > than after as in your example (and in fact your example depends on >> >> > undefined compiler semantics, since there is no sequence point between >> >> > the two operands of the | operator). Now, I don't know that hardware, >> >> > so I don't know if it makes a difference, but the niu example I gave in >> >> > my original email shows that given hardware with clear-on-read >> >> > registers, the order does very much matter. >> >> > >> >> >> >> At least for x86, the order should be low-high, because that is the >> >> order that those two transactions would be seen on a 32-bit bus >> >> downstream from the CPU if the CPU issued a 64-bit transaction. >> >> >> >> The only sane way to handle this as something other than per-driver >> >> hacks would be something like: >> >> >> >> #include <linux/io64.h> /* Any 64-bit I/O OK */ >> >> >> >> #include <linux/io64lh.h> /* Low-high splitting OK */ >> >> >> >> #include <linux/io64hl.h> /* High-low splitting OK */ >> >> >> >> #include <linux/io64atomic.h> /* 64-bit I/O must be atomic */ >> >> >> >> ... i.e. letting the driver choose what fallback method it will accept. >> > >> > Yeah - with the default being the natural low-high order. >> > >> > The other argument is that if a driver really wants some rare, oddly >> > different order it should better define its own method that is not >> > named in the same (or in a similar) way as an existing generic API. >> > Otherwise, confusion will ensue. >> I think this is a good way. >> readq/writeq are already in Linus's tree, removing these is not a good idea. >> >> And I've sent the patch to fix a little problem of Kconfig about >> readq/writeq to you. >> http://marc.info/?l=linux-kernel&m=123521109218008&w=2 >> Did you notice? >> >> Adding cautions about accessing order or non-atomic to Kconfig's help >> part may be benefit. > > It's better to add add such non-interactive help text as Makefile > comments: > > # > # This option ... > # > > and they should be invisible in make menuconfig. This is a facility > provided by architectures. I'll move the help text from Kconfig to Makefile. (My original patch also doesn't make help text visible in make menuconfig.) > > Note, the whole patchset is still incomplete - readq/writeq wrappers > should be provided on all 32-bit architectures. Are those in the > works? I'm not working on porting readq/writeq on all 32-bit architectures. If I port these, HAVE_READQ will be needless. Because there's no reason to judge that architecture provides readq/writeq. Porting readq/writeq on all architectures is radical way to solve. But the problem related to order of accessing and non-atomic still exists. I think there are 3 ways to choose: 1) Removing readq/writeq from x86_32 This is the way Roland mentioned. This way removes the bugs related to order of accessing and non-atomic forever. But driver programmers must implement their own version of readq/writeq, and Andrew Morton said such case is sucks. http://marc.info/?l=linux-kernel&m=122625885124798&w=2 2) Adding HAVE_READQ and HAVE_WRITEQ to Kconfigs of architectures which provides readq/writeq despite of 32/64bit This is the nearest with current state of Linux. But some day non-atomic or order of accessing which driver programmers didn't expect may cause subtle bugs. 3) Porting readq/writeq on all architectures despite of 32/64bit This is a very radical way. This frees us from the problem of "#ifdef readq <implement driver own version> #endif" or HAVE_READQ forever. But the possibility of subtle bugs caused by non-atomic or order of accessing still exists. Which one should we choose? I suggest 2) (or 3)). Because there's no problem since ported readq/writeq on x86_32. And as H. Peter Anvin mentioned non-atomic is generally fine. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-21 8:33 ` Hitoshi Mitake @ 2009-04-21 8:45 ` Ingo Molnar 2009-04-21 8:57 ` Hitoshi Mitake 2009-04-21 15:44 ` H. Peter Anvin 1 sibling, 1 reply; 50+ messages in thread From: Ingo Molnar @ 2009-04-21 8:45 UTC (permalink / raw) To: Hitoshi Mitake Cc: H. Peter Anvin, Roland Dreier, Thomas Gleixner, Robert P. J. Day, Linux Kernel Mailing List * Hitoshi Mitake <h.mitake@gmail.com> wrote: > On Tue, Apr 21, 2009 at 01:03, Ingo Molnar <mingo@elte.hu> wrote: > > > > * Hitoshi Mitake <h.mitake@gmail.com> wrote: > > > >> On Mon, Apr 20, 2009 at 19:53, Ingo Molnar <mingo@elte.hu> wrote: > >> > > >> > * H. Peter Anvin <hpa@zytor.com> wrote: > >> > > >> >> Roland Dreier wrote: > >> >> > > >> >> > Notice that it reads from addr+4 *before* it reads from addr, rather > >> >> > than after as in your example (and in fact your example depends on > >> >> > undefined compiler semantics, since there is no sequence point between > >> >> > the two operands of the | operator). Now, I don't know that hardware, > >> >> > so I don't know if it makes a difference, but the niu example I gave in > >> >> > my original email shows that given hardware with clear-on-read > >> >> > registers, the order does very much matter. > >> >> > > >> >> > >> >> At least for x86, the order should be low-high, because that is the > >> >> order that those two transactions would be seen on a 32-bit bus > >> >> downstream from the CPU if the CPU issued a 64-bit transaction. > >> >> > >> >> The only sane way to handle this as something other than per-driver > >> >> hacks would be something like: > >> >> > >> >> #include <linux/io64.h> /* Any 64-bit I/O OK */ > >> >> > >> >> #include <linux/io64lh.h> /* Low-high splitting OK */ > >> >> > >> >> #include <linux/io64hl.h> /* High-low splitting OK */ > >> >> > >> >> #include <linux/io64atomic.h> /* 64-bit I/O must be atomic */ > >> >> > >> >> ... i.e. letting the driver choose what fallback method it will accept. > >> > > >> > Yeah - with the default being the natural low-high order. > >> > > >> > The other argument is that if a driver really wants some rare, oddly > >> > different order it should better define its own method that is not > >> > named in the same (or in a similar) way as an existing generic API. > >> > Otherwise, confusion will ensue. > >> I think this is a good way. > >> readq/writeq are already in Linus's tree, removing these is not a good idea. > >> > >> And I've sent the patch to fix a little problem of Kconfig about > >> readq/writeq to you. > >> http://marc.info/?l=linux-kernel&m=123521109218008&w=2 > >> Did you notice? > >> > >> Adding cautions about accessing order or non-atomic to Kconfig's help > >> part may be benefit. > > > > It's better to add add such non-interactive help text as Makefile > > comments: > > > > # > > # This option ... > > # > > > > and they should be invisible in make menuconfig. This is a facility > > provided by architectures. > I'll move the help text from Kconfig to Makefile. > (My original patch also doesn't make help text visible in make menuconfig.) sorry i meant the Kconfig file - you can put comments in there too. help text is really meant for things that are interactive. Ingo ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-21 8:45 ` Ingo Molnar @ 2009-04-21 8:57 ` Hitoshi Mitake 0 siblings, 0 replies; 50+ messages in thread From: Hitoshi Mitake @ 2009-04-21 8:57 UTC (permalink / raw) To: Ingo Molnar Cc: H. Peter Anvin, Roland Dreier, Thomas Gleixner, Robert P. J. Day, Linux Kernel Mailing List On Tue, Apr 21, 2009 at 17:45, Ingo Molnar <mingo@elte.hu> wrote: > > * Hitoshi Mitake <h.mitake@gmail.com> wrote: > >> On Tue, Apr 21, 2009 at 01:03, Ingo Molnar <mingo@elte.hu> wrote: >> > >> > * Hitoshi Mitake <h.mitake@gmail.com> wrote: >> > >> >> On Mon, Apr 20, 2009 at 19:53, Ingo Molnar <mingo@elte.hu> wrote: >> >> > >> >> > * H. Peter Anvin <hpa@zytor.com> wrote: >> >> > >> >> >> Roland Dreier wrote: >> >> >> > >> >> >> > Notice that it reads from addr+4 *before* it reads from addr, rather >> >> >> > than after as in your example (and in fact your example depends on >> >> >> > undefined compiler semantics, since there is no sequence point between >> >> >> > the two operands of the | operator). Now, I don't know that hardware, >> >> >> > so I don't know if it makes a difference, but the niu example I gave in >> >> >> > my original email shows that given hardware with clear-on-read >> >> >> > registers, the order does very much matter. >> >> >> > >> >> >> >> >> >> At least for x86, the order should be low-high, because that is the >> >> >> order that those two transactions would be seen on a 32-bit bus >> >> >> downstream from the CPU if the CPU issued a 64-bit transaction. >> >> >> >> >> >> The only sane way to handle this as something other than per-driver >> >> >> hacks would be something like: >> >> >> >> >> >> #include <linux/io64.h> /* Any 64-bit I/O OK */ >> >> >> >> >> >> #include <linux/io64lh.h> /* Low-high splitting OK */ >> >> >> >> >> >> #include <linux/io64hl.h> /* High-low splitting OK */ >> >> >> >> >> >> #include <linux/io64atomic.h> /* 64-bit I/O must be atomic */ >> >> >> >> >> >> ... i.e. letting the driver choose what fallback method it will accept. >> >> > >> >> > Yeah - with the default being the natural low-high order. >> >> > >> >> > The other argument is that if a driver really wants some rare, oddly >> >> > different order it should better define its own method that is not >> >> > named in the same (or in a similar) way as an existing generic API. >> >> > Otherwise, confusion will ensue. >> >> I think this is a good way. >> >> readq/writeq are already in Linus's tree, removing these is not a good idea. >> >> >> >> And I've sent the patch to fix a little problem of Kconfig about >> >> readq/writeq to you. >> >> http://marc.info/?l=linux-kernel&m=123521109218008&w=2 >> >> Did you notice? >> >> >> >> Adding cautions about accessing order or non-atomic to Kconfig's help >> >> part may be benefit. >> > >> > It's better to add add such non-interactive help text as Makefile >> > comments: >> > >> > # >> > # This option ... >> > # >> > >> > and they should be invisible in make menuconfig. This is a facility >> > provided by architectures. >> I'll move the help text from Kconfig to Makefile. >> (My original patch also doesn't make help text visible in make menuconfig.) > > sorry i meant the Kconfig file - you can put comments in there too. > help text is really meant for things that are interactive. Can I think that I should add help text to both of Kconfig and Makefile? (I'm not a native English speaker, and I'm not good at English. I'm sorry if I misunderstand something...) ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-21 8:33 ` Hitoshi Mitake 2009-04-21 8:45 ` Ingo Molnar @ 2009-04-21 15:44 ` H. Peter Anvin 2009-04-21 17:07 ` Roland Dreier 1 sibling, 1 reply; 50+ messages in thread From: H. Peter Anvin @ 2009-04-21 15:44 UTC (permalink / raw) To: Hitoshi Mitake Cc: Ingo Molnar, Roland Dreier, Thomas Gleixner, Robert P. J. Day, Linux Kernel Mailing List Hitoshi Mitake wrote: > I suggest 2) (or 3)). > Because there's no problem since ported readq/writeq on x86_32. > And as H. Peter Anvin mentioned non-atomic is generally fine. Okay, I'm going to throw in yet another wrench in the machinery. There are devices which really only want to use writeq() if it is an inexpensive (no x86 MMX hacks) atomic operation. What follows is *NOT* a hypothetical example, I worked on a real device which behaved this way: Consider a device with a set of control registers in device memory. You can issue a whole command sequence at one point, but the command isn't activated until a write is received with a certain bit set (located in the high half of the last qword.) For performance, the device provides its registers mapped to two different physical addresses, with the intent that the OS will map one of them write-combining. Thus, what you want is to perform all writes but the very last one as a write-combining write; the very last write should be a conventional I/O write that flushes the write combiners ahead of itself and triggers the write. Now, in theory you could use writel() for this, but if you have a working writeq() [e.g. 64-bit mode] you want to use it. So it's important that the driver can know when readq/writeq are emulated at all. One way to deal with that is the <linux/io64*.h> method, another is with feature test macros. -hpa -- H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-21 15:44 ` H. Peter Anvin @ 2009-04-21 17:07 ` Roland Dreier 2009-04-21 17:19 ` H. Peter Anvin 2009-04-22 0:25 ` David Miller 0 siblings, 2 replies; 50+ messages in thread From: Roland Dreier @ 2009-04-21 17:07 UTC (permalink / raw) To: H. Peter Anvin Cc: Hitoshi Mitake, Ingo Molnar, Thomas Gleixner, Robert P. J. Day, Linux Kernel Mailing List > Okay, I'm going to throw in yet another wrench in the machinery. > There are devices which really only want to use writeq() if it is an > inexpensive (no x86 MMX hacks) atomic operation. Interesting example. It does seem there are several reasonable hardware design patters where a driver needs to know if writeq() and/or readq() is atomic. > One way to deal with that is the <linux/io64*.h> method, another is with > feature test macros. A further idea would be to add readq_atomic()/writeq_atomic() that behave as the current 64-bit versions do: a single instruction, generates a single bus cycle, etc. Then drivers that really need to use the full semantics that the 64-bit versions gave can use those, while drivers that just want a convenient way to read a 64-bit register can use readq()/writeq(). This only makes sense if we define a 32-bit fallback for readq()/writeq() for all 32-bit architectures -- in fact it would be good to do it in asm-generic so that there can be a single implementation that guarantees that non-atomic versions always do, say, low 32 bits then high 32 bits. (So eg niu can use the generic version) And then drivers like drivers/infiniband/hw/mthca can be switched to "#ifndef writeq_atomic <...hardware specific fallback...>" However I worry that this just leaves driver authors too much rope. Choosing readq_atomic() vs. readq() is just one more thing to get wrong. - R. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-21 17:07 ` Roland Dreier @ 2009-04-21 17:19 ` H. Peter Anvin 2009-04-21 17:23 ` Roland Dreier 2009-04-22 0:25 ` David Miller 1 sibling, 1 reply; 50+ messages in thread From: H. Peter Anvin @ 2009-04-21 17:19 UTC (permalink / raw) To: Roland Dreier Cc: Hitoshi Mitake, Ingo Molnar, Thomas Gleixner, Robert P. J. Day, Linux Kernel Mailing List Roland Dreier wrote: > > However I worry that this just leaves driver authors too much rope. > Choosing readq_atomic() vs. readq() is just one more thing to get wrong. > ... as is having each driver implementing their own substitutes. -hpa -- H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-21 17:19 ` H. Peter Anvin @ 2009-04-21 17:23 ` Roland Dreier 2009-04-21 19:09 ` H. Peter Anvin 0 siblings, 1 reply; 50+ messages in thread From: Roland Dreier @ 2009-04-21 17:23 UTC (permalink / raw) To: H. Peter Anvin Cc: Hitoshi Mitake, Ingo Molnar, Thomas Gleixner, Robert P. J. Day, Linux Kernel Mailing List > > However I worry that this just leaves driver authors too much rope. > > Choosing readq_atomic() vs. readq() is just one more thing to get wrong. > ... as is having each driver implementing their own substitutes. Yes, I agree with that. However at least status quo ante (readq/writeq 64-bit only) means that driver authors who use readq/writeq are forced (by a compile error) to spend a little thought on what 32-bit fallback they should use. I guess one possibility is to make readq/writeq the atomic version, and add readq_nonatomic()/writeq_nonatomic() for 32-bit architectures. Then it's much more opt-in -- but then that makes the (perhaps) more common case look a bit uglier. - R. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-21 17:23 ` Roland Dreier @ 2009-04-21 19:09 ` H. Peter Anvin 2009-04-21 21:11 ` Roland Dreier 2009-04-22 0:27 ` arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars David Miller 0 siblings, 2 replies; 50+ messages in thread From: H. Peter Anvin @ 2009-04-21 19:09 UTC (permalink / raw) To: Roland Dreier Cc: Hitoshi Mitake, Ingo Molnar, Thomas Gleixner, Robert P. J. Day, Linux Kernel Mailing List Roland Dreier wrote: > > > However I worry that this just leaves driver authors too much rope. > > > Choosing readq_atomic() vs. readq() is just one more thing to get wrong. > > > ... as is having each driver implementing their own substitutes. > > Yes, I agree with that. However at least status quo ante (readq/writeq > 64-bit only) means that driver authors who use readq/writeq are forced > (by a compile error) to spend a little thought on what 32-bit fallback > they should use. > > I guess one possibility is to make readq/writeq the atomic version, and > add readq_nonatomic()/writeq_nonatomic() for 32-bit architectures. Then > it's much more opt-in -- but then that makes the (perhaps) more common > case look a bit uglier. > Do you really expect driver authors to type writeq_nonatomic() for every register reference? I think an #include at the top is one thing, but something that heavyweight for each call site really isn't going to fly. -hpa ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-21 19:09 ` H. Peter Anvin @ 2009-04-21 21:11 ` Roland Dreier 2009-04-21 21:16 ` H. Peter Anvin 2009-04-22 0:27 ` arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars David Miller 1 sibling, 1 reply; 50+ messages in thread From: Roland Dreier @ 2009-04-21 21:11 UTC (permalink / raw) To: H. Peter Anvin Cc: Hitoshi Mitake, Ingo Molnar, Thomas Gleixner, Robert P. J. Day, Linux Kernel Mailing List > Do you really expect driver authors to type writeq_nonatomic() for > every register reference? No -- that's why I didn't even bring it up at first and why I consider it ugly. > I think an #include at the top is one thing, but something that > heavyweight for each call site really isn't going to fly. Yeah, I guess that could work, although I do worry that debugging the wrong choice of #include might be a pain (mysterious symptoms on 32-bit architectures caused by the name of an include file would be hard to track). To be honest I think the status quo ante was not really that bad. - R. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-21 21:11 ` Roland Dreier @ 2009-04-21 21:16 ` H. Peter Anvin 2009-04-22 0:31 ` David Miller 0 siblings, 1 reply; 50+ messages in thread From: H. Peter Anvin @ 2009-04-21 21:16 UTC (permalink / raw) To: Roland Dreier Cc: Hitoshi Mitake, Ingo Molnar, Thomas Gleixner, Robert P. J. Day, Linux Kernel Mailing List Roland Dreier wrote: > > To be honest I think the status quo ante was not really that bad. > That I have to vehemently disagree with. -hpa ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-21 21:16 ` H. Peter Anvin @ 2009-04-22 0:31 ` David Miller 2009-04-28 19:05 ` [PATCH] x86: Remove readq()/writeq() on 32-bit Roland Dreier 0 siblings, 1 reply; 50+ messages in thread From: David Miller @ 2009-04-22 0:31 UTC (permalink / raw) To: hpa; +Cc: rdreier, h.mitake, mingo, tglx, rpjday, linux-kernel From: "H. Peter Anvin" <hpa@zytor.com> Date: Tue, 21 Apr 2009 14:16:31 -0700 > Roland Dreier wrote: >> To be honest I think the status quo ante was not really that bad. > > That I have to vehemently disagree with. I have to agree with Roland. Unless you make it a compile failure, no driver author is going to spend any amount of time trying to figure out how to deal with this situation properly. So in this sense, the current situation works really well. If you make it just compile and make an arbitrary choice of whether the top-32bits is read first or not, you're going to end up with mysterious driver failures that only occur on some machines and the cause of which won't be determined until after a lot of painful digging. This painful debugging experience is eliminated if the driver author is told with a compile failure that there is an issue to resolve. And that is what happens right now. ^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-04-22 0:31 ` David Miller @ 2009-04-28 19:05 ` Roland Dreier 2009-04-29 5:12 ` David Miller 0 siblings, 1 reply; 50+ messages in thread From: Roland Dreier @ 2009-04-28 19:05 UTC (permalink / raw) To: hpa, mingo, tglx; +Cc: David Miller, h.mitake, rpjday, linux-kernel As discussed in <http://lkml.org/lkml/2009/4/19/164> and follow-ups, readq()/writeq() for 32-bit x86 are implemented as two readl()/writel() operations. This is not atomic (in the sense that another MMIO operation from another CPU or thread can be done in the middle of the two read/writes), and may not access the two halves of the register in the correct order to work with hardware. Rather than silently providing a 32-bit fallback that leaves a possibility for strange driver bugs, it's better to provide readq() and writeq() only for 64-bit architectures, and have a compile failure on 32-bit architectures that forces driver authors to think about what the correct solution is. This essentially reverts 2c5643b1 ("x86: provide readq()/writeq() on 32-bit too") and follow-on commits. If in the future someone wants to provide a generic solution for all 32-bit architectures, that's great, but there's not much point in providing (arguably broken) implementations for only one architecture, since any portable driver will have to implement fallbacks for other architectures anyway. Signed-off-by: Roland Dreier <rolandd@cisco.com> --- We never seemed to reach closure on this. I would strongly suggest merging something like this, and then if someone has a grand plan to unify all fallbacks, we can add that when it shows up. As it stands, the x86-32 situation is not progress towards that grand unified plans, and does nothing that I can tell beyond setting a trap for drivers. arch/x86/Kconfig | 2 -- arch/x86/include/asm/io.h | 23 ++--------------------- 2 files changed, 2 insertions(+), 23 deletions(-) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index df9e885..0be277b 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -19,8 +19,6 @@ config X86_64 config X86 def_bool y select HAVE_AOUT if X86_32 - select HAVE_READQ - select HAVE_WRITEQ select HAVE_UNSTABLE_SCHED_CLOCK select HAVE_IDE select HAVE_OPROFILE diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h index 7373932..199c7b9 100644 --- a/arch/x86/include/asm/io.h +++ b/arch/x86/include/asm/io.h @@ -51,27 +51,6 @@ build_mmio_write(__writel, "l", unsigned int, "r", ) build_mmio_read(readq, "q", unsigned long, "=r", :"memory") build_mmio_write(writeq, "q", unsigned long, "r", :"memory") -#else - -static inline __u64 readq(const volatile void __iomem *addr) -{ - const volatile u32 __iomem *p = addr; - u32 low, high; - - low = readl(p); - high = readl(p + 1); - - return low + ((u64)high << 32); -} - -static inline void writeq(__u64 val, volatile void __iomem *addr) -{ - writel(val, addr); - writel(val >> 32, addr+4); -} - -#endif - #define readq_relaxed(a) readq(a) #define __raw_readq(a) readq(a) @@ -81,6 +60,8 @@ static inline void writeq(__u64 val, volatile void __iomem *addr) #define readq readq #define writeq writeq +#endif + /** * virt_to_phys - map virtual addresses to physical * @address: address to remap ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-04-28 19:05 ` [PATCH] x86: Remove readq()/writeq() on 32-bit Roland Dreier @ 2009-04-29 5:12 ` David Miller 2009-04-29 11:56 ` Ingo Molnar 0 siblings, 1 reply; 50+ messages in thread From: David Miller @ 2009-04-29 5:12 UTC (permalink / raw) To: rdreier; +Cc: hpa, mingo, tglx, h.mitake, rpjday, linux-kernel From: Roland Dreier <rdreier@cisco.com> Date: Tue, 28 Apr 2009 12:05:10 -0700 > As discussed in <http://lkml.org/lkml/2009/4/19/164> and follow-ups, > readq()/writeq() for 32-bit x86 are implemented as two readl()/writel() > operations. This is not atomic (in the sense that another MMIO > operation from another CPU or thread can be done in the middle of the > two read/writes), and may not access the two halves of the register in > the correct order to work with hardware. > > Rather than silently providing a 32-bit fallback that leaves a > possibility for strange driver bugs, it's better to provide readq() > and writeq() only for 64-bit architectures, and have a compile failure > on 32-bit architectures that forces driver authors to think about what > the correct solution is. > > This essentially reverts 2c5643b1 ("x86: provide readq()/writeq() on > 32-bit too") and follow-on commits. If in the future someone wants to > provide a generic solution for all 32-bit architectures, that's great, > but there's not much point in providing (arguably broken) > implementations for only one architecture, since any portable driver > will have to implement fallbacks for other architectures anyway. > > Signed-off-by: Roland Dreier <rolandd@cisco.com> Acked-by: David S. Miller <davem@davemloft.net> ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-04-29 5:12 ` David Miller @ 2009-04-29 11:56 ` Ingo Molnar 2009-04-29 12:10 ` Jeff Garzik 2009-04-29 17:21 ` Roland Dreier 0 siblings, 2 replies; 50+ messages in thread From: Ingo Molnar @ 2009-04-29 11:56 UTC (permalink / raw) To: David Miller, Linus Torvalds Cc: rdreier, hpa, tglx, h.mitake, rpjday, linux-kernel (Linus Cc:-ed) * David Miller <davem@davemloft.net> wrote: > From: Roland Dreier <rdreier@cisco.com> > Date: Tue, 28 Apr 2009 12:05:10 -0700 > > > As discussed in <http://lkml.org/lkml/2009/4/19/164> and follow-ups, > > readq()/writeq() for 32-bit x86 are implemented as two readl()/writel() > > operations. This is not atomic (in the sense that another MMIO > > operation from another CPU or thread can be done in the middle of the > > two read/writes), and may not access the two halves of the register in > > the correct order to work with hardware. > > > > Rather than silently providing a 32-bit fallback that leaves a > > possibility for strange driver bugs, it's better to provide readq() > > and writeq() only for 64-bit architectures, and have a compile failure > > on 32-bit architectures that forces driver authors to think about what > > the correct solution is. > > > > This essentially reverts 2c5643b1 ("x86: provide readq()/writeq() on > > 32-bit too") and follow-on commits. If in the future someone wants to > > provide a generic solution for all 32-bit architectures, that's great, > > but there's not much point in providing (arguably broken) > > implementations for only one architecture, since any portable driver > > will have to implement fallbacks for other architectures anyway. > > > > Signed-off-by: Roland Dreier <rolandd@cisco.com> > > Acked-by: David S. Miller <davem@davemloft.net> [...] > > We never seemed to reach closure on this. I would strongly > > suggest merging something like this, and then if someone has a > > grand plan to unify all fallbacks, we can add that when it shows > > up. As it stands, the x86-32 situation is not progress towards > > that grand unified plans, and does nothing that I can tell > > beyond setting a trap for drivers. I still have no particularly strong opinion on this - other the reluctance i expressed already in the previous threads. My arguments are not reflected (and not addressed) in the changelog AFAICS, so let me repeat them here: Firstly, it doesnt really matter in practice because such use is very rare and non-spinlocked access to IO regions is even rarer. What caused 2c5643b1 was that right now we have ugly per driver #defines and inlines for readq/writeq. See: git grep 'define.*readq' drivers/ for a rough estimation of what the current practices are. The 32-bit wrapper we added 6 months ago is the obvious implementation on x86 and that it matches existing wrappers. Atomicity of a 64-bit IO space access on 32-bit platforms, on an unknown-bitness transport (it might even be a 64-bit PCI device bridged over a 32-bit bridge) is obviously not guaranteed. Not even 64-bit-on-64-bit is really guaranteed to be atomic. The bitness here is what the CPU runs its _own_ code in (and how it accesses its cached memory space) - it does not transform over to the uncached IO bus. So trying to suggest that 64-bit readq/writeq when running on a 64-bit kernel is somehow atomic (or can be made atomic) is really wrong IMO. The 32-bit wrapper is simply the expression of how the CPU would do a 64-bit access even in 64-bit mode anyway [if the transport is 32-bit]. aligned 32-bit access can be assumed atomic to a certain degree by virtue of PCI being 32 bit or better - but assuming any 64-bit IO-space read/write atomicity is wrong on many levels. Driver authors will have to think about it anyway _even on 64-bit_, regardless of the existence of a 32-bit fallback. A driver and hw _might_ be quirky and might require atomicity or might define a different order of access ... but then _that_ driver should become ugly, not all the others, right? So my (slight) preference would be to keep the default generic implementation and not make any atomicity guarantees - we never made any. _If_ you want atomicity then provide a readq_atomic() / writeq_atomic() facility, with various higher level checks that make it sure that the IO transport is really atomic. (i dont see this happening any time soon for anything else but some really rare high-end IO transport.) For the common case of there not being any atomicity assumption on the driver case it should result in cleaner code. (assuming all other 64-bit architectures implement a fallback too) But ... i might be wrong about it, so i've Cc:-ed Linus who usually has a rather strong opinion about IO APIs. I'll apply the patch if Linus acks it (or Linus might take it straight out of email). Thanks, Ingo ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-04-29 11:56 ` Ingo Molnar @ 2009-04-29 12:10 ` Jeff Garzik 2009-04-29 17:25 ` Roland Dreier 2009-04-29 17:21 ` Roland Dreier 1 sibling, 1 reply; 50+ messages in thread From: Jeff Garzik @ 2009-04-29 12:10 UTC (permalink / raw) To: Ingo Molnar Cc: David Miller, Linus Torvalds, rdreier, hpa, tglx, h.mitake, rpjday, linux-kernel Ingo Molnar wrote: > (Linus Cc:-ed) > > * David Miller <davem@davemloft.net> wrote: > >> From: Roland Dreier <rdreier@cisco.com> >> Date: Tue, 28 Apr 2009 12:05:10 -0700 >> >>> As discussed in <http://lkml.org/lkml/2009/4/19/164> and follow-ups, >>> readq()/writeq() for 32-bit x86 are implemented as two readl()/writel() >>> operations. This is not atomic (in the sense that another MMIO >>> operation from another CPU or thread can be done in the middle of the >>> two read/writes), and may not access the two halves of the register in >>> the correct order to work with hardware. >>> >>> Rather than silently providing a 32-bit fallback that leaves a >>> possibility for strange driver bugs, it's better to provide readq() >>> and writeq() only for 64-bit architectures, and have a compile failure >>> on 32-bit architectures that forces driver authors to think about what >>> the correct solution is. >>> >>> This essentially reverts 2c5643b1 ("x86: provide readq()/writeq() on >>> 32-bit too") and follow-on commits. If in the future someone wants to >>> provide a generic solution for all 32-bit architectures, that's great, >>> but there's not much point in providing (arguably broken) >>> implementations for only one architecture, since any portable driver >>> will have to implement fallbacks for other architectures anyway. >>> >>> Signed-off-by: Roland Dreier <rolandd@cisco.com> >> Acked-by: David S. Miller <davem@davemloft.net> > [...] >>> We never seemed to reach closure on this. I would strongly >>> suggest merging something like this, and then if someone has a >>> grand plan to unify all fallbacks, we can add that when it shows >>> up. As it stands, the x86-32 situation is not progress towards >>> that grand unified plans, and does nothing that I can tell >>> beyond setting a trap for drivers. > > I still have no particularly strong opinion on this - other the > reluctance i expressed already in the previous threads. My arguments > are not reflected (and not addressed) in the changelog AFAICS, so > let me repeat them here: I do. > What caused 2c5643b1 was that right now we have ugly per driver > #defines and inlines for readq/writeq. See: > > git grep 'define.*readq' drivers/ > > for a rough estimation of what the current practices are. The 32-bit > wrapper we added 6 months ago is the obvious implementation on x86 > and that it matches existing wrappers. This is the key... > So my (slight) preference would be to keep the default generic > implementation and not make any atomicity guarantees - we never made > any. Agreed. This removal patch is completely pointless, because it moves us backwards to the point where we had a bunch of drivers defining it. Why is that any better? "Forcing driver writers to think" translates in the real world to each hardware vendor putting the common "#define readq" into their driver. At least the networking drivers I messed with (until 11/2008) were always fine with a non-atomic readq. The x86 kernel 32-bit implementation of readq/writeq is the code that every hardware vendor otherwise would re-create, when doing a Linux driver. Jeff ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-04-29 12:10 ` Jeff Garzik @ 2009-04-29 17:25 ` Roland Dreier 2009-04-29 19:59 ` Jeff Garzik 0 siblings, 1 reply; 50+ messages in thread From: Roland Dreier @ 2009-04-29 17:25 UTC (permalink / raw) To: Jeff Garzik Cc: Ingo Molnar, David Miller, Linus Torvalds, hpa, tglx, h.mitake, rpjday, linux-kernel > This removal patch is completely pointless, because it moves us > backwards to the point where we had a bunch of drivers defining it. No, the current kernel still requires drivers to define it anyway, because there are tons of 32-bit architectures that are not x86. And more than that, centralizing the definition makes the API much more dangerous for driver authors. > At least the networking drivers I messed with (until 11/2008) were > always fine with a non-atomic readq. The commit to niu I keep citing (e23a59e1, "niu: Fix readq implementation when architecture does not provide one.") shows that drivers need to take care. Now, the x86 implementation would happen to work for that hardware, but eg drivers/infiniband/hw/amso1100 defines readq with the opposite order -- whether that's required or just an arbitrary choice, I don't know. And drivers/infiniband/hw/mthca has some uses of __raw_writeq() that only work if no other CPU accesses to the same page can happen between the two halves, so it adds a per-page spinlock for 32-bit architectures. etc. - R. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-04-29 17:25 ` Roland Dreier @ 2009-04-29 19:59 ` Jeff Garzik 2009-05-13 5:32 ` Hitoshi Mitake 0 siblings, 1 reply; 50+ messages in thread From: Jeff Garzik @ 2009-04-29 19:59 UTC (permalink / raw) To: Roland Dreier Cc: Ingo Molnar, David Miller, Linus Torvalds, hpa, tglx, h.mitake, rpjday, linux-kernel Roland Dreier wrote: > > This removal patch is completely pointless, because it moves us > > backwards to the point where we had a bunch of drivers defining it. > > No, the current kernel still requires drivers to define it anyway, > because there are tons of 32-bit architectures that are not x86. Then let's fix that issue... by propagating the common definition to other platforms that properly implement {read,write}[bwl] in terms of the PCI bus. > And more than that, centralizing the definition makes the API much more > dangerous for driver authors. I think that's really cranking the hyperbole level to 11. The common definition is... the one found most commonly in the wild. For weird drivers, they will do their own thing. That's pretty much how other drivers handle things. Apply your logic here to _any_ API in the kernel, for the same result. > > At least the networking drivers I messed with (until 11/2008) were > > always fine with a non-atomic readq. > > The commit to niu I keep citing (e23a59e1, "niu: Fix readq > implementation when architecture does not provide one.") shows that > drivers need to take care. Now, the x86 implementation would happen to That commit also shows that, had the driver been using a common definition, problems would not have arisen. > work for that hardware, but eg drivers/infiniband/hw/amso1100 defines > readq with the opposite order -- whether that's required or just an 'required' seems unlikely, given that a) their readq only exists when #ifndef readq, thus implying the driver-local readq is far less tested, on their most-tested, highest-volume platform. b) their readq still operates in LE order -- as it should: read,write[bwl] were defined in terms of PCI originally, and thus defined to be LE. c) their __raw_writeq writes in lower-32-bits-first, as one would expect > arbitrary choice, I don't know. And drivers/infiniband/hw/mthca has > some uses of __raw_writeq() that only work if no other CPU accesses to > the same page can happen between the two halves, so it adds a per-page > spinlock for 32-bit architectures. etc. Any use of __raw_xxx implies that You Know What You're Doing And Accept The Consequences. __raw_xxx means _you_ handle endian conversions, barriers, and other arch-specific details. I don't think that a driver intentionally using the "raw" APIs is a good source of ideas and generalizations. So, for your three examples, 1) niu - common definition is OK 2) amso1100 - common definition is OK; driver-local definition never used on common PCI platforms 3) mthca - intentionally uses raw API, an API which ditches arch-specific barriers, endian conversions, and other guarantees. Given that, I see zero justification for API removal. I see justification for propagating this code to other PCI-capable platforms. Finally, I think given all this time we've had driver-define writeq and readq, and "driver authors were forced to think about this API" -- the result was the obvious definition now in place! Jeff ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-04-29 19:59 ` Jeff Garzik @ 2009-05-13 5:32 ` Hitoshi Mitake 2009-05-13 20:19 ` H. Peter Anvin 2009-05-13 20:42 ` Jeff Garzik 0 siblings, 2 replies; 50+ messages in thread From: Hitoshi Mitake @ 2009-05-13 5:32 UTC (permalink / raw) To: Jeff Garzik Cc: Roland Dreier, Ingo Molnar, David Miller, Linus Torvalds, hpa, tglx, rpjday, linux-kernel On Thu, Apr 30, 2009 at 04:59, Jeff Garzik <jeff@garzik.org> wrote: > Roland Dreier wrote: >> >> > This removal patch is completely pointless, because it moves us >> > backwards to the point where we had a bunch of drivers defining it. >> >> No, the current kernel still requires drivers to define it anyway, >> because there are tons of 32-bit architectures that are not x86. > > Then let's fix that issue... by propagating the common definition to other > platforms that properly implement {read,write}[bwl] in terms of the PCI bus. > > >> And more than that, centralizing the definition makes the API much more >> dangerous for driver authors. > > I think that's really cranking the hyperbole level to 11. > > The common definition is... the one found most commonly in the wild. For > weird drivers, they will do their own thing. > > That's pretty much how other drivers handle things. > > Apply your logic here to _any_ API in the kernel, for the same result. > > >> > At least the networking drivers I messed with (until 11/2008) were >> > always fine with a non-atomic readq. >> >> The commit to niu I keep citing (e23a59e1, "niu: Fix readq >> implementation when architecture does not provide one.") shows that >> drivers need to take care. Now, the x86 implementation would happen to > > That commit also shows that, had the driver been using a common definition, > problems would not have arisen. > > >> work for that hardware, but eg drivers/infiniband/hw/amso1100 defines >> readq with the opposite order -- whether that's required or just an > > 'required' seems unlikely, given that > > a) their readq only exists when #ifndef readq, thus implying the > driver-local readq is far less tested, on their most-tested, highest-volume > platform. > > b) their readq still operates in LE order -- as it should: read,write[bwl] > were defined in terms of PCI originally, and thus defined to be LE. > > c) their __raw_writeq writes in lower-32-bits-first, as one would expect > > >> arbitrary choice, I don't know. And drivers/infiniband/hw/mthca has >> some uses of __raw_writeq() that only work if no other CPU accesses to >> the same page can happen between the two halves, so it adds a per-page >> spinlock for 32-bit architectures. etc. > > Any use of __raw_xxx implies that You Know What You're Doing And Accept The > Consequences. __raw_xxx means _you_ handle endian conversions, barriers, > and other arch-specific details. I don't think that a driver intentionally > using the "raw" APIs is a good source of ideas and generalizations. > > So, for your three examples, > > 1) niu - common definition is OK > > 2) amso1100 - common definition is OK; driver-local definition > never used on common PCI platforms > > 3) mthca - intentionally uses raw API, an API which ditches > arch-specific barriers, endian conversions, and other > guarantees. > > Given that, I see zero justification for API removal. I see justification > for propagating this code to other PCI-capable platforms. > > Finally, I think given all this time we've had driver-define writeq and > readq, and "driver authors were forced to think about this API" -- the > result was the obvious definition now in place! > > Jeff > > > > > I think it's good time to decide making all architectures which have readq/writeq provide HAVE_READQ/HAVE_WRITEQ or not. Adding HAVE_READQ/HAVE_WRITEQ to Kconfig of architectures needs agreement of all maintainers of these. But, David Miller, maintainer of SPARC architecture, acked Roland's patch because of the possibility of bugs non-atomicity of readq/writeq of x86-32 will cause. And, Jeff Garzik said that he saw zero justification for API removal. Which way should we choose? Remove readq/writeq from x86-32? Or add HAVE... to all architectures with readq/writeq? ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-13 5:32 ` Hitoshi Mitake @ 2009-05-13 20:19 ` H. Peter Anvin 2009-05-13 22:39 ` Jeff Garzik 2009-05-13 20:42 ` Jeff Garzik 1 sibling, 1 reply; 50+ messages in thread From: H. Peter Anvin @ 2009-05-13 20:19 UTC (permalink / raw) To: Hitoshi Mitake Cc: Jeff Garzik, Roland Dreier, Ingo Molnar, David Miller, Linus Torvalds, tglx, rpjday, linux-kernel Hitoshi Mitake wrote: > > I think it's good time to decide making all architectures > which have readq/writeq provide HAVE_READQ/HAVE_WRITEQ or not. > > Adding HAVE_READQ/HAVE_WRITEQ to Kconfig of architectures needs > agreement of all maintainers of these. > > But, David Miller, maintainer of SPARC architecture, acked Roland's patch > because of the possibility of bugs non-atomicity of readq/writeq of > x86-32 will cause. > > And, Jeff Garzik said that he saw zero justification for API removal. > > Which way should we choose? > Remove readq/writeq from x86-32? > Or add HAVE... to all architectures with readq/writeq? There is another option, which is to have canned include variants, that drivers can explicitly opt-in with: #include <linux/io64_lh.h> linux/io64_lh.h would then look like: #ifndef _LINUX_IO64_LH_H #define _LINUX_IO64_LH_H #include <linux/io.h> #ifndef HAVE_READQ /* Low-High nonatomic readq() */ #endif #ifndef HAVE_WRITEQ /* Low-High nonatomic writeq() */ #endif #endif If we need more than io64_lh.h we can then add other variants, but I suspect we won't need them -- except possibly an io64_hl.h. -hpa ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-13 20:19 ` H. Peter Anvin @ 2009-05-13 22:39 ` Jeff Garzik 2009-05-13 23:39 ` H. Peter Anvin 0 siblings, 1 reply; 50+ messages in thread From: Jeff Garzik @ 2009-05-13 22:39 UTC (permalink / raw) To: H. Peter Anvin Cc: Hitoshi Mitake, Roland Dreier, Ingo Molnar, David Miller, Linus Torvalds, tglx, rpjday, linux-kernel H. Peter Anvin wrote: > #include <linux/io64_lh.h> > > linux/io64_lh.h would then look like: > > #ifndef _LINUX_IO64_LH_H > #define _LINUX_IO64_LH_H > > #include <linux/io.h> > > #ifndef HAVE_READQ > > /* Low-High nonatomic readq() */ > > #endif > > #ifndef HAVE_WRITEQ > > /* Low-High nonatomic writeq() */ Judging from this thread and past, I think people will continue to complain and get confused, even with the above. How about 1) tree-wide rename: readq -> readq_na, writeq -> writeq_na 2) make all PCI writel-enabled arches provide readq_na and writeq_na 3) 64-bit PCI writel-enabled arches provide readq and HAVE_READQ, ditto for writeq. 32-bit provides neither readq nor HAVE_READQ. 4) If your hardware has non-standard ordering, handle it the obvious way in the driver, as you would do with any other special case. Jeff P.S. I use "writel-enabled" to note that some platforms do not provide PCI read*/write* functions at all. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-13 22:39 ` Jeff Garzik @ 2009-05-13 23:39 ` H. Peter Anvin 2009-05-14 0:49 ` Jeff Garzik 0 siblings, 1 reply; 50+ messages in thread From: H. Peter Anvin @ 2009-05-13 23:39 UTC (permalink / raw) To: Jeff Garzik Cc: Hitoshi Mitake, Roland Dreier, Ingo Molnar, David Miller, Linus Torvalds, tglx, rpjday, linux-kernel [-- Attachment #1: Type: text/plain, Size: 332 bytes --] Jeff Garzik wrote: > > Judging from this thread and past, I think people will continue to > complain and get confused, even with the above. > Do you really think so? Seems unfortunate, since an API rename would be way more invasive. This is the entirety of the header patch (compile-tested using 32-bit allyesconfig). -hpa [-- Attachment #2: diff --] [-- Type: text/plain, Size: 5111 bytes --] diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index df9e885..0be277b 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -19,8 +19,6 @@ config X86_64 config X86 def_bool y select HAVE_AOUT if X86_32 - select HAVE_READQ - select HAVE_WRITEQ select HAVE_UNSTABLE_SCHED_CLOCK select HAVE_IDE select HAVE_OPROFILE diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h index 7373932..199c7b9 100644 --- a/arch/x86/include/asm/io.h +++ b/arch/x86/include/asm/io.h @@ -51,27 +51,6 @@ build_mmio_write(__writel, "l", unsigned int, "r", ) build_mmio_read(readq, "q", unsigned long, "=r", :"memory") build_mmio_write(writeq, "q", unsigned long, "r", :"memory") -#else - -static inline __u64 readq(const volatile void __iomem *addr) -{ - const volatile u32 __iomem *p = addr; - u32 low, high; - - low = readl(p); - high = readl(p + 1); - - return low + ((u64)high << 32); -} - -static inline void writeq(__u64 val, volatile void __iomem *addr) -{ - writel(val, addr); - writel(val >> 32, addr+4); -} - -#endif - #define readq_relaxed(a) readq(a) #define __raw_readq(a) readq(a) @@ -81,6 +60,8 @@ static inline void writeq(__u64 val, volatile void __iomem *addr) #define readq readq #define writeq writeq +#endif + /** * virt_to_phys - map virtual addresses to physical * @address: address to remap diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2506592..43f14f5 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -33,6 +33,7 @@ #include "i915_reg.h" #include "intel_bios.h" #include <linux/io-mapping.h> +#include <linux/io64_lh.h> /* Simulate 64-bit I/O on 32 bits */ /* General customization: */ @@ -705,13 +706,7 @@ extern void intel_modeset_cleanup(struct drm_device *dev); #define I915_WRITE16(reg, val) writel(val, dev_priv->regs + (reg)) #define I915_READ8(reg) readb(dev_priv->regs + (reg)) #define I915_WRITE8(reg, val) writeb(val, dev_priv->regs + (reg)) -#ifdef writeq #define I915_WRITE64(reg, val) writeq(val, dev_priv->regs + (reg)) -#else -#define I915_WRITE64(reg, val) (writel(val, dev_priv->regs + (reg)), \ - writel(upper_32_bits(val), dev_priv->regs + \ - (reg) + 4)) -#endif #define POSTING_READ(reg) (void)I915_READ(reg) #define I915_VERBOSE 0 diff --git a/include/drm/drm_os_linux.h b/include/drm/drm_os_linux.h index 26641e9..3925946 100644 --- a/include/drm/drm_os_linux.h +++ b/include/drm/drm_os_linux.h @@ -5,19 +5,7 @@ #include <linux/interrupt.h> /* For task queue support */ #include <linux/delay.h> - -#ifndef readq -static inline u64 readq(void __iomem *reg) -{ - return ((u64) readl(reg)) | (((u64) readl(reg + 4UL)) << 32); -} - -static inline void writeq(u64 val, void __iomem *reg) -{ - writel(val & 0xffffffff, reg); - writel(val >> 32, reg + 0x4UL); -} -#endif +#include <linux/io64_lh.h> /* Simulate 64-bit I/O on 32 bits */ /** Current process ID */ #define DRM_CURRENTPID task_pid_nr(current) diff --git a/include/linux/io64_lh.h b/include/linux/io64_lh.h new file mode 100644 index 0000000..361931b --- /dev/null +++ b/include/linux/io64_lh.h @@ -0,0 +1,74 @@ +/* ----------------------------------------------------------------------- * + * + * Copyright (C) 2009 Intel Corporation. All rights reserved. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License version + * 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + * 02110-1301, USA. + * + * H. Peter Anvin <hpa@linux.intel.com> + * + * ----------------------------------------------------------------------- */ + +/* + * io64_lh.h + * + * One possible implementation of readq()/writeq() on 32-bit platforms. + * Emulate 64-bit I/O using nonatomic low-high 32-bit references. + */ + +#ifndef _LINUX_IO64_LH_H +#define _LINUX_IO64_LH_H + +#include <linux/io.h> +#include <linux/types.h> + +#ifndef readq +static inline __u64 readq(const volatile void __iomem *addr) +{ + const volatile u32 __iomem *p = addr; + u32 l, h; + + l = readl(p); + h = readl(p + 1); + + return l + ((u64)h << 32); +} +#define readq readq + +static inline __u64 readq_relaxed(const volatile void __iomem *addr) +{ + const volatile u32 __iomem *p = addr; + u32 l, h; + + l = readl_relaxed(p); + h = readl_relaxed(p + 1); + + return l + ((u64)h << 32); +} +#define readq_relaxed readq_relaxed + +#endif /* readq */ + +#ifndef writeq + +static inline void writeq(__u64 val, volatile void __iomem *addr) +{ + writel(val, addr); + writel(val >> 32, addr+4); +} +#define writeq writeq + +#endif /* writeq */ + +#endif /* _LINUX_IO64_LH_H */ ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-13 23:39 ` H. Peter Anvin @ 2009-05-14 0:49 ` Jeff Garzik 2009-05-14 7:19 ` Hitoshi Mitake 0 siblings, 1 reply; 50+ messages in thread From: Jeff Garzik @ 2009-05-14 0:49 UTC (permalink / raw) To: H. Peter Anvin Cc: Hitoshi Mitake, Roland Dreier, Ingo Molnar, David Miller, Linus Torvalds, tglx, rpjday, linux-kernel H. Peter Anvin wrote: > Jeff Garzik wrote: >> Judging from this thread and past, I think people will continue to >> complain and get confused, even with the above. >> > > Do you really think so? Seems unfortunate, since an API rename would be > way more invasive. This is the entirety of the header patch > (compile-tested using 32-bit allyesconfig). The header patch does not lessen the confusion, because you cannot look at the code and immediately tell what is going on... Having a single function's behavior change based on #include selection is /not/ intuitive at all, particularly for driver writers. That is unlike almost every other Linux API, where functions' behavior stays constant across platforms, regardless of magic "under the hood." That sort of trick is reserved for arch maintainers who know what they are doing :) Jeff ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-14 0:49 ` Jeff Garzik @ 2009-05-14 7:19 ` Hitoshi Mitake 2009-05-15 23:44 ` Jeff Garzik 0 siblings, 1 reply; 50+ messages in thread From: Hitoshi Mitake @ 2009-05-14 7:19 UTC (permalink / raw) To: Jeff Garzik Cc: H. Peter Anvin, Roland Dreier, Ingo Molnar, David Miller, Linus Torvalds, tglx, rpjday, linux-kernel On Wed, 13 May 2009 20:49:26 -0400 Jeff Garzik <jeff@garzik.org> wrote: > H. Peter Anvin wrote: > > Jeff Garzik wrote: > >> Judging from this thread and past, I think people will continue to > >> complain and get confused, even with the above. > >> > > > > Do you really think so? Seems unfortunate, since an API rename would be > > way more invasive. This is the entirety of the header patch > > (compile-tested using 32-bit allyesconfig). > > The header patch does not lessen the confusion, because you cannot look > at the code and immediately tell what is going on... > > Having a single function's behavior change based on #include selection > is /not/ intuitive at all, particularly for driver writers. That is > unlike almost every other Linux API, where functions' behavior stays > constant across platforms, regardless of magic "under the hood." > > That sort of trick is reserved for arch maintainers who know what they > are doing :) > > Jeff > > > I found another way: Making architecture with atomic readq/writeq provide HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC and making architecture with non-atomic readq/writeq provide HAVE_READQ/HAVE_WRITEQ. (HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC should double as HAVE_READQ/HAVE_WRITEQ.) So driver programmers who need atomic readq/writeq can judge existence of API they really need. If platform doesn't provide atomic readq/writeq, drivers need these can be disabled by Kconfig. And bugs Roland and David talking about will be banished. How about this? > Roland and David I wrote a test patch. Request for comments. Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com> --- arch/x86/Kconfig | 16 ++++++++++++++-- 1 files changed, 14 insertions(+), 2 deletions(-) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index df9e885..c94fc48 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -19,8 +19,6 @@ config X86_64 config X86 def_bool y select HAVE_AOUT if X86_32 - select HAVE_READQ - select HAVE_WRITEQ select HAVE_UNSTABLE_SCHED_CLOCK select HAVE_IDE select HAVE_OPROFILE @@ -2022,6 +2020,20 @@ config HAVE_ATOMIC_IOMAP def_bool y depends on X86_32 +config HAVE_READQ + def_bool y + +config HAVE_WRITEQ + def_bool y + +config HAVE_READQ_ATOMIC + def_bool y + depends on X86_64 + +config HAVE_WRITEQ_ATOMIC + def_bool y + depends on X86_64 + source "net/Kconfig" source "drivers/Kconfig" -- 1.5.6.5 ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-14 7:19 ` Hitoshi Mitake @ 2009-05-15 23:44 ` Jeff Garzik 2009-05-17 7:12 ` Hitoshi Mitake 0 siblings, 1 reply; 50+ messages in thread From: Jeff Garzik @ 2009-05-15 23:44 UTC (permalink / raw) To: Hitoshi Mitake Cc: H. Peter Anvin, Roland Dreier, Ingo Molnar, David Miller, Linus Torvalds, tglx, rpjday, linux-kernel Hitoshi Mitake wrote: > On Wed, 13 May 2009 20:49:26 -0400 > Jeff Garzik <jeff@garzik.org> wrote: > >> H. Peter Anvin wrote: >>> Jeff Garzik wrote: >>>> Judging from this thread and past, I think people will continue to >>>> complain and get confused, even with the above. >>>> >>> Do you really think so? Seems unfortunate, since an API rename would be >>> way more invasive. This is the entirety of the header patch >>> (compile-tested using 32-bit allyesconfig). >> The header patch does not lessen the confusion, because you cannot look >> at the code and immediately tell what is going on... >> >> Having a single function's behavior change based on #include selection >> is /not/ intuitive at all, particularly for driver writers. That is >> unlike almost every other Linux API, where functions' behavior stays >> constant across platforms, regardless of magic "under the hood." >> >> That sort of trick is reserved for arch maintainers who know what they >> are doing :) >> >> Jeff >> >> >> > > I found another way: > Making architecture with atomic readq/writeq provide HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC > and making architecture with non-atomic readq/writeq provide HAVE_READQ/HAVE_WRITEQ. > (HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC should double as HAVE_READQ/HAVE_WRITEQ.) > > So driver programmers who need atomic readq/writeq can judge existence of API they really need. > If platform doesn't provide atomic readq/writeq, drivers need these can be disabled by Kconfig. > And bugs Roland and David talking about will be banished. > How about this? > Roland and David > I wrote a test patch. Request for comments. > > Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com> > > --- > arch/x86/Kconfig | 16 ++++++++++++++-- > 1 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index df9e885..c94fc48 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -19,8 +19,6 @@ config X86_64 > config X86 > def_bool y > select HAVE_AOUT if X86_32 > - select HAVE_READQ > - select HAVE_WRITEQ > select HAVE_UNSTABLE_SCHED_CLOCK > select HAVE_IDE > select HAVE_OPROFILE > @@ -2022,6 +2020,20 @@ config HAVE_ATOMIC_IOMAP > def_bool y > depends on X86_32 > > +config HAVE_READQ > + def_bool y > + > +config HAVE_WRITEQ > + def_bool y > + > +config HAVE_READQ_ATOMIC > + def_bool y > + depends on X86_64 > + > +config HAVE_WRITEQ_ATOMIC > + def_bool y > + depends on X86_64 If you create HAVE_{READQ,WRITEQ}_ATOMIC, then you don't really need HAVE_READQ -- the other relevant 32-bit platforms simply need a definition of readq and writeq. Probably easy enough to have a common definition in asm-generic. Jeff ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-15 23:44 ` Jeff Garzik @ 2009-05-17 7:12 ` Hitoshi Mitake 2009-05-17 8:06 ` Jeff Garzik 0 siblings, 1 reply; 50+ messages in thread From: Hitoshi Mitake @ 2009-05-17 7:12 UTC (permalink / raw) To: Jeff Garzik Cc: H. Peter Anvin, Roland Dreier, Ingo Molnar, David Miller, Linus Torvalds, tglx, rpjday, linux-kernel On Fri, 15 May 2009 19:44:03 -0400 Jeff Garzik <jeff@garzik.org> wrote: > Hitoshi Mitake wrote: > > On Wed, 13 May 2009 20:49:26 -0400 > > Jeff Garzik <jeff@garzik.org> wrote: > > > >> H. Peter Anvin wrote: > >>> Jeff Garzik wrote: > >>>> Judging from this thread and past, I think people will continue to > >>>> complain and get confused, even with the above. > >>>> > >>> Do you really think so? Seems unfortunate, since an API rename would be > >>> way more invasive. This is the entirety of the header patch > >>> (compile-tested using 32-bit allyesconfig). > >> The header patch does not lessen the confusion, because you cannot look > >> at the code and immediately tell what is going on... > >> > >> Having a single function's behavior change based on #include selection > >> is /not/ intuitive at all, particularly for driver writers. That is > >> unlike almost every other Linux API, where functions' behavior stays > >> constant across platforms, regardless of magic "under the hood." > >> > >> That sort of trick is reserved for arch maintainers who know what they > >> are doing :) > >> > >> Jeff > >> > >> > >> > > > > I found another way: > > Making architecture with atomic readq/writeq provide HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC > > and making architecture with non-atomic readq/writeq provide HAVE_READQ/HAVE_WRITEQ. > > (HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC should double as HAVE_READQ/HAVE_WRITEQ.) > > > > So driver programmers who need atomic readq/writeq can judge existence of API they really need. > > If platform doesn't provide atomic readq/writeq, drivers need these can be disabled by Kconfig. > > And bugs Roland and David talking about will be banished. > > How about this? > Roland and David > > I wrote a test patch. Request for comments. > > > > Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com> > > > > --- > > arch/x86/Kconfig | 16 ++++++++++++++-- > > 1 files changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > > index df9e885..c94fc48 100644 > > --- a/arch/x86/Kconfig > > +++ b/arch/x86/Kconfig > > @@ -19,8 +19,6 @@ config X86_64 > > config X86 > > def_bool y > > select HAVE_AOUT if X86_32 > > - select HAVE_READQ > > - select HAVE_WRITEQ > > select HAVE_UNSTABLE_SCHED_CLOCK > > select HAVE_IDE > > select HAVE_OPROFILE > > @@ -2022,6 +2020,20 @@ config HAVE_ATOMIC_IOMAP > > def_bool y > > depends on X86_32 > > > > +config HAVE_READQ > > + def_bool y > > + > > +config HAVE_WRITEQ > > + def_bool y > > + > > +config HAVE_READQ_ATOMIC > > + def_bool y > > + depends on X86_64 > > + > > +config HAVE_WRITEQ_ATOMIC > > + def_bool y > > + depends on X86_64 > > If you create HAVE_{READQ,WRITEQ}_ATOMIC, then you don't really need > HAVE_READQ -- the other relevant 32-bit platforms simply need a > definition of readq and writeq. Probably easy enough to have a common > definition in asm-generic. > That's good idea. I didn't noticed the way to use asm-generic. Thanks. How is this? Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com> --- arch/x86/Kconfig | 10 ++++++++-- arch/x86/include/asm/io.h | 27 ++++++--------------------- include/asm-generic/quadrw.h | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 23 deletions(-) create mode 100644 include/asm-generic/quadrw.h diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index df9e885..151b6a0 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -19,8 +19,6 @@ config X86_64 config X86 def_bool y select HAVE_AOUT if X86_32 - select HAVE_READQ - select HAVE_WRITEQ select HAVE_UNSTABLE_SCHED_CLOCK select HAVE_IDE select HAVE_OPROFILE @@ -2022,6 +2020,14 @@ config HAVE_ATOMIC_IOMAP def_bool y depends on X86_32 +config HAVE_READQ_ATOMIC + def_bool y + depends on X86_64 + +config HAVE_WRITEQ_ATOMIC + def_bool y + depends on X86_64 + source "net/Kconfig" source "drivers/Kconfig" diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h index 7373932..bad940d 100644 --- a/arch/x86/include/asm/io.h +++ b/arch/x86/include/asm/io.h @@ -51,32 +51,17 @@ build_mmio_write(__writel, "l", unsigned int, "r", ) build_mmio_read(readq, "q", unsigned long, "=r", :"memory") build_mmio_write(writeq, "q", unsigned long, "r", :"memory") -#else - -static inline __u64 readq(const volatile void __iomem *addr) -{ - const volatile u32 __iomem *p = addr; - u32 low, high; - - low = readl(p); - high = readl(p + 1); - - return low + ((u64)high << 32); -} - -static inline void writeq(__u64 val, volatile void __iomem *addr) -{ - writel(val, addr); - writel(val >> 32, addr+4); -} - -#endif - #define readq_relaxed(a) readq(a) #define __raw_readq(a) readq(a) #define __raw_writeq(val, addr) writeq(val, addr) +#endif /* CONFIG_X86_64 */ + +#ifdef CONFIG_X86_32 +#include <asm-generic/quadrw.h> +#endif /* CONFIG_X86_32 */ + /* Let people know that we have them */ #define readq readq #define writeq writeq diff --git a/include/asm-generic/quadrw.h b/include/asm-generic/quadrw.h new file mode 100644 index 0000000..78159a2 --- /dev/null +++ b/include/asm-generic/quadrw.h @@ -0,0 +1,34 @@ +#ifndef GENERIC_QUADRW_H +#define GENERIC_QUADRW_H + +#include <asm/io.h> + +/* + * General readq/writeq implementation. + * These are not atomic operations. + * The drivers which need atomic version readq/writeq, + * they should depend on HAVE_{READQ,WRITEQ}_ATOMIC in Kconfig level. + */ + +#ifndef CONFIG_HAVE_READQ_ATOMIC +static inline __u64 readq(const volatile void __iomem *addr) +{ + const volatile u32 __iomem *p = addr; + u32 low, high; + + low = readl(p); + high = readl(p + 1); + + return low + ((u64)high << 32); +} +#endif /* CONFIG_HAVE_READQ_ATOMIC */ + +#ifndef CONFIG_HAVE_WRITEQ_ATOMIC +static inline void writeq(__u64 val, volatile void __iomem *addr) +{ + writel(val, addr); + writel(val >> 32, addr+4); +} +#endif /* CONFIG_HAVE_WRITEQ_ATOMIC */ + +#endif /* GENERIC_QUADRW_H */ -- 1.5.6.5 ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-17 7:12 ` Hitoshi Mitake @ 2009-05-17 8:06 ` Jeff Garzik 2009-05-21 11:35 ` Hitoshi Mitake 0 siblings, 1 reply; 50+ messages in thread From: Jeff Garzik @ 2009-05-17 8:06 UTC (permalink / raw) To: Hitoshi Mitake Cc: H. Peter Anvin, Roland Dreier, Ingo Molnar, David Miller, Linus Torvalds, tglx, rpjday, linux-kernel Hitoshi Mitake wrote: > On Fri, 15 May 2009 19:44:03 -0400 > Jeff Garzik <jeff@garzik.org> wrote: > >> Hitoshi Mitake wrote: >>> On Wed, 13 May 2009 20:49:26 -0400 >>> Jeff Garzik <jeff@garzik.org> wrote: >>> >>>> H. Peter Anvin wrote: >>>>> Jeff Garzik wrote: >>>>>> Judging from this thread and past, I think people will continue to >>>>>> complain and get confused, even with the above. >>>>>> >>>>> Do you really think so? Seems unfortunate, since an API rename would be >>>>> way more invasive. This is the entirety of the header patch >>>>> (compile-tested using 32-bit allyesconfig). >>>> The header patch does not lessen the confusion, because you cannot look >>>> at the code and immediately tell what is going on... >>>> >>>> Having a single function's behavior change based on #include selection >>>> is /not/ intuitive at all, particularly for driver writers. That is >>>> unlike almost every other Linux API, where functions' behavior stays >>>> constant across platforms, regardless of magic "under the hood." >>>> >>>> That sort of trick is reserved for arch maintainers who know what they >>>> are doing :) >>>> >>>> Jeff >>>> >>>> >>>> >>> I found another way: >>> Making architecture with atomic readq/writeq provide HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC >>> and making architecture with non-atomic readq/writeq provide HAVE_READQ/HAVE_WRITEQ. >>> (HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC should double as HAVE_READQ/HAVE_WRITEQ.) >>> >>> So driver programmers who need atomic readq/writeq can judge existence of API they really need. >>> If platform doesn't provide atomic readq/writeq, drivers need these can be disabled by Kconfig. >>> And bugs Roland and David talking about will be banished. >>> How about this? > Roland and David >>> I wrote a test patch. Request for comments. >>> >>> Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com> >>> >>> --- >>> arch/x86/Kconfig | 16 ++++++++++++++-- >>> 1 files changed, 14 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig >>> index df9e885..c94fc48 100644 >>> --- a/arch/x86/Kconfig >>> +++ b/arch/x86/Kconfig >>> @@ -19,8 +19,6 @@ config X86_64 >>> config X86 >>> def_bool y >>> select HAVE_AOUT if X86_32 >>> - select HAVE_READQ >>> - select HAVE_WRITEQ >>> select HAVE_UNSTABLE_SCHED_CLOCK >>> select HAVE_IDE >>> select HAVE_OPROFILE >>> @@ -2022,6 +2020,20 @@ config HAVE_ATOMIC_IOMAP >>> def_bool y >>> depends on X86_32 >>> >>> +config HAVE_READQ >>> + def_bool y >>> + >>> +config HAVE_WRITEQ >>> + def_bool y >>> + >>> +config HAVE_READQ_ATOMIC >>> + def_bool y >>> + depends on X86_64 >>> + >>> +config HAVE_WRITEQ_ATOMIC >>> + def_bool y >>> + depends on X86_64 >> If you create HAVE_{READQ,WRITEQ}_ATOMIC, then you don't really need >> HAVE_READQ -- the other relevant 32-bit platforms simply need a >> definition of readq and writeq. Probably easy enough to have a common >> definition in asm-generic. >> > That's good idea. I didn't noticed the way to use asm-generic. Thanks. > > How is this? > > Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com> > > --- > arch/x86/Kconfig | 10 ++++++++-- > arch/x86/include/asm/io.h | 27 ++++++--------------------- > include/asm-generic/quadrw.h | 34 ++++++++++++++++++++++++++++++++++ > 3 files changed, 48 insertions(+), 23 deletions(-) > create mode 100644 include/asm-generic/quadrw.h Seems fine to me, no objections here... > +#ifndef CONFIG_HAVE_READQ_ATOMIC > +static inline __u64 readq(const volatile void __iomem *addr) > +{ > + const volatile u32 __iomem *p = addr; > + u32 low, high; > + > + low = readl(p); > + high = readl(p + 1); > + > + return low + ((u64)high << 32); > +} > +#endif /* CONFIG_HAVE_READQ_ATOMIC */ Personally I would do static inline __u64 readq(const volatile void __iomem *addr) { __u64 low, high; low = readl(addr); high = readl(addr + 4); return (high << 32) | low; } but maybe that's just me. It seems inconsistent that the generic readq and writeq internals, simple as they are, differ to such a degree in this implementation. But overall, as mentioned above, the approach seems sound to me. Cheers! Jeff ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-17 8:06 ` Jeff Garzik @ 2009-05-21 11:35 ` Hitoshi Mitake 2009-05-21 11:49 ` Hitoshi Mitake 0 siblings, 1 reply; 50+ messages in thread From: Hitoshi Mitake @ 2009-05-21 11:35 UTC (permalink / raw) To: Jeff Garzik Cc: H. Peter Anvin, Roland Dreier, Ingo Molnar, David Miller, Linus Torvalds, tglx, rpjday, linux-kernel, mitake On Sun, May 17, 2009 at 17:06, Jeff Garzik <jeff@garzik.org> wrote: > Hitoshi Mitake wrote: >> >> On Fri, 15 May 2009 19:44:03 -0400 >> Jeff Garzik <jeff@garzik.org> wrote: >> >>> Hitoshi Mitake wrote: >>>> >>>> On Wed, 13 May 2009 20:49:26 -0400 >>>> Jeff Garzik <jeff@garzik.org> wrote: >>>> >>>>> H. Peter Anvin wrote: >>>>>> >>>>>> Jeff Garzik wrote: >>>>>>> >>>>>>> Judging from this thread and past, I think people will continue to >>>>>>> complain and get confused, even with the above. >>>>>>> >>>>>> Do you really think so? Seems unfortunate, since an API rename would >>>>>> be >>>>>> way more invasive. This is the entirety of the header patch >>>>>> (compile-tested using 32-bit allyesconfig). >>>>> >>>>> The header patch does not lessen the confusion, because you cannot look >>>>> at the code and immediately tell what is going on... >>>>> >>>>> Having a single function's behavior change based on #include selection >>>>> is /not/ intuitive at all, particularly for driver writers. That is unlike >>>>> almost every other Linux API, where functions' behavior stays constant >>>>> across platforms, regardless of magic "under the hood." >>>>> >>>>> That sort of trick is reserved for arch maintainers who know what they >>>>> are doing :) >>>>> >>>>> Jeff >>>>> >>>>> >>>>> >>>> I found another way: >>>> Making architecture with atomic readq/writeq provide >>>> HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC >>>> and making architecture with non-atomic readq/writeq provide >>>> HAVE_READQ/HAVE_WRITEQ. >>>> (HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC should double as >>>> HAVE_READQ/HAVE_WRITEQ.) >>>> >>>> So driver programmers who need atomic readq/writeq can judge existence >>>> of API they really need. >>>> If platform doesn't provide atomic readq/writeq, drivers need these can >>>> be disabled by Kconfig. >>>> And bugs Roland and David talking about will be banished. >>>> How about this? > Roland and David >>>> I wrote a test patch. Request for comments. >>>> >>>> Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com> >>>> >>>> --- >>>> arch/x86/Kconfig | 16 ++++++++++++++-- >>>> 1 files changed, 14 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig >>>> index df9e885..c94fc48 100644 >>>> --- a/arch/x86/Kconfig >>>> +++ b/arch/x86/Kconfig >>>> @@ -19,8 +19,6 @@ config X86_64 >>>> config X86 >>>> def_bool y >>>> select HAVE_AOUT if X86_32 >>>> - select HAVE_READQ >>>> - select HAVE_WRITEQ >>>> select HAVE_UNSTABLE_SCHED_CLOCK >>>> select HAVE_IDE >>>> select HAVE_OPROFILE >>>> @@ -2022,6 +2020,20 @@ config HAVE_ATOMIC_IOMAP >>>> def_bool y >>>> depends on X86_32 >>>> +config HAVE_READQ >>>> + def_bool y >>>> + >>>> +config HAVE_WRITEQ >>>> + def_bool y >>>> + >>>> +config HAVE_READQ_ATOMIC >>>> + def_bool y >>>> + depends on X86_64 >>>> + >>>> +config HAVE_WRITEQ_ATOMIC >>>> + def_bool y >>>> + depends on X86_64 >>> >>> If you create HAVE_{READQ,WRITEQ}_ATOMIC, then you don't really need >>> HAVE_READQ -- the other relevant 32-bit platforms simply need a definition >>> of readq and writeq. Probably easy enough to have a common definition in >>> asm-generic. >>> >> That's good idea. I didn't noticed the way to use asm-generic. Thanks. >> >> How is this? >> >> Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com> >> >> --- >> arch/x86/Kconfig | 10 ++++++++-- >> arch/x86/include/asm/io.h | 27 ++++++--------------------- >> include/asm-generic/quadrw.h | 34 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 48 insertions(+), 23 deletions(-) >> create mode 100644 include/asm-generic/quadrw.h > > > Seems fine to me, no objections here... > > > >> +#ifndef CONFIG_HAVE_READQ_ATOMIC >> +static inline __u64 readq(const volatile void __iomem *addr) >> +{ >> + const volatile u32 __iomem *p = addr; >> + u32 low, high; >> + >> + low = readl(p); >> + high = readl(p + 1); >> + >> + return low + ((u64)high << 32); >> +} >> +#endif /* CONFIG_HAVE_READQ_ATOMIC */ > > Personally I would do > > static inline __u64 readq(const volatile void __iomem *addr) > { > __u64 low, high; > > low = readl(addr); > high = readl(addr + 4); > > return (high << 32) | low; > } > > but maybe that's just me. > > It seems inconsistent that the generic readq and writeq internals, simple as > they are, differ to such a degree in this implementation. > > But overall, as mentioned above, the approach seems sound to me. > > Cheers! > > Jeff > > > Please discard this mail, this is only Ccing to my another email address of university. Because IMAP of Gmail is too slow to read LKML... So I'll switch my email address from h.mitake@gmail.com to mitake@dcl.info.waseda.ac.jp, and reply from new one. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-21 11:35 ` Hitoshi Mitake @ 2009-05-21 11:49 ` Hitoshi Mitake 0 siblings, 0 replies; 50+ messages in thread From: Hitoshi Mitake @ 2009-05-21 11:49 UTC (permalink / raw) To: Hitoshi Mitake Cc: Jeff Garzik, H. Peter Anvin, Roland Dreier, Ingo Molnar, David Miller, Linus Torvalds, tglx, rpjday, linux-kernel On Thu, 21 May 2009 20:35:54 +0900 Hitoshi Mitake <h.mitake@gmail.com> wrote: > On Sun, May 17, 2009 at 17:06, Jeff Garzik <jeff@garzik.org> wrote: > > Hitoshi Mitake wrote: > >> > >> On Fri, 15 May 2009 19:44:03 -0400 > >> Jeff Garzik <jeff@garzik.org> wrote: > >> > >>> Hitoshi Mitake wrote: > >>>> > >>>> On Wed, 13 May 2009 20:49:26 -0400 > >>>> Jeff Garzik <jeff@garzik.org> wrote: > >>>> > >>>>> H. Peter Anvin wrote: > >>>>>> > >>>>>> Jeff Garzik wrote: > >>>>>>> > >>>>>>> Judging from this thread and past, I think people will continue to > >>>>>>> complain and get confused, even with the above. > >>>>>>> > >>>>>> Do you really think so? Seems unfortunate, since an API rename would > >>>>>> be > >>>>>> way more invasive. This is the entirety of the header patch > >>>>>> (compile-tested using 32-bit allyesconfig). > >>>>> > >>>>> The header patch does not lessen the confusion, because you cannot look > >>>>> at the code and immediately tell what is going on... > >>>>> > >>>>> Having a single function's behavior change based on #include selection > >>>>> is /not/ intuitive at all, particularly for driver writers. That is unlike > >>>>> almost every other Linux API, where functions' behavior stays constant > >>>>> across platforms, regardless of magic "under the hood." > >>>>> > >>>>> That sort of trick is reserved for arch maintainers who know what they > >>>>> are doing :) > >>>>> > >>>>> Jeff > >>>>> > >>>>> > >>>>> > >>>> I found another way: > >>>> Making architecture with atomic readq/writeq provide > >>>> HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC > >>>> and making architecture with non-atomic readq/writeq provide > >>>> HAVE_READQ/HAVE_WRITEQ. > >>>> (HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC should double as > >>>> HAVE_READQ/HAVE_WRITEQ.) > >>>> > >>>> So driver programmers who need atomic readq/writeq can judge existence > >>>> of API they really need. > >>>> If platform doesn't provide atomic readq/writeq, drivers need these can > >>>> be disabled by Kconfig. > >>>> And bugs Roland and David talking about will be banished. > >>>> How about this? > Roland and David > >>>> I wrote a test patch. Request for comments. > >>>> > >>>> Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com> > >>>> > >>>> --- > >>>> arch/x86/Kconfig | 16 ++++++++++++++-- > >>>> 1 files changed, 14 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > >>>> index df9e885..c94fc48 100644 > >>>> --- a/arch/x86/Kconfig > >>>> +++ b/arch/x86/Kconfig > >>>> @@ -19,8 +19,6 @@ config X86_64 > >>>> config X86 > >>>> def_bool y > >>>> select HAVE_AOUT if X86_32 > >>>> - select HAVE_READQ > >>>> - select HAVE_WRITEQ > >>>> select HAVE_UNSTABLE_SCHED_CLOCK > >>>> select HAVE_IDE > >>>> select HAVE_OPROFILE > >>>> @@ -2022,6 +2020,20 @@ config HAVE_ATOMIC_IOMAP > >>>> def_bool y > >>>> depends on X86_32 > >>>> +config HAVE_READQ > >>>> + def_bool y > >>>> + > >>>> +config HAVE_WRITEQ > >>>> + def_bool y > >>>> + > >>>> +config HAVE_READQ_ATOMIC > >>>> + def_bool y > >>>> + depends on X86_64 > >>>> + > >>>> +config HAVE_WRITEQ_ATOMIC > >>>> + def_bool y > >>>> + depends on X86_64 > >>> > >>> If you create HAVE_{READQ,WRITEQ}_ATOMIC, then you don't really need > >>> HAVE_READQ -- the other relevant 32-bit platforms simply need a definition > >>> of readq and writeq. Probably easy enough to have a common definition in > >>> asm-generic. > >>> > >> That's good idea. I didn't noticed the way to use asm-generic. Thanks. > >> > >> How is this? > >> > >> Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com> > >> > >> --- > >> arch/x86/Kconfig | 10 ++++++++-- > >> arch/x86/include/asm/io.h | 27 ++++++--------------------- > >> include/asm-generic/quadrw.h | 34 ++++++++++++++++++++++++++++++++++ > >> 3 files changed, 48 insertions(+), 23 deletions(-) > >> create mode 100644 include/asm-generic/quadrw.h > > > > > > Seems fine to me, no objections here... > > > > > > > >> +#ifndef CONFIG_HAVE_READQ_ATOMIC > >> +static inline __u64 readq(const volatile void __iomem *addr) > >> +{ > >> + const volatile u32 __iomem *p = addr; > >> + u32 low, high; > >> + > >> + low = readl(p); > >> + high = readl(p + 1); > >> + > >> + return low + ((u64)high << 32); > >> +} > >> +#endif /* CONFIG_HAVE_READQ_ATOMIC */ > > > > Personally I would do > > > > static inline __u64 readq(const volatile void __iomem *addr) > > { > > __u64 low, high; > > > > low = readl(addr); > > high = readl(addr + 4); > > > > return (high << 32) | low; > > } > > > > but maybe that's just me. > > > > It seems inconsistent that the generic readq and writeq internals, simple as > > they are, differ to such a degree in this implementation. > > > > But overall, as mentioned above, the approach seems sound to me. > > > > Cheers! > > > > Jeff > > > > > > > I fixed readq according to Jeff's advice. I think his readq is smarter than mine. This is new version of the patch. So I want to hear Roland and David's opinion. We will be able to avoid making terrible bugs with this patch. And generic readq/writeq will be provided for the cases without requirement of atomic readq/writeq. This patch would make our life easily. How do you think? Signed-off-by: Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp> Cc: Jeff Garzik <jeff@garzik.org> Cc: Roland Dreier <rolandd@cisco.com> Cc: David S. Miller <davem@davemloft.net> --- arch/x86/Kconfig | 10 ++++++++-- arch/x86/include/asm/io.h | 27 ++++++--------------------- include/asm-generic/quadrw.h | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 23 deletions(-) create mode 100644 include/asm-generic/quadrw.h diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index a6efe0a..46ea47c 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -19,8 +19,6 @@ config X86_64 config X86 def_bool y select HAVE_AOUT if X86_32 - select HAVE_READQ - select HAVE_WRITEQ select HAVE_UNSTABLE_SCHED_CLOCK select HAVE_IDE select HAVE_OPROFILE @@ -2035,6 +2033,14 @@ config HAVE_ATOMIC_IOMAP def_bool y depends on X86_32 +config HAVE_READQ_ATOMIC + def_bool y + depends on X86_64 + +config HAVE_WRITEQ_ATOMIC + def_bool y + depends on X86_64 + source "net/Kconfig" source "drivers/Kconfig" diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h index 7373932..bad940d 100644 --- a/arch/x86/include/asm/io.h +++ b/arch/x86/include/asm/io.h @@ -51,32 +51,17 @@ build_mmio_write(__writel, "l", unsigned int, "r", ) build_mmio_read(readq, "q", unsigned long, "=r", :"memory") build_mmio_write(writeq, "q", unsigned long, "r", :"memory") -#else - -static inline __u64 readq(const volatile void __iomem *addr) -{ - const volatile u32 __iomem *p = addr; - u32 low, high; - - low = readl(p); - high = readl(p + 1); - - return low + ((u64)high << 32); -} - -static inline void writeq(__u64 val, volatile void __iomem *addr) -{ - writel(val, addr); - writel(val >> 32, addr+4); -} - -#endif - #define readq_relaxed(a) readq(a) #define __raw_readq(a) readq(a) #define __raw_writeq(val, addr) writeq(val, addr) +#endif /* CONFIG_X86_64 */ + +#ifdef CONFIG_X86_32 +#include <asm-generic/quadrw.h> +#endif /* CONFIG_X86_32 */ + /* Let people know that we have them */ #define readq readq #define writeq writeq diff --git a/include/asm-generic/quadrw.h b/include/asm-generic/quadrw.h new file mode 100644 index 0000000..15856ac --- /dev/null +++ b/include/asm-generic/quadrw.h @@ -0,0 +1,34 @@ +#ifndef GENERIC_QUADRW_H +#define GENERIC_QUADRW_H + +#include <asm/io.h> + +/* + * General readq/writeq implementation. + * These are not atomic operations. + * The drivers which need atomic version readq/writeq, + * they should depend on HAVE_{READQ,WRITEQ}_ATOMIC in Kconfig level. + */ + +#ifndef CONFIG_HAVE_READQ_ATOMIC +static inline __u64 readq(const volatile void __iomem *addr) +{ + __u64 low, high; + + low = readl(addr); + high = readl(addr + 4); + + return (high << 32) | low; +} + +#endif /* CONFIG_HAVE_READQ_ATOMIC */ + +#ifndef CONFIG_HAVE_WRITEQ_ATOMIC +static inline void writeq(__u64 val, volatile void __iomem *addr) +{ + writel(val, addr); + writel(val >> 32, addr+4); +} +#endif /* CONFIG_HAVE_WRITEQ_ATOMIC */ + +#endif /* GENERIC_QUADRW_H */ -- 1.5.6.5 ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-13 5:32 ` Hitoshi Mitake 2009-05-13 20:19 ` H. Peter Anvin @ 2009-05-13 20:42 ` Jeff Garzik 2009-05-13 21:05 ` H. Peter Anvin 2009-05-13 22:06 ` Roland Dreier 1 sibling, 2 replies; 50+ messages in thread From: Jeff Garzik @ 2009-05-13 20:42 UTC (permalink / raw) To: Hitoshi Mitake Cc: Roland Dreier, Ingo Molnar, David Miller, Linus Torvalds, hpa, tglx, rpjday, linux-kernel, H. Peter Anvin Hitoshi Mitake wrote: > I think it's good time to decide making all architectures > which have readq/writeq provide HAVE_READQ/HAVE_WRITEQ or not. > > Adding HAVE_READQ/HAVE_WRITEQ to Kconfig of architectures needs > agreement of all maintainers of these. > > But, David Miller, maintainer of SPARC architecture, acked Roland's patch > because of the possibility of bugs non-atomicity of readq/writeq of > x86-32 will cause. > > And, Jeff Garzik said that he saw zero justification for API removal. > > Which way should we choose? > Remove readq/writeq from x86-32? > Or add HAVE... to all architectures with readq/writeq? To repeat what has already been stated, each case was re-evaluated: http://marc.info/?l=linux-kernel&m=124103527326835&w=2 Roland's patch was acked, apparently, _in spite of_ the commonly accepted readq() definition already being in use! Thusfar, I see two things: (1) years of history has shown that non-atomic readq/writeq on 32-bit platforms has been sufficient, based on testing and experience. In fact, in niu's case, a common readq/writeq would have PREVENTED a bug. (2) unspecified fears continue to linger about non-atomicity We should not base decisions on fear, particularly when the weight of evidence and experience points in the other direction. Jeff ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-13 20:42 ` Jeff Garzik @ 2009-05-13 21:05 ` H. Peter Anvin 2009-05-13 21:30 ` Jeff Garzik 2009-05-13 22:06 ` Roland Dreier 1 sibling, 1 reply; 50+ messages in thread From: H. Peter Anvin @ 2009-05-13 21:05 UTC (permalink / raw) To: Jeff Garzik Cc: Hitoshi Mitake, Roland Dreier, Ingo Molnar, David Miller, Linus Torvalds, tglx, rpjday, linux-kernel Jeff Garzik wrote: > > Roland's patch was acked, apparently, _in spite of_ the commonly > accepted readq() definition already being in use! > > Thusfar, I see two things: > > (1) years of history has shown that non-atomic readq/writeq on 32-bit > platforms has been sufficient, based on testing and experience. In > fact, in niu's case, a common readq/writeq would have PREVENTED a bug. > > (2) unspecified fears continue to linger about non-atomicity > > We should not base decisions on fear, particularly when the weight of > evidence and experience points in the other direction. > I have personally dealt with at least one device who'd want to opt out of a standard readq/writeq (it's not in-tree because it never shipped, unfortunately.) Doing the opt-in headers seems like a reasonable thing to do to me, but perhaps I'm just being overly paranoid. -hpa ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-13 21:05 ` H. Peter Anvin @ 2009-05-13 21:30 ` Jeff Garzik 2009-05-13 21:31 ` Jeff Garzik 2009-05-13 21:54 ` H. Peter Anvin 0 siblings, 2 replies; 50+ messages in thread From: Jeff Garzik @ 2009-05-13 21:30 UTC (permalink / raw) To: H. Peter Anvin Cc: Hitoshi Mitake, Roland Dreier, Ingo Molnar, David Miller, Linus Torvalds, tglx, rpjday, linux-kernel H. Peter Anvin wrote: > Jeff Garzik wrote: >> Roland's patch was acked, apparently, _in spite of_ the commonly >> accepted readq() definition already being in use! >> >> Thusfar, I see two things: >> >> (1) years of history has shown that non-atomic readq/writeq on 32-bit >> platforms has been sufficient, based on testing and experience. In >> fact, in niu's case, a common readq/writeq would have PREVENTED a bug. >> >> (2) unspecified fears continue to linger about non-atomicity >> >> We should not base decisions on fear, particularly when the weight of >> evidence and experience points in the other direction. >> > > I have personally dealt with at least one device who'd want to opt out > of a standard readq/writeq (it's not in-tree because it never shipped, > unfortunately.) Doing the opt-in headers seems like a reasonable thing > to do to me, but perhaps I'm just being overly paranoid. Isn't that a variant of "punish all sane hardware, because bizarre unshipped hardware exists"? IMO the best fix is to document existing readq assumptions, and standardize that definition on other platforms. The burden of special casing for bizarre hardware should not fall on /sane/ drivers and hardware, who should be the ones opting _out_ of the standard regime. Jeff ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-13 21:30 ` Jeff Garzik @ 2009-05-13 21:31 ` Jeff Garzik 2009-05-13 21:54 ` H. Peter Anvin 1 sibling, 0 replies; 50+ messages in thread From: Jeff Garzik @ 2009-05-13 21:31 UTC (permalink / raw) To: H. Peter Anvin Cc: Hitoshi Mitake, Roland Dreier, Ingo Molnar, David Miller, Linus Torvalds, tglx, rpjday, linux-kernel Jeff Garzik wrote: > The burden of special casing for bizarre hardware should not fall on > /sane/ drivers and hardware, who should be the ones opting _out_ of the > standard regime. opting out refers to the bizarre hardware, of course. Poor sentence construction. Sometimes I myself wonder if English is my primary language... Jeff ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-13 21:30 ` Jeff Garzik 2009-05-13 21:31 ` Jeff Garzik @ 2009-05-13 21:54 ` H. Peter Anvin 1 sibling, 0 replies; 50+ messages in thread From: H. Peter Anvin @ 2009-05-13 21:54 UTC (permalink / raw) To: Jeff Garzik Cc: Hitoshi Mitake, Roland Dreier, Ingo Molnar, David Miller, Linus Torvalds, tglx, rpjday, linux-kernel Jeff Garzik wrote: >>> >> I have personally dealt with at least one device who'd want to opt out >> of a standard readq/writeq (it's not in-tree because it never shipped, >> unfortunately.) Doing the opt-in headers seems like a reasonable thing >> to do to me, but perhaps I'm just being overly paranoid. > > Isn't that a variant of "punish all sane hardware, because bizarre > unshipped hardware exists"? > > IMO the best fix is to document existing readq assumptions, and > standardize that definition on other platforms. > > The burden of special casing for bizarre hardware should not fall on > /sane/ drivers and hardware, who should be the ones opting _out_ of the > standard regime. > It sort of is, but it's also a case of "explicitly documenting your assumptions". You have do admit that having to #include a single extra header file is hardly a hardship. -hpa ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-13 20:42 ` Jeff Garzik 2009-05-13 21:05 ` H. Peter Anvin @ 2009-05-13 22:06 ` Roland Dreier 2009-05-13 22:29 ` Jeff Garzik 1 sibling, 1 reply; 50+ messages in thread From: Roland Dreier @ 2009-05-13 22:06 UTC (permalink / raw) To: Jeff Garzik Cc: Hitoshi Mitake, Ingo Molnar, David Miller, Linus Torvalds, hpa, tglx, rpjday, linux-kernel > To repeat what has already been stated, each case was re-evaluated: > http://marc.info/?l=linux-kernel&m=124103527326835&w=2 > > Roland's patch was acked, apparently, _in spite of_ the commonly > accepted readq() definition already being in use! > > Thusfar, I see two things: > > (1) years of history has shown that non-atomic readq/writeq on 32-bit > platforms has been sufficient, based on testing and experience. In > fact, in niu's case, a common readq/writeq would have PREVENTED a bug. But the fact that the 32-bit x86 define would have worked for niu is pure luck -- if the clear-on-read bits had been in the other half of the register in question, then it would have caused a bug. And I really don't trust all ASIC designers writing RTL to think about which half of a 64-bit register is going to be read first. To me, the point is that the current situation of having the defines for 32-bit x86 has zero benefit -- not one driver-specific definition can be removed, because there are other 32-bit architectures to worry about. So we just added another copy of the compatibility wrapper, in a not particularly good location -- we certainly don't want to have the same defines copied into every 32-bit architecture's <asm/io.h> header. And the risk introduced is not zero -- very few devices have 64-bit registers and very few drivers use readq or writeq, but perhaps as end-to-end 64-bit buses become more prevalent with PCIe, we may see more. And it's certainly the case that emulation 64-bit register operations by doing to 32-bit operations on the register halves carries a non-zero risk of making the hardware do something wacky. So to me the it's pretty clear: the current situation has benefit == 0 && risk > 0, so we should revert to the previous situation until someone implements something more complete like hpa's opt-in header scheme. - R. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-05-13 22:06 ` Roland Dreier @ 2009-05-13 22:29 ` Jeff Garzik 0 siblings, 0 replies; 50+ messages in thread From: Jeff Garzik @ 2009-05-13 22:29 UTC (permalink / raw) To: Roland Dreier Cc: Hitoshi Mitake, Ingo Molnar, David Miller, Linus Torvalds, hpa, tglx, rpjday, linux-kernel Roland Dreier wrote: > > To repeat what has already been stated, each case was re-evaluated: > > http://marc.info/?l=linux-kernel&m=124103527326835&w=2 > > > > Roland's patch was acked, apparently, _in spite of_ the commonly > > accepted readq() definition already being in use! > > > > Thusfar, I see two things: > > > > (1) years of history has shown that non-atomic readq/writeq on 32-bit > > platforms has been sufficient, based on testing and experience. In > > fact, in niu's case, a common readq/writeq would have PREVENTED a bug. > > But the fact that the 32-bit x86 define would have worked for niu is > pure luck -- if the clear-on-read bits had been in the other half of the > register in question, then it would have caused a bug. And I really > don't trust all ASIC designers writing RTL to think about which half of > a 64-bit register is going to be read first. AFAICS things have unerringly occurred in PCI ordering, which is what one would expect. What you call pure luck, others call 100% track record. > To me, the point is that the current situation of having the defines for > 32-bit x86 has zero benefit -- not one driver-specific definition can be > removed, because there are other 32-bit architectures to worry about. Um, this is precisely what Mitake-san is trying to address, hence the discussion... > And the risk introduced is not zero -- very few devices have 64-bit > registers and very few drivers use readq or writeq, but perhaps as > end-to-end 64-bit buses become more prevalent with PCIe, we may see > more. And it's certainly the case that emulation 64-bit register > operations by doing to 32-bit operations on the register halves carries > a non-zero risk of making the hardware do something wacky. Again, fear vs. reality, 0% case versus 100% case. You continue to lack CONCRETE examples of problems, while existing cases CONTINUE to work with the obvious ordering. Jeff ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] x86: Remove readq()/writeq() on 32-bit 2009-04-29 11:56 ` Ingo Molnar 2009-04-29 12:10 ` Jeff Garzik @ 2009-04-29 17:21 ` Roland Dreier 1 sibling, 0 replies; 50+ messages in thread From: Roland Dreier @ 2009-04-29 17:21 UTC (permalink / raw) To: Ingo Molnar Cc: David Miller, Linus Torvalds, hpa, tglx, h.mitake, rpjday, linux-kernel > What caused 2c5643b1 was that right now we have ugly per driver > #defines and inlines for readq/writeq. See: but 2c5643b1 doesn't fix that situation -- a portable driver still needs the #defines for other 32-bit architectures. And 2c5643b1 isn't really a particularly good step towards rectifying the situation, since if every 32-bit architecture follows suit and adds its own compatibility versions, then we'll want someone to go through and unify them into a generic implementation. In other words removing the x86 private version will be part of the work in getting to a final solution. > Atomicity of a 64-bit IO space access on 32-bit platforms, on an > unknown-bitness transport (it might even be a 64-bit PCI device > bridged over a 32-bit bridge) is obviously not guaranteed. Yes, some platforms may not be able to give true atomicity. eg 32-bit PowerPC has no instructions that generate 64-bit cycles, even on the CPU bus. I do think the 32-bit PCI thing is a bit of a red herring, since eg PCIe devices can rely on a 64-bit bus. > So trying to suggest that 64-bit readq/writeq when running on a > 64-bit kernel is somehow atomic (or can be made atomic) is really > wrong IMO. The 32-bit wrapper is simply the expression of how the > CPU would do a 64-bit access even in 64-bit mode anyway [if the > transport is 32-bit]. As far as I know, all 64-bit CPUs doing 64-bit accesses to a PCIe device (eg the NIC driven by the niu driver) will generate 64-bit bus cycles. The issue for me is that the benefit of having this compatibility define is rather minimal, while the cost is potentially high: spending a long time debugging platform-specific bugs -- the symptoms will not point immediately to the IO define, of course. - R. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-21 19:09 ` H. Peter Anvin 2009-04-21 21:11 ` Roland Dreier @ 2009-04-22 0:27 ` David Miller 1 sibling, 0 replies; 50+ messages in thread From: David Miller @ 2009-04-22 0:27 UTC (permalink / raw) To: hpa; +Cc: rdreier, h.mitake, mingo, tglx, rpjday, linux-kernel From: "H. Peter Anvin" <hpa@zytor.com> Date: Tue, 21 Apr 2009 12:09:20 -0700 > Do you really expect driver authors to type writeq_nonatomic() for > every register reference? They'll write local driver macros, as every driver does to save typing. It also allows to get rid of the redundant "foop->regs" in every register access too. Look at what all of these drivers do: #define nr64(reg) readq(np->regs + (reg)) #define nw64(reg, val) writeq((val), np->regs + (reg)) So nobody actually types readq() for every register access, just as they don't type foop->regs for every register access either :-) ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars 2009-04-21 17:07 ` Roland Dreier 2009-04-21 17:19 ` H. Peter Anvin @ 2009-04-22 0:25 ` David Miller 1 sibling, 0 replies; 50+ messages in thread From: David Miller @ 2009-04-22 0:25 UTC (permalink / raw) To: rdreier; +Cc: hpa, h.mitake, mingo, tglx, rpjday, linux-kernel From: Roland Dreier <rdreier@cisco.com> Date: Tue, 21 Apr 2009 10:07:50 -0700 > This only makes sense if we define a 32-bit fallback for > readq()/writeq() for all 32-bit architectures -- in fact it would be > good to do it in asm-generic so that there can be a single > implementation that guarantees that non-atomic versions always do, say, > low 32 bits then high 32 bits. (So eg niu can use the generic version) > And then drivers like drivers/infiniband/hw/mthca can be switched to > "#ifndef writeq_atomic <...hardware specific fallback...>" I think if you want to do this right you have to provide two versions of the 32-bit implementations, when the cpu cannot generate full 64-bit transactions. Especially for readq(). Some devices clear the status bits of a 64-bit register when read, so it might matter deeply whether the top-half or the bottom-half 32-bits are read first. The following are ugly names, but something like "readq_hifirst()" and "readq_lofirst()" and they just get defined to play "readq()" in situations where a full 64-bit transaction can be generated by the cpu. The driver author has to figure out which is appropriate. And I'm pretty sure similar high-first/low-first issues can exist for writeq() as well. ^ permalink raw reply [flat|nested] 50+ messages in thread
end of thread, other threads:[~2009-05-21 12:00 UTC | newest] Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-04-19 19:45 arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars Robert P. J. Day 2009-04-19 21:12 ` Roland Dreier 2009-04-19 21:46 ` Ingo Molnar 2009-04-19 22:02 ` H. Peter Anvin 2009-04-19 22:35 ` Ingo Molnar 2009-04-20 0:56 ` Roland Dreier 2009-04-20 2:08 ` Robert Hancock 2009-04-20 0:53 ` Roland Dreier 2009-04-20 1:20 ` H. Peter Anvin 2009-04-20 10:53 ` Ingo Molnar 2009-04-20 14:47 ` Hitoshi Mitake 2009-04-20 16:03 ` Ingo Molnar 2009-04-21 8:33 ` Hitoshi Mitake 2009-04-21 8:45 ` Ingo Molnar 2009-04-21 8:57 ` Hitoshi Mitake 2009-04-21 15:44 ` H. Peter Anvin 2009-04-21 17:07 ` Roland Dreier 2009-04-21 17:19 ` H. Peter Anvin 2009-04-21 17:23 ` Roland Dreier 2009-04-21 19:09 ` H. Peter Anvin 2009-04-21 21:11 ` Roland Dreier 2009-04-21 21:16 ` H. Peter Anvin 2009-04-22 0:31 ` David Miller 2009-04-28 19:05 ` [PATCH] x86: Remove readq()/writeq() on 32-bit Roland Dreier 2009-04-29 5:12 ` David Miller 2009-04-29 11:56 ` Ingo Molnar 2009-04-29 12:10 ` Jeff Garzik 2009-04-29 17:25 ` Roland Dreier 2009-04-29 19:59 ` Jeff Garzik 2009-05-13 5:32 ` Hitoshi Mitake 2009-05-13 20:19 ` H. Peter Anvin 2009-05-13 22:39 ` Jeff Garzik 2009-05-13 23:39 ` H. Peter Anvin 2009-05-14 0:49 ` Jeff Garzik 2009-05-14 7:19 ` Hitoshi Mitake 2009-05-15 23:44 ` Jeff Garzik 2009-05-17 7:12 ` Hitoshi Mitake 2009-05-17 8:06 ` Jeff Garzik 2009-05-21 11:35 ` Hitoshi Mitake 2009-05-21 11:49 ` Hitoshi Mitake 2009-05-13 20:42 ` Jeff Garzik 2009-05-13 21:05 ` H. Peter Anvin 2009-05-13 21:30 ` Jeff Garzik 2009-05-13 21:31 ` Jeff Garzik 2009-05-13 21:54 ` H. Peter Anvin 2009-05-13 22:06 ` Roland Dreier 2009-05-13 22:29 ` Jeff Garzik 2009-04-29 17:21 ` Roland Dreier 2009-04-22 0:27 ` arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars David Miller 2009-04-22 0:25 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.