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
