Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <b4b91fb3-07b0-41c2-a97c-d1a045924fdc@samsung.com>
Date:   Wed, 11 Oct 2023 13:47:04 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] i2c: brcmstb: Add support for atomic transfers
Content-Language: en-US
To:     Gregor Riepl <onitake@gmail.com>, linux-i2c@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Cc:     Kamal Dasu <kamal.dasu@broadcom.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@broadcom.com>,
        Andi Shyti <andi.shyti@kernel.org>,
        Florian Fainelli <florian.fainelli@broadcom.com>,
        Wolfram Sang <wsa@kernel.org>
From:   Marek Szyprowski <m.szyprowski@samsung.com>
In-Reply-To: <ba3c0972-2ba2-4df7-826e-a7634027b967@gmail.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <CGME20231006144123eucas1p111cbbdbd70927ffbd697f7edf6b7ae1c@eucas1p1.samsung.com>
        <20231006144117.4079796-1-m.szyprowski@samsung.com>
        <ba3c0972-2ba2-4df7-826e-a7634027b967@gmail.com>
Precedence: bulk

On 11.10.2023 12:23, Gregor Riepl wrote:
> I admit that I don't understand the I²C subsystem very well, but 
> doesn't this introduce a potential race condition?
>
> > ...
> > @@ -240,7 +241,7 @@ static int 
> brcmstb_i2c_wait_for_completion(struct brcmstb_i2c_dev *dev)
> > ...
>> -    if (dev->irq >= 0) {
>> +    if (dev->irq >= 0 && !dev->atomic) {
> > ...
> > @@ -287,7 +288,7 @@ static int brcmstb_send_i2c_cmd(struct 
> brcmstb_i2c_dev *dev,
> > ...
>> -    if (dev->irq >= 0)
>> +    if (dev->irq >= 0 && !dev->atomic)
> > ...
> > +static int brcmstb_i2c_xfer_atomic(struct i2c_adapter *adapter,
> > +                   struct i2c_msg msgs[], int num)
> > ...
>> +    dev->atomic = true;
>> +    ret = brcmstb_i2c_xfer(adapter, msgs, num);
>> +    dev->atomic = false;
>> ...
>
> What happens when one of the if() branches is taken in one thread 
> while another thread is just executing the assignment of the atomic 
> flag? My expectation would be that the first tread still sees the old 
> flag value and happily executes the branch, while 
> brcmstb_i2c_xfer_atomic() sets the flag just after and initiates a 
> transfer.
>
> I'd expect that access to the flag must be atomic as well, so maybe 
> something like 
> https://www.kernel.org/doc/html/latest/core-api/wrappers/atomic_t.html 
> is needed, or some other synchronization mechanism.
>
> Or is it guaranteed that brcmstb_i2c_wait_for_completion() and 
> brcmstb_send_i2c_cmd() can only be called from the same thread as 
> brcmstb_i2c_xfer_atomic() ?

Atomic i2c transfers are some kind of a special case.

I guess that i2c core takes care of NOT multiplexing atomic and standard 
i2c transfers. No special locking/protection is needed in the bus 
drivers. This is at least what I see from commits like 08960b022fb6 
("i2c: tegra-bpmp: convert to use new atomic callbacks") or 3d11a12ece85 
("i2c: ocores: enable atomic xfers").

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland