Received-SPF: pass (google.com: domain of linux-kernel+bounces-14822-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20230327-tegra-pmic-reboot-v7-2-18699d5dcd76@skidata.com> <20240102150350.3180741-1-mwalle@kernel.org>
In-Reply-To: <20240102150350.3180741-1-mwalle@kernel.org>
From: Benjamin Bara <bbara93@gmail.com>
Date: Tue, 2 Jan 2024 22:02:02 +0100
Message-ID: <CAJpcXm7W2vckakdFYiT4jssea-AzrZMsjHijfa+QpfzDVL+E3A@mail.gmail.com>
Subject: Re: [PATCH v7 2/5] Re: i2c: core: run atomic i2c xfer when !preemptible
To: Michael Walle <mwalle@kernel.org>
Cc: benjamin.bara@skidata.com, dmitry.osipenko@collabora.com, 
	jonathanh@nvidia.com, lee@kernel.org, linux-i2c@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org, nm@ti.com, 
	peterz@infradead.org, rafael.j.wysocki@intel.com, richard.leitner@linux.dev, 
	stable@vger.kernel.org, treding@nvidia.com, wsa+renesas@sang-engineering.com, 
	wsa@kernel.org
Content-Type: text/plain; charset="UTF-8"

Hi Michael,

On Tue, 2 Jan 2024 at 16:03, Michael Walle <mwalle@kernel.org> 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@mailbox.org/


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,
 };