linux-kernel - RE: [PATCH V8 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]
Message-ID: <BYAPR12MB3398734126428AC5881D0B28C2910@BYAPR12MB3398.namprd12.prod.outlook.com>
Date:   Thu, 31 Jan 2019 16:41:03 +0000
From:   Sowjanya Komatineni <skomatineni@...dia.com>
To:     Thierry Reding <thierry.reding@...il.com>
CC:     Jonathan Hunter <jonathanh@...dia.com>,
        Mantravadi Karthik <mkarthik@...dia.com>,
        Shardar Mohammed <smohammed@...dia.com>,
        Timo Alho <talho@...dia.com>,
        "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 V8 3/5] i2c: tegra: Add DMA Support

> > +static int tegra_i2c_init_dma_param(struct tegra_i2c_dev *i2c_dev) {
> > +	struct dma_chan *dma_chan;
> > +	u32 *dma_buf;
> > +	dma_addr_t dma_phys;
> > +
> > +	if (!i2c_dev->has_dma)
> > +		return -EINVAL;
> > +
> > +	if (!i2c_dev->rx_dma_chan) {
> > +		dma_chan = dma_request_slave_channel_reason(i2c_dev->dev, "rx");
> > +		if (IS_ERR(dma_chan))
> > +			return PTR_ERR(dma_chan);
>
> I think we want to fallback to PIO here if dma_chan is -ENODEV.

Checking if dmas property is in device tree before channel requests. So ENODEV will not be returned.
Incase if dmas property is not there in device tree, we fall back to PIO as init_dma_params returns invalid

> Although, I'm not exactly sure I understand what you're trying to achieve here. Shouldn't we move the channel request parts into probe and remove them from here? Otherwise it seems like we could get into a state where we keep trying to get the slave channels everytime we set up a DMA transfer, even if we already failed to do so during probe.
>

This patch tries to get channel request during probe but buffer allocation will not happen till xfer is needed in dma mode.
During xfer message, this function is called again for dma buffer allocation only for dma mode.

> > +
> > +	if (!i2c_dev->dma_buf && i2c_dev->msg_buf_remaining) {
> > +		dma_buf = dma_alloc_coherent(i2c_dev->dev,
> > +					     i2c_dev->dma_buf_size,
> > +					     &dma_phys, GFP_KERNEL);
> > +
> > +		if (!dma_buf) {
> > +			dev_err(i2c_dev->dev,
> > +				"failed to allocate the DMA buffer\n");
> > +			dma_release_channel(i2c_dev->tx_dma_chan);
> > +			dma_release_channel(i2c_dev->rx_dma_chan);
> > +			i2c_dev->tx_dma_chan = NULL;
> > +			i2c_dev->rx_dma_chan = NULL;
> > +			return -ENOMEM;
> > +		}
> > +
> > +		i2c_dev->dma_buf = dma_buf;
> > +		i2c_dev->dma_phys = dma_phys;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
>
>
> > @@ -749,19 +939,69 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
> >  	i2c_dev->msg_read = (msg->flags & I2C_M_RD);
> >  	reinit_completion(&i2c_dev->msg_complete);
> >  
> > +	if (i2c_dev->msg_read)
> > +		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) {
> > +		err = tegra_i2c_init_dma_param(i2c_dev);
> > +		if (err < 0) {
> > +			dev_dbg(i2c_dev->dev, "switching to PIO transfer\n");
> > +			dma = false;
> > +		}
>
>
> If we successfully got DMA channels at probe time, doesn't this turn into an error condition that is worth reporting? It seems to me like the only reason it could fail is if we fail the allocation, but then again, why don't we move the DMA buffer allocation into probe()? We already use a fixed size for that allocation, so there's no reason it couldn't be allocated at probe time.
>
> Seems like maybe you just overlooked that as you were moving around the code pieces.

Checking for dmas property is inside init_dma_param and if dmas property doesn't exist init_dma_param returns err EINVAL
Also init_dma_param fails for if buffer allocation fails. In both of those cases it will switch to PIO Mode
As per review feedback, performing channel request allocation during probe and buffer allocation is postponed till there is need for DMA and buffer allocation happens during 1st DMA mode xfer if it is not already allocated

>
>
>
> >  static const struct i2c_algorithm tegra_i2c_algo = { @@ -1002,11 
> > +1293,13 @@ static int tegra_i2c_probe(struct platform_device *pdev)
> >  	struct clk *div_clk;
> >  	struct clk *fast_clk;
> >  	void __iomem *base;
> > +	phys_addr_t base_phys;
> >  	int irq;
> >  	int ret = 0;
> >  	int clk_multiplier = I2C_CLK_MULTIPLIER_STD_FAST_MODE;
> >  
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	base_phys = res->start;
> >  	base = devm_ioremap_resource(&pdev->dev, res);
> >  	if (IS_ERR(base))
> >  		return PTR_ERR(base);
> > @@ -1029,6 +1322,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  
> >  	i2c_dev->base = base;
> > +	i2c_dev->base_phys = base_phys;
>
> We could probably do without the extra local variable by just moving the assignment here. res is still valid at this point.
Same res is used for both IORESOURCE_MEM ad IORESOURCE_IRQ and i2c_dev allocation happens later so I had to use temp variable