Re: [PATCH v3 4/5] i2c: mt7621: limit SCL_STRETCH only to Mediatek SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dynamic evaluation of `device_is_compatible()` in `mtk_i2c_reset()` causes a sleep-in-atomic bug.
- [High] The hardware reset routine acquires a mutex while executing in atomic context.
--

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

i2c: mt7621: limit SCL_STRETCH only to Mediatek SoC

This commit introduces a new compatible string for Airoha SoCs and limits
the SCL_STRETCH feature to Mediatek devices. It enforces this by checking
the compatible string during the I2C reset routine.

> diff --git a/drivers/i2c/busses/i2c-mt7621.c b/drivers/i2c/busses/i2c-mt7621.c
> index d8fa29e7e0fa3..3cde43c57a2bc 100644
> --- a/drivers/i2c/busses/i2c-mt7621.c
> +++ b/drivers/i2c/busses/i2c-mt7621.c
> @@ -88,6 +88,7 @@ static int mtk_i2c_wait_idle(struct mtk_i2c *i2c, bool atomic)
>  
>  static void mtk_i2c_reset(struct mtk_i2c *i2c)
>  {
> +	u32 reg;
>  	int ret;
>  
>  	ret = device_reset(i2c->adap.dev.parent);

[Severity: High]
Does this device_reset() call sleep in atomic context?

When an I2C transfer times out during mtk_i2c_xfer_atomic(), the driver
branches to err_timeout and calls mtk_i2c_reset(). This reset routine calls
device_reset(), which eventually invokes __reset_control_get(). That
function unconditionally acquires the reset_list_mutex.

Acquiring a mutex in atomic context (such as during panic or reboot, when
mtk_i2c_xfer_atomic() is used) triggers a 'schedule while atomic' bug and
causes a kernel panic. Commit "i2c: mt7621: rework cmd/wait OPs to support
atomic afer variant" (Patch 1 of this series) directly exposed this sleeping
reset path to atomic execution contexts.

>  	if (ret)
>  		dev_err(i2c->dev, "I2C reset failed!\n");
>  
> @@ -98,8 +99,12 @@ static void mtk_i2c_reset(struct mtk_i2c *i2c)
>  	 * Don't set SM0CTL0_ODRAIN as its bit meaning is inverted. To
>  	 * configure open-drain mode, this bit needs to be cleared.
>  	 */
> -	iowrite32(((i2c->clk_div << 16) & SM0CTL0_CLK_DIV_MASK) | SM0CTL0_EN |
> -		  SM0CTL0_SCL_STRETCH, i2c->base + REG_SM0CTL0_REG);
> +	reg = ((i2c->clk_div << 16) & SM0CTL0_CLK_DIV_MASK) | SM0CTL0_EN;
> +	/* Set SCL_STRETCH only for Mediatek SoC */
> +	if (device_is_compatible(i2c->dev, "mediatek,mt7621-i2c"))

[Severity: High]
Does evaluating device_is_compatible() here cause a sleep-in-atomic bug?

The mtk_i2c_reset() function is called from the I2C transfer timeout handler,
which runs in an atomic context when the driver executes mtk_i2c_xfer_atomic().

The device_is_compatible() function internally invokes
fwnode_property_match_string(), which unconditionally allocates memory using
kcalloc() with GFP_KERNEL. Allocating memory with GFP_KERNEL in atomic
context blocks and triggers a 'schedule while atomic' kernel panic.

Should the driver parse the compatible properties once during probe() and
cache the configuration (e.g., as a has_scl_stretch boolean) in struct mtk_i2c
instead?

[ ... ]

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