linux-kernel - Re: [PATCH] i2c: aspeed: Reset the i2c controller when timeout occurs

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: <20230905231915.rv2ysvxa3bshekjq@zenone.zhora.eu>
Date:   Wed, 6 Sep 2023 01:19:15 +0200
From:   Andi Shyti <andi.shyti@...nel.org>
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, jae.hyun.yoo@...ux.intel.com,
        linux-kernel@...r.kernel.org, BMC-SW@...eedtech.com,
        stable@...r.kernel.org
Subject: Re: [PATCH] i2c: aspeed: Reset the i2c controller when timeout occurs

Hi Tommy,

Thanks for fixing what I asked, I'm going to review here:

Reviewed-by: Andi Shyti <andi.shyti@...nel.org> 

But still there are 3 things I will ask you for your next
patches:

  1. Please add a version to the patch, this is patch version 2,
     so that the title should be "[PATCH v2] i2c...."

     You can have that with git-format-patch:

        git format-patch -v 2 ....

On Mon, Sep 04, 2023 at 09:25:05AM +0800, Tommy Huang wrote:
> Reset the i2c controller when an i2c transfer timeout occurs.
> The remaining interrupts and device should be reset to avoid
> unpredictable controller behavior.
> 
> 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+
> 
> Signed-off-by: Tommy Huang <tommy_huang@...eedtech.com>

  2. Don't leave blank lines between tags. But I think Wolfram
     can fix this before pushing.

> ---

  3. After the '---' it comes the freedom of speach section where
     you can write anything. Please add the changelog, something
     like:

       v1 -> v2
        - Fixed comment...
	- Fixed commit log...
	- etc.

     This is important for reviewers in order to understand what
     has changed from patch v1 to patch v2.

Please, before your next patch, read first the
Documentation/process/submitting-patches.rst document.

Thanks,
Andi

>  drivers/i2c/busses/i2c-aspeed.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 2e5acfeb76c8..5a416b39b818 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -698,13 +698,16 @@ static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>  
>  	if (time_left == 0) {
>  		/*
> -		 * If timed out and bus is still busy in a multi master
> -		 * environment, attempt recovery at here.
> +		 * 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.
>  		 */
>  		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);
>  
>  		/*
>  		 * If timed out and the state is still pending, drop the pending
> -- 
> 2.25.1
>