Received-SPF: pass (google.com: domain of linux-kernel+bounces-77741-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
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: AQHaYNBg0EEa3X7oHECRXZPcHJI3VbEVVFwAgAA4R6CAAIwtAIABM1cg
Date: Fri, 23 Feb 2024 03:49:17 +0000
Message-ID:
 <TYZPR06MB61912715EE2869DDB7C3763DE1552@TYZPR06MB6191.apcprd06.prod.outlook.com>
References: <20240216120455.4138642-1-tommy_huang@aspeedtech.com>
 <nbkkaktcozbhly44hii3zwie7ivsra3qxzdibyzhyhooxrudvb@zik6skmkki2c>
 <TYZPR06MB61911F076C8719C6A7D57B97E1562@TYZPR06MB6191.apcprd06.prod.outlook.com>
 <v4nawwb4rwjiy2g7xv2sfyhc545mhk4izb3g22f7jupcevjuzb@nxmqgf2zjyqs>
In-Reply-To: <v4nawwb4rwjiy2g7xv2sfyhc545mhk4izb3g22f7jupcevjuzb@nxmqgf2zjyqs>
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?RQ/iP1ovrQvLx9MtJZ+PU0pTCBfdhYSuh+l/cKaNGRZWpS+i25YaSPq61bwb?=
 =?us-ascii?Q?/XDhmfkWMQx3OhBU2OnrXlgOQWWIW+ytcPGw9H6RLjzqVTrV2lJCj79MYJE1?=
 =?us-ascii?Q?l+dnJh7evAO0D+2jPjmNeTWduVTLf2T8/NcgHuRUFtr42v6ghOMit45/ZvZ6?=
 =?us-ascii?Q?LuGiGYBBjrdWl/iKG3SsyCdLLagS9NiYCtKe1t0Sesb/USNGrb/Uo2HpwpXE?=
 =?us-ascii?Q?dg6+K9cXnh1irpt5gba97gHAAHiQKWaZYaYII3+pubBRgnzli4Z5grnh2USW?=
 =?us-ascii?Q?hNe7cXDg8++z6FjJouO5HvUlsCUAZ1qMxFFNVvoWe7o1agnq2/jqLM3uEBK5?=
 =?us-ascii?Q?qMi4YlNERCtzTunVoywBRE8ssPahrTs4ezo2WNOJ7vCPEhs5OYdTNEQsdtaW?=
 =?us-ascii?Q?Xjr2JXYUXBc79nIJZOOgUx5UqMh1kMiuSs/rFXjje7T2hSzZpNMiv6b7SiIG?=
 =?us-ascii?Q?4NSuXrLUnCuYsidmbsVfBZhOLqQEmfTzTf+HwOeve182ZE1w/1OLIVZDPvzm?=
 =?us-ascii?Q?LMi1C4KsqwpoJi8QXj050eI2ywHH3UxjKK+gNQcO8ei5kJIgg0BY1ibU98LR?=
 =?us-ascii?Q?X2rxVdZSG99k4mdfqWxY4wNvRTO2ZsarV/ktZMgLoG8zDwP0vI7jxeTD5pG5?=
 =?us-ascii?Q?LvyprxIfi3dYelo0cqmP9F/nUJyx98pxL3WZYwqo3vv8RpKaXw34MnG/OI7h?=
 =?us-ascii?Q?Zn6Yf/OJJOqhxsGrIXMw+AozL+51Bw3fNj8DUASbYfYyBsH9lFiUU8EOzVSU?=
 =?us-ascii?Q?HWRZZh2nbJ+fduiFAtf9zS3B8MMNjzFBj45svnXoy31ykq4N3GNYgOYYPHc+?=
 =?us-ascii?Q?kzGCtiI9U13LxK30VUXiuEe8ZevvvOl0C1w89mz9EZhxK0S8XcEl1e+ctl7K?=
 =?us-ascii?Q?FEkI8JPa5Yf8Smsvf2ZpvOTRvcMNXekoV+w6KxV0WIOIRkUJdTXHX1kZ4OFw?=
 =?us-ascii?Q?Ipdzb8y+dWoiUsJmuRHtt3eLL3fCM18Ct5Wl968Hi+R5p3HAGQEBH3UJe4WJ?=
 =?us-ascii?Q?foR6UsZx9YRlFLiqWzVibPCBe9QQbL4oUkTdui4zfHbRLl1l2mqtOFdWNSvr?=
 =?us-ascii?Q?JG8tGtXUTiWot29SQ93X7Xz6S0NNDICwf+2jB9BQgsM0qknp6oOjwDdmdUYr?=
 =?us-ascii?Q?D6713RI9XF6CH/m5ke241LsYtX7fZ7U6nBpeHbWjdX1hsakd9G9CNHJ3Rnlw?=
 =?us-ascii?Q?G58JB3lD77A1BY8Fjpy4omUn2q0SGHziF0XTWkauTCrDcvbhe+8PjGJc+U2I?=
 =?us-ascii?Q?MzquyFrNcjgqQmy9MQuE1c6wrm714fl5RHysmfRpZcoNsX8jyZ8ZaAT23KC0?=
 =?us-ascii?Q?mqaJhKVeo+XmNkS8eEjsNFJCcHcTr+lL3BsCC66kfjAz8gOTAb4uHD3nZWip?=
 =?us-ascii?Q?4E/KQIIpUaVS5/kPu1mn4OMtNYN/Dv6B64ui0eQVYjl9sRgx3SpgJKP9wePV?=
 =?us-ascii?Q?+9yA5s3qtUjG3LziCEHGjRkMDTwGiCUerz9aYCmj/aoylpsRWtenRY0Xcw60?=
 =?us-ascii?Q?08eRkgOPD/plcjmx3W6xEfhQk7FJKlPWkAPKsFmB6O0tUJeD6cu4Sk3XxbFz?=
 =?us-ascii?Q?mFFInGt3ikOcKY7zFCAoAvbE+fPRTaUTPGV8mf4pmicTCWgvYaeDV8hUWEtI?=
 =?us-ascii?Q?Kg=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: 24e1dc8c-477e-4a4c-8f3f-08dc342268f9
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Feb 2024 03:49:17.3927
 (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: GYHVUIN+3itbPP/SihXlxAi3PvAGjKEJWzxXGcGY/DMRBn9s4MYJM8KXgPY1gdajXvH6+BtjM7L6s9Gy0p4mi0Tnnt27Pn5zL6RK/+gWKWg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PUZPR06MB5585

Hi Andi,

	Sure~
	Below is my re-word commit and fixes tag.

	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.

	For example, when the abnormal start / stop occurred (bit 5) with normal s=
top
	status (bit 4) at same time. Then the normal stop status would not be hand=
led=20
	and it would cause irq expected print in the aspeed_i2c_bus_irq.

	...
	aspeed-i2c-bus xxxxxxxx. i2c-bus: irq handled !=3D irq. Expected 0x0000003=
0, but was 0x00000020
	...
=20
	Fixes: 3e9efc3299dd ("i2c: aspeed: Handle master/slave combined irq events=
 properly")
	Cc: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>

	Tommy

> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Thursday, February 22, 2024 4:58 PM
> 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 Thu, Feb 22, 2024 at 01:10:39AM +0000, Tommy Huang wrote:
> > > 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.
> > >
> > > I'm sorry, but I don't understand much from your log here.
> > >
> > > 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:
> > >
> > > 	dev_err(bus->dev,
> > > 		"irq handled !=3D irq. expected 0x%08x, but was 0x%08x\n",
> > > 		irq_received, irq_handled);
> > >
> > > Can you please explain better?
> > >
> >
> > Yes. If the platform met any irq error condition and the i2c wasn't sta=
ted
> 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-bu=
s
> > 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
> returned to upper layer.
> > So, I didn't think the print should be existed.
>=20
> thanks! Can you please write a commit that explains better the fix you ar=
e
> doing?
>=20
> > > If that's the case, wouldn't it make more sense to check for
> > > bus->master_state !=3D ASPEED_I2C_MASTER_INACTIVE) earlier?
> >
> > Did you suggest to add "bus->master_state !=3D
> ASPEED_I2C_MASTER_INACTIVE" judgement before print the irq not equal
> print?
>=20
> no, not really, but nevermind, on a second look, what I'm suggesting does=
n't
> make much sense.
>=20
> If you want, please reword the commit message as reply to this e-mail and=
 I
> will take care of it.
>=20
> > > And, still, If that's the case, I believe you might need the Fixes
> > > tag. It's true that you are not really failing, but you are not repor=
ting a
> failure by mistake.
>=20
> Please, also consider adding the Fixes tag if you see it necessary; I thi=
nk you
> should, but it's borderline.
>=20
> Andi