linux-kernel - Re: [PATCH v6] i2c: qcom-geni: Add support for GPI DMA

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]

Date:   Mon, 21 Feb 2022 10:46:38 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Wolfram Sang <wsa@...nel.org>, linux-arm-msm@...r.kernel.org,
        Andy Gross <agross@...nel.org>,
        Douglas Anderson <dianders@...omium.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        Alexey Minnekhanov <alexeymin@...tmarketos.org>,
        jorcrous@...zon.com, dmitry.baryshkov@...aro.org
Subject: Re: [PATCH v6] i2c: qcom-geni: Add support for GPI DMA

On 18-02-22, 11:07, Bjorn Andersson wrote:
> On Fri 18 Feb 10:55 PST 2022, Vinod Koul wrote:
> 
> > QUP Serial engines supports data transfers thru FIFO mode, SE DMA mode
> > and lastly GPI DMA mode. Former two are already supported and this adds
> > supports for the last mode.
> > 
> > In GPI DMA mode, the firmware is issued commands by driver to perform
> > DMA and setup the serial port.
> > 
> > Signed-off-by: Vinod Koul <vkoul@...nel.org>
> > ---
> > 
> > Changes since v5:
> >  - Fix comments by Bjorn, Dmitry and Jordan
> 
> Thanks, but it's useful to know what those things where. Please list the
> actual changes.

My bad or rather lazy to post late in night, here it is for record:
 - Fix the order of remove
 - make sure completion is called when status is !NOERROR
 - dont reset dma channel on exit
 - Make sure to return num on scucess for i2c xfer for both fifo and gpi
   mode

> 
> > 
> > Changes since v4:
> >  - Fix buildbot warn
> >  - Fix flase warn reported by Alexey
> >  - Fix feedback from Bjorn and cleanup the probe code and add more details
> >    in changelog
> > 
> > Changes since v3:
> >  - remove separate tx and rx function for gsi dma and make a common one
> >  - remove global structs and use local variables instead
> > 
> > 
> >  drivers/i2c/busses/i2c-qcom-geni.c | 303 ++++++++++++++++++++++++++---
> >  1 file changed, 275 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> > index 6d635a7c104c..e03544ba827c 100644
> > --- a/drivers/i2c/busses/i2c-qcom-geni.c
> > +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> > @@ -3,7 +3,9 @@
> >  
> >  #include <linux/acpi.h>
> >  #include <linux/clk.h>
> > +#include <linux/dmaengine.h>
> >  #include <linux/dma-mapping.h>
> > +#include <linux/dma/qcom-gpi-dma.h>
> >  #include <linux/err.h>
> >  #include <linux/i2c.h>
> >  #include <linux/interrupt.h>
> > @@ -48,6 +50,9 @@
> >  #define LOW_COUNTER_SHFT	10
> >  #define CYCLE_COUNTER_MSK	GENMASK(9, 0)
> >  
> > +#define I2C_PACK_TX		BIT(0)
> > +#define I2C_PACK_RX		BIT(1)
> > +
> >  enum geni_i2c_err_code {
> >  	GP_IRQ0,
> >  	NACK,
> > @@ -89,6 +94,9 @@ struct geni_i2c_dev {
> >  	void *dma_buf;
> >  	size_t xfer_len;
> >  	dma_addr_t dma_addr;
> > +	struct dma_chan *tx_c;
> > +	struct dma_chan *rx_c;
> > +	bool gpi_mode;
> >  };
> >  
> >  struct geni_i2c_err_log {
> > @@ -456,12 +464,202 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> >  	return gi2c->err;
> >  }
> >  
> > +static void i2c_gpi_cb_result(void *cb, const struct dmaengine_result *result)
> > +{
> > +	struct geni_i2c_dev *gi2c = cb;
> > +
> > +	if (result->result != DMA_TRANS_NOERROR) {
> > +		dev_err(gi2c->se.dev, "DMA txn failed:%d\n", result->result);
> > +		goto err_trans;
> 
> Maybe I'm just missing it, but aren't you completing done now to avoid
> the timeout, which causes the xfer to return success?
> 
> Note that, as I mentioned in previous review, we're under the adaptor
> lock, so I don't think you need any additional locking to pass a value
> back to xfer().

Yes we should pass the err value, will add that up in gi2c to pass
around...

> 
> > +	}
> > +
> > +	if (result->residue)
> 
> You can avoid the goto, and make it clear from the overall structure that
> only one (or none) of these paths will be taken.
> 
> 	if (NOERROR) {
> 
> 	} else if (result->residue) {
> 
> 	}

Sure

-- 
~Vinod