linux-kernel - RE: [PATCH V9 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: <BYAPR12MB339835E59C2D4DCDFC93BEF4C2920@BYAPR12MB3398.namprd12.prod.outlook.com>
Date:   Fri, 1 Feb 2019 21:36:41 +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 V9 3/5] i2c: tegra: Add DMA support


> > 
> >> 	rx_chan = dma_request_slave_channel_reason(i2c_dev->dev, "rx");
> >> 	if (IS_ERR(rx_chan))
> >> 		return PTR_ERR(rx_chan); 
> >>
> >>
> >>> +
> >>> +	dma_chan = dma_request_slave_channel_reason(i2c_dev->dev, "tx");
> >>> +	if (IS_ERR(dma_chan)) {
> >>> +		err = PTR_ERR(dma_chan);
> >>> +		goto error;
> >>
> >> It's a good practice to release resources in opposite order to the allocation. Hence better to write this as:
> >>
> >> 		goto err_release_rx;
> >>
> >>> +	}
> >>> +
> >>> +	i2c_dev->tx_dma_chan = dma_chan;
> >>> +
> >>> +	dma_buf = dma_alloc_coherent(i2c_dev->dev,
> >>> +				     i2c_dev->dma_buf_size, &dma_phys,
> >>> +				     GFP_KERNEL | __GFP_NOWARN);
> >>> +
> >>> +	if (!dma_buf) {
> >>> +		dev_err(i2c_dev->dev, "failed to allocate the DMA buffer\n");
> >>> +		err = -ENOMEM;
> >>> +		goto error;
> >>
> >> 		goto err_release_tx;
> >>
> >>> +	}
> >>> +
> >>> +	i2c_dev->dma_buf = dma_buf;
> >>> +	i2c_dev->dma_phys = dma_phys;	
>
>	i2c_dev->rx_dma_chan = rx_chan;
> >> 	i2c_dev->tx_dma_chan = tx_chan;
> >>
> >>> +	return 0;
> >>> +
> >>> +error:
> >>> +	if (i2c_dev->tx_dma_chan)
> >>> +		dma_release_channel(i2c_dev->tx_dma_chan);
> >>> +
> >>> +	if (i2c_dev->rx_dma_chan)
> >>> +		dma_release_channel(i2c_dev->rx_dma_chan); 
> >>
> >> error_release_tx:
> >> 	dma_release_channel(tx_chan);
> >> error_release_rx:
> >> 	dma_release_channel(rx_chan);
> >>
> >>> +
> >>> +	return err;
> > 
> > I am releasing resources in reverse order to allocation.
> > Trying for rx allocation followed by tx allocation During release 
> > releasing tx and then rx.
> > In case if tx allocation fails, doesn’t go thru release. If rx or buf 
> > allocation fails, releases tx first and then rx
> > 
>
>
> Okay. Anyway it's a good-n-common practice to write it in the way I'm suggesting.
>
> And please set rx_chan and tx_chan after dma_buf allocation as I'm suggesting because you current variant will crash kernel since if dma_buf allocation fails, both rx and tx channels will be released and you're not setting them to NULL in that case.

OK, my wrong assumption. Thought dma_release_channel will NULL chan after its freed
Will update and send V10