Re: [PATCH v3 2/5] i2c: mt7621: clear pending interrupt on i2c reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch mistakenly writes to the Interrupt Enable register (`REG_PINTEN_REG`) instead of the Interrupt Clear register (`REG_PINTCL_REG`), which fails to clear the interrupt and can trigger an unhandled interrupt storm.

Pre-existing issues:
- [High] The function `mtk_i2c_reset()` calls `device_reset()`, which may sleep, but it can be called from an atomic context.
--

commit d97627074d52407e96e5474c13276e0a4ad61014
Author: Christian Marangi <ansuelsmth@gmail.com>

i2c: mt7621: clear pending interrupt on i2c reset

This commit attempts to clear pending interrupts during an I2C bus reset
to ensure a consistent state for subsequent operations.

> diff --git a/drivers/i2c/busses/i2c-mt7621.c b/drivers/i2c/busses/i2c-mt7621.c
> index 700beb9e7b1a8..d8fa29e7e0fa3 100644
> --- a/drivers/i2c/busses/i2c-mt7621.c
> +++ b/drivers/i2c/busses/i2c-mt7621.c
> @@ -101,6 +101,8 @@ static void mtk_i2c_reset(struct mtk_i2c *i2c)

[Severity: High]
This is a pre-existing issue, but can this function be safely called from an
atomic context?

Earlier in this patch series, mtk_i2c_xfer_atomic() was introduced, which
can trigger a reset from an atomic context on transfer timeout:

mtk_i2c_xfer_atomic()
  mtk_i2c_xfer_common(..., atomic = true)
    mtk_i2c_reset()
      device_reset()
        __fwnode_reset_control_get()

The __fwnode_reset_control_get() function attempts to acquire a mutex.
Acquiring a mutex in an atomic context is a lock context violation that
can lead to a kernel panic.

>  	iowrite32(((i2c->clk_div << 16) & SM0CTL0_CLK_DIV_MASK) | SM0CTL0_EN |
>  		  SM0CTL0_SCL_STRETCH, i2c->base + REG_SM0CTL0_REG);
>  	iowrite32(0, i2c->base + REG_SM0CFG2_REG);
> +	/* Clear any pending interrupt */
> +	iowrite32(1, i2c->base + REG_PINTEN_REG);

[Severity: High]
Does this code inadvertently unmask the interrupt instead of clearing it?

The comment indicates the intent is to clear pending interrupts, but
REG_PINTEN_REG is the interrupt enable register, whereas REG_PINTCL_REG is
typically used to clear interrupts.

Writing to the enable register unmasks the interrupt at the hardware block
level. Since the driver operates entirely in polling mode and does not
register an IRQ handler, asserting the hardware IRQ line can lead to an
unhandled interrupt storm. If the interrupt line is shared and unmasked at
the GIC, the spurious interrupt detector might eventually disable the shared
IRQ line, potentially causing a denial of service for other devices.

Additionally, writing 1 only targets bit 0. To safely clear all pending
interrupts, should a full bitmask be written to the clear register instead?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519223253.1093-1-ansuelsmth@gmail.com?part=2