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

[Marek Szyprowski]

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