Re: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

Re: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

[Matthew Wilcox]

On Wed, May 18, 2011 at 09:37:08AM +0530, Desai, Kashyap wrote:
> On Wed, 2011-05-04 at 17:23 +0530, Kashyap, Desai wrote:
> > The following code seems to be there in /usr/src/linux/arch/x86/include/asm/io.h.
> > This is not going to work.
> > 
> > static inline void writeq(__u64 val, volatile void __iomem *addr)
> > {
> >         writel(val, addr);
> >         writel(val >> 32, addr+4);
> > }
> > 
> > So with this code turned on in the kernel, there is going to be race condition 
> > where multiple cpus can be writing to the request descriptor at the same time.
> > 
> > Meaning this could happen:
> > (A) CPU A doest 32bit write
> > (B) CPU B does 32 bit write
> > (C) CPU A does 32 bit write
> > (D) CPU B does 32 bit write
> > 
> > We need the 64 bit completed in one access pci memory write, else spin lock is required.
> > Since it's going to be difficult to know which writeq was implemented in the kernel, 
> > the driver is going to have to always acquire a spin lock each time we do 64bit write.
> > 
> > Cc: stable@kernle.org
> > Signed-off-by: Kashyap Desai <kashyap.desai@lsi.com>
> > ---
> > diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
> > index efa0255..5778334 100644
> > --- a/drivers/scsi/mpt2sas/mpt2sas_base.c
> > +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
> > @@ -1558,7 +1558,6 @@ mpt2sas_base_free_smid(struct MPT2SAS_ADAPTER *ioc, u16 smid)
> >   * care of 32 bit environment where its not quarenteed to send the entire word
> >   * in one transfer.
> >   */
> > -#ifndef writeq
> 
> Why not make this #ifndef CONFIG_64BIT?  You know that all 64 bit
> systems have writeq implemented correctly; you suspect 32 bit systems
> don't.
> 
> James
> 
> James, This issue was observed on PPC64 system. So what you have suggested will not solve this issue.
> If we are sure that writeq() is atomic across all architecture, we can use it safely. As we have seen issue on ppc64, we are not confident to use
> "writeq" call.

So have you told the powerpc people that they have a broken writeq?
And why do you obfuscate your report by talking about i386 when it's
really about powerpc64?

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

[James Bottomley]

On Tue, 2011-05-17 at 22:15 -0600, Matthew Wilcox wrote:
> On Wed, May 18, 2011 at 09:37:08AM +0530, Desai, Kashyap wrote:
> > On Wed, 2011-05-04 at 17:23 +0530, Kashyap, Desai wrote:
> > > The following code seems to be there in /usr/src/linux/arch/x86/include/asm/io.h.
> > > This is not going to work.
> > > 
> > > static inline void writeq(__u64 val, volatile void __iomem *addr)
> > > {
> > >         writel(val, addr);
> > >         writel(val >> 32, addr+4);
> > > }
> > > 
> > > So with this code turned on in the kernel, there is going to be race condition 
> > > where multiple cpus can be writing to the request descriptor at the same time.
> > > 
> > > Meaning this could happen:
> > > (A) CPU A doest 32bit write
> > > (B) CPU B does 32 bit write
> > > (C) CPU A does 32 bit write
> > > (D) CPU B does 32 bit write
> > > 
> > > We need the 64 bit completed in one access pci memory write, else spin lock is required.
> > > Since it's going to be difficult to know which writeq was implemented in the kernel, 
> > > the driver is going to have to always acquire a spin lock each time we do 64bit write.
> > > 
> > > Cc: stable@kernle.org
> > > Signed-off-by: Kashyap Desai <kashyap.desai@lsi.com>
> > > ---
> > > diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > index efa0255..5778334 100644
> > > --- a/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > @@ -1558,7 +1558,6 @@ mpt2sas_base_free_smid(struct MPT2SAS_ADAPTER *ioc, u16 smid)
> > >   * care of 32 bit environment where its not quarenteed to send the entire word
> > >   * in one transfer.
> > >   */
> > > -#ifndef writeq
> > 
> > Why not make this #ifndef CONFIG_64BIT?  You know that all 64 bit
> > systems have writeq implemented correctly; you suspect 32 bit systems
> > don't.
> > 
> > James
> > 
> > James, This issue was observed on PPC64 system. So what you have suggested will not solve this issue.
> > If we are sure that writeq() is atomic across all architecture, we can use it safely. As we have seen issue on ppc64, we are not confident to use
> > "writeq" call.
> 
> So have you told the powerpc people that they have a broken writeq?

I'm just in the process of finding them now on IRC so I can demand an
explanation: this is a really serious API problem because writeq is
supposed to be atomic on 64 bit.

> And why do you obfuscate your report by talking about i386 when it's
> really about powerpc64?

James

Re: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

[Benjamin Herrenschmidt]

(Just adding Milton to the CC list, he suspects races in the driver
instead).

On Wed, 2011-05-18 at 08:23 +0400, James Bottomley wrote:
> On Tue, 2011-05-17 at 22:15 -0600, Matthew Wilcox wrote:
> > On Wed, May 18, 2011 at 09:37:08AM +0530, Desai, Kashyap wrote:
> > > On Wed, 2011-05-04 at 17:23 +0530, Kashyap, Desai wrote:
> > > > The following code seems to be there in /usr/src/linux/arch/x86/include/asm/io.h.
> > > > This is not going to work.
> > > > 
> > > > static inline void writeq(__u64 val, volatile void __iomem *addr)
> > > > {
> > > >         writel(val, addr);
> > > >         writel(val >> 32, addr+4);
> > > > }
> > > > 
> > > > So with this code turned on in the kernel, there is going to be race condition 
> > > > where multiple cpus can be writing to the request descriptor at the same time.
> > > > 
> > > > Meaning this could happen:
> > > > (A) CPU A doest 32bit write
> > > > (B) CPU B does 32 bit write
> > > > (C) CPU A does 32 bit write
> > > > (D) CPU B does 32 bit write
> > > > 
> > > > We need the 64 bit completed in one access pci memory write, else spin lock is required.
> > > > Since it's going to be difficult to know which writeq was implemented in the kernel, 
> > > > the driver is going to have to always acquire a spin lock each time we do 64bit write.
> > > > 
> > > > Cc: stable@kernle.org
> > > > Signed-off-by: Kashyap Desai <kashyap.desai@lsi.com>
> > > > ---
> > > > diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > > index efa0255..5778334 100644
> > > > --- a/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > > +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > > @@ -1558,7 +1558,6 @@ mpt2sas_base_free_smid(struct MPT2SAS_ADAPTER *ioc, u16 smid)
> > > >   * care of 32 bit environment where its not quarenteed to send the entire word
> > > >   * in one transfer.
> > > >   */
> > > > -#ifndef writeq
> > > 
> > > Why not make this #ifndef CONFIG_64BIT?  You know that all 64 bit
> > > systems have writeq implemented correctly; you suspect 32 bit systems
> > > don't.
> > > 
> > > James
> > > 
> > > James, This issue was observed on PPC64 system. So what you have suggested will not solve this issue.
> > > If we are sure that writeq() is atomic across all architecture, we can use it safely. As we have seen issue on ppc64, we are not confident to use
> > > "writeq" call.
> > 
> > So have you told the powerpc people that they have a broken writeq?
> 
> I'm just in the process of finding them now on IRC so I can demand an
> explanation: this is a really serious API problem because writeq is
> supposed to be atomic on 64 bit.
> 
> > And why do you obfuscate your report by talking about i386 when it's
> > really about powerpc64?
> 
> James
> 

Re: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

[Milton Miller]

On Wed, 18 May 2011 around 17:00:10 +1000, Benjamin Herrenschmidt wrote:
> (Just adding Milton to the CC list, he suspects races in the
> driver instead).
>
> On Wed, 2011-05-18 at 08:23 +0400, James Bottomley wrote:
> > On Tue, 2011-05-17 at 22:15 -0600, Matthew Wilcox wrote:
> > > On Wed, May 18, 2011 at 09:37:08AM +0530, Desai, Kashyap wrote:
> > > > On Wed, 2011-05-04 at 17:23 +0530, Kashyap, Desai wrote:
> > > > > The following code seems to be there in /usr/src/linux/arch/x86/include/asm/io.h.
> > > > > This is not going to work.
> > > > > 
> > > > > static inline void writeq(__u64 val, volatile void __iomem *addr)
> > > > > {
> > > > >         writel(val, addr);
> > > > >         writel(val >> 32, addr+4);
> > > > > }
> > > > > 
> > > > > So with this code turned on in the kernel, there is going to be race condition 
> > > > > where multiple cpus can be writing to the request descriptor at the same time.
> > > > > 
> > > > > Meaning this could happen:
> > > > > (A) CPU A doest 32bit write
> > > > > (B) CPU B does 32 bit write
> > > > > (C) CPU A does 32 bit write
> > > > > (D) CPU B does 32 bit write
> > > > > 
> > > > > We need the 64 bit completed in one access pci memory write, else spin lock is required.
> > > > > Since it's going to be difficult to know which writeq was implemented in the kernel, 
> > > > > the driver is going to have to always acquire a spin lock each time we do 64bit write.
> > > > > 
> > > > > Cc: stable@kernle.org
> > > > > Signed-off-by: Kashyap Desai <kashyap.desai@lsi.com>
> > > > > ---
> > > > > diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > > > index efa0255..5778334 100644
> > > > > --- a/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > > > +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > > > @@ -1558,7 +1558,6 @@ mpt2sas_base_free_smid(struct MPT2SAS_ADAPTER *ioc, u16 smid)
> > > > >   * care of 32 bit environment where its not quarenteed to send the entire word
> > > > >   * in one transfer.
> > > > >   */
> > > > > -#ifndef writeq
> > > > 
> > > > Why not make this #ifndef CONFIG_64BIT?  You know that all 64 bit
> > > > systems have writeq implemented correctly; you suspect 32 bit systems
> > > > don't.
> > > > 
> > > > James
> > > > 
> > > > James, This issue was observed on PPC64 system. So what you have suggested will not solve this issue.
> > > > If we are sure that writeq() is atomic across all architecture, we can use it safely. As we have seen issue on ppc64, we are not confident to use
> > > > "writeq" call.
> > > 
> > > So have you told the powerpc people that they have a broken writeq?
> > 
> > I'm just in the process of finding them now on IRC so I can demand an
> > explanation: this is a really serious API problem because writeq is
> > supposed to be atomic on 64 bit.
> > 
> > > And why do you obfuscate your report by talking about i386 when it's
> > > really about powerpc64?
> > 
> > James

I checked the assembly for my complied output and it ends up with
a single std (store doubleword aka 64 bits) instruction with offset
192 decimal (0xc0) from the base register obtained from the structure.

An aligned doubleword store is atomic on 64 bit powerpc.

So I would really like more details if you are blaming 64 bit
powerpc of a non-atomic store.

That said, the patch will affect the code by adding barriers.
Specifically, while powerpc has a sync before doing the store as part
of writeq, wrapping in a spinlock adds a sync before releasing the lock
whenever a writeq (or writex x=b,w,d,q) was issued inside the lock.

(sync orders all reads and all writes to both memory and devices from
that cpu).

But looking further at the code, I see such things as:

drivers/scsi/mpt2sas/mpt2sas_base.c  line 2944

        mpt2sas_base_put_smid_default(ioc, smid);
        init_completion(&ioc->base_cmds.done);
        timeleft = wait_for_completion_timeout(&ioc->base_cmds.done,

where mpt2sas_base_put_smid_default is a routine that has a call to
_base_writeq.  This will initiate io to the adapter, then initialize
the completion, then hope that the timeout is long enough to let the io
complete and be marked done but short enough to not be a problem when
the timeout occurs because we initialized the compeltion after the irq
came in.

The code then looks at a status flag, but there is no indication how
the access to that field is serialized between the interrupt handler
and the submission routine.  It may mostly work due to barriers in
the primitives but I don't see any statement of rules.

Also, while I see a few wmb before writel in _base_interrupt, I don't
see any rmb, which I would expect between establishing a element is
valid and reading other fields in that element.

So I'd really like to hear more about what your symptoms were and how
you determined writeq on 64 bit powerpc was not atomic.

milton

RE: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

[Moore, Eric]

On Wednesday, May 18, 2011 2:24 AM, Milton Miller wrote:
> On Wed, 18 May 2011 around 17:00:10 +1000, Benjamin Herrenschmidt wrote:
> > (Just adding Milton to the CC list, he suspects races in the
> > driver instead).
> >
> > On Wed, 2011-05-18 at 08:23 +0400, James Bottomley wrote:
> > > On Tue, 2011-05-17 at 22:15 -0600, Matthew Wilcox wrote:
> > > > On Wed, May 18, 2011 at 09:37:08AM +0530, Desai, Kashyap wrote:
> > > > > On Wed, 2011-05-04 at 17:23 +0530, Kashyap, Desai wrote:
> > > > > > The following code seems to be there in
> /usr/src/linux/arch/x86/include/asm/io.h.
> > > > > > This is not going to work.
> > > > > >
> > > > > > static inline void writeq(__u64 val, volatile void __iomem *add=
r)
> > > > > > {
> > > > > >         writel(val, addr);
> > > > > >         writel(val >> 32, addr+4);
> > > > > > }
> > > > > >
> > > > > > So with this code turned on in the kernel, there is going to be
> race condition
> > > > > > where multiple cpus can be writing to the request descriptor at
> the same time.
> > > > > >
> > > > > > Meaning this could happen:
> > > > > > (A) CPU A doest 32bit write
> > > > > > (B) CPU B does 32 bit write
> > > > > > (C) CPU A does 32 bit write
> > > > > > (D) CPU B does 32 bit write
> > > > > >
> > > > > > We need the 64 bit completed in one access pci memory write, el=
se
> spin lock is required.
> > > > > > Since it's going to be difficult to know which writeq was
> implemented in the kernel,
> > > > > > the driver is going to have to always acquire a spin lock each
> time we do 64bit write.
> > > > > >
> > > > > > Cc: stable@kernle.org
> > > > > > Signed-off-by: Kashyap Desai <kashyap.desai@lsi.com>
> > > > > > ---
> > > > > > diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c
> b/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > > > > index efa0255..5778334 100644
> > > > > > --- a/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > > > > +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > > > > @@ -1558,7 +1558,6 @@ mpt2sas_base_free_smid(struct
> MPT2SAS_ADAPTER *ioc, u16 smid)
> > > > > >   * care of 32 bit environment where its not quarenteed to send
> the entire word
> > > > > >   * in one transfer.
> > > > > >   */
> > > > > > -#ifndef writeq
> > > > >
> > > > > Why not make this #ifndef CONFIG_64BIT?  You know that all 64 bit
> > > > > systems have writeq implemented correctly; you suspect 32 bit
> systems
> > > > > don't.
> > > > >
> > > > > James
> > > > >
> > > > > James, This issue was observed on PPC64 system. So what you have
> suggested will not solve this issue.
> > > > > If we are sure that writeq() is atomic across all architecture, w=
e
> can use it safely. As we have seen issue on ppc64, we are not confident t=
o
> use
> > > > > "writeq" call.
> > > >
> > > > So have you told the powerpc people that they have a broken writeq?
> > >
> > > I'm just in the process of finding them now on IRC so I can demand an
> > > explanation: this is a really serious API problem because writeq is
> > > supposed to be atomic on 64 bit.
> > >
> > > > And why do you obfuscate your report by talking about i386 when it'=
s
> > > > really about powerpc64?
> > >
> > > James
>=20
> I checked the assembly for my complied output and it ends up with
> a single std (store doubleword aka 64 bits) instruction with offset
> 192 decimal (0xc0) from the base register obtained from the structure.
>=20
> An aligned doubleword store is atomic on 64 bit powerpc.
>=20
> So I would really like more details if you are blaming 64 bit
> powerpc of a non-atomic store.
>=20
> That said, the patch will affect the code by adding barriers.
> Specifically, while powerpc has a sync before doing the store as part
> of writeq, wrapping in a spinlock adds a sync before releasing the lock
> whenever a writeq (or writex x=3Db,w,d,q) was issued inside the lock.
>=20
> (sync orders all reads and all writes to both memory and devices from
> that cpu).
>=20
> But looking further at the code, I see such things as:
>=20
> drivers/scsi/mpt2sas/mpt2sas_base.c  line 2944
>=20
>         mpt2sas_base_put_smid_default(ioc, smid);
>         init_completion(&ioc->base_cmds.done);
>         timeleft =3D wait_for_completion_timeout(&ioc->base_cmds.done,
>=20
> where mpt2sas_base_put_smid_default is a routine that has a call to
> _base_writeq.  This will initiate io to the adapter, then initialize
> the completion, then hope that the timeout is long enough to let the io
> complete and be marked done but short enough to not be a problem when
> the timeout occurs because we initialized the compeltion after the irq
> came in.
>=20
> The code then looks at a status flag, but there is no indication how
> the access to that field is serialized between the interrupt handler
> and the submission routine.  It may mostly work due to barriers in
> the primitives but I don't see any statement of rules.
>=20
> Also, while I see a few wmb before writel in _base_interrupt, I don't
> see any rmb, which I would expect between establishing a element is
> valid and reading other fields in that element.
>=20
> So I'd really like to hear more about what your symptoms were and how
> you determined writeq on 64 bit powerpc was not atomic.
>=20
> Milton


I worked the original defect a couple months ago, and Kashyap is now gettin=
g around to posting my patch's.

This original defect has nothing to do with PPC64.  The original problem wa=
s only on x86.    It only became a problem on PPC64 when I tried to fix the=
 original x86 issue by copying the writeq code from the linux headers, then=
 it broke PPC64.   I doubt that broken patch was ever posted. Anyways, back=
 to the original defect.  The reason it because a problem for x86 is becaus=
e the kernel headers had a implementation of writeq in the arch/x86 headers=
, which means our internal implementation of writeq is not being used.  The=
 writeq implementation in the kernel is total wrong for arch/x86 because it=
 doesn't not have spin locks, and if two processor simultaneously doing two=
 separate 32bit pci writes, then what is received by controller firmware is=
 out of order.   This change occurs between Red Hat RHEL5 and RHEL6.  In RH=
EL5, this writeq was not implemented in arch/x86 headers, and our driver in=
ternal implementation of write was used.

Eric

RE: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

[Milton Miller]

On Wed, 18 May 2011 about 09:35:56 -0600, Eric Moore wrote:
> On Wednesday, May 18, 2011 2:24 AM, Milton Miller wrote:
> > On Wed, 18 May 2011 around 17:00:10 +1000, Benjamin Herrenschmidt wrote:
> > > (Just adding Milton to the CC list, he suspects races in the
> > > driver instead).
> > >
> > > On Wed, 2011-05-18 at 08:23 +0400, James Bottomley wrote:
> > > > On Tue, 2011-05-17 at 22:15 -0600, Matthew Wilcox wrote:
> > > > > On Wed, May 18, 2011 at 09:37:08AM +0530, Desai, Kashyap wrote:
> > > > > > On Wed, 2011-05-04 at 17:23 +0530, Kashyap, Desai wrote:
> > > > > > > The following code seems to be there in
> > /usr/src/linux/arch/x86/include/asm/io.h.
> > > > > > > This is not going to work.
> > > > > > >
> > > > > > > static inline void writeq(__u64 val, volatile void __iomem *addr)
> > > > > > > {
> > > > > > >         writel(val, addr);
> > > > > > >         writel(val >> 32, addr+4);
> > > > > > > }
> > > > > > >
> > > > > > > So with this code turned on in the kernel, there is going to be
> > race condition
> > > > > > > where multiple cpus can be writing to the request descriptor at
> > the same time.
> > > > > > >
> > > > > > > Meaning this could happen:
> > > > > > > (A) CPU A doest 32bit write
> > > > > > > (B) CPU B does 32 bit write
> > > > > > > (C) CPU A does 32 bit write
> > > > > > > (D) CPU B does 32 bit write
> > > > > > >
> > > > > > > We need the 64 bit completed in one access pci memory write, else
> > spin lock is required.
> > > > > > > Since it's going to be difficult to know which writeq was
> > implemented in the kernel,
> > > > > > > the driver is going to have to always acquire a spin lock each
> > time we do 64bit write.
> > > > > > >
> > > > > > > Cc: stable@kernle.org
> > > > > > > Signed-off-by: Kashyap Desai <kashyap.desai@lsi.com>
> > > > > > > ---
> > > > > > > diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c
> > b/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > > > > > index efa0255..5778334 100644
> > > > > > > --- a/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > > > > > +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > > > > > @@ -1558,7 +1558,6 @@ mpt2sas_base_free_smid(struct
> > MPT2SAS_ADAPTER *ioc, u16 smid)
> > > > > > >   * care of 32 bit environment where its not quarenteed to send
> > the entire word
> > > > > > >   * in one transfer.
> > > > > > >   */
> > > > > > > -#ifndef writeq
> > > > > >
> > > > > > Why not make this #ifndef CONFIG_64BIT?  You know that all 64 bit
> > > > > > systems have writeq implemented correctly; you suspect 32 bit
> > systems
> > > > > > don't.
> > > > > >
> > > > > > James
> > > > > >
> > > > > > James, This issue was observed on PPC64 system. So what you have
> > suggested will not solve this issue.
> > > > > > If we are sure that writeq() is atomic across all architecture, we
> > can use it safely. As we have seen issue on ppc64, we are not confident to
> > use
> > > > > > "writeq" call.
> > > > >
> > > > > So have you told the powerpc people that they have a broken writeq?
> > > >
> > > > I'm just in the process of finding them now on IRC so I can demand an
> > > > explanation: this is a really serious API problem because writeq is
> > > > supposed to be atomic on 64 bit.
> > > >
> > > > > And why do you obfuscate your report by talking about i386 when it's
> > > > > really about powerpc64?
> > > >
> > > > James
> >
> > I checked the assembly for my complied output and it ends up with
> > a single std (store doubleword aka 64 bits) instruction with offset
> > 192 decimal (0xc0) from the base register obtained from the structure.
> >
> > An aligned doubleword store is atomic on 64 bit powerpc.
> >
> > So I would really like more details if you are blaming 64 bit
> > powerpc of a non-atomic store.
> >
> > That said, the patch will affect the code by adding barriers.
> > Specifically, while powerpc has a sync before doing the store as part
> > of writeq, wrapping in a spinlock adds a sync before releasing the lock
> > whenever a writeq (or writex x=b,w,d,q) was issued inside the lock.
> >
> > (sync orders all reads and all writes to both memory and devices from
> > that cpu).
> >
> > But looking further at the code, I see such things as:
> >
> > drivers/scsi/mpt2sas/mpt2sas_base.c  line 2944
> >
> >         mpt2sas_base_put_smid_default(ioc, smid);
> >         init_completion(&ioc->base_cmds.done);
> >         timeleft = wait_for_completion_timeout(&ioc->base_cmds.done,
> >
> > where mpt2sas_base_put_smid_default is a routine that has a call to
> > _base_writeq.  This will initiate io to the adapter, then initialize
> > the completion, then hope that the timeout is long enough to let the io
> > complete and be marked done but short enough to not be a problem when
> > the timeout occurs because we initialized the compeltion after the irq
> > came in.
> >
> > The code then looks at a status flag, but there is no indication how
> > the access to that field is serialized between the interrupt handler
> > and the submission routine.  It may mostly work due to barriers in
> > the primitives but I don't see any statement of rules.
> >
> > Also, while I see a few wmb before writel in _base_interrupt, I don't
> > see any rmb, which I would expect between establishing a element is
> > valid and reading other fields in that element.
> >
> > So I'd really like to hear more about what your symptoms were and how
> > you determined writeq on 64 bit powerpc was not atomic.
> >
> > Milton
> 
> 
> I worked the original defect a couple months ago, and Kashyap is now
> getting around to posting my patch's.
> 
> This original defect has nothing to do with PPC64.  The original
> problem was only on x86.    It only became a problem on PPC64 when I
> tried to fix the original x86 issue by copying the writeq code from
> the linux headers, then it broke PPC64.   I doubt that broken patch
> was ever posted. Anyways, back to the original defect.  The reason
> it because a problem for x86 is because the kernel headers had a
> implementation of writeq in the arch/x86 headers, which means our
> internal implementation of writeq is not being used.  The writeq
> implementation in the kernel is total wrong for arch/x86 because it
> doesn't not have spin locks, and if two processor simultaneously doing
> two separate 32bit pci writes, then what is received by controller
> firmware is out of order.   This change occurs between Red Hat RHEL5
> and RHEL6.  In RHEL5, this writeq was not implemented in arch/x86
> headers, and our driver internal implementation of write was used.
> 
> Eric

So the real question should be why is x86-32 supplying a broken writeq
instead of letting drivers work out what to do it when needed?

It was added in 2.6.29 with out any pci or other io acks in
the change log.

Also the only reference I find to HAVE_WRITEQ and HAVE_READQ is the
x86 selects, so I think those can just be removed (vs move to x86_64).

The original changelog is:
    Impact: add new API for drivers
    
    Add implementation of readq/writeq to x86_32, and add config value to
    the x86 architecture to determine existence of readq/writeq.

Ingo I would propose the following commits added in 2.6.29 be reverted.
I think the current concensus is drivers must know if the writeq is
not atomic so they can provide their own locking or other workaround.

2c5643b1c5c7fbb13f340d4c58944d9642f41796
	x86: provide readq()/writeq() on 32-bit too
a0b1131e479e5af32eefac8bc54c9742e23d638e
	x86: provide readq()/writeq() on 32-bit too, cleanup
93093d099e5dd0c258fd530c12668e828c20df41
	x86: provide readq()/writeq() on 32-bit too, complete

milton

RE: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

[Moore, Eric]

On Wednesday, May 18, 2011 12:31 PM Milton Miller wrote:
> Ingo I would propose the following commits added in 2.6.29 be reverted.
> I think the current concensus is drivers must know if the writeq is
> not atomic so they can provide their own locking or other workaround.
>=20


Exactly.

Re: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

[Hitoshi Mitake]

On Thu, May 19, 2011 at 04:11, Moore, Eric <Eric.Moore@lsi.com> wrote:
> On Wednesday, May 18, 2011 12:31 PM Milton Miller wrote:
>> Ingo I would propose the following commits added in 2.6.29 be reverted.
>> I think the current concensus is drivers must know if the writeq is
>> not atomic so they can provide their own locking or other workaround.
>>
>
>
> Exactly.
>

The original motivation of preparing common readq/writeq is that
letting each driver
have their own readq/writeq is bad for maintenance of source code.

But if you really dislike them, there might be two solutions:

1. changing the name of readq/writeq to readq_nonatomic/writeq_nonatomic
2. adding new C file to somewhere and defining spinlock for them.
    With spin_lock_irqsave() and spin_unlock_irqrestore() on the spinlock,
    readq/writeq can be atomic.

How do you think about them? If you cannot agree with the above two solutions,
I'll agree with reverting them.

-- 
Hitoshi Mitake
h.mitake@gmail.com

Re: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

[James Bottomley]

On Thu, 2011-05-19 at 13:08 +0900, Hitoshi Mitake wrote:
> On Thu, May 19, 2011 at 04:11, Moore, Eric <Eric.Moore@lsi.com> wrote:
> > On Wednesday, May 18, 2011 12:31 PM Milton Miller wrote:
> >> Ingo I would propose the following commits added in 2.6.29 be reverted.
> >> I think the current concensus is drivers must know if the writeq is
> >> not atomic so they can provide their own locking or other workaround.
> >>
> >
> >
> > Exactly.
> >
> 
> The original motivation of preparing common readq/writeq is that
> letting each driver
> have their own readq/writeq is bad for maintenance of source code.
> 
> But if you really dislike them, there might be two solutions:
> 
> 1. changing the name of readq/writeq to readq_nonatomic/writeq_nonatomic

This is fine, but not really very useful

> 2. adding new C file to somewhere and defining spinlock for them.
>     With spin_lock_irqsave() and spin_unlock_irqrestore() on the spinlock,
>     readq/writeq can be atomic.

This can't really be done generically.  There are several considerations
to do with hardware requirements.  I can see some hw requiring a
specific write order (I think this applies more to read order, though).

The specific mpt2sas problem is that if we write a 64 bit register non
atomically, we can't allow any interleaving writes for any other region
on the chip, otherwise the HW will take the write as complete in the 64
bit register and latch the wrong value.  The only way to achieve that
given the semantics of writeq is a global static spinlock.

> How do you think about them? If you cannot agree with the above two
> solutions, I'll agree with reverting them.

Having x86 roll its own never made any sense, so I think they need
reverting anyway.  This is a driver/platform bus problem not an
architecture problem.  The assumption we can make is that the platform
CPU can write atomically at its chip width.  We *may* be able to make
the assumption that the bus controller can translate an atomic chip
width transaction to a single atomic bus transaction; I think that
assumption holds true for at least PCI and on the parisc legacy busses,
so if we can agree on semantics, this should be a global define
somewhere.  If there are problems with the bus assumption, we'll likely
need some type of opt-in (or just not bother).

James

Re: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

[Benjamin Herrenschmidt]

On Thu, 2011-05-19 at 08:46 +0400, James Bottomley wrote:
> This can't really be done generically.  There are several considerations
> to do with hardware requirements.  I can see some hw requiring a
> specific write order (I think this applies more to read order, though).

Right. Or there can be a need for a completely different access pattern
to do 32-bit, or maybe write only one half because both might have a
side effect etc etc etc ...

Also a global lock would be suboptimal vs. a per device lock burried in
the driver.

> The specific mpt2sas problem is that if we write a 64 bit register non
> atomically, we can't allow any interleaving writes for any other region
> on the chip, otherwise the HW will take the write as complete in the 64
> bit register and latch the wrong value.  The only way to achieve that
> given the semantics of writeq is a global static spinlock.
> 
> > How do you think about them? If you cannot agree with the above two
> > solutions, I'll agree with reverting them.
> 
> Having x86 roll its own never made any sense, so I think they need
> reverting anyway. 

Agreed.

>  This is a driver/platform bus problem not an
> architecture problem.  The assumption we can make is that the platform
> CPU can write atomically at its chip width.  We *may* be able to make
> the assumption that the bus controller can translate an atomic chip
> width transaction to a single atomic bus transaction; I think that
> assumption holds true for at least PCI and on the parisc legacy busses,
> so if we can agree on semantics, this should be a global define
> somewhere.  If there are problems with the bus assumption, we'll likely
> need some type of opt-in (or just not bother).

And we want a well defined #ifdef drivers test to know whether there's a
writeq/readq (just #define writeq/readq itself is fine even if it's an
inline function, we do that elsewhere) so they can have a fallback
scenario.

This is important as these can be used in very performance critical code
path.

Cheers,
Ben.

RE: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq isnot atomic

[David Laight]

=20
> The specific mpt2sas problem is that if we write a 64 bit register non
> atomically, we can't allow any interleaving writes for any other
region
> on the chip, otherwise the HW will take the write as complete in the
64
> bit register and latch the wrong value.  The only way to achieve that
> given the semantics of writeq is a global static spinlock.

That sounds like very specific and slightly dodgy hardware.
You don't say what the scope of 'region on the chip' is, but
it looks like you need to disable ALL writes to the memory
area between the first and second writes of the 64bit value
and not just those coming from writeq().
I don't see how this can possibly be done by normal mutexing
around the writeq() sequence, surely you need to lock the bus
between the two transfers.
Even dma writes would be a problem.

The only way I can think to stop other cpus doing writes
is to use IPIs for force them into a busy wait loop.

All rather reminds me of a PCI slave that got things
horribly wrong when a read was done while a write was
still 'posted', or a 2nd master did a cycle did a read
while a read rerun sequence was still in progress.
(required a mutex and dummy cycles).
At least than one wqs confined to one driver.

	David

Re: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

[Roland Dreier]

On Wed, May 18, 2011 at 11:31 AM, Milton Miller <miltonm@bga.com> wrote:
> So the real question should be why is x86-32 supplying a broken writeq
> instead of letting drivers work out what to do it when needed?

Sounds a lot like what I was asking a couple of years ago :)
http://lkml.org/lkml/2009/4/19/164

But Ingo insisted that non-atomic writeq would be fine:
http://lkml.org/lkml/2009/4/19/167

 - R.

Re: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

[Benjamin Herrenschmidt]

On Wed, 2011-05-18 at 21:16 -0700, Roland Dreier wrote:
> On Wed, May 18, 2011 at 11:31 AM, Milton Miller <miltonm@bga.com> wrote:
> > So the real question should be why is x86-32 supplying a broken writeq
> > instead of letting drivers work out what to do it when needed?
> 
> Sounds a lot like what I was asking a couple of years ago :)
> http://lkml.org/lkml/2009/4/19/164
> 
> But Ingo insisted that non-atomic writeq would be fine:
> http://lkml.org/lkml/2009/4/19/167

Yuck... Ingo, I think that was very wrong.

Those are for MMIO, which must almost ALWAYS know precisely what the
resulting access size is going to be. It's not even about atomicity
between multiple CPUs. I have seen plenty of HW for which a 64-bit
access to a register is -not- equivalent to two 32-bit ones. In fact, in
some case, you can get the side effects twice ... or none at all.

The only case where you can be lax is when you explicitely know that
there is no side effects -and- the HW cope with different access sizes.
This is not the general case and drivers need at the very least a way to
know what the behaviour will be.

Cheers,
Ben.

Re: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

[Ingo Molnar]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Wed, 2011-05-18 at 21:16 -0700, Roland Dreier wrote:
> > On Wed, May 18, 2011 at 11:31 AM, Milton Miller <miltonm@bga.com> wrote:
> > > So the real question should be why is x86-32 supplying a broken writeq
> > > instead of letting drivers work out what to do it when needed?
> > 
> > Sounds a lot like what I was asking a couple of years ago :)
> > http://lkml.org/lkml/2009/4/19/164
> > 
> > But Ingo insisted that non-atomic writeq would be fine:
> > http://lkml.org/lkml/2009/4/19/167
> 
> Yuck... Ingo, I think that was very wrong.
> 
> Those are for MMIO, which must almost ALWAYS know precisely what the
> resulting access size is going to be. It's not even about atomicity
> between multiple CPUs. I have seen plenty of HW for which a 64-bit
> access to a register is -not- equivalent to two 32-bit ones. In fact, in
> some case, you can get the side effects twice ... or none at all.
> 
> The only case where you can be lax is when you explicitely know that
> there is no side effects -and- the HW cope with different access sizes.
> This is not the general case and drivers need at the very least a way to
> know what the behaviour will be.

Ok, that's pretty convincing.

Unless hpa or tglx disagrees with reverting this, could any of you send a patch 
with a proper changelog etc. that applies cleanly to v2.6.39?

Thanks,

	Ingo

RE: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

[Benjamin Herrenschmidt]

On Wed, 2011-05-18 at 09:35 -0600, Moore, Eric wrote:
> I worked the original defect a couple months ago, and Kashyap is now
> getting around to posting my patch's.
> 
> This original defect has nothing to do with PPC64.  The original
> problem was only on x86.    It only became a problem on PPC64 when I
> tried to fix the original x86 issue by copying the writeq code from
> the linux headers, then it broke PPC64.   I doubt that broken patch
> was ever posted. Anyways, back to the original defect.  The reason it
> because a problem for x86 is because the kernel headers had a
> implementation of writeq in the arch/x86 headers, which means our
> internal implementation of writeq is not being used.  The writeq
> implementation in the kernel is total wrong for arch/x86 because it
> doesn't not have spin locks, and if two processor simultaneously doing
> two separate 32bit pci writes, then what is received by controller
> firmware is out of order.   This change occurs between Red Hat RHEL5
> and RHEL6.  In RHEL5, this writeq was not implemented in arch/x86
> headers, and our driver internal implementation of write was used.

You may also want to look at Milton's comments, it looks like the way
you do init_completion followed immediately by wait_completion is racy.

You should init the completion before you do the IO that will eventually
trigger complete() to be called.

Cheers,
Ben.

RE: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

[Moore, Eric]

T24gV2VkbmVzZGF5LCBNYXkgMTgsIDIwMTEgMzozMCBQTSwgQmVuamFtaW4gSGVycmVuc2NobWlk
dCB3cm90ZToNCj4gDQo+IFlvdSBtYXkgYWxzbyB3YW50IHRvIGxvb2sgYXQgTWlsdG9uJ3MgY29t
bWVudHMsIGl0IGxvb2tzIGxpa2UgdGhlIHdheQ0KPiB5b3UgZG8gaW5pdF9jb21wbGV0aW9uIGZv
bGxvd2VkIGltbWVkaWF0ZWx5IGJ5IHdhaXRfY29tcGxldGlvbiBpcyByYWN5Lg0KPiANCj4gWW91
IHNob3VsZCBpbml0IHRoZSBjb21wbGV0aW9uIGJlZm9yZSB5b3UgZG8gdGhlIElPIHRoYXQgd2ls
bCBldmVudHVhbGx5DQo+IHRyaWdnZXIgY29tcGxldGUoKSB0byBiZSBjYWxsZWQuDQo+IA0KDQpJ
IGFncmVlLiAgVGhlIGluaXRfY29tcGxldGlvbiBuZWVkcyB0byBiZSBkb25lIHByaW9yIHRvIHBv
c3RpbmcgdGhlIHNtaWQuICBJJ20gbm90IHN1cmUgd2h5IEkgZGlkIGl0IHRoYXQgd2F5Lg0K

RE: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq isnot atomic

[David Laight]

=20

> > > > static inline void writeq(__u64 val, volatile void __iomem
*addr)
> > > > {
> > > >         writel(val, addr);
> > > >         writel(val >> 32, addr+4);
> > > > }
...
> > > > We need the 64 bit completed in one access pci memory write,
else spin lock is required.
> > > > Since it's going to be difficult to know which writeq was
implemented in the kernel,=20
> > > > the driver is going to have to always acquire a spin lock each
time we do 64bit write.
...
> I'm just in the process of finding them now on IRC so I can demand an
> explanation: this is a really serious API problem because writeq is
> supposed to be atomic on 64 bit.

Most 32 bit systems don't have atomic 64bit writes.
I'd also have thought there would be code which wouldn't mind the
write being done as two cycles.

I'm not sure that some of the ppc soc systems are capable of
doing a 64bit data pci/pcie cycle except by dma.
So your driver is probably doomed to require a lock.

	David

Re: [PATCH 1/3] mpt2sas: remove the use of writeq, since writeq is not atomic

[Benjamin Herrenschmidt]

> > > Cc: stable@kernle.org
> > > Signed-off-by: Kashyap Desai <kashyap.desai@lsi.com>
> > > ---
> > > diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > index efa0255..5778334 100644
> > > --- a/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
> > > @@ -1558,7 +1558,6 @@ mpt2sas_base_free_smid(struct MPT2SAS_ADAPTER *ioc, u16 smid)
> > >   * care of 32 bit environment where its not quarenteed to send the entire word
> > >   * in one transfer.
> > >   */
> > > -#ifndef writeq
> > 
> > Why not make this #ifndef CONFIG_64BIT?  You know that all 64 bit
> > systems have writeq implemented correctly; you suspect 32 bit systems
> > don't.
> > 
> > James
> > 
> > James, This issue was observed on PPC64 system. So what you have
> suggested will not solve this issue.
> > If we are sure that writeq() is atomic across all architecture, we
> can use it safely. As we have seen issue on ppc64, we are not
> confident to use
> > "writeq" call.
> 
> So have you told the powerpc people that they have a broken writeq?
> And why do you obfuscate your report by talking about i386 when it's
> really about powerpc64?

Well, our writeq isn't supposed to be broken :-)

It's defined as an std instruction (and "ld" for readq) so that's
perfectly atomic ... provided your access is aligned. Is it ?

Cheers,
Be