linux-kernel - Re: [PATCH v2] i2c: omap: fix IRQ storms

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250311123947.jce4i5heeufzwmji@serotonin>
Date: Tue, 11 Mar 2025 07:39:47 -0500
From: Nishanth Menon <nm@...com>
To: Andreas Kemnade <andreas@...nade.info>
CC: <vigneshr@...com>, <aaro.koskinen@....fi>, <khilman@...libre.com>,
        <rogerq@...nel.org>, <tony@...mide.com>, <jmkrzyszt@...il.com>,
        <andi.shyti@...nel.org>, <reidt@...com>, <wsa@...nel.org>,
        <linux-omap@...r.kernel.org>, <linux-i2c@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <stable@...nel.org>
Subject: Re: [PATCH v2] i2c: omap: fix IRQ storms

On 15:04-20250228, Andreas Kemnade wrote:
> On the GTA04A5 writing a reset command to the gyroscope causes IRQ
> storms because NACK IRQs are enabled and therefore triggered but not
> acked.
> 
> Sending a reset command to the gyroscope by
> i2cset 1 0x69 0x14 0xb6
> with an additional debug print in the ISR (not the thread) itself
> causes
> 
> [ 363.353515] i2c i2c-1: ioctl, cmd=0x720, arg=0xbe801b00
> [ 363.359039] omap_i2c 48072000.i2c: addr: 0x0069, len: 2, flags: 0x0, stop: 1
> [ 363.366180] omap_i2c 48072000.i2c: IRQ LL (ISR = 0x1110)
> [ 363.371673] omap_i2c 48072000.i2c: IRQ (ISR = 0x0010)
> [ 363.376892] omap_i2c 48072000.i2c: IRQ LL (ISR = 0x0102)
> [ 363.382263] omap_i2c 48072000.i2c: IRQ LL (ISR = 0x0102)
> [ 363.387664] omap_i2c 48072000.i2c: IRQ LL (ISR = 0x0102)
> repeating till infinity
> [...]
> (0x2 = NACK, 0x100 = Bus free, which is not enabled)
> Apparently no other IRQ bit gets set, so this stalls.
> 
> Do not ignore enabled interrupts and make sure they are acked.
> If the NACK IRQ is not needed, it should simply not enabled, but
> according to the above log, caring about it is necessary unless
> the Bus free IRQ is enabled and handled. The assumption that is
> will always come with a ARDY IRQ, which was the idea behind
> ignoring it, proves wrong.
> It is true for simple reads from an unused address.
> 
> To still avoid the i2cdetect trouble which is the reason for
> commit c770657bd261 ("i2c: omap: Fix standard mode false ACK readings"),
> avoid doing much about NACK in omap_i2c_xfer_data() which is used
> by both IRQ mode and polling mode, so also the false detection fix
> is extended to polling usage and IRQ storms are avoided.
> 
> By changing this, the hardirq handler is not needed anymore to filter
> stuff.
> 
> The mentioned gyro reset now just causes a -ETIMEDOUT instead of
> hanging the system.
> 
> Fixes: c770657bd261 ("i2c: omap: Fix standard mode false ACK readings").
> CC: <stable@...nel.org>
> Signed-off-by: Andreas Kemnade <andreas@...nade.info>
> ---
> This needs at least to be tested on systems where false acks were
> detected.

At least on BeaglePlay, I have not been able to reproduce the original
bug which was the trigger for commit c770657bd261

I also ran basic boot tests on other K3 platforms and none seem to show
regressions at the very least.

Tested-by: Nishanth Menon <nm@...com>

> 
>  drivers/i2c/busses/i2c-omap.c | 26 +++++++-------------------
>  1 file changed, 7 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 92faf03d64cf..f18c3e74b076 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1048,23 +1048,6 @@ static int omap_i2c_transmit_data(struct omap_i2c_dev *omap, u8 num_bytes,
>  	return 0;
>  }
>  
> -static irqreturn_t
> -omap_i2c_isr(int irq, void *dev_id)
> -{
> -	struct omap_i2c_dev *omap = dev_id;
> -	irqreturn_t ret = IRQ_HANDLED;
> -	u16 mask;
> -	u16 stat;
> -
> -	stat = omap_i2c_read_reg(omap, OMAP_I2C_STAT_REG);
> -	mask = omap_i2c_read_reg(omap, OMAP_I2C_IE_REG) & ~OMAP_I2C_STAT_NACK;
> -
> -	if (stat & mask)
> -		ret = IRQ_WAKE_THREAD;
> -
> -	return ret;
> -}
> -
>  static int omap_i2c_xfer_data(struct omap_i2c_dev *omap)
>  {
>  	u16 bits;
> @@ -1095,8 +1078,13 @@ static int omap_i2c_xfer_data(struct omap_i2c_dev *omap)
>  		}
>  
>  		if (stat & OMAP_I2C_STAT_NACK) {
> -			err |= OMAP_I2C_STAT_NACK;
> +			omap->cmd_err |= OMAP_I2C_STAT_NACK;
>  			omap_i2c_ack_stat(omap, OMAP_I2C_STAT_NACK);
> +
> +			if (!(stat & ~OMAP_I2C_STAT_NACK)) {
> +				err = -EAGAIN;
> +				break;
> +			}
>  		}
>  
>  		if (stat & OMAP_I2C_STAT_AL) {
> @@ -1472,7 +1460,7 @@ omap_i2c_probe(struct platform_device *pdev)
>  				IRQF_NO_SUSPEND, pdev->name, omap);
>  	else
>  		r = devm_request_threaded_irq(&pdev->dev, omap->irq,
> -				omap_i2c_isr, omap_i2c_isr_thread,
> +				NULL, omap_i2c_isr_thread,
>  				IRQF_NO_SUSPEND | IRQF_ONESHOT,
>  				pdev->name, omap);
>  
> -- 
> 2.39.5
> 
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D