linux-kernel - Re: [PATCH v3 2/2] i2c: aspeed: Acknowledge Tx done with and without ACK irq late

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 11 Dec 2023 11:06:45 +0700
From:   Quan Nguyen <quan@...amperecomputing.com>
To:     Andi Shyti <andi.shyti@...nel.org>
Cc:     Brendan Higgins <brendan.higgins@...ux.dev>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Joel Stanley <joel@....id.au>,
        Andrew Jeffery <andrew@...econstruct.com.au>,
        Wolfram Sang <wsa@...nel.org>,
        Jae Hyun Yoo <jae.hyun.yoo@...ux.intel.com>,
        Guenter Roeck <linux@...ck-us.net>, 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 <chou.cosmo@...il.com>,
        Open Source Submission <patches@...erecomputing.com>,
        Phong Vo <phong@...amperecomputing.com>,
        "Thang Q . Nguyen" <thang@...amperecomputing.com>
Subject: Re: [PATCH v3 2/2] i2c: aspeed: Acknowledge Tx done with and without
 ACK irq late



On 10/12/2023 03:44, Andi Shyti wrote:
> Hi Quan,
> 
> [...]
> 
>> -	/* Ack all interrupts except for Rx done */
>> -	writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
>> -	       bus->base + ASPEED_I2C_INTR_STS_REG);
>> +
>> +	/*
>> +	 * Early acking of INTR_RX_DONE and INTR_TX_[ACK|NAK] would indicate HW to
>> +	 * start receiving or sending new data, and this may cause a race condition
>> +	 * as the irq handler has not yet handled these irqs but is being acked.
>> +	 * Let's ack them late at the end of the irq handler when those are truly processed.
>> +	 */
>> +	irq_ack_last = ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK;
>> +	writel(irq_received & ~irq_ack_last, bus->base + ASPEED_I2C_INTR_STS_REG);
> 
> I like Andrews suggestion of having irq_ack_last as a define that
> is already negated, instead of negating it in the writel, which
> makes it a bit difficult to read.
> 

Yes, but the it still need to negate again when do the write to late ack 
them later in the end of irq handler. So I'll keep the define as below 
in my v4:

+#define ASPEED_I2CD_INTR_ACK_RX_TX	    \
+		(ASPEED_I2CD_INTR_RX_DONE | \
+		 ASPEED_I2CD_INTR_TX_ACK |  \
+		 ASPEED_I2CD_INTR_TX_NAK)

The early ack will look like this:

+		writel(irq_received & ~ASPEED_I2CD_INTR_ACK_RX_TX,
+		       bus->base + ASPEED_I2C_INTR_STS_REG);
+		readl(bus->base + ASPEED_I2C_INTR_STS_REG);

And the late ack:

-	/* Ack Rx done */
-	if (irq_received & ASPEED_I2CD_INTR_RX_DONE) {
-		writel(ASPEED_I2CD_INTR_RX_DONE,
+	if (irq_received & ASPEED_I2CD_INTR_ACK_RX_TX) {
+		writel(irq_received & ASPEED_I2CD_INTR_ACK_RX_TX,
  		       bus->base + ASPEED_I2C_INTR_STS_REG);
  		readl(bus->base + ASPEED_I2C_INTR_STS_REG);
  	}

> Besides, ack_last, as a name is not very meaningful, I'd rather
> call it irq_ack_rx_tx (or something similar).
> 
> But I'm not going to block it for this, up to you if you want to
> send a new version.
> 
> Reviewed-by: Andi Shyti <andi.shyti@...nel.org>
> 

Thanks, Andi for the comments.

I will send out v4 to address those.

- Quan