linux-kernel - RE: [PATCH V7 3/5] i2c: tegra: Add DMA Support

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:   Thu, 31 Jan 2019 01:19:13 +0000
From:   Sowjanya Komatineni <skomatineni@...dia.com>
To:     Dmitry Osipenko <digetx@...il.com>,
        "thierry.reding@...il.com" <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Mantravadi Karthik <mkarthik@...dia.com>,
        Shardar Mohammed <smohammed@...dia.com>,
        Timo Alho <talho@...dia.com>
CC:     "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
Subject: RE: [PATCH V7 3/5] i2c: tegra: Add DMA Support

> > +			dma_sync_single_for_device(i2c_dev->dev,
> > +						   i2c_dev->dma_phys,
> > +						   xfer_size,
> > +						   DMA_FROM_DEVICE);
> > +			ret = tegra_i2c_dma_submit(i2c_dev, xfer_size);
> > +			if (ret < 0) {
> > +				dev_err(i2c_dev->dev,
> > +					"Starting RX DMA failed, err %d\n",
> > +					ret);
> > +				goto exit;
> > +			}
> > +		} else {
> > +			chan = i2c_dev->tx_dma_chan;
> > +			tegra_i2c_config_fifo_trig(i2c_dev, xfer_size,
> > +						   DATA_DMA_DIR_TX);
> > +			/* Make the dma buffer to read by cpu */> +			dma_sync_single_for_cpu(i2c_dev->dev,
> > +						i2c_dev->dma_phys,
> > +						xfer_size,
> > +						DMA_TO_DEVICE);
>
> This is not correct, you need to call dma_sync_single_for_cpu() after completion of the transfer to give back dma buffer ownership to CPU, see below. This dma_sync_single_for_cpu() invocation should be removed.

Transfer is not done yet. Dma buffer ownership is given to CPU here before copying header bytes and msg bytes into dma buffer before actually transmitting

> > +
> > +		if (i2c_dev->msg_read) {
> > +			if (likely(i2c_dev->msg_err == I2C_ERR_NONE)) {
> > +				dma_sync_single_for_cpu(i2c_dev->dev,
> > +							i2c_dev->dma_phys,
> > +							xfer_size,
> > +							DMA_FROM_DEVICE);
> > +
> > +				memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf,
> > +					msg->len);
> > +			}
> > +		}
>
> Here you should give back dma buffer ownership to CPU:

After transfer is done, for msg reads, dma buffer ownership is given to CPU and read data from dma buffer is copied to msg buffer
For msg writes, no need of ownership to CPU.
> >
> >		else {
> >			dma_sync_single_for_cpu(i2c_dev->dev,
> >						i2c_dev->dma_phys,
> >						xfer_size,
> >						DMA_TO_DEVICE);
> >		}
>
> >  	time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> >  						TEGRA_I2C_TIMEOUT);
> >  	tegra_i2c_mask_irq(i2c_dev, int_mask);
> >  
> >  	if (time_left == 0) {
> >  		dev_err(i2c_dev->dev, "i2c transfer timed out\n");
> > +		if (dma) {
> > +			dmaengine_terminate_all(chan);
> > +			complete(&i2c_dev->dma_complete);
> > +		}
>
> DMA transfer has been completed at this point, hence this hunk isn't needed. Please remove it.

DMA complete alone doesn’t guarantee the transfer. Packets/All packets xfer interrupt from I2C confirms complete transaction along with dma complete check.
So still need to check for msg_complete timeout. 

> > @@ -740,6 +925,32 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
> >  	u32 int_mask;
> >  	unsigned long time_left;
> >  	unsigned long flags;
> > +	size_t xfer_size;
> > +	u32 *buffer = 0;
> > +	int ret = 0;
> > +	bool dma = false;
> > +
> > +	if (msg->flags & I2C_M_RD)
> > +		xfer_size = msg->len;
> > +	else
> > +		xfer_size = msg->len + I2C_PACKET_HEADER_SIZE;
> > +
> > +	xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD);
> > +	dma = (xfer_size > I2C_PIO_MODE_MAX_LEN);
> > +	if (dma) {
> > +		if ((msg->flags & I2C_M_RD) && !i2c_dev->rx_dma_chan)
> > +			ret = tegra_i2c_init_dma_param(i2c_dev, true);
> > +		else if (!i2c_dev->tx_dma_chan)
> > +			ret = tegra_i2c_init_dma_param(i2c_dev, false);
>
> In the comment to V3 I mentioned that it's not a good idea to request channels dynamically because suspend-resume order is based on devices registration order, in this case APB DMA must be probed before I2C. Please move channels allocation into the probe.
>
> This also raises the question about the need to register I2C driver from the subsys-init level because APB driver is getting registered from the module-init level and hence I2C probing will be deferred until APB DMA driver is registered. It looks to me that the subsys-init is a relict of the past and it should be fine to move I2C driver registration into the module-init level, of course it's not strictly necessary and could be done later on if desired.
>
> > +		if (ret < 0) {
> > +			dev_dbg(i2c_dev->dev, "Switching to PIO mode\n");
> > +			dma = false;
> > +			ret = 0;
> > +		}
> > +	}
> > +
> > +	i2c_dev->is_curr_dma_xfer = dma;
>
>
Since your previous feedback suggest "let's postpone channels requesting and dma_buf allocation until they are really needed", I thought it make sense to not request channels and allocate till DMA is needed.
So moved from probe to xfer_msg function. By the time it gets to xfer msg function, devices registration should be done already along with apb dma probe.