linux-kernel - Re: [PATCH] i2c: brcmstb: Add support for atomic transfers

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]

Message-ID: <b4b91fb3-07b0-41c2-a97c-d1a045924fdc@samsung.com>
Date:   Wed, 11 Oct 2023 13:47:04 +0200
From:   Marek Szyprowski <m.szyprowski@...sung.com>
To:     Gregor Riepl <onitake@...il.com>, linux-i2c@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Cc:     Kamal Dasu <kamal.dasu@...adcom.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        Andi Shyti <andi.shyti@...nel.org>,
        Florian Fainelli <florian.fainelli@...adcom.com>,
        Wolfram Sang <wsa@...nel.org>
Subject: Re: [PATCH] i2c: brcmstb: Add support for atomic transfers

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