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 <[email protected]>
---
drivers/i2c/busses/i2c-brcmstb.c | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
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,
};
--
2.34.1
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 <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
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
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
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
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
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 <[email protected]>
Reviewed-by: Andi Shyti <[email protected]>
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.
--
Florian