linux-kernel - Re: [PATCH 1/1] i2c: i2c-qcom-geni: Correct I2C TRE sequence

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, 12 Jan 2024 14:54:47 +0000
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Viken Dadhaniya <quic_vdadhani@...cinc.com>, andersson@...nel.org,
 konrad.dybcio@...aro.org, andi.shyti@...nel.org,
 linux-arm-msm@...r.kernel.org, linux-i2c@...r.kernel.org,
 linux-kernel@...r.kernel.org, vkoul@...nel.org
Cc: quic_msavaliy@...cinc.com, quic_vtanuku@...cinc.com
Subject: Re: [PATCH 1/1] i2c: i2c-qcom-geni: Correct I2C TRE sequence

On 12/01/2024 13:53, Viken Dadhaniya wrote:
> For i2c read operation, we are getting gsi mode timeout due
> to malformed TRE(Transfer Ring Element). currently for read opreration,
> we are configuring incorrect TRE sequence(config->dma->go).
> 
> So correct TRE sequence(config->go->dma) to resolve timeout
> issue for read operation.

I don't think this commit log really captures what the code does.

- Sets up optional RX DMA
- Sets up TX DMA
- Issues optional RX dma_async_issue_pending
- Issues TX dma_async_issue_pending

What your change does is sets up the TX DMA first

- Sets up TX DMA
- Sets up optional RX DMA
- Issues optional RX dma_async_issue_pending
- Issues TX dma_async_issue_pending

but you've not really root-caused by re-ordering the calls fixes 
anything for you.

This may be the right fix but I don't really think you've captured here 
in the commit log _why_ its the right fix if indeed it is correct.

> Signed-off-by: Viken Dadhaniya <quic_vdadhani@...cinc.com>

You should have a Fixes: tag

> ---
>   drivers/i2c/busses/i2c-qcom-geni.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 0d2e7171e3a6..5904fc8bba71 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -613,6 +613,11 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>   
>   		peripheral.addr = msgs[i].addr;
>   
> +		ret =  geni_i2c_gpi(gi2c, &msgs[i], &config,
> +				    &tx_addr, &tx_buf, I2C_WRITE, gi2c->tx_c);
> +		if (ret)
> +			goto err;
> +
>   		if (msgs[i].flags & I2C_M_RD) {
>   			ret =  geni_i2c_gpi(gi2c, &msgs[i], &config,
>   					    &rx_addr, &rx_buf, I2C_READ, gi2c->rx_c);
> @@ -620,11 +625,6 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>   				goto err;
>   		}
>   
> -		ret =  geni_i2c_gpi(gi2c, &msgs[i], &config,
> -				    &tx_addr, &tx_buf, I2C_WRITE, gi2c->tx_c);
> -		if (ret)
> -			goto err;
> -
>   		if (msgs[i].flags & I2C_M_RD)
>   			dma_async_issue_pending(gi2c->rx_c);

If TX gets moved up top then the second check for if (msgs[i].flags & 
I2C_M_RD) is redundant.

You could just have

if (msgs[i].flags & I2C_M_RD) {
         ret =  geni_i2c_gpi(gi2c, &msgs[i], &config,
                             &rx_addr, &rx_buf, I2C_READ, gi2c->rx_c);
         if (ret)
                 goto err;

         dma_async_issue_pending(gi2c->rx_c);
}

- Please investigate further.
   Why/how does the new sequence

   TX DMA setup
   RX DMA setup
   RX DMA sync
   TX DMA sync

   Improve the situation over the existing and more logical

   RX DMA setup
   TX DMA setup
   RX DMA sync
   TX DMA sync

- Add a Fixes tag if you work that out so we know
   which kernel version to back port to

- Include the SoC version(s) you have tested on in the commit
   or cover letter

- And drop the redundant check

---
bod