Hi Michael,
On Tue, 2 Jan 2024 at 16:03, Michael Walle <mwalle@xxxxxxxxxx> wrote:
> With preemption disabled, this boils down to
> return system_state > SYSTEM_RUNNING (&& !0)
>
> and will then generate a backtrace splash on each reboot on our
> board:
>
> # reboot -f
> [ 12.687169] No atomic I2C transfer handler for 'i2c-0'
> ...
> [ 12.806359] Call trace:
> [ 12.808793] i2c_smbus_xfer+0x100/0x118
> ...
>
> I'm not sure if this is now the expected behavior or not. There will be
> no backtraces, if I build a preemptible kernel, nor will there be
> backtraces if I revert this patch.
thanks for the report.
In your case, the warning comes from shutting down a regulator during
device_shutdown(), so nothing really problematic here. However, later in
the "restart sequence", IRQs are disabled before the restart handlers
are called. If the reboot handlers would rely on irq-based
("non-atomic") i2c transfer, they might not work properly.
> OTOH, the driver I'm using (drivers/i2c/busses/i2c-mt65xx.c) has no
> *_atomic(). So the warning is correct. There is also [1], which seems to
> be the same issue I'm facing.
>
> -michael
>
> [1] https://lore.kernel.org/linux-i2c/13271b9b-4132-46ef-abf8-2c311967bb46@xxxxxxxxxxx/
I tried to implement an atomic handler for the mt65xx, but I don't have
the respective hardware available to test it. I decided to use a similar
approach as done in drivers/i2c/busses/i2c-rk3x.c, which calls the IRQ
handler in a while loop if an atomic xfer is requested. IMHO, this
should work with IRQs enabled and disabled, but I am not sure if this is
the best approach...
diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index a8b5719c3372..3c18305e6059 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -16,6 +16,7 @@
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/iopoll.h>
+#include <linux/jiffies.h>
#include <linux/kernel.h>
#include <linux/mm.h>
#include <linux/module.h>
@@ -307,6 +308,8 @@ struct mtk_i2c {
bool ignore_restart_irq;
struct mtk_i2c_ac_timing ac_timing;
const struct mtk_i2c_compatible *dev_comp;
+ bool atomic_xfer;
+ bool xfer_complete;
};
/**
@@ -994,6 +997,20 @@ static void i2c_dump_register(struct mtk_i2c *i2c)
readl(i2c->pdmabase + OFFSET_RX_4G_MODE));
}
+static irqreturn_t mtk_i2c_irq(int irqno, void *dev_id);
+
+static int mtk_i2c_wait_xfer_atomic(struct mtk_i2c *i2c)
+{
+ unsigned long timeout = jiffies + i2c->adap.timeout;
+
+ do {
+ udelay(10);
+ mtk_i2c_irq(0, i2c);
+ } while (!i2c->xfer_complete && time_before(jiffies, timeout));
+
+ return i2c->xfer_complete;
+}
+
static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
int num, int left_num)
{
@@ -1015,7 +1032,10 @@ static int mtk_i2c_do_transfer(struct mtk_i2c
*i2c, struct i2c_msg *msgs,
if (i2c->auto_restart)
restart_flag = I2C_RS_TRANSFER;
- reinit_completion(&i2c->msg_complete);
+ if (i2c->atomic_xfer)
+ i2c->xfer_complete = false;
+ else
+ reinit_completion(&i2c->msg_complete);
if (i2c->dev_comp->apdma_sync &&
i2c->op != I2C_MASTER_WRRD && num > 1) {
@@ -1195,8 +1215,12 @@ static int mtk_i2c_do_transfer(struct mtk_i2c
*i2c, struct i2c_msg *msgs,
}
mtk_i2c_writew(i2c, start_reg, OFFSET_START);
- ret = wait_for_completion_timeout(&i2c->msg_complete,
- i2c->adap.timeout);
+ if (i2c->atomic_xfer)
+ /* We can't rely on wait_for_completion* calls in atomic mode. */
+ ret = mtk_i2c_wait_xfer_atomic(i2c);
+ else
+ ret = wait_for_completion_timeout(&i2c->msg_complete,
+ i2c->adap.timeout);
/* Clear interrupt mask */
mtk_i2c_writew(i2c, ~(restart_flag | I2C_HS_NACKERR | I2C_ACKERR |
@@ -1238,8 +1262,8 @@ static int mtk_i2c_do_transfer(struct mtk_i2c
*i2c, struct i2c_msg *msgs,
return 0;
}
-static int mtk_i2c_transfer(struct i2c_adapter *adap,
- struct i2c_msg msgs[], int num)
+static int mtk_i2c_transfer_common(struct i2c_adapter *adap,
+ struct i2c_msg msgs[], int num, bool atomic)
{
int ret;
int left_num = num;
@@ -1249,6 +1273,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
if (ret)
return ret;
+ i2c->atomic_xfer = atomic;
i2c->auto_restart = i2c->dev_comp->auto_restart;
/* checking if we can skip restart and optimize using WRRD mode */
@@ -1303,6 +1328,18 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
return ret;
}
+static int mtk_i2c_transfer(struct i2c_adapter *adap,
+ struct i2c_msg msgs[], int num)
+{
+ return mtk_i2c_transfer_common(adap, msgs, num, false);
+}
+
+static int mtk_i2c_transfer_atomic(struct i2c_adapter *adap,
+ struct i2c_msg msgs[], int num)
+{
+ return mtk_i2c_transfer_common(adap, msgs, num, true);
+}
+
static irqreturn_t mtk_i2c_irq(int irqno, void *dev_id)
{
struct mtk_i2c *i2c = dev_id;
@@ -1328,8 +1365,12 @@ static irqreturn_t mtk_i2c_irq(int irqno, void *dev_id)
mtk_i2c_writew(i2c, I2C_RS_MUL_CNFG | I2C_RS_MUL_TRIG |
I2C_TRANSAC_START, OFFSET_START);
} else {
- if (i2c->irq_stat & (I2C_TRANSAC_COMP | restart_flag))
- complete(&i2c->msg_complete);
+ if (i2c->irq_stat & (I2C_TRANSAC_COMP | restart_flag)) {
+ if (i2c->atomic_xfer)
+ i2c->xfer_complete = true;
+ else
+ complete(&i2c->msg_complete);
+ }
}
return IRQ_HANDLED;
@@ -1346,6 +1387,7 @@ static u32 mtk_i2c_functionality(struct i2c_adapter *adap)
static const struct i2c_algorithm mtk_i2c_algorithm = {
.master_xfer = mtk_i2c_transfer,
+ .master_xfer_atomic = mtk_i2c_transfer_atomic,
.functionality = mtk_i2c_functionality,
};
