linux-kernel - RE: [PATCH] i2c: aspeed: Consider i2c reset for muti-master case

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:
 <TYZPR06MB6191EBA63B87FE5152AF029DE14C2@TYZPR06MB6191.apcprd06.prod.outlook.com>
Date: Tue, 22 Oct 2024 02:42:08 +0000
From: Tommy Huang <tommy_huang@...eedtech.com>
To: Andrew Jeffery <andrew@...econstruct.com.au>, "brendanhiggins@...gle.com"
	<brendanhiggins@...gle.com>, "benh@...nel.crashing.org"
	<benh@...nel.crashing.org>, "joel@....id.au" <joel@....id.au>,
	"andi.shyti@...nel.org" <andi.shyti@...nel.org>
CC: BMC-SW <BMC-SW@...eedtech.com>, "linux-aspeed@...ts.ozlabs.org"
	<linux-aspeed@...ts.ozlabs.org>, "openbmc@...ts.ozlabs.org"
	<openbmc@...ts.ozlabs.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "stable@...r.kernel.org"
	<stable@...r.kernel.org>, "linux-i2c@...r.kernel.org"
	<linux-i2c@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH] i2c: aspeed: Consider i2c reset for muti-master case

Hi Andrew,

	Thanks for your comments.
	I want to fix the situation when our controller is set as target mode and reading / writing by other i2c host.
	However, this host is stopped by any other reason (DC on/off..etc).
	It will cause the controller is stuck in this situation.
	But I find it might not have clear hints to identify this situation is normal or abnormal.
	So, this patch should not be applied into mainstream.

	BR,

	By Tommy

> -----Original Message-----
> From: Andrew Jeffery <andrew@...econstruct.com.au>
> Sent: Monday, October 21, 2024 7:49 PM
> To: Tommy Huang <tommy_huang@...eedtech.com>;
> brendanhiggins@...gle.com; benh@...nel.crashing.org; joel@....id.au;
> andi.shyti@...nel.org
> Cc: BMC-SW <BMC-SW@...eedtech.com>; linux-aspeed@...ts.ozlabs.org;
> openbmc@...ts.ozlabs.org; linux-kernel@...r.kernel.org;
> stable@...r.kernel.org; linux-i2c@...r.kernel.org;
> linux-arm-kernel@...ts.infradead.org
> Subject: Re: [PATCH] i2c: aspeed: Consider i2c reset for muti-master case
> 
> Hi Tommy,
> 
> On Fri, 2024-10-18 at 11:49 +0800, Tommy Huang wrote:
> > In the original code, the device reset would not be triggered when the
> > driver is set to multi-master and bus is free.
> 
> That's not how I read the existing code. As it stands, if it's multi- master and
> busy we do the recovery, however, if it's multi-master and free, or busy but not
> multi-master, or free and not multi-master, then we do the reset.
> 

> > It needs to be considered with multi-master condition.
> 
> Is there a specific circumstance you've found that's problematic? Can you
> provide some more details about that scenario?
> 
> >
> > Fixes: <f327c686d3ba> ("i2c: aspeed: Reset the i2c controller when
> > timeout occurs")
> >
> > Signed-off-by: Tommy Huang <tommy_huang@...eedtech.com>
> > ---
> >  drivers/i2c/busses/i2c-aspeed.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-aspeed.c
> > b/drivers/i2c/busses/i2c-aspeed.c index cc5a26637fd5..7639ae3ace67
> > 100644
> > --- a/drivers/i2c/busses/i2c-aspeed.c
> > +++ b/drivers/i2c/busses/i2c-aspeed.c
> > @@ -716,14 +716,15 @@ static int aspeed_i2c_master_xfer(struct
> i2c_adapter *adap,
> >  	if (time_left == 0) {
> >  		/*
> >  		 * In a multi-master setup, if a timeout occurs, attempt
> > -		 * recovery. But if the bus is idle, we still need to reset the
> > -		 * i2c controller to clear the remaining interrupts.
> > +		 * recovery device. But if the bus is idle,
> > +		 * we still need to reset the i2c controller to clear
> > +		 * the remaining interrupts or reset device abnormal condition.
> >  		 */
> > -		if (bus->multi_master &&
> > -		    (readl(bus->base + ASPEED_I2C_CMD_REG) &
> > -		     ASPEED_I2CD_BUS_BUSY_STS))
> > -			aspeed_i2c_recover_bus(bus);
> > -		else
> > +		if ((readl(bus->base + ASPEED_I2C_CMD_REG) &
> > +			ASPEED_I2CD_BUS_BUSY_STS)){
> > +			if (bus->multi_master)
> > +				aspeed_i2c_recover_bus(bus);
> 
> The change doesn't seem match the commit message. In this case you've
> punched a hole - if the bus is busy but _not_ multi-master, we neither do the
> reset _nor_ the recovery.
> 
> Which is what you intended? The implementation? Or the prose description?
> 
> Now, back to the implementation, punching this hole seems reasonable on the
> surface, but I guess we need to keep in mind that time_left has also expired...
> 
> > +		} else
> >  			aspeed_i2c_reset(bus);
> >
> >  		/*
> 
> Andrew