linux-kernel - Re: [PATCH v5 3/4] dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00

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: <20191105174823.GF952516@vkoul-mobl>
Date:   Tue, 5 Nov 2019 23:18:23 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Green Wan <green.wan@...ive.com>
Cc:     kbuild test robot <lkp@...el.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Palmer Dabbelt <palmer@...ive.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Dan Williams <dan.j.williams@...el.com>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        "Paul E. McKenney" <paulmck@...ux.ibm.com>,
        Yash Shah <yash.shah@...ive.com>,
        Bin Meng <bmeng.cn@...il.com>,
        Sagar Kadam <sagar.kadam@...ive.com>,
        dmaengine@...r.kernel.org, devicetree@...r.kernel.org,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/4] dmaengine: sf-pdma: add platform DMA support for
 HiFive Unleashed A00

On 28-10-19, 15:56, Green Wan wrote:
> Add PDMA driver, sf-pdma, to enable DMA engine on HiFive Unleashed
> Rev A00 board.
> 
>  - Implement dmaengine APIs, support MEM_TO_MEM async copy.
>  - Tested by DMA Test client
>  - Supports 4 channels DMA, each channel has 1 done and 1 err
>    interrupt connected to platform-level interrupt controller (PLIC).
>  - Depends on DMA_ENGINE and DMA_VIRTUAL_CHANNELS
> 
> The datasheet is here:
> 
>   https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> 
> Follow the DMAengine controller doc,
> "./Documentation/driver-api/dmaengine/provider.rst" to implement DMA
> engine. And use the dma test client in doc,
> "./Documentation/driver-api/dmaengine/dmatest.rst", to test.
> 
> Each DMA channel has separate HW regs and support done and error ISRs.
> 4 channels share 1 done and 1 err ISRs. There's no expander/arbitrator
> in DMA HW.
> 
>    ------               ------
>    |    |--< done 23 >--|ch 0|
>    |    |--< err  24 >--|    |     (dma0chan0)
>    |    |               ------
>    |    |               ------
>    |    |--< done 25 >--|ch 1|
>    |    |--< err  26 >--|    |     (dma0chan1)
>    |PLIC|               ------
>    |    |               ------
>    |    |--< done 27 >--|ch 2|
>    |    |--< err  28 >--|    |     (dma0chan2)
>    |    |               ------
>    |    |               ------
>    |    |--< done 29 >--|ch 3|
>    |    |--< err  30 >--|    |     (dma0chan3)
>    ------               ------
> 
> Reviewed-by: Vinod Koul <vkoul@...nel.org>

when did i provide this?

> Signed-off-by: Green Wan <green.wan@...ive.com>
> Reported-by: kbuild test robot <lkp@...el.com>
> Fixes: 31c3b98b5a01 ("dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00")

Fixes what... this is not a upstream commit?

> Signed-off-by: kbuild test robot <lkp@...el.com>
> ---

Please list the changes done from prev version, here or in cover letter

> +static struct sf_pdma_desc *sf_pdma_alloc_desc(struct sf_pdma_chan *chan)
> +{
> +	struct sf_pdma_desc *desc;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&chan->lock, flags);
> +
> +	if (chan->desc && !chan->desc->in_use) {
> +		spin_unlock_irqrestore(&chan->lock, flags);
> +		return chan->desc;
> +	}
> +
> +	spin_unlock_irqrestore(&chan->lock, flags);
> +
> +	desc = kzalloc(sizeof(*desc), GFP_NOWAIT);
> +

this empty line in not required

> +static struct dma_async_tx_descriptor *
> +	sf_pdma_prep_dma_memcpy(struct dma_chan *dchan,
> +				dma_addr_t dest,

please make it left justified

> +static int sf_pdma_slave_config(struct dma_chan *dchan,
> +				struct dma_slave_config *cfg)
> +{
> +	struct sf_pdma_chan *chan = to_sf_pdma_chan(dchan);
> +
> +	memcpy(&chan->cfg, cfg, sizeof(*cfg));
> +	chan->dma_dir = DMA_MEM_TO_MEM;

?? looking at changelog we have only memcpy support, so this should not
be here, pls remove this.

> +static enum dma_status
> +sf_pdma_tx_status(struct dma_chan *dchan,
> +		  dma_cookie_t cookie,
> +		  struct dma_tx_state *txstate)
> +{
> +	struct sf_pdma_chan *chan = to_sf_pdma_chan(dchan);
> +	enum dma_status status;
> +
> +	status = dma_cookie_status(dchan, cookie, txstate);
> +
> +	if (txstate && status != DMA_ERROR)
> +		dma_set_residue(txstate, sf_pdma_desc_residue(chan));

which residue? the query can be for a cookie which is still in pending
list! you need to check the cookie and only read register for cookie if
submitted

> +static int sf_pdma_remove(struct platform_device *pdev)
> +{
> +	struct sf_pdma *pdma = platform_get_drvdata(pdev);
> +	struct sf_pdma_chan *ch;
> +	int i;
> +
> +	for (i = 0; i < PDMA_NR_CH; i++) {
> +		ch = &pdma->chans[i];
> +
> +		list_del(&ch->vchan.chan.device_node);
> +		tasklet_kill(&ch->vchan.task);
> +		tasklet_kill(&ch->done_tasklet);
> +		tasklet_kill(&ch->err_tasklet);

you have an isr registered which can fire and schedule tasklets..
-- 
~Vinod