scsi: lpfc: Switch memcpy_fromio() to __read32_copy()

Commit Message

Huacai Chen March 5, 2018, 6:56 a.m. UTC

No one has the same problem? Or this patch need improve?

Huacai
 
------------------ Original ------------------
From:  "陈华才"<[e-mail]>;

Date:  Thu, Mar 1, 2018 01:59 PM
To:  "James Smart"<[e-mail]>; "Dick Kennedy"<[e-mail]>;
Cc:  "James E . J . Bottomley"<[e-mail]>; "Martin K . Petersen"<[e-mail]>; "Fuxin Zhang"<[e-mail]>; "linux-scsi"<[e-mail]>; "stable"<[e-mail]>;
Subject:  Re:[PATCH] scsi: lpfc: Switch memcpy_fromio() to __read32_copy()
 
Ping?
 
------------------ Original ------------------
From:  "Huacai Chen"<[e-mail]>;

Date:  Fri, Jan 26, 2018 08:53 PM
To:  "James Smart"<[e-mail]>; "Dick Kennedy"<[e-mail]>;
Cc:  "James E . J . Bottomley"<[e-mail]>; "Martin K . Petersen"<[e-mail]>; "Fuxin Zhang"<[e-mail]>; "linux-scsi"<[e-mail]>; "Huacai Chen"<[e-mail]>; "stable"<[e-mail]>;
Subject:  [PATCH] scsi: lpfc: Switch memcpy_fromio() to __read32_copy()
 
In commit bc73905abf770192 ("[SCSI] lpfc 8.3.16: SLI Additions, updates,
and code cleanup"), lpfc_memcpy_to_slim() have switched memcpy_toio() to
__write32_copy() in order to prevent unaligned 64 bit copy. Recently, we
found that lpfc_memcpy_from_slim() have similar issues, so let it switch
memcpy_fromio() to __read32_copy().

Cc: [e-mail]
Signed-off-by: Huacai Chen <[e-mail]>

---
 drivers/scsi/lpfc/lpfc_compat.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.7.0

Comments

James Smart March 5, 2018, 3:37 p.m. UTC | #1

On 3/4/2018 10:56 PM, 陈华才 wrote:
> No one has the same problem? Or this patch need improve?
>
> Huacai
>   
> ------------------ Original ------------------
> From:  "陈华才"<[e-mail]>;
> Date:  Thu, Mar 1, 2018 01:59 PM
> To:  "James Smart"<[e-mail]>; "Dick Kennedy"<[e-mail]>;
> Cc:  "James E . J . Bottomley"<[e-mail]>; "Martin K . Petersen"<[e-mail]>; "Fuxin Zhang"<[e-mail]>; "linux-scsi"<[e-mail]>; "stable"<[e-mail]>;
> Subject:  Re:[PATCH] scsi: lpfc: Switch memcpy_fromio() to __read32_copy()
>   
> Ping?
>   
> ------------------ Original ------------------
> From:  "Huacai Chen"<[e-mail]>;
> Date:  Fri, Jan 26, 2018 08:53 PM
> To:  "James Smart"<[e-mail]>; "Dick Kennedy"<[e-mail]>;
> Cc:  "James E . J . Bottomley"<[e-mail]>; "Martin K . Petersen"<[e-mail]>; "Fuxin Zhang"<[e-mail]>; "linux-scsi"<[e-mail]>; "Huacai Chen"<[e-mail]>; "stable"<[e-mail]>;
> Subject:  [PATCH] scsi: lpfc: Switch memcpy_fromio() to __read32_copy()
>   
> In commit bc73905abf770192 ("[SCSI] lpfc 8.3.16: SLI Additions, updates,
> and code cleanup"), lpfc_memcpy_to_slim() have switched memcpy_toio() to
> __write32_copy() in order to prevent unaligned 64 bit copy. Recently, we
> found that lpfc_memcpy_from_slim() have similar issues, so let it switch
> memcpy_fromio() to __read32_copy().
>
> Cc: [e-mail]
> Signed-off-by: Huacai Chen <[e-mail]>
> ---
>   drivers/scsi/lpfc/lpfc_compat.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/lpfc/lpfc_compat.h b/drivers/scsi/lpfc/lpfc_compat.h
> index 6b32b0a..47d4fad 100644
> --- a/drivers/scsi/lpfc/lpfc_compat.h
> +++ b/drivers/scsi/lpfc/lpfc_compat.h
> @@ -91,8 +91,8 @@ lpfc_memcpy_to_slim( void __iomem *dest, void *src, unsigned int bytes)
>   static inline void
>   lpfc_memcpy_from_slim( void *dest, void __iomem *src, unsigned int bytes)
>   {
> -	/* actually returns 1 byte past dest */
> -	memcpy_fromio( dest, src, bytes);
> +	/* convert bytes in argument list to word count for copy function */
> +	__ioread32_copy(dest, src, bytes / sizeof(uint32_t));
>   }
>   
>   #endif	/* __BIG_ENDIAN */

Huacai,

Note: the original change was 7 yrs ago and we've had no reports of 
issues. but....

This change didn't look necessary as the memcpy_XXXio() should allow 
byte alignments and byte-based lengths. Which means the original patch 
also wasn't necessary, which implies that the platform that had the 
issues has an api restriction not normally found on the other platforms.

I have no problem with the change though.

-- james

Signed-off-by: James Smart  <[e-mail]>

Huacai Chen March 6, 2018, 1:19 a.m. UTC | #2

Hi, James,

At least on MIPS (Loongson-3), we found that this patch is necessary.
 
Huacai

------------------ Original ------------------
From:  "James Smart"<[e-mail]>;

Date:  Mon, Mar 5, 2018 11:37 PM
To:  "陈华才"<[e-mail]>; "Dick Kennedy"<[e-mail]>;
Cc:  "James E . J . Bottomley"<[e-mail]>; "Martin K . Petersen"<[e-mail]>; "Fuxin Zhang"<[e-mail]>; "linux-scsi"<[e-mail]>; "stable"<[e-mail]>;
Subject:  Re: [PATCH] scsi: lpfc: Switch memcpy_fromio() to __read32_copy()
 

On 3/4/2018 10:56 PM, 陈华才 wrote:
> No one has the same problem? Or this patch need improve?

>

> Huacai

>   

> ------------------ Original ------------------

> From:  "陈华才"<[e-mail]>;

> Date:  Thu, Mar 1, 2018 01:59 PM

> To:  "James Smart"<[e-mail]>; "Dick Kennedy"<[e-mail]>;

> Cc:  "James E . J . Bottomley"<[e-mail]>; "Martin K . Petersen"<[e-mail]>; "Fuxin Zhang"<[e-mail]>; "linux-scsi"<[e-mail]>; "stable"<[e-mail]>;

> Subject:  Re:[PATCH] scsi: lpfc: Switch memcpy_fromio() to __read32_copy()

>   

> Ping?

>   

> ------------------ Original ------------------

> From:  "Huacai Chen"<[e-mail]>;

> Date:  Fri, Jan 26, 2018 08:53 PM

> To:  "James Smart"<[e-mail]>; "Dick Kennedy"<[e-mail]>;

> Cc:  "James E . J . Bottomley"<[e-mail]>; "Martin K . Petersen"<[e-mail]>; "Fuxin Zhang"<[e-mail]>; "linux-scsi"<[e-mail]>; "Huacai Chen"<[e-mail]>; "stable"<[e-mail]>;

> Subject:  [PATCH] scsi: lpfc: Switch memcpy_fromio() to __read32_copy()

>   

> In commit bc73905abf770192 ("[SCSI] lpfc 8.3.16: SLI Additions, updates,

> and code cleanup"), lpfc_memcpy_to_slim() have switched memcpy_toio() to

> __write32_copy() in order to prevent unaligned 64 bit copy. Recently, we

> found that lpfc_memcpy_from_slim() have similar issues, so let it switch

> memcpy_fromio() to __read32_copy().

>

> Cc: [e-mail]

> Signed-off-by: Huacai Chen <[e-mail]>

> ---

>   drivers/scsi/lpfc/lpfc_compat.h | 4 ++--

>   1 file changed, 2 insertions(+), 2 deletions(-)

>

> diff --git a/drivers/scsi/lpfc/lpfc_compat.h b/drivers/scsi/lpfc/lpfc_compat.h

> index 6b32b0a..47d4fad 100644

> --- a/drivers/scsi/lpfc/lpfc_compat.h

> +++ b/drivers/scsi/lpfc/lpfc_compat.h

> @@ -91,8 +91,8 @@ lpfc_memcpy_to_slim( void __iomem *dest, void *src, unsigned int bytes)

>   static inline void

>   lpfc_memcpy_from_slim( void *dest, void __iomem *src, unsigned int bytes)

>   {

> -	/* actually returns 1 byte past dest */

> -	memcpy_fromio( dest, src, bytes);

> +	/* convert bytes in argument list to word count for copy function */

> +	__ioread32_copy(dest, src, bytes / sizeof(uint32_t));

>   }

>   

>   #endif	/* __BIG_ENDIAN */


Huacai,

Note: the original change was 7 yrs ago and we've had no reports of 
issues. but....

This change didn't look necessary as the memcpy_XXXio() should allow 
byte alignments and byte-based lengths. Which means the original patch 
also wasn't necessary, which implies that the platform that had the 
issues has an api restriction not normally found on the other platforms.

I have no problem with the change though.

-- james

Signed-off-by: James Smart  <[e-mail]>

Huacai Chen July 9, 2018, 1:36 a.m. UTC | #3

Hi, James,

I think I found the root cause: lpfc's device memory doesn't support 128bit load/store. But Loongson-3's optimised memcpy() will use 128bit load/store instructions (gslq/gssq). So is this patch neccesory to avoid 128bit memory access (maybe there are other platforms have the same problem)?
 
Huacai
 
------------------ Original ------------------
From:  "James Smart"<[e-mail]>;

Date:  Mon, Mar 5, 2018 11:37 PM
To:  "陈华才"<[e-mail]>; "Dick Kennedy"<[e-mail]>;
Cc:  "James E . J . Bottomley"<[e-mail]>; "Martin K . Petersen"<[e-mail]>; "Fuxin Zhang"<[e-mail]>; "linux-scsi"<[e-mail]>; "stable"<[e-mail]>;
Subject:  Re: [PATCH] scsi: lpfc: Switch memcpy_fromio() to __read32_copy()
 

On 3/4/2018 10:56 PM, 陈华才 wrote:
> No one has the same problem? Or this patch need improve?

>

> Huacai

>   

> ------------------ Original ------------------

> From:  "陈华才"<[e-mail]>;

> Date:  Thu, Mar 1, 2018 01:59 PM

> To:  "James Smart"<[e-mail]>; "Dick Kennedy"<[e-mail]>;

> Cc:  "James E . J . Bottomley"<[e-mail]>; "Martin K . Petersen"<[e-mail]>; "Fuxin Zhang"<[e-mail]>; "linux-scsi"<[e-mail]>; "stable"<[e-mail]>;

> Subject:  Re:[PATCH] scsi: lpfc: Switch memcpy_fromio() to __read32_copy()

>   

> Ping?

>   

> ------------------ Original ------------------

> From:  "Huacai Chen"<[e-mail]>;

> Date:  Fri, Jan 26, 2018 08:53 PM

> To:  "James Smart"<[e-mail]>; "Dick Kennedy"<[e-mail]>;

> Cc:  "James E . J . Bottomley"<[e-mail]>; "Martin K . Petersen"<[e-mail]>; "Fuxin Zhang"<[e-mail]>; "linux-scsi"<[e-mail]>; "Huacai Chen"<[e-mail]>; "stable"<[e-mail]>;

> Subject:  [PATCH] scsi: lpfc: Switch memcpy_fromio() to __read32_copy()

>   

> In commit bc73905abf770192 ("[SCSI] lpfc 8.3.16: SLI Additions, updates,

> and code cleanup"), lpfc_memcpy_to_slim() have switched memcpy_toio() to

> __write32_copy() in order to prevent unaligned 64 bit copy. Recently, we

> found that lpfc_memcpy_from_slim() have similar issues, so let it switch

> memcpy_fromio() to __read32_copy().

>

> Cc: [e-mail]

> Signed-off-by: Huacai Chen <[e-mail]>

> ---

>   drivers/scsi/lpfc/lpfc_compat.h | 4 ++--

>   1 file changed, 2 insertions(+), 2 deletions(-)

>

> diff --git a/drivers/scsi/lpfc/lpfc_compat.h b/drivers/scsi/lpfc/lpfc_compat.h

> index 6b32b0a..47d4fad 100644

> --- a/drivers/scsi/lpfc/lpfc_compat.h

> +++ b/drivers/scsi/lpfc/lpfc_compat.h

> @@ -91,8 +91,8 @@ lpfc_memcpy_to_slim( void __iomem *dest, void *src, unsigned int bytes)

>   static inline void

>   lpfc_memcpy_from_slim( void *dest, void __iomem *src, unsigned int bytes)

>   {

> -	/* actually returns 1 byte past dest */

> -	memcpy_fromio( dest, src, bytes);

> +	/* convert bytes in argument list to word count for copy function */

> +	__ioread32_copy(dest, src, bytes / sizeof(uint32_t));

>   }

>   

>   #endif	/* __BIG_ENDIAN */


Huacai,

Note: the original change was 7 yrs ago and we've had no reports of 
issues. but....

This change didn't look necessary as the memcpy_XXXio() should allow 
byte alignments and byte-based lengths. Which means the original patch 
also wasn't necessary, which implies that the platform that had the 
issues has an api restriction not normally found on the other platforms.

I have no problem with the change though.

-- james

Signed-off-by: James Smart  <[e-mail]>

Patch

diff --git a/drivers/scsi/lpfc/lpfc_compat.h b/drivers/scsi/lpfc/lpfc_compat.h
index 6b32b0a..47d4fad 100644
--- a/drivers/scsi/lpfc/lpfc_compat.h
+++ b/drivers/scsi/lpfc/lpfc_compat.h
@@ -91,8 +91,8 @@  lpfc_memcpy_to_slim( void __iomem *dest, void *src, unsigned int bytes)
 static inline void
 lpfc_memcpy_from_slim( void *dest, void __iomem *src, unsigned int bytes)
 {
-	/* actually returns 1 byte past dest */
-	memcpy_fromio( dest, src, bytes);
+	/* convert bytes in argument list to word count for copy function */
+	__ioread32_copy(dest, src, bytes / sizeof(uint32_t));
 }
 
 #endif	/* __BIG_ENDIAN */