linux-kernel - Re: [PATCH] scsi: mpt3sas: remove redundant wmb on arm/arm64

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 07 Apr 2017 10:25:30 -0700
From:   James Bottomley <jejb@...ux.vnet.ibm.com>
To:     Sinan Kaya <okaya@...eaurora.org>, linux-scsi@...r.kernel.org,
        timur@...eaurora.org
Cc:     linux-arm-msm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        Sathya Prakash <sathya.prakash@...adcom.com>,
        Chaitra P B <chaitra.basappa@...adcom.com>,
        Suganath Prabu Subramani 
        <suganath-prabu.subramani@...adcom.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        "open list:LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)" 
        <MPT-FusionLinux.pdl@...adcom.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] scsi: mpt3sas: remove redundant wmb on arm/arm64

On Fri, 2017-04-07 at 12:41 -0400, Sinan Kaya wrote:
> 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.
> 
> The kernel has been updated to support relaxed read/write
> API to be supported across all architectures now.
> 
> The right thing was to either call __raw_writel/__raw_readl or
> write_relaxed/read_relaxed for multi-arch compatibility.

writeX_relaxed and thus your patch is definitely wrong.  The reason is
that we have two ordering domains: the CPU and the Bus.  wmb forces
ordering in the CPU domain but not the bus domain.  writeX originally
forced ordering in the bus domain but not the CPU domain, but since the
raw primitives I think it now orders in both and writeX_relaxed orders
in neither domain, so your patch would currently eliminate the bus
ordering.

James