linux-kernel - RE: [PATCH V2 3/4] 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] [day] [month] [year] [list]

Date:   Sat, 26 Jan 2019 01:51:59 +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 V2 3/4] i2c: tegra: Add DMA Support

> > +static int tegra_i2c_init_dma_param(struct tegra_i2c_dev *i2c_dev,
> > +				    bool dma_to_memory)
> > +{
> > +	struct dma_chan *dma_chan;
> > +	u32 *dma_buf;
> > +	dma_addr_t dma_phys;
> > +	int ret;
> > +	const char *chan_name = dma_to_memory ? "rx" : "tx";
> > +
> > +	dma_chan = dma_request_slave_channel_reason(i2c_dev->dev, chan_name);
> > +	if (IS_ERR(dma_chan))
> > +		return PTR_ERR(dma_chan);
>
> Here shall be a check of whether dma_buf is already allocated, otherwise it will be allocated twice and the first allocation turned into memleak:
>
>	if (i2c_dev->dma_buf)
>		return 0;

If dma_buf is allocated already then dma channels will be assigned and it will not perform init dma again.
So if dma buf allocation happens already then it will not come here again.

> > +
> > +	dma_buf = dma_alloc_coherent(i2c_dev->dev, i2c_dev->dma_buf_size,
> > +				     &dma_phys, GFP_KERNEL);
> > +
>
> I'm wondering whether a write-combined DMA mapping will be more optimal than having L2 flushes / invalidation for the "coherent" allocations. 
>
> And I'm now questioning whether there is any real benefit from the DMA transferring at all, given the DMA bounce-buffer CPU read/write overhead. It looks to me that the whole purpose of the I2C DMA transferring is to move data from (to) some I2C device to a > > some device on the APB bus, bypassing the CPU. If that's is the case, then this patch may not really worth the effort and instead could only hurt the transferring performance. Please provide some performance results or correct me if I'm wrong.
>
> [snip]

DMA transfer helps to reduce overhead only during large transfer which is very rare operation. Will check and provide some performance data...

> >  
> >  	if (!i2c_dev->hw->has_single_clk_source) {
> >  		fast_clk = devm_clk_get(&pdev->dev, "fast-clk"); @@ -1079,6 
> > +1402,15 @@ static int tegra_i2c_probe(struct platform_device *pdev)
> >  		}
> >  	}
> >  
> > +	if (i2c_dev->has_dma) {
> > +		ret = tegra_i2c_init_dma_param(i2c_dev, true);
> > +		if (ret == -EPROBE_DEFER)
> > +			goto disable_div_clk;
> > +		ret = tegra_i2c_init_dma_param(i2c_dev, false);
> > +		if (ret == -EPROBE_DEFER)
> > +			goto disable_div_clk;
>
> Missing dma_buf freeing and channel releasing in a case of error.

dma buf freeing and channel release happens inside init_dma when buffer allocation fails.
If dma_request_slave_channel_reason fails no need to release channel but if channel request succeeds and buffer allocation fails, then buffer free and channel release will happen.

> > +	}
> > +
> >  	ret = tegra_i2c_init(i2c_dev);
> >  	if (ret) {
> >  		dev_err(&pdev->dev, "Failed to initialize i2c controller\n");
> >