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

[Gregor Riepl <onitake@gmail.com>]

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