Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i9e814621:Fastmail
Date:   Thu, 21 Jul 2022 20:08:24 -0700
From:   Peter Delevoryas <peter@pjd.dev>
To:     ryan_chen <ryan_chen@aspeedtech.com>
Cc:     brendanhiggins@google.com, benh@kernel.crashing.org,
        joel@jms.id.au, andrew@aj.id.au, linux-i2c@vger.kernel.org,
        openbmc@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org,
        linux-aspeed@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] i2c: aspeed: Fix slave mode unexpected irq handler
Message-ID: <YtoUqH6uSNEKBbt5@pdel-mbp>
References: <20220531093140.28770-1-ryan_chen@aspeedtech.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220531093140.28770-1-ryan_chen@aspeedtech.com>
Precedence: bulk

On Tue, May 31, 2022 at 05:31:40PM +0800, ryan_chen wrote:
> When i2c master send the new i2c transfer immediately
> after stop. the i2c slave will see the stop and new
> address match stage together. And it needs handle the
> stop first. otherwise will occur unexpected handle
> isr.

I think it would be helpful if you could include some driver trace messages to
indicate the sequence of events that lead you to make this change, like an
example of the problem happening.

> 
> Fixes: f327c686d3ba ("i2c: aspeed: added drover for Aspeed I2C)

Typo on "drover"? Actually, slave support wasn't included in this commit. I
think it should be:

Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver")

> Signed-off-by: ryan_chen <ryan_chen@aspeedtech.com>
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 771e53d3d197..9f21e090ce47 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -252,6 +252,12 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>  
>  	/* Slave was requested, restart state machine. */
>  	if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
> +		if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP &&
> +			bus->slave_state == ASPEED_I2C_SLAVE_WRITE_RECEIVED) {
> +			irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
> +			irq_status &= ~ASPEED_I2CD_INTR_NORMAL_STOP;
> +			i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
> +		}

Ok, we might receive the STOP and START signals at the same time.

So, we need to make sure that we handle the STOP first. [1]

Why is this within the START case then? Can't we untangle this from the START
handling?

irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH means SLAVE START, right?

We know we're in slave mode already because this is the slave IRQ handler.

And we can access the state of the bus through bus->slave_state, to see if it's
a spurious STOP or an expected one.

It would also be nice to unify the STOP handling code too, to make sure this
matches the normal STOP path that already exists below here.

[1] Although if that's the case, it seems like we might need to handle the
START first sometimes, depending on the current state, right?

Can we have the reverse case, where we see a START and the matching STOP
simultaneously? Perhaps that's already handled properly by the code since
it's structured chronologically.

>  		irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
>  		bus->slave_state = ASPEED_I2C_SLAVE_START;
>  	}
> -- 
> 2.17.1
>