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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: 
 <SJ0PR02MB7758BB7F88DB478E5A7B130DEB7E2@SJ0PR02MB7758.namprd02.prod.outlook.com>
Date: Mon, 29 Jan 2024 06:15:13 +0000
From: Viken Dadhaniya <vdadhani@....qualcomm.com>
To: "bryan.odonoghue@...aro.org" <bryan.odonoghue@...aro.org>,
        "Viken
 Dadhaniya (QUIC)" <quic_vdadhani@...cinc.com>,
        "andersson@...nel.org"
	<andersson@...nel.org>,
        "konrad.dybcio@...aro.org"
	<konrad.dybcio@...aro.org>,
        "andi.shyti@...nel.org" <andi.shyti@...nel.org>,
        "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "vkoul@...nel.org" <vkoul@...nel.org>,
        "Bjorn Andersson (QUIC)"
	<quic_bjorande@...cinc.com>,
        "manivannan.sadhasivam@...aro.org"
	<manivannan.sadhasivam@...aro.org>
CC: "Mukesh Savaliya (QUIC)" <quic_msavaliy@...cinc.com>,
        "Visweswara Tanuku
 (QUIC)" <quic_vtanuku@...cinc.com>
Subject: RE: [PATCH 1/1] i2c: i2c-qcom-geni: Correct I2C TRE sequence



> -----Original Message-----
> From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
> Sent: Friday, January 12, 2024 8:25 PM
> To: Viken Dadhaniya (QUIC) <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: Mukesh Savaliya (QUIC) <quic_msavaliy@...cinc.com>; Visweswara Tanuku
> (QUIC) <quic_vtanuku@...cinc.com>
> Subject: Re: [PATCH 1/1] i2c: i2c-qcom-geni: Correct I2C TRE sequence
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> 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.

Updated commit massage with proper information.

> 
> > Signed-off-by: Viken Dadhaniya <quic_vdadhani@...cinc.com>
> 
> You should have a Fixes: tag

Added 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

Removed redundant check.
Added SoC information.

> 
> ---
> bod