linux-kernel - Re: [PATCH] i2c: cadence: Avoid fifo clear after start

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]

Message-ID: <lzvb6oyinlmdrbaat6ayxioca5r2qf7wi3kt63lzorbjytmmbn@wfuz63znrzbr>
Date: Wed, 17 Jan 2024 14:19:56 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Sai Pavan Boddu <sai.pavan.boddu@....com>
Cc: linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, Michal Simek <michal.simek@....com>, 
	Lars-Peter Clausen <lars@...afoo.de>, Wolfram Sang <wsa@...nel.org>
Subject: Re: [PATCH] i2c: cadence: Avoid fifo clear after start

Hi Sai,

sorry, but I'm not really understanding the issue here.
On Fri, Jan 05, 2024 at 06:22:58PM +0530, Sai Pavan Boddu wrote:
> Driver unintentionally programs ctrl reg to clear fifo which is
> happening after start of transaction

what does it mean "unintentionally"?

> this was not the case previously
> as it was read-modified-write. This issue breaks i2c reads on QEMU as
> i2c-read is done before guest starts programming ctrl register.

this log can be improved. How about something like

The driver unintentionally programs the control register to clear
the FIFO, which occurs after the start of the transaction.
Previously, this was not an issue as it involved
read-modify-write operations. However, this current issue
disrupts I2C reads on QEMU, as the I2C read is executed before
the guest starts programming the control register.

> Fixes: ff0cf7bca6309 ("i2c: cadence: Remove unnecessary register reads")
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@....com>
> ---
>  drivers/i2c/busses/i2c-cadence.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index de3f58b60dce..6f7d753a8197 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -633,6 +633,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>  
>  	if (hold_clear) {
>  		ctrl_reg &= ~CDNS_I2C_CR_HOLD;
> +		ctrl_reg &= ~CDNS_I2C_CR_CLR_FIFO;

I'm wondering whether the whole ctrl_reg should be reset after
the first write.

Andi

>  		/*
>  		 * In case of Xilinx Zynq SOC, clear the HOLD bit before transfer size
>  		 * register reaches '0'. This is an IP bug which causes transfer size
> -- 
> 2.25.1
>