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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 28 Oct 2009 09:31:29 +0100 From: Linus Walleij <linus.ml.walleij@...il.com> To: Dan Williams <dan.j.williams@...el.com> Cc: Linus Walleij <linus.walleij@...ricsson.com>, Maciej Sosnowski <maciej.sosnowski@...el.com>, linux-kernel@...r.kernel.org Subject: Re: [PATCH] Add COH 901 318 DMA block driver v2 Hi Dan, thanks *a lot* for the review, a v3 version will follow shortly, hopefully this will be mainline material. Most remarks have been fixed, some notices below... 2009/10/13 Dan Williams <dan.j.williams@...el.com>: >> + if (cohc_chan_conf(cohc)->priority_high) >> + tasklet_hi_schedule(&cohc->tasklet); >> + else >> + tasklet_schedule(&cohc->tasklet); >> +} >> + > > Just curious, can you say a bit about why this driver needs two > tasklet priority levels? It is a safety precaution but perhaps we're overdoing it. The high prio channels are used to transfer audio data during a voice call from the application subsystem to the modem in the U300 platforms (one of the most user-annoying things in an interactive system is when audio skips so this must never happen). We haven't really made any measurements to see if this is a real-world issue and whether the tasklet prio levels helps in some corner case. (Having the entire audio path under DMA helps a lot though.) Also see next answer... >> +int coh901318_channel_link(struct dma_chan *chan, struct dma_chan *chan_dest, >> + int size) >> +{ >> + struct coh901318_chan *cohc = to_coh901318_chan(chan); >> + struct coh901318_chan *cohc_dst = to_coh901318_chan(chan_dest); >> + int err = 0; >> + u32 ctrl = cohc_chan_param(cohc)->ctrl_lli_chained; >> + u32 config = cohc_chan_param(cohc)->config; >> + struct coh901318_lli lli; >> + unsigned long flags; >> + spin_lock_irqsave(&cohc->lock, flags); >> + >> + dev_vdbg(COHC_2_DEV(cohc), >> + "[%s] chan_src %d chan_dst %d, size %d\n", >> + __func__, cohc->id, cohc_dst->id, size); >> + >> + /* TODO: Add support for size != -1 */ >> + if (size != -1) { >> + err = -EINVAL; >> + goto out; >> + } >> + >> + ctrl &= ~COH901318_CX_CTRL_MASTER_MODE_MASK; >> + ctrl |= cohc_chan_param(cohc_dst)->ctrl_lli_chained & >> + COH901318_CX_CTRL_MASTER_MODE_MASK; >> + >> + /* transfer between two peripheral, src and dest is constant */ >> + ctrl &= ~COH901318_CX_CTRL_SRC_ADDR_INC_ENABLE; >> + ctrl &= ~COH901318_CX_CTRL_DST_ADDR_INC_ENABLE; >> + >> + /* set COH901318_CX_CTRL_PRDD_SOURCE */ >> + ctrl &= ~COH901318_CX_CTRL_PRDD_DEST; >> + >> + /* Handshake both two primary and secondary peripheral */ >> + ctrl |= COH901318_CX_CTRL_HSP_ENABLE; >> + ctrl |= COH901318_CX_CTRL_HSS_ENABLE; >> + >> + config |= COH901318_CX_CFG_RM_PRIMARY_TO_SECONDARY; >> + config |= cohc_dst->id << COH901318_CX_CFG_LCRF_SHIFT; >> + >> + if (size == -1) >> + ctrl &= ~COH901318_CX_CTRL_TC_ENABLE; >> + >> + coh901318_set_conf(cohc, config); >> + lli.control = ctrl; >> + lli.src_addr = cohc_dev_addr(cohc); >> + lli.dst_addr = cohc_dev_addr(cohc_dst); >> + lli.link_addr = 0; >> + >> + coh901318_list_print(cohc, &lli); >> + >> + coh901318_prep_linked_list(cohc, &lli); >> + >> + coh901318_start(cohc); >> + >> + out: >> + spin_unlock_irqrestore(&cohc->lock, flags); >> + return err; >> +} >> +EXPORT_SYMBOL(coh901318_channel_link); > > ?? > > What scenario would you use this functionality? Let's defer this > piece and the other exported symbols to the (upcoming?) patches that > add users for these exports. This is used to let the DMA transfer data between two devices without involving the CPU. For example we use this to transfer audio data directly from a modem interface to an I2S channel. No memory is involved, it's just the AMBA bus transmitting bursts from one device to the other. This offloads the CPU of the entire data shuffle task. This is a typical feature of embedded DMA and something the we believe will find its way into the DMAengine sooner or later so we'll be happy to refactor the code at that point. We've removed this function and also coh901318_prep_single while keeping some of the internals like coh901318_get_bytes_left, coh901318_stop and coh901318_continue. >> diff --git a/include/linux/coh901318.h b/include/linux/coh901318.h >> new file mode 100644 >> index 0000000..ca9ab68 >> --- /dev/null >> +++ b/include/linux/coh901318.h > > I do not think this file belongs in include/linux/. How about > arch/arm/mach-u300/include/mach/? This was based on the fact that the other embedded architectures using the DMA engine has its include file in include/linux/dw_dmac.h include/linux/at_hdmac.h We've moved the header to the mach-u300 include, though this block can theoretically be used in other hardware. Do you want a patch moving include/linux/dw_dmac.h -> arch/avr32/mach-at32ap/include/mach include/linux/at_hdmac.h -> arch/arm/mach-at91/include/mach as well? I can probably fix that, I just love patching up archs I know nothing about just as entertainment :-) Linus Walleij -- 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