mpt2sas: [email protected], since writeq is not atomic

-----Original Message-----
From: [email protected] [mailto:linux-scsi- [email protected]] On Behalf Of Kashyap, Desai
Sent: Wednesday, May 04, 2011 4:34 PM
To: [email protected]
Cc: [email protected]; Moore, Eric; Prakash, Sathya
Subject: [PATCH 1/3] mpt2sas: [email protected], 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
@@ -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.
*/

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

-#ifndef writeq
 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

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.
*/

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

-#ifndef writeq
 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));

-----Original Message-----
From: Matthew Wilcox [mailto:[email protected]]
Sent: Thursday, May 05, 2011 1:12 AM
To: Desai, Kashyap
Cc: [email protected]; [email protected]; Moore, Eric; Prakash, Sathya
Subject: Re: [PATCH 1/3] mpt2sas: [email protected], since writeq is not atomic

Quoted 1 line · Desai

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

spin lock is required.

Quoted 1 line

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

in the kernel,

Quoted 1 line

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

we do 64bit write.

Quoted 3 lines

*/
-#ifndef writeq
static inline void _base_writeq(__u64 b, volatile void __iomem

*addr,

Review static inline void _base_writeq(__u64 b,

Quoted 3 lines

spinlock_t *writeq_lock)
{

(quoted diff) +0 -0

@@ -1570,13 +1569,6 @@ static inline void _base_writeq(__u64 b,

volatile void __iomem *addr,

drivers/scsi/mpt2sas/mpt2sas_base.c static inline void _base_writeq(__u64 b, volatile void __iomem *addr,

Quoted 5 lines

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

 	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,

drivers/scsi/mpt2sas/mpt2sas_base.c static inline void _base_writeq(__u64 b, volatile void __iomem *addr, · line 1572

Quoted 6 lines

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

-    spinlock_t *writeq_lock)
-{
-	writeq(cpu_to_le64(b), addr);
-}
-#endif
 

Instead of taking out this optimisation (which is going to hurt massively
on 8-socket systems), why not simply change:

(quoted diff) +1 -1

-#ifndef writeq
+#if BITS_PER_LONG < 64

(OK, there's an assumption that all 64-bit systems have an atomic 64- bit
MMIO store operation ... but I think that's a valid assumption).

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