mpt2sas: remove the use of writeq, since writeq is not atomic

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 [email protected]

Signed-off-by Kashyap Desai <[email protected]>

---

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -0

index efa0255..5778334 100644

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -8

 static inline void _base_writeq(__u64 b, volatile void __iomem *addr,
     spinlock_t *writeq_lock)
 {

@@ -1570,13 +1569,6 @@ static inline void _base_writeq(__u64 b, volatile void __iomem *addr,
 	writel((u32)(data_out >> 32), (addr + 4));
 	spin_unlock_irqrestore(writeq_lock, flags);
 }
-#else
-static inline void _base_writeq(__u64 b, volatile void __iomem *addr,
-    spinlock_t *writeq_lock)
-{
-	writeq(cpu_to_le64(b), addr);
-}
-#endif
 
 /**
  * mpt2sas_base_put_smid_scsi_io - send SCSI_IO request to firmware
--

To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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 [email protected]

Signed-off-by Kashyap Desai <[email protected]>

---

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -0

index efa0255..5778334 100644

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -8

 static inline void _base_writeq(__u64 b, volatile void __iomem *addr,
     spinlock_t *writeq_lock)
 {

@@ -1570,13 +1569,6 @@ static inline void _base_writeq(__u64 b, volatile void __iomem *addr,
 	writel((u32)(data_out >> 32), (addr + 4));
 	spin_unlock_irqrestore(writeq_lock, flags);
 }
-#else
-static inline void _base_writeq(__u64 b, volatile void __iomem *addr,
-    spinlock_t *writeq_lock)
-{
-	writeq(cpu_to_le64(b), addr);
-}
-#endif
 
 /**
  * mpt2sas_base_put_smid_scsi_io - send SCSI_IO request to firmware
--

To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 30 lines · Desai

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 [email protected]

Signed-off-by Kashyap Desai <[email protected]>

---

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -0

index efa0255..5778334 100644

drivers/scsi/mpt2sas/mpt2sas_base.c mpt2sas_base_free_smid(struct MPT2SAS_ADAPTER *ioc, u16 smid) · line 1561

Quoted 4 lines

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -1

@@ -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.

Quoted 40 lines · Kashyap

Quoted 30 lines · Desai

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 [email protected]

Signed-off-by Kashyap Desai <[email protected]>

---

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -0

index efa0255..5778334 100644

drivers/scsi/mpt2sas/mpt2sas_base.c mpt2sas_base_free_smid(struct MPT2SAS_ADAPTER *ioc, u16 smid) · line 1561

Quoted 4 lines

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -1

@@ -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.

Quoted 5 lines

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?

Quoted 41 lines · Matthew Wilcox

Quoted 40 lines · Kashyap

Quoted 30 lines · Desai

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 [email protected]

Signed-off-by Kashyap Desai <[email protected]>

---

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -0

index efa0255..5778334 100644

drivers/scsi/mpt2sas/mpt2sas_base.c mpt2sas_base_free_smid(struct MPT2SAS_ADAPTER *ioc, u16 smid) · line 1561

Quoted 4 lines

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -1

@@ -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.

Quoted 7 lines

Quoted 5 lines

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.

Quoted 2 lines

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

James

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

Quoted 58 lines · James Bottomley

Quoted 41 lines · Matthew Wilcox

Quoted 40 lines · Kashyap

Quoted 30 lines · Desai

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 [email protected]

Signed-off-by Kashyap Desai <[email protected]>

---

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -0

index efa0255..5778334 100644

drivers/scsi/mpt2sas/mpt2sas_base.c mpt2sas_base_free_smid(struct MPT2SAS_ADAPTER *ioc, u16 smid) · line 1561

Quoted 4 lines

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -1

@@ -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.

Quoted 7 lines

Quoted 5 lines

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.

Quoted 2 lines

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

James

Quoted 8 lines · Benjamin Herrenschmidt

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

Quoted 4 lines · James Bottomley

Quoted 3 lines · Matthew Wilcox

Quoted 2 lines · Kashyap

Quoted 1 line · Desai

The following code seems to be there in

/usr/src/linux/arch/x86/include/asm/io.h.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

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

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

where multiple cpus can be writing to the request descriptor at

the same time.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Since it's going to be difficult to know which writeq was

implemented in the kernel,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

the driver is going to have to always acquire a spin lock each

time we do 64bit write.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 5 lines

Cc [email protected]

Signed-off-by Kashyap Desai <[email protected]>

---

(quoted diff) +0 -0

b/drivers/scsi/mpt2sas/mpt2sas_base.c

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/scsi/mpt2sas/mpt2sas_base.c mpt2sas_base_free_smid(struct

Quoted 4 lines

index efa0255..5778334 100644

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -0

@@ -1558,7 +1558,6 @@ mpt2sas_base_free_smid(struct

MPT2SAS_ADAPTER *ioc, u16 smid)

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

drivers/scsi/mpt2sas/mpt2sas_base.c mpt2sas_base_free_smid(struct · line 1558

Quoted 1 line

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -0

  * care of 32 bit environment where its not quarenteed to send

the entire word

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/scsi/mpt2sas/mpt2sas_base.c mpt2sas_base_free_smid(struct · line 1561

Quoted 3 lines

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -1

  * 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

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

don't.

James

James, This issue was observed on PPC64 system. So what you have

suggested will not solve this issue.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

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

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

"writeq" call.

So have you told the powerpc people that they have a broken writeq?

Quoted 8 lines

Quoted 8 lines

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.

Quoted 2 lines

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

Quoted 114 lines · Milton Miller

Quoted 8 lines · Benjamin Herrenschmidt

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

Quoted 4 lines · James Bottomley

Quoted 3 lines · Matthew Wilcox

Quoted 2 lines · Kashyap

Quoted 1 line · Desai

The following code seems to be there in

/usr/src/linux/arch/x86/include/asm/io.h.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

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

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

where multiple cpus can be writing to the request descriptor at

the same time.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Since it's going to be difficult to know which writeq was

implemented in the kernel,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

the driver is going to have to always acquire a spin lock each

time we do 64bit write.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 5 lines

Cc [email protected]

Signed-off-by Kashyap Desai <[email protected]>

---

(quoted diff) +0 -0

b/drivers/scsi/mpt2sas/mpt2sas_base.c

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/scsi/mpt2sas/mpt2sas_base.c mpt2sas_base_free_smid(struct

Quoted 4 lines

index efa0255..5778334 100644

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -0

@@ -1558,7 +1558,6 @@ mpt2sas_base_free_smid(struct

MPT2SAS_ADAPTER *ioc, u16 smid)

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

drivers/scsi/mpt2sas/mpt2sas_base.c mpt2sas_base_free_smid(struct · line 1558

Quoted 1 line

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -0

  * care of 32 bit environment where its not quarenteed to send

the entire word

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/scsi/mpt2sas/mpt2sas_base.c mpt2sas_base_free_smid(struct · line 1561

Quoted 3 lines

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -1

  * 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

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

don't.

James

James, This issue was observed on PPC64 system. So what you have

suggested will not solve this issue.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

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

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

"writeq" call.

So have you told the powerpc people that they have a broken writeq?

Quoted 8 lines

Quoted 8 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

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

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.

Quoted 4 lines · Milton Miller

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.

Quoted 9 lines · Eric

Quoted 4 lines · Milton Miller

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.

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.

Quoted 2 lines

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).

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

on the chip, otherwise the HW will take the write as complete in the

bit register and latch the wrong value. The only way to achieve that given the semantics of writeq is a global static spinlock.

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?

Quoted 2 lines · Milton Miller

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

Quoted 9 lines · Roland Dreier

Quoted 2 lines · Milton Miller

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.

From: Roland Dreier <[email protected]>

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/[email protected]>).  To fix this, revert 2c5643b1c5c7 ("x86: provide readq()/writeq() on 32-bit too") and follow-on cleanups.

This unfortunately leads to pushing non-atomic definitions of readq() and write() to various x86-only drivers that in the mean time started using the definitions in the x86 version of <asm/io.h>.  However as discussed exhaustively, this is actually the right thing to do, because the right way to split a 64-bit transaction is hardware dependent and therefore belongs in the hardware driver (eg mpt2sas needs a spinlock to make sure no other accesses occur in between the two halves of the access).

Build tested on 32- and 64-bit x86 allmodconfig.

Link http://lkml.kernel.org/r/[email protected] open here

Cc Hitoshi Mitake <[email protected]>

Cc Kashyap Desai <[email protected]>

Cc Len Brown <[email protected]>

Cc Ravi Anand <[email protected]>

Cc Vikas Chaudhary <[email protected]>

Cc Matthew Garrett <[email protected]>

Signed-off-by Roland Dreier <[email protected]>

---
 arch/x86/Kconfig                 |    2 -- arch/x86/include/asm/io.h        |   24 ++---------------------- drivers/acpi/apei/einj.c         |    8 ++++++++ drivers/acpi/atomicio.c          |    4 ++++ drivers/edac/i3200_edac.c        |   13 +++++++++++++ drivers/platform/x86/ibm_rtl.c   |   13 +++++++++++++ drivers/platform/x86/intel_ips.c |   13 +++++++++++++ drivers/scsi/qla4xxx/ql4_nx.c    |   21 +++++++++++++++++++++

 8 files changed, 74 insertions(+), 24 deletions(-)

arch/x86/Kconfig +0 -0

index cc6c53a..ceb41f3 100644
@@ -16,8 +16,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

arch/x86/include/asm/io.h +2 -22

index 0722730..d02804d 100644
@@ -38,7 +38,6 @@
 
  #include <linux/string.h>
  #include <linux/compiler.h>
-#include <asm-generic/int-ll64.h>
  #include <asm/page.h>
 
  #include <xen/xen.h>

@@ -87,27 +86,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)

@@ -117,6 +95,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

drivers/acpi/apei/einj.c +8 -0

index 096aebf..f74b2ea 100644
@@ -101,6 +101,14 @@ static DEFINE_MUTEX(einj_mutex);
 
  static struct einj_parameter *einj_param;
 
+#ifndef writeq
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+       writel(val, addr);
+       writel(val >> 32, addr+4);
+}
+#endif
+
  static void einj_exec_ctx_init(struct apei_exec_context *ctx)
  {
        apei_exec_ctx_init(ctx, einj_ins_type, ARRAY_SIZE(einj_ins_type),

drivers/acpi/atomicio.c +0 -0

index 542e539..7489b89 100644
@@ -280,9 +280,11 @@ static int acpi_atomic_read_mem(u64 paddr, u64 *val, u32 width)

       case 32:
               *val = readl(addr);
               break;
+#ifdef readq
       case 64:
               *val = readq(addr);
               break;
+#endif
       default:
               return -EINVAL;
       }

drivers/acpi/atomicio.c +0 -0

@@ -307,9 +309,11 @@ static int acpi_atomic_write_mem(u64 paddr, u64 val, u32 width)

       case 32:
               writel(val, addr);
               break;
+#ifdef writeq
       case 64:
               writeq(val, addr);
               break;
+#endif
       default:
               return -EINVAL;
       }

drivers/edac/i3200_edac.c +13 -0

index d41f900..aa08497 100644
@@ -101,6 +101,19 @@ struct i3200_priv {
 
  static int nr_channels;
 
+#ifndef readq
+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
+
  static int how_many_channels(struct pci_dev *pdev)
  {

       unsigned char capid0_8b; /* 8th byte of CAPID0 */

drivers/platform/x86/ibm_rtl.c +13 -0

index 94a114a..b1396e5 100644
@@ -81,6 +81,19 @@ static void __iomem *rtl_cmd_addr;
  static u8 rtl_cmd_type;
  static u8 rtl_cmd_width;
 
+#ifndef readq
+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
+
  static void __iomem *rtl_port_map(phys_addr_t addr, unsigned long len)
  {
        if (rtl_cmd_type == RTL_ADDR_TYPE_MMIO)

drivers/platform/x86/intel_ips.c +0 -0

index 85c8ad4..5ffe7c3 100644
@@ -344,6 +344,19 @@ struct ips_driver {

 static bool
 ips_gpu_turbo_enabled(struct ips_driver *ips);

drivers/platform/x86/intel_ips.c +13 -0

+#ifndef readq
+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
+

 /**
 * ips_cpu_busy - is CPU busy?
 * @ips: IPS driver struct

drivers/scsi/qla4xxx/ql4_nx.c +21 -0

index 35381cb..03e522b 100644
@@ -655,6 +655,27 @@ static int qla4_8xxx_pci_is_same_window(struct scsi_qla_host *ha,
        return 0;
  }
 
+#ifndef readq
+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
+
+#ifndef writeq
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+       writel(val, addr);
+       writel(val >> 32, addr+4);
+}
+#endif
+
  static int qla4_8xxx_pci_mem_read_direct(struct scsi_qla_host *ha,
                u64 off, void *data, int size)
  {

From: Roland Dreier <[email protected]>

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/[email protected]>).  To fix this, revert 2c5643b1c5c7 ("x86: provide readq()/writeq() on 32-bit too") and follow-on cleanups.

This unfortunately leads to pushing non-atomic definitions of readq() and write() to various x86-only drivers that in the mean time started using the definitions in the x86 version of <asm/io.h>.  However as discussed exhaustively, this is actually the right thing to do, because the right way to split a 64-bit transaction is hardware dependent and therefore belongs in the hardware driver (eg mpt2sas needs a spinlock to make sure no other accesses occur in between the two halves of the access).

Build tested on 32- and 64-bit x86 allmodconfig.

Link http://lkml.kernel.org/r/[email protected] open here

Cc Hitoshi Mitake <[email protected]>

Cc Kashyap Desai <[email protected]>

Cc Len Brown <[email protected]>

Cc Ravi Anand <[email protected]>

Cc Vikas Chaudhary <[email protected]>

Cc Matthew Garrett <[email protected]>

Signed-off-by Roland Dreier <[email protected]>

---
 arch/x86/Kconfig                 |    2 -- arch/x86/include/asm/io.h        |   24 ++---------------------- drivers/acpi/apei/einj.c         |    8 ++++++++ drivers/acpi/atomicio.c          |    4 ++++ drivers/edac/i3200_edac.c        |   13 +++++++++++++ drivers/platform/x86/ibm_rtl.c   |   13 +++++++++++++ drivers/platform/x86/intel_ips.c |   13 +++++++++++++ drivers/scsi/qla4xxx/ql4_nx.c    |   21 +++++++++++++++++++++

 8 files changed, 74 insertions(+), 24 deletions(-)

arch/x86/Kconfig +0 -0

index cc6c53a..ceb41f3 100644
@@ -16,8 +16,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

arch/x86/include/asm/io.h +2 -22

index 0722730..d02804d 100644
@@ -38,7 +38,6 @@
 
  #include <linux/string.h>
  #include <linux/compiler.h>
-#include <asm-generic/int-ll64.h>
  #include <asm/page.h>
 
  #include <xen/xen.h>

@@ -87,27 +86,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)

@@ -117,6 +95,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

drivers/acpi/apei/einj.c +8 -0

index 096aebf..f74b2ea 100644
@@ -101,6 +101,14 @@ static DEFINE_MUTEX(einj_mutex);
 
  static struct einj_parameter *einj_param;
 
+#ifndef writeq
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+       writel(val, addr);
+       writel(val >> 32, addr+4);
+}
+#endif
+
  static void einj_exec_ctx_init(struct apei_exec_context *ctx)
  {
        apei_exec_ctx_init(ctx, einj_ins_type, ARRAY_SIZE(einj_ins_type),

drivers/acpi/atomicio.c +0 -0

index 542e539..7489b89 100644
@@ -280,9 +280,11 @@ static int acpi_atomic_read_mem(u64 paddr, u64 *val, u32 width)

       case 32:
               *val = readl(addr);
               break;
+#ifdef readq
       case 64:
               *val = readq(addr);
               break;
+#endif
       default:
               return -EINVAL;
       }

drivers/acpi/atomicio.c +0 -0

@@ -307,9 +309,11 @@ static int acpi_atomic_write_mem(u64 paddr, u64 val, u32 width)

       case 32:
               writel(val, addr);
               break;
+#ifdef writeq
       case 64:
               writeq(val, addr);
               break;
+#endif
       default:
               return -EINVAL;
       }

drivers/edac/i3200_edac.c +13 -0

index d41f900..aa08497 100644
@@ -101,6 +101,19 @@ struct i3200_priv {
 
  static int nr_channels;
 
+#ifndef readq
+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
+
  static int how_many_channels(struct pci_dev *pdev)
  {

       unsigned char capid0_8b; /* 8th byte of CAPID0 */

drivers/platform/x86/ibm_rtl.c +13 -0

index 94a114a..b1396e5 100644
@@ -81,6 +81,19 @@ static void __iomem *rtl_cmd_addr;
  static u8 rtl_cmd_type;
  static u8 rtl_cmd_width;
 
+#ifndef readq
+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
+
  static void __iomem *rtl_port_map(phys_addr_t addr, unsigned long len)
  {
        if (rtl_cmd_type == RTL_ADDR_TYPE_MMIO)

drivers/platform/x86/intel_ips.c +0 -0

index 85c8ad4..5ffe7c3 100644
@@ -344,6 +344,19 @@ struct ips_driver {

 static bool
 ips_gpu_turbo_enabled(struct ips_driver *ips);

drivers/platform/x86/intel_ips.c +13 -0

+#ifndef readq
+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
+

 /**
 * ips_cpu_busy - is CPU busy?
 * @ips: IPS driver struct

drivers/scsi/qla4xxx/ql4_nx.c +21 -0

index 35381cb..03e522b 100644
@@ -655,6 +655,27 @@ static int qla4_8xxx_pci_is_same_window(struct scsi_qla_host *ha,
        return 0;
  }
 
+#ifndef readq
+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
+
+#ifndef writeq
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+       writel(val, addr);
+       writel(val >> 32, addr+4);
+}
+#endif
+
  static int qla4_8xxx_pci_mem_read_direct(struct scsi_qla_host *ha,
                u64 off, void *data, int size)
  {

From: Roland Dreier <[email protected]>

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/[email protected]>). To fix this, revert 2c5643b1c5c7 ("x86: provide readq()/writeq() on 32-bit too") and follow-on cleanups.

This unfortunately leads to pushing non-atomic definitions of readq() and write() to various x86-only drivers that in the mean time started using the definitions in the x86 version of <asm/io.h>. However as discussed exhaustively, this is actually the right thing to do, because the right way to split a 64-bit transaction is hardware dependent and therefore belongs in the hardware driver (eg mpt2sas needs a spinlock to make sure no other accesses occur in between the two halves of the access).

Build tested on 32- and 64-bit x86 allmodconfig.

Link http://lkml.kernel.org/r/[email protected] open here

Cc Hitoshi Mitake <[email protected]>

Cc Kashyap Desai <[email protected]>

Cc Len Brown <[email protected]>

Cc Ravi Anand <[email protected]>

Cc Vikas Chaudhary <[email protected]>

Cc Matthew Garrett <[email protected]>

Signed-off-by Roland Dreier <[email protected]>

---
 arch/x86/Kconfig                 |    2 --
 arch/x86/include/asm/io.h        |   24 ++----------------------
 drivers/acpi/apei/einj.c         |    8 ++++++++
 drivers/acpi/atomicio.c          |    4 ++++
 drivers/edac/i3200_edac.c        |   13 +++++++++++++
 drivers/platform/x86/ibm_rtl.c   |   13 +++++++++++++
 drivers/platform/x86/intel_ips.c |   13 +++++++++++++
 drivers/scsi/qla4xxx/ql4_nx.c    |   21 +++++++++++++++++++++
 8 files changed, 74 insertions(+), 24 deletions(-)

Quoted 28 lines · Roland Dreier

From: Roland Dreier <[email protected]>

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/[email protected]>). To fix this, revert 2c5643b1c5c7 ("x86: provide readq()/writeq() on 32-bit too") and follow-on cleanups.

This unfortunately leads to pushing non-atomic definitions of readq() and write() to various x86-only drivers that in the mean time started using the definitions in the x86 version of <asm/io.h>. However as discussed exhaustively, this is actually the right thing to do, because the right way to split a 64-bit transaction is hardware dependent and therefore belongs in the hardware driver (eg mpt2sas needs a spinlock to make sure no other accesses occur in between the two halves of the access).

Build tested on 32- and 64-bit x86 allmodconfig.

Link http://lkml.kernel.org/r/[email protected] open here

Cc Hitoshi Mitake <[email protected]>

Cc Kashyap Desai <[email protected]>

Cc Len Brown <[email protected]>

Cc Ravi Anand <[email protected]>

Cc Vikas Chaudhary <[email protected]>

Cc Matthew Garrett <[email protected]>

Signed-off-by Roland Dreier <[email protected]>

Quoted 10 lines

---
 arch/x86/Kconfig                 |    2 --
 arch/x86/include/asm/io.h        |   24 ++----------------------
 drivers/acpi/apei/einj.c         |    8 ++++++++
 drivers/acpi/atomicio.c          |    4 ++++
 drivers/edac/i3200_edac.c        |   13 +++++++++++++
 drivers/platform/x86/ibm_rtl.c   |   13 +++++++++++++
 drivers/platform/x86/intel_ips.c |   13 +++++++++++++
 drivers/scsi/qla4xxx/ql4_nx.c    |   21 +++++++++++++++++++++
 8 files changed, 74 insertions(+), 24 deletions(-)

Hm, this patch is wider than i thought - might be better to do this via one of the driver trees or -mm?

The x86 bits are:

Acked-by Ingo Molnar <[email protected]>

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

static inline void writeq(__u64 val, volatile void __iomem

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

{
        writel(val, addr);
        writel(val >> 32, addr+4);
}

Quoted 1 line

Quoted 1 line

Quoted 1 line

We need the 64 bit completed in one access pci memory write,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Since it's going to be difficult to know which writeq was

Quoted 1 line

Quoted 1 line

Quoted 1 line

the driver is going to have to always acquire a spin lock each

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.

Quoted 18 lines

drivers/scsi/mpt2sas/mpt2sas_base.c mpt2sas_base_free_smid(struct MPT2SAS_ADAPTER *ioc, u16 smid) · line 1561

Quoted 13 lines

Cc [email protected]

Signed-off-by Kashyap Desai <[email protected]>

---

drivers/scsi/mpt2sas/mpt2sas_base.c +0 -1

index efa0255..5778334 100644
@@ -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.

Quoted 3 lines

James

James, This issue was observed on PPC64 system. So what you have

suggested will not solve this issue.

Quoted 1 line

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

Quoted 1 line

"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?