diff mbox series

i2c: aspeed: Fix the dummy irq expected print

Message ID 20240216120455.4138642-1-tommy_huang@aspeedtech.com
State New
Headers show
Series i2c: aspeed: Fix the dummy irq expected print | expand

Commit Message

Tommy Huang Feb. 16, 2024, 12:04 p.m. UTC 
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.

Signed-off-by: Tommy Huang <tommy_huang@aspeedtech.com>
---
 drivers/i2c/busses/i2c-aspeed.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Andi Shyti Feb. 21, 2024, 9:14 p.m. UTC | #1 
Hi Tommy,

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?

If that's the case, wouldn't it make more sense to check for 
bus->master_state != ASPEED_I2C_MASTER_INACTIVE) earlier?

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.

Thanks,
Andi

> Signed-off-by: Tommy Huang <tommy_huang@aspeedtech.com>
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 5511fd46a65e..ce8c4846b7fa 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -445,6 +445,7 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>  			irq_status);
>  		irq_handled |= (irq_status & ASPEED_I2CD_INTR_MASTER_ERRORS);
>  		if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> +			irq_handled = irq_status;
>  			bus->cmd_err = ret;
>  			bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>  			goto out_complete;
> -- 
> 2.25.1
>
Tommy Huang Feb. 22, 2024, 1:10 a.m. UTC | #2 
Hi Andy,

	Thanks for your comment.

> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Thursday, February 22, 2024 5:15 AM
> 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
> 
> Hi Tommy,
> 
> 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.

> 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?

> 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.
> 
> Thanks,
> Andi
> 
> > Signed-off-by: Tommy Huang <tommy_huang@aspeedtech.com>
> > ---
> >  drivers/i2c/busses/i2c-aspeed.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-aspeed.c
> > b/drivers/i2c/busses/i2c-aspeed.c index 5511fd46a65e..ce8c4846b7fa
> > 100644
> > --- a/drivers/i2c/busses/i2c-aspeed.c
> > +++ b/drivers/i2c/busses/i2c-aspeed.c
> > @@ -445,6 +445,7 @@ static u32 aspeed_i2c_master_irq(struct
> aspeed_i2c_bus *bus, u32 irq_status)
> >  			irq_status);
> >  		irq_handled |= (irq_status & ASPEED_I2CD_INTR_MASTER_ERRORS);
> >  		if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> > +			irq_handled = irq_status;
> >  			bus->cmd_err = ret;
> >  			bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> >  			goto out_complete;
> > --
> > 2.25.1
> >
Andi Shyti Feb. 22, 2024, 8:57 a.m. UTC | #3 
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
Tommy Huang Feb. 23, 2024, 3:49 a.m. UTC | #4 
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@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
> 
> 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
Wolfram Sang March 4, 2024, 8:56 p.m. UTC | #5 
> 	Sure~
> 	Below is my re-word commit and fixes tag.

Please resend the patch with the reworded commit and the fixes tag
added.
Tommy Huang March 5, 2024, 12:09 a.m. UTC | #6 
Hi Wolfram,

	Thanks for your comment.
	I will resend it again.

	BR,

	By Tommy

> -----Original Message-----
> From: Wolfram Sang <wsa@kernel.org>
> Sent: Tuesday, March 5, 2024 4:57 AM
> To: Tommy Huang <tommy_huang@aspeedtech.com>
> Cc: Andi Shyti <andi.shyti@kernel.org>; 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
> 
> 
> > 	Sure~
> > 	Below is my re-word commit and fixes tag.
> 
> Please resend the patch with the reworded commit and the fixes tag added.
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 5511fd46a65e..ce8c4846b7fa 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -445,6 +445,7 @@  static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 			irq_status);
 		irq_handled |= (irq_status & ASPEED_I2CD_INTR_MASTER_ERRORS);
 		if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
+			irq_handled = irq_status;
 			bus->cmd_err = ret;
 			bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
 			goto out_complete;