linux-kernel - Re: [PATCH v3 2/4] dmaengine: tegra: Add tegra gpcdma driver

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: <64505970-1d68-b4cb-490b-7bfa1c842a1f@nvidia.com>
Date:   Thu, 2 Sep 2021 11:57:19 +0100
From:   Jon Hunter <jonathanh@...dia.com>
To:     Akhil R <akhilrajeev@...dia.com>, <rgumasta@...dia.com>
CC:     <dan.j.williams@...el.com>, <dmaengine@...r.kernel.org>,
        <kyarlagadda@...dia.com>, <ldewangan@...dia.com>,
        <linux-kernel@...r.kernel.org>, <linux-tegra@...r.kernel.org>,
        <p.zabel@...gutronix.de>, <thierry.reding@...il.com>,
        <vkoul@...nel.org>, Pavan Kunapuli <pkunapuli@...dia.com>
Subject: Re: [PATCH v3 2/4] dmaengine: tegra: Add tegra gpcdma driver


On 01/09/2021 21:56, Jon Hunter wrote:
> 
> On 27/08/2021 07:04, Akhil R wrote:
>> Adding GPC DMA controller driver for Tegra186 and Tegra194. The driver
>> supports dma transfers between memory to memory, IO peripheral to memory
>> and memory to IO peripheral.
>>
>> Signed-off-by: Pavan Kunapuli <pkunapuli@...dia.com>
>> Signed-off-by: Rajesh Gumasta <rgumasta@...dia.com>
>> Signed-off-by: Akhil R <akhilrajeev@...dia.com>
>> ---
>>   drivers/dma/Kconfig         |   12 +
>>   drivers/dma/Makefile        |    1 +
>>   drivers/dma/tegra-gpc-dma.c | 1343 
>> +++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 1356 insertions(+)
>>   create mode 100644 drivers/dma/tegra-gpc-dma.c

...

>> +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;
>> +    bool was_busy;
>> +    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);
>> +        if (err) {
>> +            raw_spin_unlock_irqrestore(&tdc->lock, flags);
>> +            return err;
>> +        }
>> +    } else {
>> +        /* Before Reading DMA status to figure out number
>> +         * of bytes transferred by DMA channel:
>> +         * Change the client associated with the DMA channel
>> +         * to stop DMA engine from starting any more bursts for
>> +         * the given client and wait for in flight bursts to complete
>> +         */
>> +        tegra_dma_reset_client(tdc);
>> +
>> +        /* Wait for in flight data transfer to finish */
>> +        udelay(TEGRA_GPCDMA_BURST_COMPLETE_TIME);
>> +
>> +        /* If TX/RX path is still active wait till it becomes
>> +         * inactive
>> +         */
>> +
>> +        if (readl_relaxed_poll_timeout_atomic(tdc->tdma->base_addr +
>> +                tdc->chan_base_offset +
>> +                TEGRA_GPCDMA_CHAN_STATUS,
>> +                status,
>> +                !(status & (TEGRA_GPCDMA_STATUS_CHANNEL_TX |
>> +                TEGRA_GPCDMA_STATUS_CHANNEL_RX)),
>> +                5,
>> +                TEGRA_GPCDMA_BURST_COMPLETION_TIMEOUT)) {
>> +            dev_dbg(tdc2dev(tdc), "Timeout waiting for DMA burst 
>> completion!\n");
>> +            tegra_dma_dump_chan_regs(tdc);
>> +        }
> 
> I would be tempted to make the code in the 'else' clause 
> tegra_dma_sw_pause(). Then you could have tegra_dma_hw_pause() and 
> tegra_dma_sw_pause().


Thinking some more tegra_dma_hw_pause() and tegra_dma_sw_pause() it not 
very clear/accurate. I would be tempted to call these tegra_dma_pause() 
and tegra_dma_stop_client() or tegra_dma_stop_transactions(), because 
without having a proper hardware pause, you are simply ignoring the 
client sync events.

Jon
-- 
nvpublic