Messages in this thread | | | Subject | Re: [PATCH V9 3/5] i2c: tegra: Add DMA support | From | Dmitry Osipenko <> | Date | Fri, 1 Feb 2019 23:30:55 +0300 |
| |
01.02.2019 23:21, Sowjanya Komatineni пишет: > >> 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.
| |