02.02.2019 0:36, 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.
>
> OK, my wrong assumption. Thought dma_release_channel will NULL chan after its freed
> Will update and send V10
>
There is no need to rush, you may wait at least for the testing feedback for now. Also maybe Thierry or somebody else will have something to say as well.
