[PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Matthew Wilcox]

The only places that uses readq/writeq are in the initialisation
path.  Since they're not performance critical, always use readl/writel.

Reported-by: H. Peter Anvin <hpa@linux.intel.com>
Signed-off-by: Matthew Wilcox <matthew.r.wilcox@intel.com>
---
 drivers/block/nvme.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/block/nvme.c b/drivers/block/nvme.c
index c1dc4d8..f27deec 100644
--- a/drivers/block/nvme.c
+++ b/drivers/block/nvme.c
@@ -972,6 +972,20 @@ static __devinit struct nvme_queue *nvme_create_queue(struct nvme_dev *dev,
 	return ERR_PTR(result);
 }
 
+static inline unsigned long long nvme_readq(const volatile void __iomem *addr)
+{
+	unsigned long long result = readl(addr);
+	result |= (unsigned long long)readl(addr + 4) << 32;
+	return result;
+}
+
+static inline void
+nvme_writeq(unsigned long long val, volatile void __iomem *addr)
+{
+	writel(val, addr);
+	writel(val >> 32, addr + 4);
+}
+
 static int __devinit nvme_configure_admin_queue(struct nvme_dev *dev)
 {
 	int result;
@@ -996,11 +1010,11 @@ static int __devinit nvme_configure_admin_queue(struct nvme_dev *dev)
 
 	writel(0, &dev->bar->cc);
 	writel(aqa, &dev->bar->aqa);
-	writeq(nvmeq->sq_dma_addr, &dev->bar->asq);
-	writeq(nvmeq->cq_dma_addr, &dev->bar->acq);
+	nvme_writeq(nvmeq->sq_dma_addr, &dev->bar->asq);
+	nvme_writeq(nvmeq->cq_dma_addr, &dev->bar->acq);
 	writel(dev->ctrl_config, &dev->bar->cc);
 
-	cap = readq(&dev->bar->cap);
+	cap = nvme_readq(&dev->bar->cap);
 	timeout = ((NVME_CAP_TIMEOUT(cap) + 1) * HZ / 2) + jiffies;
 	dev->db_stride = NVME_CAP_STRIDE(cap);
 
-- 
1.7.7.3

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Linus Torvalds]

On Thu, Jan 19, 2012 at 5:01 PM, Matthew Wilcox
<matthew.r.wilcox@intel.com> wrote:
> The only places that uses readq/writeq are in the initialisation
> path.  Since they're not performance critical, always use readl/writel.

The arch rules are that i fthe architecture has readq/writeq, they
will be #define'd (they may be inline functions, but there will be a

  #define readq readq

to make it visible to the preprocessor as well).

So if you don't need the atomicity guarantees of a "real" readq, you
can do this instead:

  #ifndef readq
  static inline u64 readq(void __iomem *addr)
  {
        return readl(addr) | (((u64) readl(addr + 4)) << 32LL);
  }
  #endif

and then use readq() as if it existed.

And I do think we should expose this in some generic manner. Because
we currently don't, we already have that pattern copied in quite a few
drivers.

Maybe <asm-generic/io-nonatomic.h> or something? Making it clear that
its not atomic, but avoiding the silly duplication we do now..

This whole mess was introduced in commit dbee8a0affd5 ("x86: remove
32-bit versions of readq()/writeq()"), and it already talked about the
problems but didn't help with the drivers that simply don't care.

All the people in those threads were doing their self-satisfied
"writeq is broken", without much acknowledging that 99% of users
simply don't seem to care.

"Occupy Writeq - We are the 99%"

                Linus

RE: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Wilcox, Matthew R]

Yep, I knew I could check for readq with an ifdef, but I'd rather just use the 32-bit versions everywhere and have consistent behaviour between bittedness.  If this were a performance path, I'd absolutely do what you suggest.

One minor point is that '|' is not a sequence point, so it's not defined whether you'll read the top or bottom half of the register first.  And some hardware people are crazy enough to care.

For this particular hardware, it's defined to work if you read the low order bits first, so I went with the nvme_readq() function to emphasise that this solution works for the nvme driver.

-----Original Message-----
From: linus971@gmail.com [mailto:linus971@gmail.com] On Behalf Of Linus Torvalds
Sent: Thursday, January 19, 2012 5:22 PM
To: Wilcox, Matthew R; Roland Dreier; Andrew Morton; Hitoshi Mitake; James Bottomley; Ingo Molnar
Cc: linux-kernel@vger.kernel.org; linux-nvme@infradead.org; hpa@linux.intel.com
Subject: Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

On Thu, Jan 19, 2012 at 5:01 PM, Matthew Wilcox
<matthew.r.wilcox@intel.com> wrote:
> The only places that uses readq/writeq are in the initialisation
> path.  Since they're not performance critical, always use readl/writel.

The arch rules are that i fthe architecture has readq/writeq, they
will be #define'd (they may be inline functions, but there will be a

  #define readq readq

to make it visible to the preprocessor as well).

So if you don't need the atomicity guarantees of a "real" readq, you
can do this instead:

  #ifndef readq
  static inline u64 readq(void __iomem *addr)
  {
        return readl(addr) | (((u64) readl(addr + 4)) << 32LL);
  }
  #endif

and then use readq() as if it existed.

And I do think we should expose this in some generic manner. Because
we currently don't, we already have that pattern copied in quite a few
drivers.

Maybe <asm-generic/io-nonatomic.h> or something? Making it clear that
its not atomic, but avoiding the silly duplication we do now..

This whole mess was introduced in commit dbee8a0affd5 ("x86: remove
32-bit versions of readq()/writeq()"), and it already talked about the
problems but didn't help with the drivers that simply don't care.

All the people in those threads were doing their self-satisfied
"writeq is broken", without much acknowledging that 99% of users
simply don't seem to care.

"Occupy Writeq - We are the 99%"

                Linus

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Jan 19, 2012 at 5:01 PM, Matthew Wilcox
> <matthew.r.wilcox@intel.com> wrote:
> > The only places that uses readq/writeq are in the initialisation
> > path.  Since they're not performance critical, always use readl/writel.
> 
> The arch rules are that i fthe architecture has readq/writeq, they
> will be #define'd (they may be inline functions, but there will be a
> 
>   #define readq readq
> 
> to make it visible to the preprocessor as well).
> 
> So if you don't need the atomicity guarantees of a "real" readq, you
> can do this instead:
> 
>   #ifndef readq
>   static inline u64 readq(void __iomem *addr)
>   {
>         return readl(addr) | (((u64) readl(addr + 4)) << 32LL);
>   }
>   #endif
> 
> and then use readq() as if it existed.
> 
> And I do think we should expose this in some generic manner. Because
> we currently don't, we already have that pattern copied in quite a few
> drivers.
> 
> Maybe <asm-generic/io-nonatomic.h> or something? Making it 
> clear that its not atomic, but avoiding the silly duplication 
> we do now..
> 
> This whole mess was introduced in commit dbee8a0affd5 ("x86: 
> remove 32-bit versions of readq()/writeq()"), and it already 
> talked about the problems but didn't help with the drivers 
> that simply don't care.
> 
> All the people in those threads were doing their 
> self-satisfied "writeq is broken", without much acknowledging 
> that 99% of users simply don't seem to care.
> 
> "Occupy Writeq - We are the 99%"

Agreed, and offering a generic facility for silly duplication 
was the motivation of the original commit by Hitoshi Mitake.

This:

| The presense of a writeq() implementation on 32-bit x86 that 
| splits the 64-bit write into two 32-bit writes turns out to 
| break the mpt2sas driver (and in general is risky for drivers 
| as was discussed in
|   <http://lkml.kernel.org/r/adaab6c1h7c.fsf@cisco.com>).

is actually a mostly bogus statement and creates more problems 
than it solves.

Hitoshi-san, would you be interested in re-adding the generic 
readq/writeq definitions in a slight variation to 2c5643b1c5, to 
a separate io-nonatomic.h file, so that drivers that want it can
#include that file and be happy?
 
Thanks,

	Ingo

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Hitoshi Mitake]

On Sat, Jan 21, 2012 at 17:28, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> On Thu, Jan 19, 2012 at 5:01 PM, Matthew Wilcox
>> <matthew.r.wilcox@intel.com> wrote:
>> > The only places that uses readq/writeq are in the initialisation
>> > path.  Since they're not performance critical, always use readl/writel.
>>
>> The arch rules are that i fthe architecture has readq/writeq, they
>> will be #define'd (they may be inline functions, but there will be a
>>
>>   #define readq readq
>>
>> to make it visible to the preprocessor as well).
>>
>> So if you don't need the atomicity guarantees of a "real" readq, you
>> can do this instead:
>>
>>   #ifndef readq
>>   static inline u64 readq(void __iomem *addr)
>>   {
>>         return readl(addr) | (((u64) readl(addr + 4)) << 32LL);
>>   }
>>   #endif
>>
>> and then use readq() as if it existed.
>>
>> And I do think we should expose this in some generic manner. Because
>> we currently don't, we already have that pattern copied in quite a few
>> drivers.
>>
>> Maybe <asm-generic/io-nonatomic.h> or something? Making it
>> clear that its not atomic, but avoiding the silly duplication
>> we do now..
>>
>> This whole mess was introduced in commit dbee8a0affd5 ("x86:
>> remove 32-bit versions of readq()/writeq()"), and it already
>> talked about the problems but didn't help with the drivers
>> that simply don't care.
>>
>> All the people in those threads were doing their
>> self-satisfied "writeq is broken", without much acknowledging
>> that 99% of users simply don't seem to care.
>>
>> "Occupy Writeq - We are the 99%"
>
> Agreed, and offering a generic facility for silly duplication
> was the motivation of the original commit by Hitoshi Mitake.
>
> This:
>
> | The presense of a writeq() implementation on 32-bit x86 that
> | splits the 64-bit write into two 32-bit writes turns out to
> | break the mpt2sas driver (and in general is risky for drivers
> | as was discussed in
> |   <http://lkml.kernel.org/r/adaab6c1h7c.fsf@cisco.com>).
>
> is actually a mostly bogus statement and creates more problems
> than it solves.
>
> Hitoshi-san, would you be interested in re-adding the generic
> readq/writeq definitions in a slight variation to 2c5643b1c5, to
> a separate io-nonatomic.h file, so that drivers that want it can
> #include that file and be happy?

It sounds nice. In the previous discussion, I suggested that
chaning the name of non-atomic readq/writeq to
readq_nonatomic/writeq_nonatomic. And James Bottomley
replied that it is fine but not really very useful:

https://lkml.org/lkml/2011/5/19/13

The idea of providing non-atomic readq/writeq in the new file
with the name which express non-atomicity clearly might be
able to satisfy both of safety and usefulness.

It will reduce the duplication of the definition. In addition
readq/writeq users don't have to type the long symbols with
_nonatomic suffix and can know non-atomicity from the name
of header file.

I'd like to hear opinions from James, Roland and folks who
dislike non-atomic readq/writeq.

-- 
Hitoshi Mitake
h.mitake@gmail.com

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Ingo Molnar]

* Hitoshi Mitake <h.mitake@gmail.com> wrote:

> On Sat, Jan 21, 2012 at 17:28, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >
> >> On Thu, Jan 19, 2012 at 5:01 PM, Matthew Wilcox
> >> <matthew.r.wilcox@intel.com> wrote:
> >> > The only places that uses readq/writeq are in the initialisation
> >> > path.  Since they're not performance critical, always use readl/writel.
> >>
> >> The arch rules are that i fthe architecture has readq/writeq, they
> >> will be #define'd (they may be inline functions, but there will be a
> >>
> >>   #define readq readq
> >>
> >> to make it visible to the preprocessor as well).
> >>
> >> So if you don't need the atomicity guarantees of a "real" readq, you
> >> can do this instead:
> >>
> >>   #ifndef readq
> >>   static inline u64 readq(void __iomem *addr)
> >>   {
> >>         return readl(addr) | (((u64) readl(addr + 4)) << 32LL);
> >>   }
> >>   #endif
> >>
> >> and then use readq() as if it existed.
> >>
> >> And I do think we should expose this in some generic manner. Because
> >> we currently don't, we already have that pattern copied in quite a few
> >> drivers.
> >>
> >> Maybe <asm-generic/io-nonatomic.h> or something? Making it
> >> clear that its not atomic, but avoiding the silly duplication
> >> we do now..
> >>
> >> This whole mess was introduced in commit dbee8a0affd5 ("x86:
> >> remove 32-bit versions of readq()/writeq()"), and it already
> >> talked about the problems but didn't help with the drivers
> >> that simply don't care.
> >>
> >> All the people in those threads were doing their
> >> self-satisfied "writeq is broken", without much acknowledging
> >> that 99% of users simply don't seem to care.
> >>
> >> "Occupy Writeq - We are the 99%"
> >
> > Agreed, and offering a generic facility for silly duplication
> > was the motivation of the original commit by Hitoshi Mitake.
> >
> > This:
> >
> > | The presense of a writeq() implementation on 32-bit x86 that
> > | splits the 64-bit write into two 32-bit writes turns out to
> > | break the mpt2sas driver (and in general is risky for drivers
> > | as was discussed in
> > |   <http://lkml.kernel.org/r/adaab6c1h7c.fsf@cisco.com>).
> >
> > is actually a mostly bogus statement and creates more problems
> > than it solves.
> >
> > Hitoshi-san, would you be interested in re-adding the generic
> > readq/writeq definitions in a slight variation to 2c5643b1c5, to
> > a separate io-nonatomic.h file, so that drivers that want it can
> > #include that file and be happy?
> 
> It sounds nice. In the previous discussion, I suggested that
> chaning the name of non-atomic readq/writeq to
> readq_nonatomic/writeq_nonatomic. And James Bottomley
> replied that it is fine but not really very useful:
> 
> https://lkml.org/lkml/2011/5/19/13
> 
> The idea of providing non-atomic readq/writeq in the new file
> with the name which express non-atomicity clearly might be
> able to satisfy both of safety and usefulness.
> 
> It will reduce the duplication of the definition. In addition
> readq/writeq users don't have to type the long symbols with
> _nonatomic suffix and can know non-atomicity from the name
> of header file.
> 
> I'd like to hear opinions from James, Roland and folks who
> dislike non-atomic readq/writeq.

Drivers that want the definition add this line to their driver:

#include <asm/io-nonatomic.h>

and then readq()/writeq() does the obvious thing. No need for 
readq_nonatomic()/writeq_nonatomic() - that extra line declares 
things clearly enough and cannot be added accidentally.

Thanks,

	Ingo

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Hitoshi Mitake]

On Sun, Jan 22, 2012 at 01:58, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Hitoshi Mitake <h.mitake@gmail.com> wrote:
>
>> On Sat, Jan 21, 2012 at 17:28, Ingo Molnar <mingo@elte.hu> wrote:
>> >
>> > * Linus Torvalds <torvalds@linux-foundation.org> wrote:
>> >
>> >> On Thu, Jan 19, 2012 at 5:01 PM, Matthew Wilcox
>> >> <matthew.r.wilcox@intel.com> wrote:
>> >> > The only places that uses readq/writeq are in the initialisation
>> >> > path.  Since they're not performance critical, always use readl/writel.
>> >>
>> >> The arch rules are that i fthe architecture has readq/writeq, they
>> >> will be #define'd (they may be inline functions, but there will be a
>> >>
>> >>   #define readq readq
>> >>
>> >> to make it visible to the preprocessor as well).
>> >>
>> >> So if you don't need the atomicity guarantees of a "real" readq, you
>> >> can do this instead:
>> >>
>> >>   #ifndef readq
>> >>   static inline u64 readq(void __iomem *addr)
>> >>   {
>> >>         return readl(addr) | (((u64) readl(addr + 4)) << 32LL);
>> >>   }
>> >>   #endif
>> >>
>> >> and then use readq() as if it existed.
>> >>
>> >> And I do think we should expose this in some generic manner. Because
>> >> we currently don't, we already have that pattern copied in quite a few
>> >> drivers.
>> >>
>> >> Maybe <asm-generic/io-nonatomic.h> or something? Making it
>> >> clear that its not atomic, but avoiding the silly duplication
>> >> we do now..
>> >>
>> >> This whole mess was introduced in commit dbee8a0affd5 ("x86:
>> >> remove 32-bit versions of readq()/writeq()"), and it already
>> >> talked about the problems but didn't help with the drivers
>> >> that simply don't care.
>> >>
>> >> All the people in those threads were doing their
>> >> self-satisfied "writeq is broken", without much acknowledging
>> >> that 99% of users simply don't seem to care.
>> >>
>> >> "Occupy Writeq - We are the 99%"
>> >
>> > Agreed, and offering a generic facility for silly duplication
>> > was the motivation of the original commit by Hitoshi Mitake.
>> >
>> > This:
>> >
>> > | The presense of a writeq() implementation on 32-bit x86 that
>> > | splits the 64-bit write into two 32-bit writes turns out to
>> > | break the mpt2sas driver (and in general is risky for drivers
>> > | as was discussed in
>> > |   <http://lkml.kernel.org/r/adaab6c1h7c.fsf@cisco.com>).
>> >
>> > is actually a mostly bogus statement and creates more problems
>> > than it solves.
>> >
>> > Hitoshi-san, would you be interested in re-adding the generic
>> > readq/writeq definitions in a slight variation to 2c5643b1c5, to
>> > a separate io-nonatomic.h file, so that drivers that want it can
>> > #include that file and be happy?
>>
>> It sounds nice. In the previous discussion, I suggested that
>> chaning the name of non-atomic readq/writeq to
>> readq_nonatomic/writeq_nonatomic. And James Bottomley
>> replied that it is fine but not really very useful:
>>
>> https://lkml.org/lkml/2011/5/19/13
>>
>> The idea of providing non-atomic readq/writeq in the new file
>> with the name which express non-atomicity clearly might be
>> able to satisfy both of safety and usefulness.
>>
>> It will reduce the duplication of the definition. In addition
>> readq/writeq users don't have to type the long symbols with
>> _nonatomic suffix and can know non-atomicity from the name
>> of header file.
>>
>> I'd like to hear opinions from James, Roland and folks who
>> dislike non-atomic readq/writeq.
>
> Drivers that want the definition add this line to their driver:
>
> #include <asm/io-nonatomic.h>
>
> and then readq()/writeq() does the obvious thing. No need for
> readq_nonatomic()/writeq_nonatomic() - that extra line declares
> things clearly enough and cannot be added accidentally.
>

I wrote the patch which adds the new file include/asm-generic/io-nonatomic.h.
io-nonatomic.h provides non-atomic version readq()/writeq().

The patch also removes some duplicated readq()/writeq() definitions
which are added in the commit: dbee8a0affd5e6eaa5d7c816c4bc233f6f110f50.
The commit was made by Roland Dreier and removed readq()/writeq()
from arch/x86/include/asm.h.

If I find no error after the building allyesconfig and allmodconfig test,
I'll send the patch for review. This may take a few hour.

BTW, I placed io-nonatomic.h under include/asm-generic/ because
non-atomic version readq()/writeq() is architecture dependent.
If you have comments on this point, I'd like to hear.

Thanks,
-- 
Hitoshi Mitake
h.mitake@gmail.com

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Linus Torvalds]

On Mon, Jan 23, 2012 at 8:05 AM, Hitoshi Mitake <h.mitake@gmail.com> wrote:
>
> I wrote the patch which adds the new file include/asm-generic/io-nonatomic.h.
> io-nonatomic.h provides non-atomic version readq()/writeq().

I do wonder if we should do "little-endian" and "big-endian" variations?

Quoting Willy:

  "For this particular hardware, it's defined to work if you read the
low order bits first"

so I think we need make that explicit, and make two include files:

   include/asm-generic/io-64b-lo-hi.h
   include/asm-generic/io-64b-hi-lo.h

or something like that. And thus indirectly document these kinds of
requirements.

Hmm?

                Linus

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[H. Peter Anvin]

On 01/23/2012 08:57 AM, Linus Torvalds wrote:
> On Mon, Jan 23, 2012 at 8:05 AM, Hitoshi Mitake<h.mitake@gmail.com>  wrote:
>>
>> I wrote the patch which adds the new file include/asm-generic/io-nonatomic.h.
>> io-nonatomic.h provides non-atomic version readq()/writeq().
>
> I do wonder if we should do "little-endian" and "big-endian" variations?
>
> Quoting Willy:
>
>    "For this particular hardware, it's defined to work if you read the
> low order bits first"
>
> so I think we need make that explicit, and make two include files:
>
>     include/asm-generic/io-64b-lo-hi.h
>     include/asm-generic/io-64b-hi-lo.h
>
> or something like that. And thus indirectly document these kinds of
> requirements.
>

I would concur with this (and I have suggested exactly this in the past.)

	-hpa

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Hitoshi Mitake]

On Tue, Jan 24, 2012 at 01:57, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Jan 23, 2012 at 8:05 AM, Hitoshi Mitake <h.mitake@gmail.com> wrote:
>>
>> I wrote the patch which adds the new file include/asm-generic/io-nonatomic.h.
>> io-nonatomic.h provides non-atomic version readq()/writeq().
>
> I do wonder if we should do "little-endian" and "big-endian" variations?

Thanks, I'll prepare two variations based on CPU_LITTLE_ENDIAN.

>
> Quoting Willy:
>
>  "For this particular hardware, it's defined to work if you read the
> low order bits first"
>
> so I think we need make that explicit, and make two include files:
>
>   include/asm-generic/io-64b-lo-hi.h
>   include/asm-generic/io-64b-hi-lo.h
>
> or something like that. And thus indirectly document these kinds of
> requirements.
>
> Hmm?
>
>                Linus

I have a question about the order of readl/writel.
If the non-atomic readq/writeq is placed under asm-generic/,
they will be used by every architecture.

I don't know about the minor architectures, but some of them,
like alpha, seems to do reordering of memory access agressively.

Is the reordering is applied to io rw?
Should memory barriers be placed between two readl/writel?

-- 
Hitoshi Mitake
h.mitake@gmail.com

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Linus Torvalds]

On Sun, Jan 29, 2012 at 12:02 AM, Hitoshi Mitake <h.mitake@gmail.com> wrote:
>
> I don't know about the minor architectures, but some of them,
> like alpha, seems to do reordering of memory access agressively.
>
> Is the reordering is applied to io rw?
> Should memory barriers be placed between two readl/writel?

No need to place barriers - the "readl/writel()" functions are ordered
in themselves. There are non-ordered versions in theory
("writel_relaxed()") for things like frame buffers etc that actively
want the ordering, but that's a separate issue entirely.

You do want to make sure that they aren't in the same C expression, so
that the compiler doesn't re-order the expression. IOW, if you just do

  return (readl(addr+4) << 32) | readl(addr);

then that doesn't have any ordering at all simply because there is
none at the C level. But

  u64 val;
  val = readl(addr);
  val |= readl(addr+4) << 32;

is well-defined and must read the low word first - both at the C level
*and* at the CPU level. Anything else would be a bug in the
architecture "readl()" implementation or the hardware.

(On x86, for example, a "readl()" is just a memory access, but while
x86 can re-order reads to regular memory in hardware, that is *not*
true of IO memory accesses. On architectures like POWER, 'readl()'
implies synchronization instructions)

                   Linus

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Linus Torvalds]

On Mon, Jan 30, 2012 at 7:03 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>  val |= readl(addr+4) << 32;

Oops. That needs a cast to u64 before the shift, of course.

Take all code I write in emails with a pinch of salt - think of it
more as pseudo-code than something that necessarily would work as-is.

                  Linus

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Hitoshi Mitake]

On Tue, Jan 31, 2012 at 12:05, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Jan 30, 2012 at 7:03 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>  val |= readl(addr+4) << 32;
>
> Oops. That needs a cast to u64 before the shift, of course.
>
> Take all code I write in emails with a pinch of salt - think of it
> more as pseudo-code than something that necessarily would work as-is.
>
>                  Linus

I'll take care, thanks :)


-- 
Hitoshi Mitake
h.mitake@gmail.com

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Alan Cox]

>   u64 val;
>   val = readl(addr);
>   val |= readl(addr+4) << 32;
> 
> is well-defined and must read the low word first - both at the C level
> *and* at the CPU level. Anything else would be a bug in the
> architecture "readl()" implementation or the hardware.

That doesn't make the access atomic to hardware however as a true 64bit
readq/writeq would be ?

It seems to me the two are not quite the same semantically

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> >   u64 val;
> >   val = readl(addr);
> >   val |= readl(addr+4) << 32;
> > 
> > is well-defined and must read the low word first - both at the C level
> > *and* at the CPU level. Anything else would be a bug in the
> > architecture "readl()" implementation or the hardware.
> 
> That doesn't make the access atomic to hardware however as a true 64bit
> readq/writeq would be ?
> 
> It seems to me the two are not quite the same semantically

Correct, and that's what the:

	#include <asm/io-inatomic.h>

line in the driver would express.

Thanks,

	Ingo

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Alan Cox]

On Tue, 31 Jan 2012 13:09:22 +0100
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> 
> > >   u64 val;
> > >   val = readl(addr);
> > >   val |= readl(addr+4) << 32;
> > > 
> > > is well-defined and must read the low word first - both at the C level
> > > *and* at the CPU level. Anything else would be a bug in the
> > > architecture "readl()" implementation or the hardware.
> > 
> > That doesn't make the access atomic to hardware however as a true 64bit
> > readq/writeq would be ?
> > 
> > It seems to me the two are not quite the same semantically
> 
> Correct, and that's what the:
> 
> 	#include <asm/io-inatomic.h>
> 
> line in the driver would express.

Why would "inatomic" indicate that - I'm confused. It would imply to me
they were extra specially atomic ?  

(atomos if from the Greek so in- as a prefix isn't the same in- as in
many other words, welcome to English hell - who needs perl)

non-atomic.h might be better, or 'un-atomic' or 'multi-read' or
something ?

Alan

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> On Tue, 31 Jan 2012 13:09:22 +0100
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > * Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > 
> > > >   u64 val;
> > > >   val = readl(addr);
> > > >   val |= readl(addr+4) << 32;
> > > > 
> > > > is well-defined and must read the low word first - both at the C level
> > > > *and* at the CPU level. Anything else would be a bug in the
> > > > architecture "readl()" implementation or the hardware.
> > > 
> > > That doesn't make the access atomic to hardware however as a true 64bit
> > > readq/writeq would be ?
> > > 
> > > It seems to me the two are not quite the same semantically
> > 
> > Correct, and that's what the:
> > 
> > 	#include <asm/io-inatomic.h>
> > 
> > line in the driver would express.
> 
> Why would "inatomic" indicate that - I'm confused. It would 
> imply to me they were extra specially atomic ?

Yeah, s/inatomic/non-atomic.

inatomic would be doubly confusing for the reason that it's 
already used as an 'in atomic section' sense in the kernel.

> (atomos if from the Greek so in- as a prefix isn't the same 
> in- as in many other words, welcome to English hell - who 
> needs perl)
> 
> non-atomic.h might be better, or 'un-atomic' or 'multi-read' 
> or something ?

non-atomic sounds good to me too.

Thanks,

	Ingo

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Linus Torvalds]

On Tue, Jan 31, 2012 at 4:23 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> non-atomic sounds good to me too.

You both apparently missed the related discussion that some devices
really do care about order, even if they don't care about atomicity.

So we'd actually have two versions of the header file, one
little-endian, and one big-endian. Then the driver that knows it
doesn't need the atomic 'readq()' that is always defined, but wants a
low-bytes-first version would just do

   #include <linux/io64-little-endian.h>

(or "big-endian" if it wants to read/write high bits first). Most
drivers probably don't care, but apparently NVMe does.

                 Linus

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[James Bottomley]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1623 bytes --]

On Wed, 2012-02-01 at 15:35 -0800, Linus Torvalds wrote:
> On Tue, Jan 31, 2012 at 4:23 AM, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > non-atomic sounds good to me too.
> 
> You both apparently missed the related discussion that some devices
> really do care about order, even if they don't care about atomicity.
> 
> So we'd actually have two versions of the header file, one
> little-endian, and one big-endian. Then the driver that knows it
> doesn't need the atomic 'readq()' that is always defined, but wants a
> low-bytes-first version would just do
> 
>    #include <linux/io64-little-endian.h>
> 
> (or "big-endian" if it wants to read/write high bits first). Most
> drivers probably don't care, but apparently NVMe does.

And this was about the point I concluded last time that it simply wasn't
worth it with the number of different possibilities for the primitives
and trying to come up with a sensible naming scheme ... it's just easier
to open code because then you get exactly what you meant.

Incidentally, the last time this came up was with mpt fusion: for a
write to a 64 bit register, it didn't care about order, but it did care
about interleaving as in if you write one half of a 64 bit register and
then write to another register, the 64 bit register effectively gets
written with zeros in the part you didn't write to, so we had to put a
spin lock in the open coded writeb/w/l/q() to make sure the card didn't
get interleaved writes.

James

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Linus Torvalds]

On Wed, Feb 1, 2012 at 5:05 PM, James Bottomley
<jbottomley@parallels.com> wrote:
>
> And this was about the point I concluded last time that it simply wasn't
> worth it with the number of different possibilities for the primitives

Umm? Two?

Two really doesn't seem like a big number to me.

                    Linus

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[H. Peter Anvin]

On 02/01/2012 05:05 PM, James Bottomley wrote:
>
> Incidentally, the last time this came up was with mpt fusion: for a
> write to a 64 bit register, it didn't care about order, but it did care
> about interleaving as in if you write one half of a 64 bit register and
> then write to another register, the 64 bit register effectively gets
> written with zeros in the part you didn't write to, so we had to put a
> spin lock in the open coded writeb/w/l/q() to make sure the card didn't
> get interleaved writes.
>

There are always going to be hardware which have specific needs, and for 
those open-coding makes sense, but the littleendian/bigendian pair is 
going to cover ~90% of users and make sense to can.

I worked myself on a driver (which sadly never shipped) which had an WC 
window and a UC window... the final write in a series had a completion 
bit in it and would go to the UC window after setting up a whole chunk 
of operations in the WC window (writing UC memory flushes WC memory 
ahead of it.)

Thus, the two-part breakdown of writeq() to the UC window had to write 
the low half to the WC window instead.  This is clearly not generic.

	-hpa

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Hitoshi Mitake]

On Fri, Feb 3, 2012 at 00:05, H. Peter Anvin <hpa@linux.intel.com> wrote:
> On 02/01/2012 05:05 PM, James Bottomley wrote:
>>
>>
>> Incidentally, the last time this came up was with mpt fusion: for a
>> write to a 64 bit register, it didn't care about order, but it did care
>> about interleaving as in if you write one half of a 64 bit register and
>> then write to another register, the 64 bit register effectively gets
>> written with zeros in the part you didn't write to, so we had to put a
>> spin lock in the open coded writeb/w/l/q() to make sure the card didn't
>> get interleaved writes.
>>
>
> There are always going to be hardware which have specific needs, and for
> those open-coding makes sense, but the littleendian/bigendian pair is going
> to cover ~90% of users and make sense to can.
>
> I worked myself on a driver (which sadly never shipped) which had an WC
> window and a UC window... the final write in a series had a completion bit
> in it and would go to the UC window after setting up a whole chunk of
> operations in the WC window (writing UC memory flushes WC memory ahead of
> it.)
>
> Thus, the two-part breakdown of writeq() to the UC window had to write the
> low half to the WC window instead.  This is clearly not generic.
>
>        -hpa
>
>

Because of my ignorant, I don't know the words "UC window" and "WC window"
in this context. Could you teach me?

-- 
Hitoshi Mitake
h.mitake@gmail.com

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Geert Uytterhoeven]

On Sat, Feb 4, 2012 at 16:39, Hitoshi Mitake <h.mitake@gmail.com> wrote:
>> I worked myself on a driver (which sadly never shipped) which had an WC
>> window and a UC window... the final write in a series had a completion bit
>> in it and would go to the UC window after setting up a whole chunk of
>> operations in the WC window (writing UC memory flushes WC memory ahead of
>> it.)
>>
>> Thus, the two-part breakdown of writeq() to the UC window had to write the
>> low half to the WC window instead.  This is clearly not generic.
>
> Because of my ignorant, I don't know the words "UC window" and "WC window"
> in this context. Could you teach me?

UN = uncached, WC = writecombining.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Hitoshi Mitake]

On Sun, Feb 5, 2012 at 15:53, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Sat, Feb 4, 2012 at 16:39, Hitoshi Mitake <h.mitake@gmail.com> wrote:
>>> I worked myself on a driver (which sadly never shipped) which had an WC
>>> window and a UC window... the final write in a series had a completion bit
>>> in it and would go to the UC window after setting up a whole chunk of
>>> operations in the WC window (writing UC memory flushes WC memory ahead of
>>> it.)
>>>
>>> Thus, the two-part breakdown of writeq() to the UC window had to write the
>>> low half to the WC window instead.  This is clearly not generic.
>>
>> Because of my ignorant, I don't know the words "UC window" and "WC window"
>> in this context. Could you teach me?
>
> UN = uncached, WC = writecombining.

Thanks a lot for your teaching!

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds



-- 
Hitoshi Mitake
h.mitake@gmail.com

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Hitoshi Mitake]

On Thu, Feb 2, 2012 at 10:05, James Bottomley <jbottomley@parallels.com> wrote:
> On Wed, 2012-02-01 at 15:35 -0800, Linus Torvalds wrote:
>> On Tue, Jan 31, 2012 at 4:23 AM, Ingo Molnar <mingo@elte.hu> wrote:
>> >
>> > non-atomic sounds good to me too.
>>
>> You both apparently missed the related discussion that some devices
>> really do care about order, even if they don't care about atomicity.
>>
>> So we'd actually have two versions of the header file, one
>> little-endian, and one big-endian. Then the driver that knows it
>> doesn't need the atomic 'readq()' that is always defined, but wants a
>> low-bytes-first version would just do
>>
>>    #include <linux/io64-little-endian.h>
>>
>> (or "big-endian" if it wants to read/write high bits first). Most
>> drivers probably don't care, but apparently NVMe does.
>
> And this was about the point I concluded last time that it simply wasn't
> worth it with the number of different possibilities for the primitives
> and trying to come up with a sensible naming scheme ... it's just easier
> to open code because then you get exactly what you meant.
>
> Incidentally, the last time this came up was with mpt fusion: for a
> write to a 64 bit register, it didn't care about order, but it did care
> about interleaving as in if you write one half of a 64 bit register and
> then write to another register, the 64 bit register effectively gets
> written with zeros in the part you didn't write to, so we had to put a
> spin lock in the open coded writeb/w/l/q() to make sure the card didn't
> get interleaved writes.
>
> James
>

As you say, readq/writeq without any description about the semantics
of atomicity will cause confusion in such a case.

But new plan for non-atomic readq/writeq is defining non-atomic readq/writeq
in the header file like asm-generic/io-nonatomic-hi-lo.h, and the file name
is a good documentation for the description.

The drivers which use readq/writeq without the line like
#include <asm-generic/io-nonatomic-hi-lo.h>
will cause compile error in the 32-bit environment.

-- 
Hitoshi Mitake
h.mitake@gmail.com

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Hitoshi Mitake]

On Sun, Feb 5, 2012 at 00:34, Hitoshi Mitake <h.mitake@gmail.com> wrote:
> On Thu, Feb 2, 2012 at 10:05, James Bottomley <jbottomley@parallels.com> wrote:
>> On Wed, 2012-02-01 at 15:35 -0800, Linus Torvalds wrote:
>>> On Tue, Jan 31, 2012 at 4:23 AM, Ingo Molnar <mingo@elte.hu> wrote:
>>> >
>>> > non-atomic sounds good to me too.
>>>
>>> You both apparently missed the related discussion that some devices
>>> really do care about order, even if they don't care about atomicity.
>>>
>>> So we'd actually have two versions of the header file, one
>>> little-endian, and one big-endian. Then the driver that knows it
>>> doesn't need the atomic 'readq()' that is always defined, but wants a
>>> low-bytes-first version would just do
>>>
>>>    #include <linux/io64-little-endian.h>
>>>
>>> (or "big-endian" if it wants to read/write high bits first). Most
>>> drivers probably don't care, but apparently NVMe does.
>>
>> And this was about the point I concluded last time that it simply wasn't
>> worth it with the number of different possibilities for the primitives
>> and trying to come up with a sensible naming scheme ... it's just easier
>> to open code because then you get exactly what you meant.
>>
>> Incidentally, the last time this came up was with mpt fusion: for a
>> write to a 64 bit register, it didn't care about order, but it did care
>> about interleaving as in if you write one half of a 64 bit register and
>> then write to another register, the 64 bit register effectively gets
>> written with zeros in the part you didn't write to, so we had to put a
>> spin lock in the open coded writeb/w/l/q() to make sure the card didn't
>> get interleaved writes.
>>
>> James
>>
>
> As you say, readq/writeq without any description about the semantics
> of atomicity will cause confusion in such a case.
>
> But new plan for non-atomic readq/writeq is defining non-atomic readq/writeq
> in the header file like asm-generic/io-nonatomic-hi-lo.h, and the file name
> is a good documentation for the description.
>
> The drivers which use readq/writeq without the line like
> #include <asm-generic/io-nonatomic-hi-lo.h>
> will cause compile error in the 32-bit environment.
>
> --
> Hitoshi Mitake
> h.mitake@gmail.com

I sent the patch which implements readq/writeq in the way.
If you have comments, I'd like to hear.

Thanks,

-- 
Hitoshi Mitake
h.mitake@gmail.com

Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq

[Hitoshi Mitake]

On Tue, Jan 31, 2012 at 12:03, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sun, Jan 29, 2012 at 12:02 AM, Hitoshi Mitake <h.mitake@gmail.com> wrote:
>>
>> I don't know about the minor architectures, but some of them,
>> like alpha, seems to do reordering of memory access agressively.
>>
>> Is the reordering is applied to io rw?
>> Should memory barriers be placed between two readl/writel?
>
> No need to place barriers - the "readl/writel()" functions are ordered
> in themselves. There are non-ordered versions in theory
> ("writel_relaxed()") for things like frame buffers etc that actively
> want the ordering, but that's a separate issue entirely.
>
> You do want to make sure that they aren't in the same C expression, so
> that the compiler doesn't re-order the expression. IOW, if you just do
>
>  return (readl(addr+4) << 32) | readl(addr);
>
> then that doesn't have any ordering at all simply because there is
> none at the C level. But
>
>  u64 val;
>  val = readl(addr);
>  val |= readl(addr+4) << 32;
>
> is well-defined and must read the low word first - both at the C level
> *and* at the CPU level. Anything else would be a bug in the
> architecture "readl()" implementation or the hardware.
>
> (On x86, for example, a "readl()" is just a memory access, but while
> x86 can re-order reads to regular memory in hardware, that is *not*
> true of IO memory accesses. On architectures like POWER, 'readl()'
> implies synchronization instructions)
>
>                   Linus

Thanks for your description.
Now I can understand the semantics of readl/writel of the kernel.

-- 
Hitoshi Mitake
h.mitake@gmail.com