linux-kernel - Re: [PATCH] i2c: aspeed: Acknowledge most interrupts early in interrupt handler

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:   Fri, 14 Sep 2018 09:28:43 -0700
From:   Jae Hyun Yoo <jae.hyun.yoo@...ux.intel.com>
To:     Guenter Roeck <linux@...ck-us.net>,
        Brendan Higgins <brendanhiggins@...gle.com>
Cc:     linux-aspeed@...ts.ozlabs.org, Andrew Jeffery <andrew@...id.au>,
        openbmc@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        Cédric Le Goater <clg@...d.org>,
        linux-i2c@...r.kernel.org
Subject: Re: [PATCH] i2c: aspeed: Acknowledge most interrupts early in
 interrupt handler

On 9/13/2018 8:30 PM, Guenter Roeck wrote:
> Commit 3e9efc3299dd ("i2c: aspeed: Handle master/slave combined irq events
> properly") moved interrupt acknowledgment to the end of the interrupt
> handler. In part this was done because the AST2500 datasheet says:
> 
>   I2CD10 Interrupt Status Register
>     bit 2 Receive Done Interrupt status
>           S/W needs to clear this status bit to allow next data receiving.
> 
> Acknowledging Receive Done before receive data was handled resulted in
> receive errors on high speed I2C busses.
> 
> However, interrupt acknowledgment was not only moved to the end of the
> interrupt handler for Receive Done Interrupt status, but for all interrupt
> status bits. This could result in race conditions if a second interrupt was
> received during interrupt handling and not handled but still acknowledged
> at the end of the interrupt handler.
> 
> Acknowledge only "Receive Done Interrupt status" late in the interrupt
> handler to solve the problem.
> 
> Fixes: 3e9efc3299dd ("i2c: aspeed: Handle master/slave combined irq events properly")
> Cc: Jae Hyun Yoo <jae.hyun.yoo@...ux.intel.com>
> Cc: Joel Stanley <joel@....id.au>
> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
> ---
>   drivers/i2c/busses/i2c-aspeed.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index c258c4d9a4c0..3d518e09369f 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>   
>   	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_remaining = irq_received;
>   
>   #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>   			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>   			irq_received, irq_handled);
>   
> -	/* Ack all interrupt bits. */
> -	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> +	/* Ack Rx done */
> +	if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
> +		writel(ASPEED_I2CD_INTR_RX_DONE,
> +		       bus->base + ASPEED_I2C_INTR_STS_REG);
>   	spin_unlock(&bus->lock);
>   	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>   }
> 

Looks good to me. Thanks! :)

Acked-by: Jae Hyun Yoo <jae.hyun.yoo@...ux.intel.com>