[RFC PATCH] i2c: designware: add atomic transfer support for IRQ-off contexts

[RFC PATCH] i2c: designware: add atomic transfer support for IRQ-off contexts

[Baineng Shou]

The DesignWare I2C controller driver cannot perform transfers when
IRQs are disabled, e.g. during noirq system resume where an I2C
client (GPIO expander, PMIC) must be accessed before IRQs are
re-enabled: the interrupt-driven path calls wait_for_completion_timeout()
which deadlocks with IRQs off.

The i2c core already routes to master_xfer_atomic() when
i2c_in_atomic_xfer_mode() is true, but that gate requires
system_state > SYSTEM_RUNNING, which does not hold during resume_noirq
(system_state is already SYSTEM_RUNNING there).  So the framework
atomic path does not cover resume_noirq either, and designware does
not implement master_xfer_atomic at all.

Implement i2c_dw_xfer_atomic() and register it as ->xfer_atomic.
It reuses the existing i2c_dw_process_transfer() state machine (the
TX/RX/STOP/ABRT handling is identical to the interrupt path) but:

  - drives the clock directly via i2c_dw_prepare_clk() instead of
    pm_runtime (which may sleep),
  - sets ACCESS_POLLING for the duration of the transfer so register
    reads use IC_RAW_INTR_STAT and the hardware interrupt is masked,
  - polls with udelay() + a retry count instead of usleep_range() +
    jiffies, neither of which is safe with IRQs disabled (the tick is
    frozen so jiffies does not advance, and usleep_range() may sleep).

Also fall back to i2c_dw_xfer_atomic() from i2c_dw_xfer() when IRQs
are disabled, since i2c_in_atomic_xfer_mode() does not cover
resume_noirq.

This is an RFC: the resume_noirq coverage relies on the driver-side
irqs_disabled() fallback because the framework gate is closed there.
I'd like feedback on whether that fallback is acceptable or whether
the gate should be widened in the i2c core instead.

Signed-off-by: Baineng Shou <shoubaineng@gmail.com>
---
 drivers/i2c/busses/i2c-designware-common.c |   1 +
 drivers/i2c/busses/i2c-designware-core.h   |   1 +
 drivers/i2c/busses/i2c-designware-master.c | 109 +++++++++++++++++++++
 3 files changed, 111 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index e4dfa2ec58bb..561efcd39c7f 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -873,6 +873,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 
 static const struct i2c_algorithm i2c_dw_algo = {
 	.xfer = i2c_dw_xfer,
+	.xfer_atomic = i2c_dw_xfer_atomic,
 	.functionality = i2c_dw_func,
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	.reg_slave = i2c_dw_reg_slave,
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index c71aa2dd368d..4bc22b53d470 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -398,6 +398,7 @@ extern void i2c_dw_configure_master(struct dw_i2c_dev *dev);
 extern int i2c_dw_probe_master(struct dw_i2c_dev *dev);
 
 int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num);
+int i2c_dw_xfer_atomic(struct i2c_adapter *adap, struct i2c_msg *msgs, int num);
 
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 extern void i2c_dw_configure_slave(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index 7a301c8b604e..9a94abf86233 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -918,6 +918,104 @@ i2c_dw_xfer_common(struct dw_i2c_dev *dev, struct i2c_msg msgs[], int num)
 	return num;
 }
 
+/* Poll up to ~1s in 10us steps; bounded fallback for the IRQ-off path. */
+#define I2C_DESIGNWARE_ATOMIC_POLL_RETRIES	100000
+
+/*
+ * Atomic-context variant of i2c_dw_wait_transfer().  The normal polling
+ * path (ACCESS_POLLING branch in i2c_dw_wait_transfer()) uses usleep_range()
+ * and a jiffies deadline, neither of which is safe when IRQs are disabled
+ * (noirq system resume, shutdown): the tick is frozen so jiffies does not
+ * advance, and usleep_range() may sleep.  Poll IC_RAW_INTR_STAT with
+ * udelay() and a retry count instead, while reusing the shared
+ * i2c_dw_process_transfer() state machine so TX/RX/STOP/ABRT handling is
+ * identical to the interrupt path.  Caller must have set ACCESS_POLLING.
+ */
+static int i2c_dw_wait_transfer_atomic(struct dw_i2c_dev *dev)
+{
+	unsigned int stat;
+	int retries = I2C_DESIGNWARE_ATOMIC_POLL_RETRIES;
+
+	do {
+		if (try_wait_for_completion(&dev->cmd_complete))
+			return 0;
+
+		stat = i2c_dw_read_clear_intrbits(dev);
+		if (stat)
+			i2c_dw_process_transfer(dev, stat);
+		else
+			udelay(10);
+	} while (--retries > 0);
+
+	return -ETIMEDOUT;
+}
+
+/*
+ * i2c_dw_xfer_atomic - transfer messages in atomic context.
+ *
+ * Used when IRQs are disabled, e.g. during noirq system resume where an
+ * I2C client (GPIO expander, PMIC) must be accessed before IRQs are
+ * re-enabled.  pm_runtime and mutexes may sleep, so drive the clock
+ * directly via i2c_dw_prepare_clk(); ACCESS_POLLING makes register reads
+ * use IC_RAW_INTR_STAT and routes the wait through
+ * i2c_dw_wait_transfer_atomic().
+ */
+int
+i2c_dw_xfer_atomic(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+	unsigned int flags = dev->flags;
+	int ret;
+
+	dev->flags |= ACCESS_POLLING;
+
+	ret = i2c_dw_prepare_clk(dev, true);
+	if (ret)
+		goto out_flags;
+
+	ret = i2c_dw_acquire_lock(dev);
+	if (ret)
+		goto out_clk;
+
+	reinit_completion(&dev->cmd_complete);
+	dev->msgs = msgs;
+	dev->msgs_num = num;
+	dev->cmd_err = 0;
+	dev->msg_write_idx = 0;
+	dev->msg_read_idx = 0;
+	dev->msg_err = 0;
+	dev->status = 0;
+	dev->abort_source = 0;
+	dev->rx_outstanding = 0;
+
+	i2c_dw_xfer_init(dev);
+
+	ret = i2c_dw_wait_transfer_atomic(dev);
+
+	if (i2c_dw_is_controller_active(dev)) {
+		i2c_recover_bus(&dev->adapter);
+		i2c_dw_init(dev);
+	} else {
+		__i2c_dw_disable_nowait(dev);
+	}
+
+	if (!ret) {
+		if (likely(!dev->cmd_err && !dev->status))
+			ret = 0;
+		else if (dev->cmd_err == DW_IC_ERR_TX_ABRT)
+			ret = i2c_dw_handle_tx_abort(dev);
+		else
+			ret = -EIO;
+	}
+
+	i2c_dw_release_lock(dev);
+out_clk:
+	i2c_dw_prepare_clk(dev, false);
+out_flags:
+	dev->flags = flags;
+	return ret < 0 ? ret : num;
+}
+
 int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
 	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
@@ -925,6 +1023,17 @@ int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU)
 		return amd_i2c_dw_xfer_quirk(dev, msgs, num);
 
+	/*
+	 * Fall back to the atomic path when IRQs are disabled, e.g. during
+	 * noirq system resume where an I2C client (GPIO expander, PMIC)
+	 * must be accessed before IRQs are re-enabled.  The i2c core's
+	 * i2c_in_atomic_xfer_mode() gate does not cover resume_noirq
+	 * (system_state is already SYSTEM_RUNNING there), so the driver has
+	 * to route the transfer itself.
+	 */
+	if (IS_ENABLED(CONFIG_PREEMPT_COUNT) ? !preemptible() : irqs_disabled())
+		return i2c_dw_xfer_atomic(adap, msgs, num);
+
 	return i2c_dw_xfer_common(dev, msgs, num);
 }
 
-- 
2.34.1

Re: [RFC PATCH] i2c: designware: add atomic transfer support for IRQ-off contexts

[Andy Shevchenko]

+Cc: @bigeasy (the question below)

On Fri, Aug 07, 2026 at 07:33:33PM +0800, Baineng Shou wrote:
> The DesignWare I2C controller driver cannot perform transfers when
> IRQs are disabled, e.g. during noirq system resume where an I2C
> client (GPIO expander, PMIC) must be accessed before IRQs are
> re-enabled: the interrupt-driven path calls wait_for_completion_timeout()
> which deadlocks with IRQs off.
> 
> The i2c core already routes to master_xfer_atomic() when
> i2c_in_atomic_xfer_mode() is true, but that gate requires
> system_state > SYSTEM_RUNNING, which does not hold during resume_noirq
> (system_state is already SYSTEM_RUNNING there).  So the framework
> atomic path does not cover resume_noirq either, and designware does
> not implement master_xfer_atomic at all.
> 
> Implement i2c_dw_xfer_atomic() and register it as ->xfer_atomic.
> It reuses the existing i2c_dw_process_transfer() state machine (the
> TX/RX/STOP/ABRT handling is identical to the interrupt path) but:
> 
>   - drives the clock directly via i2c_dw_prepare_clk() instead of
>     pm_runtime (which may sleep),
>   - sets ACCESS_POLLING for the duration of the transfer so register
>     reads use IC_RAW_INTR_STAT and the hardware interrupt is masked,
>   - polls with udelay() + a retry count instead of usleep_range() +
>     jiffies, neither of which is safe with IRQs disabled (the tick is
>     frozen so jiffies does not advance, and usleep_range() may sleep).
> 
> Also fall back to i2c_dw_xfer_atomic() from i2c_dw_xfer() when IRQs
> are disabled, since i2c_in_atomic_xfer_mode() does not cover
> resume_noirq.

The below paragraph should go...

> This is an RFC: the resume_noirq coverage relies on the driver-side
> irqs_disabled() fallback because the framework gate is closed there.
> I'd like feedback on whether that fallback is acceptable or whether
> the gate should be widened in the i2c core instead.
> 
> Signed-off-by: Baineng Shou <shoubaineng@gmail.com>
> ---

...here as a comment / question.

> +/* Poll up to ~1s in 10us steps; bounded fallback for the IRQ-off path. */
> +#define I2C_DESIGNWARE_ATOMIC_POLL_RETRIES	100000

Just add a constant for the step time, this will give some clarification to the
above comment.

#define I2C_DESIGNWARE_ATOMIC_POLL_STEP_US	10

> +/*
> + * Atomic-context variant of i2c_dw_wait_transfer().  The normal polling
> + * path (ACCESS_POLLING branch in i2c_dw_wait_transfer()) uses usleep_range()
> + * and a jiffies deadline, neither of which is safe when IRQs are disabled
> + * (noirq system resume, shutdown): the tick is frozen so jiffies does not
> + * advance, and usleep_range() may sleep.  Poll IC_RAW_INTR_STAT with
> + * udelay() and a retry count instead, while reusing the shared
> + * i2c_dw_process_transfer() state machine so TX/RX/STOP/ABRT handling is
> + * identical to the interrupt path.  Caller must have set ACCESS_POLLING.
> + */
> +static int i2c_dw_wait_transfer_atomic(struct dw_i2c_dev *dev)
> +{
> +	unsigned int stat;
> +	int retries = I2C_DESIGNWARE_ATOMIC_POLL_RETRIES;
> +
> +	do {
> +		if (try_wait_for_completion(&dev->cmd_complete))
> +			return 0;
> +
> +		stat = i2c_dw_read_clear_intrbits(dev);
> +		if (stat)
> +			i2c_dw_process_transfer(dev, stat);
> +		else
> +			udelay(10);
> +	} while (--retries > 0);

' > 0' is redundant and gives actually off-by-one (the amount of retries).

> +	return -ETIMEDOUT;
> +}
> +
> +/*
> + * i2c_dw_xfer_atomic - transfer messages in atomic context.
> + *
> + * Used when IRQs are disabled, e.g. during noirq system resume where an
> + * I2C client (GPIO expander, PMIC) must be accessed before IRQs are
> + * re-enabled.  pm_runtime and mutexes may sleep, so drive the clock
> + * directly via i2c_dw_prepare_clk(); ACCESS_POLLING makes register reads
> + * use IC_RAW_INTR_STAT and routes the wait through
> + * i2c_dw_wait_transfer_atomic().
> + */

> +int
> +i2c_dw_xfer_atomic(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)

Make it a single line.

> +{
> +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> +	unsigned int flags = dev->flags;
> +	int ret;
> +
> +	dev->flags |= ACCESS_POLLING;
> +
> +	ret = i2c_dw_prepare_clk(dev, true);
> +	if (ret)
> +		goto out_flags;
> +
> +	ret = i2c_dw_acquire_lock(dev);
> +	if (ret)
> +		goto out_clk;
> +
> +	reinit_completion(&dev->cmd_complete);
> +	dev->msgs = msgs;
> +	dev->msgs_num = num;
> +	dev->cmd_err = 0;
> +	dev->msg_write_idx = 0;
> +	dev->msg_read_idx = 0;
> +	dev->msg_err = 0;
> +	dev->status = 0;
> +	dev->abort_source = 0;
> +	dev->rx_outstanding = 0;
> +
> +	i2c_dw_xfer_init(dev);
> +
> +	ret = i2c_dw_wait_transfer_atomic(dev);
> +
> +	if (i2c_dw_is_controller_active(dev)) {
> +		i2c_recover_bus(&dev->adapter);
> +		i2c_dw_init(dev);
> +	} else {
> +		__i2c_dw_disable_nowait(dev);
> +	}

> +	if (!ret) {

Use traditional pattern, id est

	if (ret)
		goto out_release_lock;

> +		if (likely(!dev->cmd_err && !dev->status))
> +			ret = 0;

		goto out_release_lock;

> +		else if (dev->cmd_err == DW_IC_ERR_TX_ABRT)
> +			ret = i2c_dw_handle_tx_abort(dev);
> +		else
> +			ret = -EIO;
> +	}
> +
> +	i2c_dw_release_lock(dev);
> +out_clk:
> +	i2c_dw_prepare_clk(dev, false);
> +out_flags:
> +	dev->flags = flags;
> +	return ret < 0 ? ret : num;
> +}

...

> +	/*
> +	 * Fall back to the atomic path when IRQs are disabled, e.g. during
> +	 * noirq system resume where an I2C client (GPIO expander, PMIC)
> +	 * must be accessed before IRQs are re-enabled.  The i2c core's
> +	 * i2c_in_atomic_xfer_mode() gate does not cover resume_noirq
> +	 * (system_state is already SYSTEM_RUNNING there), so the driver has
> +	 * to route the transfer itself.
> +	 */
> +	if (IS_ENABLED(CONFIG_PREEMPT_COUNT) ? !preemptible() : irqs_disabled())

I don't like this. Do we have something better for this?
Perhaps @bigeasy knows?

> +		return i2c_dw_xfer_atomic(adap, msgs, num);

-- 
With Best Regards,
Andy Shevchenko