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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

+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