lkml.org 
[lkml]   [2019]   [Feb]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH V9 3/5] i2c: tegra: Add DMA support
    Date

    > >
    > >> 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
    \
     
     \ /
      Last update: 2019-02-01 22:37    [W:4.041 / U:0.148 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site