linux-kernel - RE: [PATCH] drivers:i2c:add controller reset when the timeout occurred

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] [day] [month] [year] [list]

Message-ID: <KL1PR0601MB5614646277B7E656535799B2E1E9A@KL1PR0601MB5614.apcprd06.prod.outlook.com>
Date:   Mon, 4 Sep 2023 00:14:31 +0000
From:   Tommy Huang <tommy_huang@...eedtech.com>
To:     Andi Shyti <andi.shyti@...nel.org>
CC:     "brendan.higgins@...ux.dev" <brendan.higgins@...ux.dev>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>,
        "benh@...nel.crashing.org" <benh@...nel.crashing.org>,
        "joel@....id.au" <joel@....id.au>,
        "andrew@...id.au" <andrew@...id.au>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        BMC-SW <BMC-SW@...eedtech.com>,
        Jae Hyun Yoo <jae.hyun.yoo@...ux.intel.com>
Subject: RE: [PATCH] drivers:i2c:add controller reset when the timeout
 occurred

Hi Andi,

	Thanks for your review.
	I will fix the patch with your comment and resend it.

	BR,

	by Tommy

> -----Original Message-----
> From: Andi Shyti <andi.shyti@...nel.org>
> Sent: Sunday, September 3, 2023 10:15 PM
> To: Tommy Huang <tommy_huang@...eedtech.com>
> Cc: brendan.higgins@...ux.dev; p.zabel@...gutronix.de;
> linux-i2c@...r.kernel.org; openbmc@...ts.ozlabs.org;
> benh@...nel.crashing.org; joel@....id.au; andrew@...id.au;
> linux-arm-kernel@...ts.infradead.org; linux-aspeed@...ts.ozlabs.org;
> linux-kernel@...r.kernel.org; BMC-SW <BMC-SW@...eedtech.com>; Jae
> Hyun Yoo <jae.hyun.yoo@...ux.intel.com>
> Subject: Re: [PATCH] drivers:i2c:add controller reset when the timeout
> occurred
> 
> Hi Tommy,
> 
> Please fix the title of the patch from:
> 
>    drivers:i2c:add controller reset when the timeout occurred
> 
> to something like:
> 
>    i2c: aspeed: Reset the controller when timeout occurs
> 
> Note:
> 
>  - leave a space after the ':'
>  - start with a capital letter after the last ':'
> 
> On Mon, Aug 14, 2023 at 07:15:34PM +0800, Tommy Huang wrote:
> > 1.Call i2c controller reset when the i2c transfer timeout occurred.
> > The rest of interrupts and device should be reset avoid unperdicted
> > controller behavior occurred.
> 
> Please remove the '1.' and please rewrite this sentence in order to be
> grammatically correct, something like:
> 
> "Call the i2c controller reset when an i2c transfer timeout occurs. The
> remaining interrupts and the device should be reset to avoid unpredictable
> controller behavior."
> 
> > Signed-off-by: Tommy Huang <tommy_huang@...eedtech.com>
> 
> Is this a fix? If so please add:
> 
> Fixes: 2e57b7cebb98 ("i2c: aspeed: Add multi-master use case support")
> Cc: Jae Hyun Yoo <jae.hyun.yoo@...ux.intel.com>
> Cc: <stable@...r.kernel.org> # v5.1+
> 
> Jae Hyun is the author of the line you are changing therefore he needs to be
> Cc'ed
> 
> [...]
> 
> >  		/*
> >  		 * If timed out and bus is still busy in a multi master
> > -		 * environment, attempt recovery at here.
> > +		 * environment, attempt recovery at here. Even the bus is
> > +		 * idle, we still need reset i2c controller avoid rest of
> > +		 * interrupts.
> 
> Please fix the grammar here, as well
> 
>    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.
> 
> We take this chance to improve the previous comment, as well.
> 
> >  		 */
> >  		if (bus->multi_master &&
> >  		    (readl(bus->base + ASPEED_I2C_CMD_REG) &
> >  		     ASPEED_I2CD_BUS_BUSY_STS))
> >  			aspeed_i2c_recover_bus(bus);
> > +		else
> > +			aspeed_i2c_reset(bus);
> 
> I'd like also someone from Jae Hyun, Brendan, Benjamin or Joel to take a look
> here, as well. Thanks!
> 
> Andi