linux-kernel - RE: [PATCH 2/2] dw_dmac: add cyclic API to DW DMA 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: <129600E5E5FB004392DDC3FB599660D790F8C3F0@irsmsx504.ger.corp.intel.com>
Date:	Tue, 31 Mar 2009 15:03:07 +0100
From:	"Sosnowski, Maciej" <maciej.sosnowski@...el.com>
To:	Hans-Christian Egtvedt <hans-christian.egtvedt@...el.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Williams, Dan J" <dan.j.williams@...el.com>
Subject: RE: [PATCH 2/2] dw_dmac: add cyclic API to DW DMA driver

Hans-Christian Egtvedt wrote:
> This patch adds a cyclic DMA interface to the DW DMA driver. This is
> very useful if you want to use the DMA controller in combination with a 
> sound device which uses cyclic buffers.
> 
> Using a DMA channel for cyclic DMA will disable the possibility to use
> it as a normal DMA engine until the user calls the cyclic free function 
> on the DMA channel. Also a cyclic DMA list can not be prepared if the
> channel is already active.
> 
> Signed-off-by: Hans-Christian Egtvedt <hans-christian.egtvedt@...el.com>
> ---
>  drivers/dma/dw_dmac.c      |  303  +++++++++++++++++++++++++++++++++++++++++++-
>  drivers/dma/dw_dmac_regs.h |    7 +- include/linux/dw_dmac.h    |    19 +++
>  3 files changed, 327 insertions(+), 2 deletions(-)
>

The patch looks good to me.
Just a few remarks below:

> +		for (i = 0; i < dwc->cdesc->periods; i++)
> +			dwc_dump_lli(dwc, &dwc->cdesc->desc[i]->lli);
> +
> +		return;
> +	}
> +}

This return is not needed

> +void dw_dma_cyclic_stop(struct dma_chan *chan)
> +{
> +	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
> +	struct dw_dma		*dw = to_dw_dma(dwc->chan.device);
> +
> +	channel_clear_bit(dw, CH_EN, dwc->mask);
> +	while (dma_readl(dw, CH_EN) & dwc->mask)
> +		cpu_relax();
> +}

Don't you need locks in dw_dma_cyclic_stop?

> +	was_cyclic = test_and_set_bit(DW_DMA_IS_CYCLIC, &dwc->flags);
> +	if (was_cyclic) {
> +		spin_unlock_bh(&dwc->lock);
> +		dev_dbg(chan2dev(&dwc->chan),
> +				"channel already prepared for cyclic DMA\n");
> +		return ERR_PTR(-EBUSY);
> +	}
> +	spin_unlock_bh(&dwc->lock);

In this case both spin_unlock_bh() could be replaced by single one called before "if (was_cyclic)":

+	was_cyclic = test_and_set_bit(DW_DMA_IS_CYCLIC, &dwc->flags);
+	spin_unlock_bh(&dwc->lock);
+	if (was_cyclic) {
+		dev_dbg(chan2dev(&dwc->chan),
+				"channel already prepared for cyclic DMA\n");
+		return ERR_PTR(-EBUSY);
+	} 

Regards,
Maciej--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/