* [PATCH 1/8] i2c: nomadik: optimize struct layout, change unsigned char into u8
2026-11-03 8:29 [PATCH 0/8] i2c: nomadik: fix several issues in fault handling, optimize layout Dmitry Guzman
@ 2026-11-03 8:29 ` Dmitry Guzman
2026-11-03 8:29 ` [PATCH 2/8] i2c: nomadik: do not try to retransmit I2C message series on errors Dmitry Guzman
` (6 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Dmitry Guzman @ 2026-11-03 8:29 UTC (permalink / raw)
To: Linus Walleij, Andi Shyti, srinidhi kasagar, Ben Dooks, Andrea Gallo
Cc: linux-i2c, linux-arm-kernel, linux-kernel, Dmitry Guzman
Change type of `tft` and `rft` fields of `nmk_i2c_dev` structure from
`unsigned char` to `u8` and `buffer` field of `i2c_nmk_client` structure
from `unsigned char *` to `u8 *`.
Put two bool variables `xfer_done` and `has_32b_bus` and two u8 variables
`tft` and `rft` together in order to reduce struct size wasted for padding.
Reviewed-by: Linus Walleij <linusw@kernel.org>
Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
drivers/i2c/busses/i2c-nomadik.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index c3878670da9c..45b4815b0945 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -170,7 +170,7 @@ enum i2c_operating_mode {
struct i2c_nmk_client {
unsigned short slave_adr;
unsigned long count;
- unsigned char *buffer;
+ u8 *buffer;
unsigned long xfer_bytes;
enum i2c_operation operation;
};
@@ -187,13 +187,13 @@ struct i2c_nmk_client {
* @clk_freq: clock frequency for the operation mode
* @tft: Tx FIFO Threshold in bytes
* @rft: Rx FIFO Threshold in bytes
+ * @xfer_done: xfer done boolean.
+ * @has_32b_bus: controller is on a bus that only supports 32-bit accesses.
* @timeout_usecs: Slave response timeout
* @sm: speed mode
* @stop: stop condition.
* @xfer_wq: xfer done wait queue.
- * @xfer_done: xfer done boolean.
* @result: controller propogated result.
- * @has_32b_bus: controller is on a bus that only supports 32-bit accesses.
*/
struct nmk_i2c_dev {
struct i2c_vendor_data *vendor;
@@ -204,15 +204,15 @@ struct nmk_i2c_dev {
struct clk *clk;
struct i2c_nmk_client cli;
u32 clk_freq;
- unsigned char tft;
- unsigned char rft;
+ u8 tft;
+ u8 rft;
+ bool xfer_done;
+ bool has_32b_bus;
u32 timeout_usecs;
enum i2c_freq_mode sm;
int stop;
struct wait_queue_head xfer_wq;
- bool xfer_done;
int result;
- bool has_32b_bus;
};
/* controller's abort causes */
--
2.43.0
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 2/8] i2c: nomadik: do not try to retransmit I2C message series on errors
2026-11-03 8:29 [PATCH 0/8] i2c: nomadik: fix several issues in fault handling, optimize layout Dmitry Guzman
2026-11-03 8:29 ` [PATCH 1/8] i2c: nomadik: optimize struct layout, change unsigned char into u8 Dmitry Guzman
@ 2026-11-03 8:29 ` Dmitry Guzman
2026-11-03 8:29 ` [PATCH 3/8] i2c: nomadik: return proper fault codes Dmitry Guzman
` (5 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Dmitry Guzman @ 2026-11-03 8:29 UTC (permalink / raw)
To: Linus Walleij, Andi Shyti, srinidhi kasagar, Ben Dooks, Andrea Gallo
Cc: linux-i2c, linux-arm-kernel, linux-kernel, Dmitry Guzman
i2c-nomadik driver of I2C bus controller in xfer() callback retransmits
the whole message series in cause of any fault, and returns fault only
after third failed attempt. This behavior contradicts with API because
not only it hides hardware faults, but also re-sends messages, while
they are not guaranteed to be idempotent.
Remove the triple attempt to send messages in xfer() callback.
Reviewed-by: Linus Walleij <linusw@kernel.org>
Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
drivers/i2c/busses/i2c-nomadik.c | 30 ++++++++++++------------------
1 file changed, 12 insertions(+), 18 deletions(-)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 45b4815b0945..ca9366e09e71 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -716,27 +716,21 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
int status = 0;
int i;
struct nmk_i2c_dev *priv = i2c_get_adapdata(i2c_adap);
- int j;
pm_runtime_get_sync(&priv->adev->dev);
- /* Attempt three times to send the message queue */
- for (j = 0; j < 3; j++) {
- /* setup the i2c controller */
- setup_i2c_controller(priv);
-
- for (i = 0; i < num_msgs; i++) {
- priv->cli.slave_adr = msgs[i].addr;
- priv->cli.buffer = msgs[i].buf;
- priv->cli.count = msgs[i].len;
- priv->stop = (i < (num_msgs - 1)) ? 0 : 1;
- priv->result = 0;
-
- status = nmk_i2c_xfer_one(priv, msgs[i].flags);
- if (status != 0)
- break;
- }
- if (status == 0)
+ /* setup the i2c controller */
+ setup_i2c_controller(priv);
+
+ for (i = 0; i < num_msgs; i++) {
+ priv->cli.slave_adr = msgs[i].addr;
+ priv->cli.buffer = msgs[i].buf;
+ priv->cli.count = msgs[i].len;
+ priv->stop = (i < (num_msgs - 1)) ? 0 : 1;
+ priv->result = 0;
+
+ status = nmk_i2c_xfer_one(priv, msgs[i].flags);
+ if (status != 0)
break;
}
--
2.43.0
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 3/8] i2c: nomadik: return proper fault codes
2026-11-03 8:29 [PATCH 0/8] i2c: nomadik: fix several issues in fault handling, optimize layout Dmitry Guzman
2026-11-03 8:29 ` [PATCH 1/8] i2c: nomadik: optimize struct layout, change unsigned char into u8 Dmitry Guzman
2026-11-03 8:29 ` [PATCH 2/8] i2c: nomadik: do not try to retransmit I2C message series on errors Dmitry Guzman
@ 2026-11-03 8:29 ` Dmitry Guzman
2026-11-03 8:29 ` [PATCH 4/8] i2c: nomadik: change print level for fault messages to debug Dmitry Guzman
` (4 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Dmitry Guzman @ 2026-11-03 8:29 UTC (permalink / raw)
To: Linus Walleij, Andi Shyti, srinidhi kasagar, Ben Dooks, Andrea Gallo
Cc: linux-i2c, linux-arm-kernel, linux-kernel, Dmitry Guzman
I2C documentation Documentation/i2c/fault-codes.rst defines fault codes
for different negative results in I2C transmittion. Previously,
i2c-nomadik driver didn't implement them properly - it returned
ETIMEDOUT on most errors and EIO on master arbitration lost.
To comply with the documentation, return the proper fault codes for
different conditions, namely:
- EAGAIN if arbitration lost
- EOVERFLOW if message is too long (>2047 bytes)
- ENXIO if target address is not acknowledged
- EIO on other errors detected by controller (for example, NACK on data)
- ETIMEDOUT if driver gets timeout waiting for message completion
without any fault condition detected by the controller (for example,
too long message, or SDA/SCL line stuck on 0).
Reviewed-by: Linus Walleij <linusw@kernel.org>
Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
drivers/i2c/busses/i2c-nomadik.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index ca9366e09e71..a3cef4b66190 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -226,6 +226,18 @@ static const char *abort_causes[] = {
"overflow, maxsize is 2047 bytes",
};
+/* Linux fault codes for controller abort causes */
+static int fault_codes[] = {
+ ENXIO,
+ EIO,
+ EIO,
+ EAGAIN,
+ EIO,
+ EIO,
+ EOVERFLOW,
+ EIO
+};
+
static inline void i2c_set_bit(void __iomem *reg, u32 mask)
{
writel(readl(reg) | mask, reg);
@@ -653,6 +665,8 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, u16 flags)
cause >= ARRAY_SIZE(abort_causes) ?
"unknown reason" :
abort_causes[cause]);
+ priv->result = -fault_codes[cause];
+ status = priv->result;
}
init_hw(priv);
@@ -865,7 +879,7 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
/* Master Arbitration lost interrupt */
case I2C_IT_MAL:
- priv->result = -EIO;
+ priv->result = -EAGAIN;
init_hw(priv);
i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_MAL);
--
2.43.0
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 4/8] i2c: nomadik: change print level for fault messages to debug
2026-11-03 8:29 [PATCH 0/8] i2c: nomadik: fix several issues in fault handling, optimize layout Dmitry Guzman
` (2 preceding siblings ...)
2026-11-03 8:29 ` [PATCH 3/8] i2c: nomadik: return proper fault codes Dmitry Guzman
@ 2026-11-03 8:29 ` Dmitry Guzman
2026-11-03 8:29 ` [PATCH 5/8] i2c: nomadik: add quirks max_len=2047 and no_zero_len_read Dmitry Guzman
` (3 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Dmitry Guzman @ 2026-11-03 8:29 UTC (permalink / raw)
To: Linus Walleij, Andi Shyti, srinidhi kasagar, Ben Dooks, Andrea Gallo
Cc: linux-i2c, linux-arm-kernel, linux-kernel, Dmitry Guzman
i2c-nomadik driver prints error message on every faulted message. This
is not a good practice, because in I2C a fault not always is an error,
sometimes it is the expected result. For example, scanning bus with
`i2cdetect` prints over 100 messages in dmesg (two messages per each
target address).
To avoid excessive prints in the log, change the print level from err to
debug.
Reviewed-by: Linus Walleij <linusw@kernel.org>
Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
drivers/i2c/busses/i2c-nomadik.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index a3cef4b66190..9c7658148bd4 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -627,7 +627,7 @@ static int write_i2c(struct nmk_i2c_dev *priv, u16 flags)
if (!xfer_done) {
/* Controller timed out */
- dev_err(&priv->adev->dev, "write to slave 0x%x timed out\n",
+ dev_dbg(&priv->adev->dev, "write to slave 0x%x timed out\n",
priv->cli.slave_adr);
status = -ETIMEDOUT;
}
@@ -661,7 +661,7 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, u16 flags)
i2c_sr = readl(priv->virtbase + I2C_SR);
if (FIELD_GET(I2C_SR_STATUS, i2c_sr) == I2C_ABORT) {
cause = FIELD_GET(I2C_SR_CAUSE, i2c_sr);
- dev_err(&priv->adev->dev, "%s\n",
+ dev_dbg(&priv->adev->dev, "%s\n",
cause >= ARRAY_SIZE(abort_causes) ?
"unknown reason" :
abort_causes[cause]);
--
2.43.0
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 5/8] i2c: nomadik: add quirks max_len=2047 and no_zero_len_read
2026-11-03 8:29 [PATCH 0/8] i2c: nomadik: fix several issues in fault handling, optimize layout Dmitry Guzman
` (3 preceding siblings ...)
2026-11-03 8:29 ` [PATCH 4/8] i2c: nomadik: change print level for fault messages to debug Dmitry Guzman
@ 2026-11-03 8:29 ` Dmitry Guzman
2026-11-05 8:40 ` Linus Walleij
2026-11-03 8:29 ` [PATCH 6/8] i2c: nomadik: handle pm_runtime errors Dmitry Guzman
` (2 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Dmitry Guzman @ 2026-11-03 8:29 UTC (permalink / raw)
To: Linus Walleij, Andi Shyti, srinidhi kasagar, Ben Dooks, Andrea Gallo
Cc: linux-i2c, linux-arm-kernel, linux-kernel, Dmitry Guzman
In Nomadik I2c controller, register I2C_MCR has 11-bit wide LENGTH
field. Its maximum value is 2047, so this is the maximum length of a
single message. It is less than the common maximum I2C message length in
I2C subsystem (8192), so define a quirk in order to report the
unsupported message without any attempt to transfer it.
Zero length reading doesn't work properly on this controller, so add
`I2C_AQ_NO_ZERO_LEN_READ` quirk flag.
Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
drivers/i2c/busses/i2c-nomadik.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 9c7658148bd4..d07246d11b35 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -79,6 +79,9 @@
#define I2C_MCR_STOP BIT(14) /* Stop condition */
#define I2C_MCR_LENGTH GENMASK(25, 15) /* Transaction length */
+/* Controller hardware limitation of the message length */
+#define I2C_MAX_MSG_LENGTH (I2C_MCR_LENGTH >> 15)
+
/* Status register (SR) */
#define I2C_SR_OP GENMASK(1, 0) /* Operation */
#define I2C_SR_STATUS GENMASK(3, 2) /* controller status */
@@ -1087,6 +1090,12 @@ static const struct of_device_id nmk_i2c_eyeq_match_table[] = {
{ /* sentinel */ }
};
+static const struct i2c_adapter_quirks nmk_i2c_quirks = {
+ .flags = I2C_AQ_NO_ZERO_LEN_READ,
+ .max_read_len = I2C_MAX_MSG_LENGTH,
+ .max_write_len = I2C_MAX_MSG_LENGTH,
+};
+
static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
{
struct i2c_vendor_data *vendor = id->data;
@@ -1161,6 +1170,7 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
adap->class = I2C_CLASS_DEPRECATED;
adap->algo = &nmk_i2c_algo;
adap->timeout = usecs_to_jiffies(priv->timeout_usecs);
+ adap->quirks = &nmk_i2c_quirks;
snprintf(adap->name, sizeof(adap->name),
"Nomadik I2C at %pR", &adev->res);
--
2.43.0
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 5/8] i2c: nomadik: add quirks max_len=2047 and no_zero_len_read
2026-11-03 8:29 ` [PATCH 5/8] i2c: nomadik: add quirks max_len=2047 and no_zero_len_read Dmitry Guzman
@ 2026-11-05 8:40 ` Linus Walleij
0 siblings, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2026-11-05 8:40 UTC (permalink / raw)
To: Dmitry Guzman
Cc: Andi Shyti, srinidhi kasagar, Ben Dooks, Andrea Gallo, linux-i2c,
linux-arm-kernel, linux-kernel
On Tue, Nov 3, 2026 at 9:29 AM Dmitry Guzman <Dmitry.Guzman@mobileye.com> wrote:
> In Nomadik I2c controller, register I2C_MCR has 11-bit wide LENGTH
> field. Its maximum value is 2047, so this is the maximum length of a
> single message. It is less than the common maximum I2C message length in
> I2C subsystem (8192), so define a quirk in order to report the
> unsupported message without any attempt to transfer it.
>
> Zero length reading doesn't work properly on this controller, so add
> `I2C_AQ_NO_ZERO_LEN_READ` quirk flag.
>
> Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
Reviewed-by: Linus Walleij <linusw@kernel.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 6/8] i2c: nomadik: handle pm_runtime errors
2026-11-03 8:29 [PATCH 0/8] i2c: nomadik: fix several issues in fault handling, optimize layout Dmitry Guzman
` (4 preceding siblings ...)
2026-11-03 8:29 ` [PATCH 5/8] i2c: nomadik: add quirks max_len=2047 and no_zero_len_read Dmitry Guzman
@ 2026-11-03 8:29 ` Dmitry Guzman
2026-11-05 8:42 ` Linus Walleij
2026-11-03 8:29 ` [PATCH 7/8] i2c: nomadik: fix race condition with interrupt handler Dmitry Guzman
2026-11-03 8:29 ` [PATCH 8/8] i2c: nomadik: reduce FIFO flush timeout to avoid long interrupt handling Dmitry Guzman
7 siblings, 1 reply; 14+ messages in thread
From: Dmitry Guzman @ 2026-11-03 8:29 UTC (permalink / raw)
To: Linus Walleij, Andi Shyti, srinidhi kasagar, Ben Dooks, Andrea Gallo
Cc: linux-i2c, linux-arm-kernel, linux-kernel, Dmitry Guzman
In nmk_i2c_xfer(), pm_runtime_get_sync() is called on start and then
the controller registers are accessed without checking the return value.
If pm_runtime_get_sync() fails, access to the register of the device that
was not correctly powered on may cause a bus fault or other unpredictable
system behavior.
Check the return value of pm_runtime_get_sync() and it if is not 0,
return the error immediately.
Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
drivers/i2c/busses/i2c-nomadik.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index d07246d11b35..361a5fc6a587 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -734,7 +734,9 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
int i;
struct nmk_i2c_dev *priv = i2c_get_adapdata(i2c_adap);
- pm_runtime_get_sync(&priv->adev->dev);
+ i = pm_runtime_get_sync(&priv->adev->dev);
+ if (i)
+ return i;
/* setup the i2c controller */
setup_i2c_controller(priv);
--
2.43.0
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 6/8] i2c: nomadik: handle pm_runtime errors
2026-11-03 8:29 ` [PATCH 6/8] i2c: nomadik: handle pm_runtime errors Dmitry Guzman
@ 2026-11-05 8:42 ` Linus Walleij
2026-11-05 8:51 ` Wolfram Sang
0 siblings, 1 reply; 14+ messages in thread
From: Linus Walleij @ 2026-11-05 8:42 UTC (permalink / raw)
To: Dmitry Guzman
Cc: Andi Shyti, srinidhi kasagar, Ben Dooks, Andrea Gallo, linux-i2c,
linux-arm-kernel, linux-kernel
On Tue, Nov 3, 2026 at 9:30 AM Dmitry Guzman <Dmitry.Guzman@mobileye.com> wrote:
> In nmk_i2c_xfer(), pm_runtime_get_sync() is called on start and then
> the controller registers are accessed without checking the return value.
> If pm_runtime_get_sync() fails, access to the register of the device that
> was not correctly powered on may cause a bus fault or other unpredictable
> system behavior.
>
> Check the return value of pm_runtime_get_sync() and it if is not 0,
> return the error immediately.
>
> Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
(...)
> - pm_runtime_get_sync(&priv->adev->dev);
> + i = pm_runtime_get_sync(&priv->adev->dev);
> + if (i)
> + return i;
It's a bit odd to reuse the i (= iterator...) variable for this.
I would just add an int ret or int err for this.
Either way semantics are right so:
Reviewed-by: Linus Walleij <linusw@kernel.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 6/8] i2c: nomadik: handle pm_runtime errors
2026-11-05 8:42 ` Linus Walleij
@ 2026-11-05 8:51 ` Wolfram Sang
0 siblings, 0 replies; 14+ messages in thread
From: Wolfram Sang @ 2026-11-05 8:51 UTC (permalink / raw)
To: Linus Walleij
Cc: Dmitry Guzman, Andi Shyti, srinidhi kasagar, Ben Dooks,
Andrea Gallo, linux-i2c, linux-arm-kernel, linux-kernel
On Thu, Nov 05, 2026 at 09:42:02AM +0100, Linus Walleij wrote:
> On Tue, Nov 3, 2026 at 9:30 AM Dmitry Guzman <Dmitry.Guzman@mobileye.com> wrote:
>
> > In nmk_i2c_xfer(), pm_runtime_get_sync() is called on start and then
> > the controller registers are accessed without checking the return value.
> > If pm_runtime_get_sync() fails, access to the register of the device that
> > was not correctly powered on may cause a bus fault or other unpredictable
> > system behavior.
> >
> > Check the return value of pm_runtime_get_sync() and it if is not 0,
> > return the error immediately.
> >
> > Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
> (...)
> > - pm_runtime_get_sync(&priv->adev->dev);
> > + i = pm_runtime_get_sync(&priv->adev->dev);
> > + if (i)
> > + return i;
>
> It's a bit odd to reuse the i (= iterator...) variable for this.
> I would just add an int ret or int err for this.
Yes, please.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 7/8] i2c: nomadik: fix race condition with interrupt handler
2026-11-03 8:29 [PATCH 0/8] i2c: nomadik: fix several issues in fault handling, optimize layout Dmitry Guzman
` (5 preceding siblings ...)
2026-11-03 8:29 ` [PATCH 6/8] i2c: nomadik: handle pm_runtime errors Dmitry Guzman
@ 2026-11-03 8:29 ` Dmitry Guzman
2026-11-05 8:42 ` Linus Walleij
2026-11-03 8:29 ` [PATCH 8/8] i2c: nomadik: reduce FIFO flush timeout to avoid long interrupt handling Dmitry Guzman
7 siblings, 1 reply; 14+ messages in thread
From: Dmitry Guzman @ 2026-11-03 8:29 UTC (permalink / raw)
To: Linus Walleij, Andi Shyti, srinidhi kasagar, Ben Dooks, Andrea Gallo
Cc: linux-i2c, linux-arm-kernel, linux-kernel, Dmitry Guzman
If nmk_i2c_wait_xfer_done() gets timeout on waiting for wakeup from
interrupt handler, there is no synchronization with interrupt handler
that may run in parallel. This can potentially lead to various race
conditions between thread and interrupt handler.
Disable and clear all pending interrupt requests and wait until running
interrupt handler finishes after the timeout.
Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
drivers/i2c/busses/i2c-nomadik.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 361a5fc6a587..cc8b5a357f06 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -505,6 +505,11 @@ static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout);
}
+ /* Disable interrupts and wait for pending interrupts */
+ disable_all_interrupts(priv);
+ clear_all_interrupts(priv);
+ synchronize_irq(priv->irq);
+
return priv->xfer_done;
}
--
2.43.0
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 7/8] i2c: nomadik: fix race condition with interrupt handler
2026-11-03 8:29 ` [PATCH 7/8] i2c: nomadik: fix race condition with interrupt handler Dmitry Guzman
@ 2026-11-05 8:42 ` Linus Walleij
0 siblings, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2026-11-05 8:42 UTC (permalink / raw)
To: Dmitry Guzman
Cc: Andi Shyti, srinidhi kasagar, Ben Dooks, Andrea Gallo, linux-i2c,
linux-arm-kernel, linux-kernel
On Tue, Nov 3, 2026 at 9:30 AM Dmitry Guzman <Dmitry.Guzman@mobileye.com> wrote:
> If nmk_i2c_wait_xfer_done() gets timeout on waiting for wakeup from
> interrupt handler, there is no synchronization with interrupt handler
> that may run in parallel. This can potentially lead to various race
> conditions between thread and interrupt handler.
>
> Disable and clear all pending interrupt requests and wait until running
> interrupt handler finishes after the timeout.
>
> Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
Reviewed-by: Linus Walleij <linusw@kernel.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 8/8] i2c: nomadik: reduce FIFO flush timeout to avoid long interrupt handling
2026-11-03 8:29 [PATCH 0/8] i2c: nomadik: fix several issues in fault handling, optimize layout Dmitry Guzman
` (6 preceding siblings ...)
2026-11-03 8:29 ` [PATCH 7/8] i2c: nomadik: fix race condition with interrupt handler Dmitry Guzman
@ 2026-11-03 8:29 ` Dmitry Guzman
2026-11-05 8:44 ` Linus Walleij
7 siblings, 1 reply; 14+ messages in thread
From: Dmitry Guzman @ 2026-11-03 8:29 UTC (permalink / raw)
To: Linus Walleij, Andi Shyti, srinidhi kasagar, Ben Dooks, Andrea Gallo
Cc: linux-i2c, linux-arm-kernel, linux-kernel, Dmitry Guzman
In flush_i2c_fifo(), the driver sets FTX and FRX flags and waits until
they are reset (these bits are self-clearing). If after the first read
one or both of these bits is set, it waits until both are reset. The
timeout is 10 * I2C transfer timeout. I2C transfer timeout is by default
200 milliseconds. 2 seconds are much more than needed: as stated in the
comment above, the flushing operation takes just several cycles before
completed.
flush_i2c_fifo() is called from init_hw() that may be called from
interrupt handler. So, if the I2C controller is stuck and the FIFO does
not get flushed, the CPU may spend up to 2 seconds in the interrupt
handler and cause watchdog timeout or other unwanted consequences.
Moreover, if the interrupt is handled by the same CPU core that updates
jiffies, the loop may not finish at all.
To avoid too long waiting, reduce the timeout to 100 microseconds and
use read_poll_timeout_atomic() instead of relying on jiffies.
Fixes: 3f9900f17cdb ("i2c: Add support for Ux500/Nomadik I2C controller")
Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
drivers/i2c/busses/i2c-nomadik.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index cc8b5a357f06..1f332e66a243 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -278,9 +278,8 @@ static inline void nmk_i2c_writeb(const struct nmk_i2c_dev *priv, u32 val,
*/
static int flush_i2c_fifo(struct nmk_i2c_dev *priv)
{
-#define LOOP_ATTEMPTS 10
- ktime_t timeout;
- int i;
+#define FIFO_FLUSH_TIMEOUT_US 100
+ u32 reg, err;
/*
* flush the transmit and receive FIFO. The flushing
@@ -291,21 +290,16 @@ static int flush_i2c_fifo(struct nmk_i2c_dev *priv)
*/
writel((I2C_CR_FTX | I2C_CR_FRX), priv->virtbase + I2C_CR);
- for (i = 0; i < LOOP_ATTEMPTS; i++) {
- timeout = ktime_add_us(ktime_get(), priv->timeout_usecs);
-
- while (ktime_after(timeout, ktime_get())) {
- if ((readl(priv->virtbase + I2C_CR) &
- (I2C_CR_FTX | I2C_CR_FRX)) == 0)
- return 0;
- }
- }
+ /* This may execute in interrupt handler, so use read_poll_timeout_atomic() */
+ err = read_poll_timeout_atomic(readl, reg, (reg & (I2C_CR_FTX | I2C_CR_FRX)) == 0,
+ 1, FIFO_FLUSH_TIMEOUT_US, 0, priv->virtbase + I2C_CR);
- dev_err(&priv->adev->dev,
- "flushing operation timed out giving up after %d attempts",
- LOOP_ATTEMPTS);
+ if (err)
+ dev_err(&priv->adev->dev,
+ "flushing operation timed out giving up after %d microseconds",
+ FIFO_FLUSH_TIMEOUT_US);
- return -ETIMEDOUT;
+ return err;
}
/**
--
2.43.0
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 8/8] i2c: nomadik: reduce FIFO flush timeout to avoid long interrupt handling
2026-11-03 8:29 ` [PATCH 8/8] i2c: nomadik: reduce FIFO flush timeout to avoid long interrupt handling Dmitry Guzman
@ 2026-11-05 8:44 ` Linus Walleij
0 siblings, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2026-11-05 8:44 UTC (permalink / raw)
To: Dmitry Guzman
Cc: Andi Shyti, srinidhi kasagar, Ben Dooks, Andrea Gallo, linux-i2c,
linux-arm-kernel, linux-kernel
On Tue, Nov 3, 2026 at 9:30 AM Dmitry Guzman <Dmitry.Guzman@mobileye.com> wrote:
> In flush_i2c_fifo(), the driver sets FTX and FRX flags and waits until
> they are reset (these bits are self-clearing). If after the first read
> one or both of these bits is set, it waits until both are reset. The
> timeout is 10 * I2C transfer timeout. I2C transfer timeout is by default
> 200 milliseconds. 2 seconds are much more than needed: as stated in the
> comment above, the flushing operation takes just several cycles before
> completed.
>
> flush_i2c_fifo() is called from init_hw() that may be called from
> interrupt handler. So, if the I2C controller is stuck and the FIFO does
> not get flushed, the CPU may spend up to 2 seconds in the interrupt
> handler and cause watchdog timeout or other unwanted consequences.
> Moreover, if the interrupt is handled by the same CPU core that updates
> jiffies, the loop may not finish at all.
>
> To avoid too long waiting, reduce the timeout to 100 microseconds and
> use read_poll_timeout_atomic() instead of relying on jiffies.
>
> Fixes: 3f9900f17cdb ("i2c: Add support for Ux500/Nomadik I2C controller")
Is it really "fixing" that? I don't know if I agree that this is an error
in the initial submission in 2010...
>
> Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
Surplus newline above? Maintainer can remove it when applying.
Reviewed-by: Linus Walleij <linusw@kernel.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 14+ messages in thread