linux-kernel - Re: [PATCH 2/6] i2c: spacemit: remove stop function to avoid bus error

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: <B8E4D8DD9B16E81B+aNHtcoS_a4sGChZ7@LT-Guozexi>
Date: Tue, 23 Sep 2025 08:44:34 +0800
From: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
To: Troy Mitchell <troy.mitchell@...ux.spacemit.com>,
	Andi Shyti <andi.shyti@...nel.org>, Yixun Lan <dlan@...too.org>,
	Alex Elder <elder@...cstar.com>,
	Troy Mitchell <troymitchell988@...il.com>,
	linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-riscv@...ts.infradead.org, spacemit@...ts.linux.dev
Subject: Re: [PATCH 2/6] i2c: spacemit: remove stop function to avoid bus
 error

On Mon, Sep 22, 2025 at 10:55:57PM +0200, Aurelien Jarno wrote:
> On 2025-08-27 15:39, Troy Mitchell wrote:
> > Previously, STOP handling was split into two separate steps:
> >   1) clear TB/STOP/START/ACK bits
> >   2) issue STOP by calling spacemit_i2c_stop()
> > 
> > This left a small window where the control register was updated
> > twice, which can confuse the controller. While this race has not
> > been observed with interrupt-driven transfers, it reliably causes
> > bus errors in PIO mode.
> > 
> > Inline the STOP sequence into the IRQ handler and ensure that
> > control register bits are updated atomically in a single writel().
> > 
> > Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
> > Signed-off-by: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
> > ---
> >  drivers/i2c/busses/i2c-k1.c | 28 +++++++++-------------------
> >  1 file changed, 9 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> > index ee08811f4087c8e709d25dd314854ed643cc5a47..d342752030d077953adf84a2886211de96e843c4 100644
> > --- a/drivers/i2c/busses/i2c-k1.c
> > +++ b/drivers/i2c/busses/i2c-k1.c
> > @@ -429,14 +415,18 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
> >  	}
> >  
> >  	if (i2c->state != SPACEMIT_STATE_IDLE) {
> > +		val |= SPACEMIT_CR_TB;
> > +		if (i2c->is_pio)
> > +			val |= SPACEMIT_CR_ALDIE;
> > +
> 
> This needs to be moved to the last patch introducing PIO support.
Nice catch!
I'll fix it and send v2 today.
> 
> >  		if (spacemit_i2c_is_last_msg(i2c)) {
> >  			/* trigger next byte with stop */
> > -			spacemit_i2c_stop(i2c);
> > -		} else {
> > -			/* trigger next byte */
> > -			val |= SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB;
> > -			writel(val, i2c->base + SPACEMIT_ICR);
> > +			val |= SPACEMIT_CR_STOP;
> > +
> > +			if (i2c->read)
> > +				val |= SPACEMIT_CR_ACKNAK;
> >  		}
> > +		writel(val, i2c->base + SPACEMIT_ICR);
> >  	}
> >  
> >  err_out:
> 
> Otherwise sounds good.
> 
> -- 
> Aurelien Jarno                          GPG: 4096R/1DDD8C9B
> aurelien@...el32.net                     http://aurel32.net
>