| Message ID | 20231006144117.4079796-1-m.szyprowski@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: brcmstb: Add support for atomic transfers | expand |
On 10/6/23 07:41, Marek Szyprowski wrote: > Add support for atomic transfers using polling mode with interrupts > intentionally disabled to get rid of the warning introduced by commit > 63b96983a5dd ("i2c: core: introduce callbacks for atomic transfers") > during system reboot and power-off. Is there an existing system that you have access to which needs atomic transfer support, or is this a forward looking change? > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
On 09.10.2023 22:41, Florian Fainelli wrote: > On 10/6/23 07:41, Marek Szyprowski wrote: >> Add support for atomic transfers using polling mode with interrupts >> intentionally disabled to get rid of the warning introduced by commit >> 63b96983a5dd ("i2c: core: introduce callbacks for atomic transfers") >> during system reboot and power-off. > > Is there an existing system that you have access to which needs atomic > transfer support, or is this a forward looking change? Frankly speaking I've observed the mentioned warning during system reboot on RaspberryPi4 with linux-next kernel compiled from multi_v7_defconfig. It looks that this driver is used by VC4 DRM for DDC. This issue doesn't look critical, but the fix seems to be trivial, thus my patch. Best regards
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
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
Hi Marek, On Fri, Oct 06, 2023 at 04:41:17PM +0200, Marek Szyprowski wrote: > Add support for atomic transfers using polling mode with interrupts > intentionally disabled to get rid of the warning introduced by commit > 63b96983a5dd ("i2c: core: introduce callbacks for atomic transfers") > during system reboot and power-off. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Andi
> I guess that i2c core takes care of NOT multiplexing atomic and standard > i2c transfers. Atomic transfers are only used iff the system is in a certain state, check i2c_in_atomic_xfer_mode(). Then and only then, transfers are atomic. All of them. Neither bus drivers nor clients can choose them.
On 10/11/2023 2:57 AM, Marek Szyprowski wrote: > On 09.10.2023 22:41, Florian Fainelli wrote: >> On 10/6/23 07:41, Marek Szyprowski wrote: >>> Add support for atomic transfers using polling mode with interrupts >>> intentionally disabled to get rid of the warning introduced by commit >>> 63b96983a5dd ("i2c: core: introduce callbacks for atomic transfers") >>> during system reboot and power-off. >> >> Is there an existing system that you have access to which needs atomic >> transfer support, or is this a forward looking change? > > Frankly speaking I've observed the mentioned warning during system > reboot on RaspberryPi4 with linux-next kernel compiled from > multi_v7_defconfig. It looks that this driver is used by VC4 DRM for > DDC. This issue doesn't look critical, but the fix seems to be trivial, > thus my patch. Makes sense, thanks for confirming this fixes an actual issue you have seen and providing a fix for it.
diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c index acee76732544..38f276c99193 100644 --- a/drivers/i2c/busses/i2c-brcmstb.c +++ b/drivers/i2c/busses/i2c-brcmstb.c @@ -160,6 +160,7 @@ struct brcmstb_i2c_dev { struct completion done; u32 clk_freq_hz; int data_regsz; + bool atomic; }; /* register accessors for both be and le cpu arch */ @@ -240,7 +241,7 @@ static int brcmstb_i2c_wait_for_completion(struct brcmstb_i2c_dev *dev) int ret = 0; unsigned long timeout = msecs_to_jiffies(I2C_TIMEOUT); - if (dev->irq >= 0) { + if (dev->irq >= 0 && !dev->atomic) { if (!wait_for_completion_timeout(&dev->done, timeout)) ret = -ETIMEDOUT; } else { @@ -287,7 +288,7 @@ static int brcmstb_send_i2c_cmd(struct brcmstb_i2c_dev *dev, return rc; /* only if we are in interrupt mode */ - if (dev->irq >= 0) + if (dev->irq >= 0 && !dev->atomic) reinit_completion(&dev->done); /* enable BSC CTL interrupt line */ @@ -520,6 +521,23 @@ static int brcmstb_i2c_xfer(struct i2c_adapter *adapter, } +static int brcmstb_i2c_xfer_atomic(struct i2c_adapter *adapter, + struct i2c_msg msgs[], int num) +{ + struct brcmstb_i2c_dev *dev = i2c_get_adapdata(adapter); + int ret; + + if (dev->irq >= 0) + disable_irq(dev->irq); + dev->atomic = true; + ret = brcmstb_i2c_xfer(adapter, msgs, num); + dev->atomic = false; + if (dev->irq >= 0) + enable_irq(dev->irq); + + return ret; +} + static u32 brcmstb_i2c_functionality(struct i2c_adapter *adap) { return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR @@ -528,6 +546,7 @@ static u32 brcmstb_i2c_functionality(struct i2c_adapter *adap) static const struct i2c_algorithm brcmstb_i2c_algo = { .master_xfer = brcmstb_i2c_xfer, + .master_xfer_atomic = brcmstb_i2c_xfer_atomic, .functionality = brcmstb_i2c_functionality, };
Add support for atomic transfers using polling mode with interrupts intentionally disabled to get rid of the warning introduced by commit 63b96983a5dd ("i2c: core: introduce callbacks for atomic transfers") during system reboot and power-off. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/i2c/busses/i2c-brcmstb.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)