Re: [PATCH V2] scsi: mpt3sas: remove redundant wmb

[Martin K. Petersen]

Sinan,

> Due to relaxed ordering requirements on multiple architectures,
> drivers are required to use wmb/rmb/mb combinations when they need to
> guarantee observability between the memory and the HW.

Applied to 4.12/scsi-queue, thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH V2] scsi: mpt3sas: remove redundant wmb

[Brian King]

On 04/21/2017 02:56 AM, Sreekanth Reddy wrote:
> On Thu, Apr 20, 2017 at 7:58 AM, Martin K. Petersen
>  wrote:
>> Sinan Kaya  writes:
>>
>>> Due to relaxed ordering requirements on multiple architectures,
>>> drivers are required to use wmb/rmb/mb combinations when they need to
>>> guarantee observability between the memory and the HW.
>>>
>>> The mpt3sas driver is already using wmb() for this purpose.  However,
>>> it issues a writel following wmb(). writel() function on arm/arm64
>>> arhictectures have an embedded wmb() call inside.
> 
> [Sreekanth] Whether same thing applicable for SPARC & POWER
> architectures. If yes then we are fine with this patch changes.

This is also true for Power. 

Reviewed-by: Brian King 

-Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [PATCH V2] scsi: mpt3sas: remove redundant wmb

[Sinan Kaya]

On 4/21/2017 3:56 AM, Sreekanth Reddy wrote:
> [Sreekanth] Whether same thing applicable for SPARC & POWER
> architectures. If yes then we are fine with this patch changes.

This behavior is common for all architectures according to this document.

Who would be the best person to comment on SPARC and POWER architectures
in specific? James and I exchanged some comments on the first version. 

James? can you comment on POWER behavior.

https://www.kernel.org/doc/Documentation/memory-barriers.txt

Inside of the Linux kernel, I/O should be done through the appropriate accessor
routines - such as inb() or writel() - which know how to make such accesses
appropriately sequential.  

"Whilst this, for the most part, renders the explicit
use of memory barriers unnecessary", 

there are a couple of situations where they might be needed:

 (1) On some systems, I/O stores are not strongly ordered across all CPUs, and
 so for _all_ general drivers locks should be used and mmiowb() must be
 issued prior to unlocking the critical section.

 (2) If the accessor functions are used to refer to an I/O memory window with
 relaxed memory access properties, then _mandatory_ memory barriers are
 required to enforce ordering.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

Re: [PATCH V2] scsi: mpt3sas: remove redundant wmb

[Sreekanth Reddy]

On Thu, Apr 20, 2017 at 7:58 AM, Martin K. Petersen
 wrote:
> Sinan Kaya  writes:
>
>> Due to relaxed ordering requirements on multiple architectures,
>> drivers are required to use wmb/rmb/mb combinations when they need to
>> guarantee observability between the memory and the HW.
>>
>> The mpt3sas driver is already using wmb() for this purpose.  However,
>> it issues a writel following wmb(). writel() function on arm/arm64
>> arhictectures have an embedded wmb() call inside.

[Sreekanth] Whether same thing applicable for SPARC & POWER
architectures. If yes then we are fine with this patch changes.

>>
>> This results in unnecessary performance loss and code duplication.
>>
>> writel already guarantees ordering for both cpu and bus. we don't need
>> additional wmb()
>
> Broadcom folks, please review!
>
> --
> Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH V2] scsi: mpt3sas: remove redundant wmb

[Martin K. Petersen]

Sinan Kaya  writes:

> Due to relaxed ordering requirements on multiple architectures,
> drivers are required to use wmb/rmb/mb combinations when they need to
> guarantee observability between the memory and the HW.
>
> The mpt3sas driver is already using wmb() for this purpose.  However,
> it issues a writel following wmb(). writel() function on arm/arm64
> arhictectures have an embedded wmb() call inside.
>
> This results in unnecessary performance loss and code duplication.
>
> writel already guarantees ordering for both cpu and bus. we don't need
> additional wmb()

Broadcom folks, please review!

-- 
Martin K. Petersen  Oracle Linux Engineering

[PATCH V2] scsi: mpt3sas: remove redundant wmb

[Sinan Kaya]

Due to relaxed ordering requirements on multiple architectures,
drivers are required to use wmb/rmb/mb combinations when they
need to guarantee observability between the memory and the HW.

The mpt3sas driver is already using wmb() for this purpose.
However, it issues a writel following wmb(). writel() function
on arm/arm64 arhictectures have an embedded wmb() call inside.

This results in unnecessary performance loss and code duplication.

writel already guarantees ordering for both cpu and bus. we don't need
additional wmb()

Signed-off-by: Sinan Kaya 
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 5b7aec5..18039bb 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -1025,7 +1025,6 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
0 : ioc->reply_free_host_index + 1;
ioc->reply_free[ioc->reply_free_host_index] =
cpu_to_le32(reply);
-   wmb();
writel(ioc->reply_free_host_index,
>chip->ReplyFreeHostIndex);
}
@@ -1074,7 +1073,6 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
return IRQ_NONE;
}
 
-   wmb();
if (ioc->is_warpdrive) {
writel(reply_q->reply_post_host_index,
ioc->reply_post_host_index[msix_index]);
-- 
1.9.1