Re: [PATCH] i2c: aspeed: Fix the dummy irq expected print

[Andi Shyti]

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