Subject: Re: [PATCH v2 0/2] Add ADXL367 driver
To:     Cosmin Tanislav <demonsingur@gmail.com>
Cc:     cosmin.tanislav@analog.com,
        Michael Hennerich <Michael.Hennerich@analog.com>,
        Rob Herring <robh+dt@kernel.org>, linux-iio@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20211207094337.59300-1-cosmin.tanislav@analog.com>
From:   Lars-Peter Clausen <lars@metafoo.de>
Message-ID: <fbf1bc12-5013-0b09-a6f4-a602f0afbe70@metafoo.de>
Date:   Mon, 13 Dec 2021 12:34:19 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <20211207094337.59300-1-cosmin.tanislav@analog.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

On 12/7/21 10:43 AM, Cosmin Tanislav wrote:
> I have one question that is not actually specific to this driver but would
> help me clear up some issues.
>
> I used mutex_lock and mutex_unlock when accessing anything in driver's
> state that could potentially be written by another process in parallel.
>
> I heard mixed opinions about this. Some people said that it is not
> necessary to lock everywhere because loads and stores for data with size
> smaller or equal than register size would be done in one single atomic
> instruction.
>
> On the other hand, I also heard that this is not true unless WRITE_ONCE
> and READ_ONCE is used.
>
> It felt weird using WRITE_ONCE and READ_ONCE in this driver, so I kept
> using mutexes.
>
> Could I get some opinions on this matter?

What you wrote sums it up very well. READ_ONCE/WRITE_ONCE are required 
for correctness when no lock is used. The compiler is allowed to do all 
sorts of optimizations that could break multi-threading, when 
READ_ONCE/WRITE_ONCE is not used. E.g.

if (x)
   foo->bar = 10;
else
   foo->bar = 20;

Could be implemented as

foo->bar = 20;
if (x)
   foo->bar = 10;

In the absence of multi-threading the result will be the same. But if 
another thread reads foo->bar just at the right time it will read the 
incorrect 20.

For simple things like `foo->bar = x;` it is unlikely that the compiler 
will do anything other than the single store. But it could and the code 
is not correct without the WRITE_ONCE.

Using a mutex is OK, since non of this is performance critical.