Received-SPF: pass (google.com: domain of linux-kernel+bounces-75753-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Tommy Huang <tommy_huang@aspeedtech.com>
To: Andi Shyti <andi.shyti@kernel.org>
CC: "brendan.higgins@linux.dev" <brendan.higgins@linux.dev>,
	"p.zabel@pengutronix.de" <p.zabel@pengutronix.de>,
	"linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
	"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
	"benh@kernel.crashing.org" <benh@kernel.crashing.org>, "joel@jms.id.au"
	<joel@jms.id.au>, "andrew@aj.id.au" <andrew@aj.id.au>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>, "linux-aspeed@lists.ozlabs.org"
	<linux-aspeed@lists.ozlabs.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, BMC-SW <BMC-SW@aspeedtech.com>
Subject: RE: [PATCH] i2c: aspeed: Fix the dummy irq expected print
Thread-Topic: [PATCH] i2c: aspeed: Fix the dummy irq expected print
Thread-Index: AQHaYNBg0EEa3X7oHECRXZPcHJI3VbEVVFwAgAA4R6A=
Date: Thu, 22 Feb 2024 01:10:39 +0000
Message-ID:
 <TYZPR06MB61911F076C8719C6A7D57B97E1562@TYZPR06MB6191.apcprd06.prod.outlook.com>
References: <20240216120455.4138642-1-tommy_huang@aspeedtech.com>
 <nbkkaktcozbhly44hii3zwie7ivsra3qxzdibyzhyhooxrudvb@zik6skmkki2c>
In-Reply-To: <nbkkaktcozbhly44hii3zwie7ivsra3qxzdibyzhyhooxrudvb@zik6skmkki2c>
Accept-Language: zh-TW, en-US
Content-Language: zh-TW
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?f9HL50F3UvtPozuC8hLj4ns8McsfcLm5ovag2Dbd3Yx2SNqmya5g2DFZtyLk?=
 =?us-ascii?Q?YZQzNhP284jTE1cY3Hn/8QvZ60GtorvPN2AWMid16fURcqYxbxJLuwjA9MM0?=
 =?us-ascii?Q?q5DZpk2eW8nrDm8LD7g/i2kU3n/CPVqj0ZCxr4rbHacwonaVL3dvqtXhScKu?=
 =?us-ascii?Q?uSVvxwkDleUCWpGtTPeM2Ih3LA3o23uGg5g4CDJ8mprAIAbf+6JJYYMqqkAP?=
 =?us-ascii?Q?Vpk3VBruzgDVmbb/xwlKBu8F3oUVhLCYSQ9svgOBxj9GRaYFeUwneRIRPJlC?=
 =?us-ascii?Q?pr7TYesmlkpCf5raHlj1jPqnDkzQWEOhJBuz/twzR405goYJWUOQck5+hLxn?=
 =?us-ascii?Q?dylHHHe7DIsRVYNEXmq011TAPx5xWvQ0SIqGe+7cQuALXyIbdYhP5YN0h40E?=
 =?us-ascii?Q?6mWPfkHPtW1U+Wgf7X7LElzSPBeAo0bkKMGObIvHDkjEMYB7cCwgU91cdOUI?=
 =?us-ascii?Q?e2vLptTJhy+XaO/+MNEhSPL4bvYLnmHpmEPbQb16c2COBLaySsd7KxZAK4LE?=
 =?us-ascii?Q?kg80TVd7aZ1dPTx3RqEbr6av+UD0L/i121fF8bB9a9rKNfPC4w4HGXmRDsNK?=
 =?us-ascii?Q?gqncGgoJS0V13gXCfzExyDn+YEX4YNNom0THhw9Kjohsqoh0z3W3roUO561c?=
 =?us-ascii?Q?hMgaKe2drH94MbAvulKBuHeYscT3/20A62F442q9ZqcdLsUVbwBsMzDqnMus?=
 =?us-ascii?Q?1R6oD8D7aH9qatXBoaSlypAEn8S0e+Y2OH9knW/Gf8J7OELFCWNBIJ3PMNw1?=
 =?us-ascii?Q?K7May5lz86jqeJpZoNLrFzgK8TOufIfEVa4YFOBFCuk11z2NCTNr9fQ94H6G?=
 =?us-ascii?Q?fYd3yGA3R0e6TaB7EOT1XeeJdrmT1qGhDt6BzNAH20K9TrEwu0AYHPiAwytg?=
 =?us-ascii?Q?GYiDYlYPrnL07oRnDRsbV/gdrBd+1e8W0+bAHFtJ2oPIlalj+LWFVZn/J26H?=
 =?us-ascii?Q?q5hyn8pPvDG+vLVXve8mTVEPAJoQxze2r46YB2RiVb54mdy42GdU//2+aIgY?=
 =?us-ascii?Q?kzy+aZQktr98PljOTLBmvgyfkqmvtUg3yYxnJvFWr8rfV02a4jsKtg5MEbUI?=
 =?us-ascii?Q?q6eRbGhRHMn3aMKc0cg0xm179x4+VOlQ3ZbkI9MFde44kPj5eEygVTHKFu8N?=
 =?us-ascii?Q?kc+s5e9mEDSJTlaBYlvpaw4Gr5oe5P7GJQo5eZxdfFMDG8y7UEur4H+vHirE?=
 =?us-ascii?Q?WHuJuNW2s2+TDm9yHJSqter3NXj2R1h5NS7L9eFHq1EmYBMokQuTsvqW68fu?=
 =?us-ascii?Q?mWE7yryxiU0n+NNn/A714idZ3rGxHztlHjak6y9bXTzVJHV0CdhqSD+GxPbw?=
 =?us-ascii?Q?6TlFOwCB1bteLLx8X9m3MfdECZr4AUxgSOl16uYJf3DesLCYlHmEle6OiKwi?=
 =?us-ascii?Q?Dja1U3CTJi+WU6UQWQuusI80n6/ZezSqunVIJlV8iDwlH4ZH0tWBG9P3slM/?=
 =?us-ascii?Q?SJlGMjMtctoZNBNE8vl0xcg5bmEzB7rwRiL42FSnmfT7EeWMBnUh7HjE/SRT?=
 =?us-ascii?Q?hL8eqLRtcmF4h6kOgmBiClA+GOdwSYuaEtRzE0vZkBKPjSFWrIX6plDvdqmM?=
 =?us-ascii?Q?t8349FEgP1eC3k+u4QMFReXh4nBQ4qgk+C/IiKBoOHH8CjGFTpPMBPHzs9NS?=
 =?us-ascii?Q?RQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: aspeedtech.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: TYZPR06MB6191.apcprd06.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d8820bd0-5a2c-42f3-ce93-08dc3343159c
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Feb 2024 01:10:39.8079
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 43d4aa98-e35b-4575-8939-080e90d5a249
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Fjlo6n68PIu8T6ZuEifCMasBn5ESt02J1FIhKXFzbRwF17Q4l2SUbyX/zgVjeonPzQqt3oW3XmkfsyFcdwPLmCAQKz0QzcxDaqwX8bG9uMw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: TY0PR06MB5471

Hi Andy,

	Thanks for your comment.

> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Thursday, February 22, 2024 5:15 AM
> To: Tommy Huang <tommy_huang@aspeedtech.com>
> Cc: brendan.higgins@linux.dev; p.zabel@pengutronix.de;
> linux-i2c@vger.kernel.org; openbmc@lists.ozlabs.org;
> benh@kernel.crashing.org; joel@jms.id.au; andrew@aj.id.au;
> linux-arm-kernel@lists.infradead.org; linux-aspeed@lists.ozlabs.org;
> linux-kernel@vger.kernel.org; BMC-SW <BMC-SW@aspeedtech.com>
> Subject: Re: [PATCH] i2c: aspeed: Fix the dummy irq expected print
>=20
> Hi Tommy,
>=20
> On Fri, Feb 16, 2024 at 08:04:55PM +0800, Tommy Huang wrote:
> > When the i2c error condition occurred and master state was not idle,
> > the master irq function will goto complete state without any other
> > interrupt handling. It would cause dummy irq expected print. Under
> > this condition, assign the irq_status into irq_handle.
>=20
> I'm sorry, but I don't understand much from your log here.
>=20
> Do you mean that irq_handled in aspeed_i2c_master_irq() is left with some
> states that is not supposed to have and then you end up printing here:
>=20
> 	dev_err(bus->dev,
> 		"irq handled !=3D irq. expected 0x%08x, but was 0x%08x\n",
> 		irq_received, irq_handled);
>=20
> Can you please explain better?
>=20

Yes. If the platform met any irq error condition and the i2c wasn't stated =
under ASPEED_I2C_MASTER_INACTIVE.
Then the code flow would goto the end of aspeed_i2c_master_irq.

	ret =3D aspeed_i2c_is_irq_error(irq_status);
	if (ret) {
		...
		irq_handled |=3D (irq_status & ASPEED_I2CD_INTR_MASTER_ERRORS);
		if (bus->master_state !=3D ASPEED_I2C_MASTER_INACTIVE) {
			bus->cmd_err =3D ret;
			bus->master_state =3D ASPEED_I2C_MASTER_INACTIVE;
			goto out_complete;
		}
	}

Some master interrupt states were not handled under this situation.
The fake irq not equaled message would be filled into whole of demsg.
It's most like below example.

..
aspeed-i2c-bus 1e78a780. i2c-bus: irq handled !=3D irq. expected 0x00000030=
, but was 0x00000020
aspeed-i2c-bus 1e78a780. i2c-bus: irq handled !=3D irq. expected 0x00000030=
, but was 0x00000020
aspeed-i2c-bus 1e78a780. i2c-bus: irq handled !=3D irq. expected 0x00000030=
, but was 0x00000020
..

I thought the bus->cmd_err has been filled error reason and it would be ret=
urned to upper layer.
So, I didn't think the print should be existed.

> If that's the case, wouldn't it make more sense to check for
> bus->master_state !=3D ASPEED_I2C_MASTER_INACTIVE) earlier?
>=20

Did you suggest to add "bus->master_state !=3D ASPEED_I2C_MASTER_INACTIVE" =
judgement before print the irq not equal print?

> And, still, If that's the case, I believe you might need the Fixes tag. I=
t's true that
> you are not really failing, but you are not reporting a failure by mistak=
e.
>=20
> Thanks,
> Andi
>=20
> > Signed-off-by: Tommy Huang <tommy_huang@aspeedtech.com>
> > ---
> >  drivers/i2c/busses/i2c-aspeed.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-aspeed.c
> > b/drivers/i2c/busses/i2c-aspeed.c index 5511fd46a65e..ce8c4846b7fa
> > 100644
> > --- a/drivers/i2c/busses/i2c-aspeed.c
> > +++ b/drivers/i2c/busses/i2c-aspeed.c
> > @@ -445,6 +445,7 @@ static u32 aspeed_i2c_master_irq(struct
> aspeed_i2c_bus *bus, u32 irq_status)
> >  			irq_status);
> >  		irq_handled |=3D (irq_status & ASPEED_I2CD_INTR_MASTER_ERRORS);
> >  		if (bus->master_state !=3D ASPEED_I2C_MASTER_INACTIVE) {
> > +			irq_handled =3D irq_status;
> >  			bus->cmd_err =3D ret;
> >  			bus->master_state =3D ASPEED_I2C_MASTER_INACTIVE;
> >  			goto out_complete;
> > --
> > 2.25.1
> >