Re: [PATCH v15 2/4] dmaengine: tegra: Add tegra gpcdma driver

[Dmitry Osipenko <digetx@gmail.com>]

20.12.2021 18:23, Akhil R пишет:
>> 16.12.2021 20:11, Akhil R пишет:
>>> +static int tegra_dma_terminate_all(struct dma_chan *dc) {
>>> +     struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>> +     unsigned long wcount = 0;
>>> +     unsigned long status;
>>> +     unsigned long flags;
>>> +     int err;
>>> +
>>> +     raw_spin_lock_irqsave(&tdc->lock, flags);
>>> +
>>> +     if (!tdc->dma_desc) {
>>> +             raw_spin_unlock_irqrestore(&tdc->lock, flags);
>>> +             return 0;
>>> +     }
>>> +
>>> +     if (!tdc->busy)
>>> +             goto skip_dma_stop;
>>> +
>>> +     if (tdc->tdma->chip_data->hw_support_pause)
>>> +             err = tegra_dma_pause(tdc);
>>> +     else
>>> +             err = tegra_dma_stop_client(tdc);
>>> +
>>> +     if (err) {
>>> +             raw_spin_unlock_irqrestore(&tdc->lock, flags);
>>> +             return err;
>>> +     }
>>> +
>>> +     status = tdc_read(tdc, TEGRA_GPCDMA_CHAN_STATUS);
>>> +     if (status & TEGRA_GPCDMA_STATUS_ISE_EOC) {
>>> +             dev_dbg(tdc2dev(tdc), "%s():handling isr\n", __func__);
>>> +             tegra_dma_xfer_complete(tdc);
>>> +             status = tdc_read(tdc, TEGRA_GPCDMA_CHAN_STATUS);
>>> +     }
>>> +
>>> +     wcount = tdc_read(tdc, TEGRA_GPCDMA_CHAN_XFER_COUNT);
>>> +     tegra_dma_stop(tdc);
>>> +
>>> +     tdc->dma_desc->bytes_transferred +=
>>> +                     tdc->dma_desc->bytes_requested - (wcount * 4);
>>> +
>>> +skip_dma_stop:
>>> +     tegra_dma_sid_free(tdc);
>>> +     vchan_free_chan_resources(&tdc->vc);
>>> +     kfree(&tdc->vc);
>>
>> You really going to kfree the head of tegra_dma_channel here? Once again, this
>> code was 100% untested :/
> I did validate this using DMATEST which did not show any error.
> https://www.kernel.org/doc/html/latest/driver-api/dmaengine/dmatest.html
> Do you suggest something better?
> 
>> You're not allowed to free channel from the dma_terminate_all() callback. This
>> callback terminates submitted descs, that's it.
>>
> Sorry, I am relatively new to DMA framework (probably you get it from the patch 
> version no. :)). I read your previous comment as to use tdc->vc instead of dma_desc.
> I would learn a bit more and update with a change. Thanks for the inputs.

Looks like DMATEST doesn't try to terminate in a middle of transfer and
then check that further transfers work. You may try to extend DMATEST or
simulate I2C error to test it, you should also test it with enabled KASAN.