| 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
| ||
|
Message-ID: <4D9C618A.9020708@atmel.com>
Date: Wed, 06 Apr 2011 14:50:18 +0200
From: Nicolas Ferre <nicolas.ferre@...el.com>
To: "Koul, Vinod" <vinod.koul@...el.com>
CC: dan.j.williams@...el.com, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/5] dmaengine: at_hdmac: add cyclic DMA operation support
Le 06/04/2011 12:08, Koul, Vinod :
> On Fri, 2011-04-01 at 19:19 +0200, Nicolas Ferre wrote:
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@...el.com>
>> ---
>> drivers/dma/at_hdmac.c | 188 +++++++++++++++++++++++++++++++++++++++---
>> drivers/dma/at_hdmac_regs.h | 14 +++-
>> 2 files changed, 186 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
>> index 13050e6..fe9e1de 100644
>> --- a/drivers/dma/at_hdmac.c
>> +++ b/drivers/dma/at_hdmac.c
>> @@ -14,6 +14,8 @@
>> * The driver has currently been tested with the Atmel AT91SAM9RL
>> * and AT91SAM9G45 series.
>> */
>> +#define DEBUG 12
>> +#define VERBOSE_DEBUG 12
> Please don't define here and use debug options available for build
Yes, sure sorry.
>> +/**
>> + * atc_handle_cyclic - at the end of a period, run callback function
>> + * @atchan: channel used for cyclic operations
>> + *
>> + * Called with atchan->lock held and bh disabled
>> + */
>> +static void atc_handle_cyclic(struct at_dma_chan *atchan)
>> +{
>> + struct at_desc *first = atc_first_active(atchan);
>> + struct dma_async_tx_descriptor *txd = &first->txd;
>> + dma_async_tx_callback callback = txd->callback;
>> + void *param = txd->callback_param;
>> +
>> + dev_vdbg(chan2dev(&atchan->chan_common),
>> + "new cyclic period llp 0x%08x\n",
>> + channel_readl(atchan, DSCR));
>> +
>> + if (callback)
>> + callback(param);
>> +}
> You dont seem to be doing much expect calling callback, so doesn't it
> make sense to write so much code for just calling callback?
I do not totally follow you here: you mean that I should reduce the
amount of variables in this function or is it a more global comment
about my way of handling cyclic operations?
>>
>> /*-- IRQ & Tasklet ---------------------------------------------------*/
>>
>> @@ -434,8 +459,10 @@ static void atc_tasklet(unsigned long data)
>> }
>>
>> spin_lock(&atchan->lock);
>> - if (test_and_clear_bit(0, &atchan->error_status))
>> + if (test_and_clear_bit(ATC_IS_ERROR, &atchan->status))
>> atc_handle_error(atchan);
>> + else if (test_bit(ATC_IS_CYCLIC, &atchan->status))
>> + atc_handle_cyclic(atchan);
>> else
>> atc_advance_work(atchan);
>>
>> @@ -469,7 +496,7 @@ static irqreturn_t at_dma_interrupt(int irq, void *dev_id)
>> /* Disable channel on AHB error */
>> dma_writel(atdma, CHDR, atchan->mask);
>> /* Give information to tasklet */
>> - set_bit(0, &atchan->error_status);
>> + set_bit(ATC_IS_ERROR, &atchan->status);
>> }
>> tasklet_schedule(&atchan->tasklet);
>> ret = IRQ_HANDLED;
>> @@ -759,6 +786,127 @@ err_desc_get:
>> return NULL;
>> }
>>
>> +/**
>> + * atc_dma_cyclic_prep - prepare the cyclic DMA transfer
>> + * @chan: the DMA channel to prepare
>> + * @buf_addr: physical DMA address where the buffer starts
>> + * @buf_len: total number of bytes for the entire buffer
>> + * @period_len: number of bytes for each period
>> + * @direction: transfer direction, to or from device
>> + */
>> +static struct dma_async_tx_descriptor *
>> +atc_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
>> + size_t period_len, enum dma_data_direction direction)
>> +{
>> + struct at_dma_chan *atchan = to_at_dma_chan(chan);
>> + struct at_dma_slave *atslave = chan->private;
>> + struct at_desc *first = NULL;
>> + struct at_desc *prev = NULL;
>> + unsigned long was_cyclic;
>> + unsigned int periods = buf_len / period_len;
>> + unsigned int reg_width;
>> + u32 ctrla;
>> + u32 ctrlb;
>> + unsigned int i;
>> +
>> + dev_vdbg(chan2dev(chan), "prep_dma_cyclic: %s buf@...08x - %d (%d/%d)\n",
>> + direction == DMA_TO_DEVICE ? "TO DEVICE" : "FROM DEVICE",
>> + buf_addr,
>> + periods, buf_len, period_len);
>> +
>> + if (unlikely(!atslave || !buf_len || !period_len)) {
>> + dev_dbg(chan2dev(chan), "prep_dma_cyclic: length is zero!\n");
>> + return NULL;
>> + }
>> +
>> + was_cyclic = test_and_set_bit(ATC_IS_CYCLIC, &atchan->status);
>> + if (was_cyclic) {
>> + dev_dbg(chan2dev(chan), "prep_dma_cyclic: channel in use!\n");
>> + return NULL;
>> + }
>> +
>> + reg_width = atslave->reg_width;
>> +
>> + /* Check for too big/unaligned periods and unaligned DMA buffer */
>> + if (period_len > (ATC_BTSIZE_MAX << reg_width))
>> + goto err_out;
>> + if (unlikely(period_len & ((1 << reg_width) - 1)))
>> + goto err_out;
>> + if (unlikely(buf_addr & ((1 << reg_width) - 1)))
>> + goto err_out;
>> + if (unlikely(!(direction & (DMA_TO_DEVICE | DMA_FROM_DEVICE))))
>> + goto err_out;
>> +
>> + /* prepare common CRTLA/CTRLB values */
>> + ctrla = ATC_DEFAULT_CTRLA | atslave->ctrla
>> + | ATC_DST_WIDTH(reg_width)
>> + | ATC_SRC_WIDTH(reg_width)
>> + | period_len >> reg_width;
>> + ctrlb = ATC_DEFAULT_CTRLB;
>> +
>> + /* build cyclic linked list */
>> + for (i = 0; i < periods; i++) {
> would help making code look cleaner if you split this one to seprate
> function
Ok, I will see what I can do... and will post a v2 soon.
>> + struct at_desc *desc;
>> +
>> + desc = atc_desc_get(atchan);
>> + if (!desc)
>> + goto err_desc_get;
>> +
>> + switch (direction) {
>> + case DMA_TO_DEVICE:
>> + desc->lli.saddr = buf_addr + (period_len * i);
>> + desc->lli.daddr = atslave->tx_reg;
>> + desc->lli.ctrla = ctrla;
>> + desc->lli.ctrlb = ctrlb
>> + | ATC_DST_ADDR_MODE_FIXED
>> + | ATC_SRC_ADDR_MODE_INCR
>> + | ATC_FC_MEM2PER;
>> + break;
>> +
>> + case DMA_FROM_DEVICE:
>> + desc->lli.saddr = atslave->rx_reg;
>> + desc->lli.daddr = buf_addr + (period_len * i);
>> + desc->lli.ctrla = ctrla;
>> + desc->lli.ctrlb = ctrlb
>> + | ATC_DST_ADDR_MODE_INCR
>> + | ATC_SRC_ADDR_MODE_FIXED
>> + | ATC_FC_PER2MEM;
>> + break;
>> +
>> + default:
>> + return NULL;
>> + }
>> +
>> + if (!first) {
>> + first = desc;
>> + } else {
>> + /* inform the HW lli about chaining */
>> + prev->lli.dscr = desc->txd.phys;
>> + /* insert the link descriptor to the LD ring */
>> + list_add_tail(&desc->desc_node,
>> + &first->tx_list);
>> + }
>> + prev = desc;
>> + }
>> +
>> + /* lets make a cyclic list */
>> + prev->lli.dscr = first->txd.phys;
>> +
>> + /* First descriptor of the chain embedds additional information */
>> + first->txd.cookie = -EBUSY;
>> + first->len = buf_len;
>> +
>> + return &first->txd;
>> +
>> +err_desc_get:
>> + dev_err(chan2dev(chan), "not enough descriptors available\n");
>> + atc_desc_put(atchan, first);
>> +err_out:
>> + clear_bit(ATC_IS_CYCLIC, &atchan->status);
>> + return NULL;
>> +}
>> +
>> +
>> static int atc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
>> unsigned long arg)
>> {
>> @@ -793,6 +941,9 @@ static int atc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
>> list_for_each_entry_safe(desc, _desc, &list, desc_node)
>> atc_chain_complete(atchan, desc);
>>
>> + /* if channel dedicated to cyclic operations, free it */
>> + clear_bit(ATC_IS_CYCLIC, &atchan->status);
>> +
>> spin_unlock_bh(&atchan->lock);
>>
>> return 0;
>> @@ -853,6 +1004,10 @@ static void atc_issue_pending(struct dma_chan *chan)
>>
>> dev_vdbg(chan2dev(chan), "issue_pending\n");
>>
>> + /* Not needed for cyclic transfers */
>> + if (test_bit(ATC_IS_CYCLIC, &atchan->status))
>> + return;
>> +
>> spin_lock_bh(&atchan->lock);
>> if (!atc_chan_is_enabled(atchan)) {
>> atc_advance_work(atchan);
>> @@ -1092,10 +1247,15 @@ static int __init at_dma_probe(struct platform_device *pdev)
>> if (dma_has_cap(DMA_MEMCPY, atdma->dma_common.cap_mask))
>> atdma->dma_common.device_prep_dma_memcpy = atc_prep_dma_memcpy;
>>
>> - if (dma_has_cap(DMA_SLAVE, atdma->dma_common.cap_mask)) {
>> + if (dma_has_cap(DMA_SLAVE, atdma->dma_common.cap_mask))
>> atdma->dma_common.device_prep_slave_sg = atc_prep_slave_sg;
>> +
>> + if (dma_has_cap(DMA_CYCLIC, atdma->dma_common.cap_mask))
>> + atdma->dma_common.device_prep_dma_cyclic = atc_prep_dma_cyclic;
>> +
>> + if (dma_has_cap(DMA_SLAVE, atdma->dma_common.cap_mask) ||
>> + dma_has_cap(DMA_CYCLIC, atdma->dma_common.cap_mask))
>> atdma->dma_common.device_control = atc_control;
>> - }
>>
>> dma_writel(atdma, EN, AT_DMA_ENABLE);
>>
>> diff --git a/drivers/dma/at_hdmac_regs.h b/drivers/dma/at_hdmac_regs.h
>> index 8303306..c79a9e0 100644
>> --- a/drivers/dma/at_hdmac_regs.h
>> +++ b/drivers/dma/at_hdmac_regs.h
>> @@ -181,12 +181,22 @@ txd_to_at_desc(struct dma_async_tx_descriptor *txd)
>> /*-- Channels --------------------------------------------------------*/
>>
>> /**
>> + * atc_status - information bits stored in channel status flag
>> + *
>> + * Manipulated with atomic operations.
>> + */
>> +enum atc_status {
>> + ATC_IS_ERROR = 0,
>> + ATC_IS_CYCLIC = 24,
>> +};
>> +
>> +/**
>> * struct at_dma_chan - internal representation of an Atmel HDMAC channel
>> * @chan_common: common dmaengine channel object members
>> * @device: parent device
>> * @ch_regs: memory mapped register base
>> * @mask: channel index in a mask
>> - * @error_status: transmit error status information from irq handler
>> + * @status: transmit status information from irq/prep* functions
>> * to tasklet (use atomic operations)
>> * @tasklet: bottom half to finish transaction work
>> * @lock: serializes enqueue/dequeue operations to descriptors lists
>> @@ -201,7 +211,7 @@ struct at_dma_chan {
>> struct at_dma *device;
>> void __iomem *ch_regs;
>> u8 mask;
>> - unsigned long error_status;
>> + unsigned long status;
>> struct tasklet_struct tasklet;
>>
>> spinlock_t lock;
Thanks for your review, best regards,
--
Nicolas Ferre
--
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/
Powered by blists - more mailing lists