linux-kernel - RE: [PATCH] i2c: aspeed: Fix the dummy irq expected print

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:
 <TYZPR06MB61912715EE2869DDB7C3763DE1552@TYZPR06MB6191.apcprd06.prod.outlook.com>
Date: Fri, 23 Feb 2024 03:49:17 +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>
Subject: RE: [PATCH] i2c: aspeed: Fix the dummy irq expected print

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 stop
	status (bit 4) at same time. Then the normal stop status would not be handled 
	and it would cause irq expected print in the aspeed_i2c_bus_irq.

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

	Tommy

> -----Original Message-----
> From: Andi Shyti <andi.shyti@...nel.org>
> Sent: Thursday, February 22, 2024 4:58 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>
> Subject: Re: [PATCH] i2c: aspeed: Fix the dummy irq expected print
> 
> Hi Tommy,
> 
> 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 != 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 stated
> under ASPEED_I2C_MASTER_INACTIVE.
> > Then the code flow would goto the end of aspeed_i2c_master_irq.
> >
> > 	ret = aspeed_i2c_is_irq_error(irq_status);
> > 	if (ret) {
> > 		...
> > 		irq_handled |= (irq_status & ASPEED_I2CD_INTR_MASTER_ERRORS);
> > 		if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> > 			bus->cmd_err = ret;
> > 			bus->master_state = 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 != irq. expected
> > 0x00000030, but was 0x00000020 aspeed-i2c-bus 1e78a780. i2c-bus: irq
> > handled != irq. expected 0x00000030, but was 0x00000020 aspeed-i2c-bus
> > 1e78a780. i2c-bus: irq handled != 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.
> 
> thanks! Can you please write a commit that explains better the fix you are
> doing?
> 
> > > If that's the case, wouldn't it make more sense to check for
> > > bus->master_state != ASPEED_I2C_MASTER_INACTIVE) earlier?
> >
> > Did you suggest to add "bus->master_state !=
> ASPEED_I2C_MASTER_INACTIVE" judgement before print the irq not equal
> print?
> 
> no, not really, but nevermind, on a second look, what I'm suggesting doesn't
> make much sense.
> 
> If you want, please reword the commit message as reply to this e-mail and I
> will take care of it.
> 
> > > 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 reporting a
> failure by mistake.
> 
> Please, also consider adding the Fixes tag if you see it necessary; I think you
> should, but it's borderline.
> 
> Andi