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

From: Gregor Riepl
Date: Wed Oct 11 2023 - 06:23:41 EST


Hi,

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() ?

Regards,
Gregor