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: <v4nawwb4rwjiy2g7xv2sfyhc545mhk4izb3g22f7jupcevjuzb@nxmqgf2zjyqs>
Date: Thu, 22 Feb 2024 09:57:54 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Tommy Huang <tommy_huang@...eedtech.com>
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 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