scsi: mpt3sas: Use lo_hi_writeq() helper
Subsystems (2)
PATCH 1/1 scsi: mpt3sas: Use lo_hi_writeq() helper
Since we have a writeq() for 32-bit architectures as provided by IO non-atomic helpers, there is no need to open code it.
Moreover sparse complains about this:
drivers/scsi/mpt3sas/mpt3sas_base.c:2975:16: expected unsigned long long val drivers/scsi/mpt3sas/mpt3sas_base.c:2975:16: got restricted __le64 <noident>
Fixing this by replacing custom writeq() with one provided by io-64-nonatomic-lo-hi.h header.
--- drivers/scsi/mpt3sas/mpt3sas_base.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)
drivers/scsi/mpt3sas/mpt3sas_base.c +3 -11
@@ -56,6 +56,7 @@#include <linux/interrupt.h> #include <linux/dma-mapping.h> #include <linux/io.h> +#include <linux/io-64-nonatomic-lo-hi.h> #include <linux/time.h> #include <linux/ktime.h> #include <linux/kthread.h>
@@ -2968,16 +2969,9 @@ mpt3sas_base_free_smid(struct MPT3SAS_ADAPTER *ioc, u16 smid)* @writeq_lock: spin lock * * Glue for handling an atomic 64 bit word to MMIO. This special handling takes -* care of 32 bit environment where its not quarenteed to send the entire word +* care of 32 bit environment where its not guaranteed to send the entire word * in one transfer. */ -#if defined(writeq) && defined(CONFIG_64BIT ) -static inline void -_base_writeq (__u64 b,volatile void __iomem *addr,spinlock_t *writeq_lock) -{ -writeq (cpu_to_le64 (b), addr); -} -#else static inline void _base_writeq (__u64 b,volatile void __iomem *addr,spinlock_t *writeq_lock) {
@@ -2985,11 +2979,9 @@ _base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t *writeq_lock)__u64 data_out =cpu_to_le64 (b);spin_lock_irqsave (writeq_lock, flags); -writel ((u32)(data_out), addr); -writel ((u32)(data_out >>32 ), (addr +4 )); +writeq (data_out, addr);spin_unlock_irqrestore (writeq_lock, flags); } -#endif /** * _base_put_smid_scsi_io - send SCSI_IO request to firmware
Signature · 1 line
2.15.1
Quoted 64 lines · Andy Shevchenko
Since we have a
writeq()for 32-bit architectures as provided by IO non-atomic helpers, there is no need to open code it.Moreover sparse complains about this:
drivers/scsi/mpt3sas/mpt3sas_base.c:2975:16: expected unsigned long long valdrivers/scsi/mpt3sas/mpt3sas_base.c:2975:16: got restricted__le64<noident>Fixing this by replacing custom
writeq()with one provided byio-64-nonatomic-lo-hi.hheader.Reported-by kbuild test robot <[email protected]>Signed-off-by Andy Shevchenko <[email protected]>---
drivers/scsi/mpt3sas/mpt3sas_base.c | 14 +++-----------1 file changed, 3 insertions(+), 11 deletions(-)drivers/scsi/mpt3sas/mpt3sas_base.c +1
-0@@ -56,6 +56,7 @@ #include <linux/interrupt.h> #include <linux/dma-mapping.h> #include <linux/io.h> +#include <linux/io-64-nonatomic-lo-hi.h> #include <linux/time.h> #include <linux/ktime.h> #include <linux/kthread.h>@@ -2968,16 +2969,9 @@ mpt3sas_base_free_smid(struct MPT3SAS_ADAPTER *ioc, u16 smid) * @writeq_lock: spin lock * * Glue for handling an atomic 64 bit word to MMIO. This specialhandling takes
- * care of 32 bit environment where its not quarenteed to send the entire word
+ * care of 32 bit environment where its not guaranteed to send the entire word
* in one transfer.
*/drivers/scsi/mpt3sas/mpt3sas_base.c +0
-3-#if defined(writeq) && defined(CONFIG_64BIT ) -static inline void -_base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t
*writeq_lock)drivers/scsi/mpt3sas/mpt3sas_base.c +0
-4-{ -writeq (cpu_to_le64 (b), addr); -} -#else static inline void
_base_writeq(__u64 b, volatile void__iomem*addr,spinlock_t*writeq_lock)
{drivers/scsi/mpt3sas/mpt3sas_base.c +0
-0@@ -2985,11 +2979,9 @@ _base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t *writeq_lock) __u64 data_out = cpu_to_le64(b);
spin_lock_irqsave(writeq_lock, flags);drivers/scsi/mpt3sas/mpt3sas_base.c +1
-2-writel ((u32)(data_out), addr); -writel ((u32)(data_out >>32 ), (addr +4 )); +writeq (data_out, addr);
Quoted 3 lines
spin_unlock_irqrestore(writeq_lock, flags); }
-#endif
This would rather defeat the purpose of the original code, I think. The coding seems to indicate that if the platform can do a writeq atomically (i.e. it's 64 bit and has the primitive) then it should and not take the hit of using a lock. Otherwise the 64 bit value is updated by two 32 bit writes protected by a lock to ensure we don't get interleaving of the write.
So I think you can replace the double writel with a lo_hi_writeq but you have to lose the lock if the platform supports the atomic 64 bit write for performance reasons.
James
Quoted 31 lines · James Bottomley
Quoted 30 lines · Andy Shevchenko
Since we have a
writeq()for 32-bit architectures as provided by IO non-atomic helpers, there is no need to open code it.Moreover sparse complains about this:
drivers/scsi/mpt3sas/mpt3sas_base.c:2975:16: expected unsigned long long valdrivers/scsi/mpt3sas/mpt3sas_base.c:2975:16: got restricted__le64<noident>Fixing this by replacing custom
writeq()with one provided byio-64-nonatomic-lo-hi.hheader.drivers/scsi/mpt3sas/mpt3sas_base.c +0
-3-#if defined(writeq) && defined(CONFIG_64BIT ) -static inline void -_base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t
*writeq_lock)drivers/scsi/mpt3sas/mpt3sas_base.c +0
-4-{ -writeq (cpu_to_le64 (b), addr); -} -#else static inline void _base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t
*writeq_lock)
{drivers/scsi/mpt3sas/mpt3sas_base.c +0
-0@@ -2985,11 +2979,9 @@ _base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t *writeq_lock)__u64 data_out =cpu_to_le64 (b);
Quoted 9 lines
drivers/scsi/mpt3sas/mpt3sas_base.c _base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t *writeq_lock) · line 2992
Quoted 7 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +1
-3spin_lock_irqsave (writeq_lock, flags); -writel ((u32)(data_out), addr); -writel ((u32)(data_out >>32 ), (addr +4 )); +writeq (data_out, addr);spin_unlock_irqrestore (writeq_lock, flags); } -#endif This would rather defeat the purpose of the original code, I think.
James, TBH, I don't see any value of that lock. What it's protecting from? simultaneous thread doing writeq()? But this is pointless if at the same time you will have writel() to the device.
For my opinion perhaps complete removal of the custom writeq() and replacing it by just writeq() with enabled non-atomic helpers will do the job.
The code is very old, and I have no idea why it's done this (strange) way.
Quoted 10 lines
The coding seems to indicate that if the platform can do a writeq
atomically (i.e. it's 64 bit and has the primitive) then it should and not take the hit of using a lock. Otherwise the 64 bit value is updated by two 32 bit writes protected by a lock to ensure we don't get
interleaving of the write.So I think you can replace the double writel with a
lo_hi_writeqbut you have to lose the lock if the platform supports the atomic 64 bit write for performance reasons.
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
Quoted 50 lines · Andy Shevchenko
Quoted 40 lines · James Bottomley
Quoted 38 lines · Andy Shevchenko
Since we have a
writeq()for 32-bit architectures as provided by IO
non-atomic helpers, there is no need to open code it.Moreover sparse complains about this:
drivers/scsi/mpt3sas/mpt3sas_base.c:2975:16: expected unsigned long
long valdrivers/scsi/mpt3sas/mpt3sas_base.c:2975:16: got restricted__le64
<noident>Fixing this by replacing custom
writeq()with one provided byio-64-nonatomic-lo-hi.hheader.drivers/scsi/mpt3sas/mpt3sas_base.c +0
-3-#if defined(writeq) && defined(CONFIG_64BIT ) -static inline void -_base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t
*writeq_lock)drivers/scsi/mpt3sas/mpt3sas_base.c +0
-4-{ -writeq (cpu_to_le64 (b), addr); -} -#else static inline void
_base_writeq(__u64 b, volatile void__iomem*addr,spinlock_t*writeq_lock)
{(quoted diff) +0
-0@@ -2985,11 +2979,9 @@ _base_writeq(__u64 b, volatile void
__iomem
*addr,spinlock_t*writeq_lock)
__u64data_out=cpu_to_le64(b);
spin_lock_irqsave(writeq_lock, flags);Quoted 8 lines
Quoted 6 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +1
-2-writel ((u32)(data_out), addr); -writel ((u32)(data_out >>32 ), (addr +4 )); +writeq (data_out, addr);
spin_unlock_irqrestore(writeq_lock, flags); }
-#endifThis would rather defeat the purpose of the original code, I think.
Quoted 3 lines
James, TBH, I don't see any value of that lock. What it's protecting from? simultaneous thread doing
writeq()? But this is pointless if at the same time you will havewritel()to the device.
The lock prevents two threads doing an interleaved write to this specific address which could be caused by two threads writing to the same address. If I remember correctly the firmware hangs in that case.
Quoted 6 lines
For my opinion perhaps complete removal of the custom
writeq()and replacing it by justwriteq()with enabled non-atomic helpers will do the job.The code is very old, and I have no idea why it's done this (strange) way.
The write seems to trigger starting the engine on the HBA if you look at the code, which is why it must be written completely and in order. It's equivalent to a mailbox post.
James
Sathya Prakash Veerichetty
maintainerThe writeq is defined to ensure both the registers are written without any interleaved access in between the two writels in 32 bit systems. Also we want to retain the order of writing the registers. If needed I can dig out the issue we have faced using writeq in 32 bit systems (happened five/six years back). So I would prefer leave the code as it is.
Thanks
Sathya
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of James Bottomley
Sent: Wednesday, February 14, 2018 1:02 PM
To: Andy Shevchenko; Sathya Prakash; Chaitra P B; Suganath Prabu Subramani; [email protected]; Martin K. Petersen; [email protected]
Subject: Re: [PATCH v1] scsi: mpt3sas: Use lo_hi_writeq() helper
Quoted 46 lines · Andy Shevchenko
Quoted 35 lines · James Bottomley
Quoted 33 lines · Andy Shevchenko
Since we have a
writeq()for 32-bit architectures as provided by IO non-atomic helpers, there is no need to open code it.Moreover sparse complains about this:
drivers/scsi/mpt3sas/mpt3sas_base.c:2975:16: expected unsigned long long valdrivers/scsi/mpt3sas/mpt3sas_base.c:2975:16: got restricted__le64
<noident>Fixing this by replacing custom
writeq()with one provided byio-64-nonatomic-lo-hi.hheader.(quoted diff) +0
-2-#if defined(writeq) && defined(CONFIG_64BIT) -static inline void -_base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t
*writeq_lock)drivers/scsi/mpt3sas/mpt3sas_base.c +0
-4-{ -writeq (cpu_to_le64 (b), addr); -} -#else static inline void _base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t
*writeq_lock)
{drivers/scsi/mpt3sas/mpt3sas_base.c +0
-0@@ -2985,11 +2979,9 @@ _base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t *writeq_lock)__u64 data_out =cpu_to_le64 (b);Quoted 9 lines
drivers/scsi/mpt3sas/mpt3sas_base.c _base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t *writeq_lock) · line 2992
Quoted 7 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +1
-3spin_lock_irqsave (writeq_lock, flags); -writel ((u32)(data_out), addr); -writel ((u32)(data_out >>32 ), (addr +4 )); +writeq (data_out, addr);spin_unlock_irqrestore (writeq_lock, flags); } -#endif This would rather defeat the purpose of the original code, I think.
Quoted 3 lines
James, TBH, I don't see any value of that lock. What it's protecting from? simultaneous thread doing
writeq()? But this is pointless if at the same time you will havewritel()to the device.
The lock prevents two threads doing an interleaved write to this specific address which could be caused by two threads writing to the same address. If I remember correctly the firmware hangs in that case.
Quoted 6 lines
For my opinion perhaps complete removal of the custom
writeq()and replacing it by justwriteq()with enabled non-atomic helpers will do the job.The code is very old, and I have no idea why it's done this (strange) way.
The write seems to trigger starting the engine on the HBA if you look at the code, which is why it must be written completely and in order.
It's equivalent to a mailbox post.
James