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: <20090401071053.5ab0fb94@hcegtvedt>
Date:	Wed, 1 Apr 2009 07:10:53 +0200
From:	Hans-Christian Egtvedt <hans-christian.egtvedt@...el.com>
To:	"Sosnowski, Maciej" <maciej.sosnowski@...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

On Tue, 31 Mar 2009 15:03:07 +0100
"Sosnowski, Maciej" <maciej.sosnowski@...el.com> wrote:

> 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.
>

Thanks

> 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
> 

Agreed, will fixup.

> > +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?
> 

Hmmm, I'll look into this.

> > +	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)":
> 

Ah yes, will fixup.

> +	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);
> +	} 
> 

-- 
Best regards,
Hans-Christian Egtvedt
--
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/