linux-kernel - Re: [PATCH] i2c: aspeed: Acknowledge Tx ack late when in SLAVE_READ

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231127222522.wjdd6btbug6shd7y@zenone.zhora.eu>
Date:   Mon, 27 Nov 2023 23:25:22 +0100
From:   Andi Shyti <andi.shyti@...nel.org>
To:     Cosmo Chou <chou.cosmo@...il.com>
Cc:     brendan.higgins@...ux.dev, benh@...nel.crashing.org,
        joel@....id.au, andrew@...econstruct.com.au, linux@...ck-us.net,
        wsa@...nel.org, jae.hyun.yoo@...ux.intel.com,
        linux-i2c@...r.kernel.org, openbmc@...ts.ozlabs.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-aspeed@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        cosmo.chou@...ntatw.com
Subject: Re: [PATCH] i2c: aspeed: Acknowledge Tx ack late when in
 SLAVE_READ_PROCESSED

Hi Cosmo,

On Mon, Nov 20, 2023 at 05:17:46PM +0800, Cosmo Chou wrote:
> commit 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early
> in interrupt handler") moved most interrupt acknowledgments to the
> start of the interrupt handler to avoid race conditions. However,
> slave Tx ack status shouldn't be cleared before SLAVE_READ_PROCESSED
> is handled.
> 
> Acknowledge Tx ack status after handling SLAVE_READ_PROCESSED to fix
> the problem that the next byte is not sent correctly.
> 
> Fixes: 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in interrupt handler")
> Signed-off-by: Cosmo Chou <chou.cosmo@...il.com>
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 28e2a5fc4528..c2d74e4b7e50 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -337,6 +337,12 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>  		break;
>  	}
>  
> +	/* Ack Tx ack */
> +	if (irq_handled & ASPEED_I2CD_INTR_TX_ACK) {
> +		writel(ASPEED_I2CD_INTR_TX_ACK, bus->base + ASPEED_I2C_INTR_STS_REG);
> +		readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> +	}
> +
>  	return irq_handled;
>  }
>  #endif /* CONFIG_I2C_SLAVE */
> @@ -602,13 +608,18 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>  static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>  {
>  	struct aspeed_i2c_bus *bus = dev_id;
> -	u32 irq_received, irq_remaining, irq_handled;
> +	u32 irq_received, irq_remaining, irq_handled, irq_acked;
>  
>  	spin_lock(&bus->lock);
>  	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>  	/* Ack all interrupts except for Rx done */
> -	writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> -	       bus->base + ASPEED_I2C_INTR_STS_REG);
> +	irq_acked = irq_received & ~ASPEED_I2CD_INTR_RX_DONE;
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +	/* shouldn't ack Slave Tx Ack before it's handled */
> +	if (bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED)
> +		irq_acked &= ~ASPEED_I2CD_INTR_TX_ACK;
> +#endif
> +	writel(irq_acked, bus->base + ASPEED_I2C_INTR_STS_REG);

which branch are you? You don't look like being in the latest.
Please update your branch.

Andi

>  	readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>  	irq_received &= ASPEED_I2CD_INTR_RECV_MASK;
>  	irq_remaining = irq_received;
> -- 
> 2.34.1
>