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
