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
| ||
|
Date: Sat, 12 Dec 2009 11:11:34 +0900 From: Nobuhiro Iwamatsu <iwamatsu@...auri.org> To: Dan Williams <dan.j.williams@...el.com> Cc: Guennadi Liakhovetski <g.liakhovetski@....de>, linux-kernel@...r.kernel.org, linux-sh@...r.kernel.org Subject: Re: [PATCH 3/5] sh: fix DMA driver's descriptor chaining and cookie assignment Hi, all. I will test this patch next week. Please wait. Best regards, Nobuhiro 2009/12/8 Dan Williams <dan.j.williams@...el.com>: > I would like an acked-by from Nobuhiro on this one. > > [ full patch quoted below ] > > Thanks, > Dan > > On Fri, Dec 4, 2009 at 11:44 AM, Guennadi Liakhovetski > <g.liakhovetski@....de> wrote: >> The SH DMA driver wrongly assigns negative cookies to transfer descriptors, >> also, its chaining of partial descriptors is broken. The latter problem is >> usually invisible, because maximum transfer size per chunk is 16M, but if you >> artificially set this limit lower, the driver fails. Since cookies are also >> used in chunk management, both these problems are fixed in one patch. >> >> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@....de> >> --- >> drivers/dma/shdma.c | 246 ++++++++++++++++++++++++++++++++------------------- >> drivers/dma/shdma.h | 1 - >> 2 files changed, 153 insertions(+), 94 deletions(-) >> >> diff --git a/drivers/dma/shdma.c b/drivers/dma/shdma.c >> index f5fae12..aac8f78 100644 >> --- a/drivers/dma/shdma.c >> +++ b/drivers/dma/shdma.c >> @@ -30,9 +30,12 @@ >> #include "shdma.h" >> >> /* DMA descriptor control */ >> -#define DESC_LAST (-1) >> -#define DESC_COMP (1) >> -#define DESC_NCOMP (0) >> +enum sh_dmae_desc_status { >> + DESC_PREPARED, >> + DESC_SUBMITTED, >> + DESC_COMPLETED, /* completed, have to call callback */ >> + DESC_WAITING, /* callback called, waiting for ack / re-submit */ >> +}; >> >> #define NR_DESCS_PER_CHANNEL 32 >> /* >> @@ -45,6 +48,8 @@ >> */ >> #define RS_DEFAULT (RS_DUAL) >> >> +static void sh_dmae_chan_ld_cleanup(struct sh_dmae_chan *sh_chan, bool all); >> + >> #define SH_DMAC_CHAN_BASE(id) (dma_base_addr[id]) >> static void sh_dmae_writel(struct sh_dmae_chan *sh_dc, u32 data, u32 reg) >> { >> @@ -185,7 +190,7 @@ static int dmae_set_dmars(struct sh_dmae_chan *sh_chan, u16 val) >> >> static dma_cookie_t sh_dmae_tx_submit(struct dma_async_tx_descriptor *tx) >> { >> - struct sh_desc *desc = tx_to_sh_desc(tx); >> + struct sh_desc *desc = tx_to_sh_desc(tx), *chunk; >> struct sh_dmae_chan *sh_chan = to_sh_chan(tx->chan); >> dma_cookie_t cookie; >> >> @@ -196,45 +201,40 @@ static dma_cookie_t sh_dmae_tx_submit(struct dma_async_tx_descriptor *tx) >> if (cookie < 0) >> cookie = 1; >> >> - /* If desc only in the case of 1 */ >> - if (desc->async_tx.cookie != -EBUSY) >> - desc->async_tx.cookie = cookie; >> - sh_chan->common.cookie = desc->async_tx.cookie; >> - >> - list_splice_init(&desc->tx_list, sh_chan->ld_queue.prev); >> + tx->cookie = cookie; >> + dev_dbg(sh_chan->dev, "submit %p #%d start %u\n", >> + tx, tx->cookie, desc->hw.sar); >> + sh_chan->common.cookie = tx->cookie; >> + >> + /* Mark all chunks of this descriptor as submitted */ >> + list_for_each_entry(chunk, desc->node.prev, node) { >> + /* >> + * All chunks are on the global ld_queue, so, we have to find >> + * the end of the chain ourselves >> + */ >> + if (chunk != desc && (chunk->async_tx.cookie > 0 || >> + chunk->async_tx.cookie == -EBUSY)) >> + break; >> + chunk->mark = DESC_SUBMITTED; >> + } >> >> spin_unlock_bh(&sh_chan->desc_lock); >> >> return cookie; >> } >> >> +/* Called with desc_lock held */ >> static struct sh_desc *sh_dmae_get_desc(struct sh_dmae_chan *sh_chan) >> { >> - struct sh_desc *desc, *_desc, *ret = NULL; >> - >> - spin_lock_bh(&sh_chan->desc_lock); >> - list_for_each_entry_safe(desc, _desc, &sh_chan->ld_free, node) { >> - if (async_tx_test_ack(&desc->async_tx)) { >> - list_del(&desc->node); >> - ret = desc; >> - break; >> - } >> - } >> - spin_unlock_bh(&sh_chan->desc_lock); >> - >> - return ret; >> -} >> + struct sh_desc *desc; >> >> -static void sh_dmae_put_desc(struct sh_dmae_chan *sh_chan, struct sh_desc *desc) >> -{ >> - if (desc) { >> - spin_lock_bh(&sh_chan->desc_lock); >> + if (list_empty(&sh_chan->ld_free)) >> + return NULL; >> >> - list_splice_init(&desc->tx_list, &sh_chan->ld_free); >> - list_add(&desc->node, &sh_chan->ld_free); >> + desc = list_entry(sh_chan->ld_free.next, struct sh_desc, node); >> + list_del(&desc->node); >> >> - spin_unlock_bh(&sh_chan->desc_lock); >> - } >> + return desc; >> } >> >> static int sh_dmae_alloc_chan_resources(struct dma_chan *chan) >> @@ -254,10 +254,9 @@ static int sh_dmae_alloc_chan_resources(struct dma_chan *chan) >> &sh_chan->common); >> desc->async_tx.tx_submit = sh_dmae_tx_submit; >> desc->async_tx.flags = DMA_CTRL_ACK; >> - INIT_LIST_HEAD(&desc->tx_list); >> - sh_dmae_put_desc(sh_chan, desc); >> >> spin_lock_bh(&sh_chan->desc_lock); >> + list_add(&desc->node, &sh_chan->ld_free); >> sh_chan->descs_allocated++; >> } >> spin_unlock_bh(&sh_chan->desc_lock); >> @@ -274,7 +273,10 @@ static void sh_dmae_free_chan_resources(struct dma_chan *chan) >> struct sh_desc *desc, *_desc; >> LIST_HEAD(list); >> >> - BUG_ON(!list_empty(&sh_chan->ld_queue)); >> + /* Prepared and not submitted descriptors can still be on the queue */ >> + if (!list_empty(&sh_chan->ld_queue)) >> + sh_dmae_chan_ld_cleanup(sh_chan, true); >> + >> spin_lock_bh(&sh_chan->desc_lock); >> >> list_splice_init(&sh_chan->ld_free, &list); >> @@ -293,6 +295,7 @@ static struct dma_async_tx_descriptor *sh_dmae_prep_memcpy( >> struct sh_dmae_chan *sh_chan; >> struct sh_desc *first = NULL, *prev = NULL, *new; >> size_t copy_size; >> + LIST_HEAD(tx_list); >> >> if (!chan) >> return NULL; >> @@ -302,43 +305,70 @@ static struct dma_async_tx_descriptor *sh_dmae_prep_memcpy( >> >> sh_chan = to_sh_chan(chan); >> >> + /* Have to lock the whole loop to protect against concurrent release */ >> + spin_lock_bh(&sh_chan->desc_lock); >> + >> + /* >> + * Chaining: >> + * first descriptor is what user is dealing with in all API calls, its >> + * cookie is at first set to -EBUSY, at tx-submit to a positive >> + * number >> + * if more than one chunk is needed further chunks have cookie = -EINVAL >> + * the last chunk, if not equal to the first, has cookie = -ENOSPC >> + * all chunks are linked onto the tx_list head with their .node heads >> + * only during this function, then they are immediately spliced >> + * onto the queue >> + */ >> do { >> /* Allocate the link descriptor from DMA pool */ >> new = sh_dmae_get_desc(sh_chan); >> if (!new) { >> dev_err(sh_chan->dev, >> "No free memory for link descriptor\n"); >> - goto err_get_desc; >> + list_splice(&tx_list, &sh_chan->ld_free); >> + spin_unlock_bh(&sh_chan->desc_lock); >> + return NULL; >> } >> >> - copy_size = min(len, (size_t)SH_DMA_TCR_MAX); >> + copy_size = min(len, (size_t)SH_DMA_TCR_MAX + 1); >> >> new->hw.sar = dma_src; >> new->hw.dar = dma_dest; >> new->hw.tcr = copy_size; >> - if (!first) >> + if (!first) { >> + /* First desc */ >> + new->async_tx.cookie = -EBUSY; >> first = new; >> + } else { >> + /* Other desc - invisible to the user */ >> + new->async_tx.cookie = -EINVAL; >> + } >> + >> + dev_dbg(sh_chan->dev, >> + "chaining %u of %u with %p, start %u, cookie %d\n", >> + copy_size, len, &new->async_tx, dma_src, >> + new->async_tx.cookie); >> >> - new->mark = DESC_NCOMP; >> - async_tx_ack(&new->async_tx); >> + new->mark = DESC_PREPARED; >> + new->async_tx.flags = flags; >> >> prev = new; >> len -= copy_size; >> dma_src += copy_size; >> dma_dest += copy_size; >> /* Insert the link descriptor to the LD ring */ >> - list_add_tail(&new->node, &first->tx_list); >> + list_add_tail(&new->node, &tx_list); >> } while (len); >> >> - new->async_tx.flags = flags; /* client is in control of this ack */ >> - new->async_tx.cookie = -EBUSY; /* Last desc */ >> + if (new != first) >> + new->async_tx.cookie = -ENOSPC; >> >> - return &first->async_tx; >> + /* Put them immediately on the queue, so, they don't get lost */ >> + list_splice_tail(&tx_list, sh_chan->ld_queue.prev); >> >> -err_get_desc: >> - sh_dmae_put_desc(sh_chan, first); >> - return NULL; >> + spin_unlock_bh(&sh_chan->desc_lock); >> >> + return &first->async_tx; >> } >> >> /* >> @@ -346,64 +376,87 @@ err_get_desc: >> * >> * This function clean up the ld_queue of DMA channel. >> */ >> -static void sh_dmae_chan_ld_cleanup(struct sh_dmae_chan *sh_chan) >> +static void sh_dmae_chan_ld_cleanup(struct sh_dmae_chan *sh_chan, bool all) >> { >> struct sh_desc *desc, *_desc; >> >> spin_lock_bh(&sh_chan->desc_lock); >> list_for_each_entry_safe(desc, _desc, &sh_chan->ld_queue, node) { >> - dma_async_tx_callback callback; >> - void *callback_param; >> + struct dma_async_tx_descriptor *tx = &desc->async_tx; >> >> - /* non send data */ >> - if (desc->mark == DESC_NCOMP) >> + /* unsent data */ >> + if (desc->mark == DESC_SUBMITTED && !all) >> break; >> >> - /* send data sesc */ >> - callback = desc->async_tx.callback; >> - callback_param = desc->async_tx.callback_param; >> + if (desc->mark == DESC_PREPARED && !all) >> + continue; >> + >> + /* Call callback on the "exposed" descriptor (cookie > 0) */ >> + if (tx->cookie > 0 && (desc->mark == DESC_COMPLETED || >> + desc->mark == DESC_WAITING)) { >> + struct sh_desc *chunk; >> + bool head_acked = async_tx_test_ack(tx); >> + /* sent data desc */ >> + dma_async_tx_callback callback = tx->callback; >> + >> + /* Run the link descriptor callback function */ >> + if (callback && desc->mark == DESC_COMPLETED) { >> + spin_unlock_bh(&sh_chan->desc_lock); >> + dev_dbg(sh_chan->dev, >> + "descriptor %p callback\n", tx); >> + callback(tx->callback_param); >> + spin_lock_bh(&sh_chan->desc_lock); >> + } >> >> - /* Remove from ld_queue list */ >> - list_splice_init(&desc->tx_list, &sh_chan->ld_free); >> + list_for_each_entry(chunk, desc->node.prev, node) { >> + /* >> + * All chunks are on the global ld_queue, so, we >> + * have to find the end of the chain ourselves >> + */ >> + if (chunk != desc && >> + (chunk->async_tx.cookie > 0 || >> + chunk->async_tx.cookie == -EBUSY)) >> + break; >> + >> + if (head_acked) >> + async_tx_ack(&chunk->async_tx); >> + else >> + chunk->mark = DESC_WAITING; >> + } >> + } >> >> - dev_dbg(sh_chan->dev, "link descriptor %p will be recycle.\n", >> - desc); >> + dev_dbg(sh_chan->dev, "descriptor %p #%d completed.\n", >> + tx, tx->cookie); >> >> - list_move(&desc->node, &sh_chan->ld_free); >> - /* Run the link descriptor callback function */ >> - if (callback) { >> - spin_unlock_bh(&sh_chan->desc_lock); >> - dev_dbg(sh_chan->dev, "link descriptor %p callback\n", >> - desc); >> - callback(callback_param); >> - spin_lock_bh(&sh_chan->desc_lock); >> - } >> + if (((desc->mark == DESC_COMPLETED || >> + desc->mark == DESC_WAITING) && >> + async_tx_test_ack(&desc->async_tx)) || all) >> + /* Remove from ld_queue list */ >> + list_move(&desc->node, &sh_chan->ld_free); >> } >> spin_unlock_bh(&sh_chan->desc_lock); >> } >> >> static void sh_chan_xfer_ld_queue(struct sh_dmae_chan *sh_chan) >> { >> - struct list_head *ld_node; >> struct sh_dmae_regs hw; >> + struct sh_desc *sd; >> >> /* DMA work check */ >> if (dmae_is_idle(sh_chan)) >> return; >> >> + spin_lock_bh(&sh_chan->desc_lock); >> /* Find the first un-transfer desciptor */ >> - for (ld_node = sh_chan->ld_queue.next; >> - (ld_node != &sh_chan->ld_queue) >> - && (to_sh_desc(ld_node)->mark == DESC_COMP); >> - ld_node = ld_node->next) >> - cpu_relax(); >> - >> - if (ld_node != &sh_chan->ld_queue) { >> - /* Get the ld start address from ld_queue */ >> - hw = to_sh_desc(ld_node)->hw; >> - dmae_set_reg(sh_chan, hw); >> - dmae_start(sh_chan); >> - } >> + list_for_each_entry(sd, &sh_chan->ld_queue, node) >> + if (sd->mark == DESC_SUBMITTED) { >> + /* Get the ld start address from ld_queue */ >> + hw = sd->hw; >> + dmae_set_reg(sh_chan, hw); >> + dmae_start(sh_chan); >> + break; >> + } >> + spin_unlock_bh(&sh_chan->desc_lock); >> } >> >> static void sh_dmae_memcpy_issue_pending(struct dma_chan *chan) >> @@ -421,12 +474,11 @@ static enum dma_status sh_dmae_is_complete(struct dma_chan *chan, >> dma_cookie_t last_used; >> dma_cookie_t last_complete; >> >> - sh_dmae_chan_ld_cleanup(sh_chan); >> + sh_dmae_chan_ld_cleanup(sh_chan, false); >> >> last_used = chan->cookie; >> last_complete = sh_chan->completed_cookie; >> - if (last_complete == -EBUSY) >> - last_complete = last_used; >> + BUG_ON(last_complete < 0); >> >> if (done) >> *done = last_complete; >> @@ -481,11 +533,13 @@ static irqreturn_t sh_dmae_err(int irq, void *data) >> err = sh_dmae_rst(0); >> if (err) >> return err; >> +#ifdef SH_DMAC_BASE1 >> if (shdev->pdata.mode & SHDMA_DMAOR1) { >> err = sh_dmae_rst(1); >> if (err) >> return err; >> } >> +#endif >> disable_irq(irq); >> return IRQ_HANDLED; >> } >> @@ -499,30 +553,36 @@ static void dmae_do_tasklet(unsigned long data) >> u32 sar_buf = sh_dmae_readl(sh_chan, SAR); >> list_for_each_entry_safe(desc, _desc, >> &sh_chan->ld_queue, node) { >> - if ((desc->hw.sar + desc->hw.tcr) == sar_buf) { >> + if ((desc->hw.sar + desc->hw.tcr) == sar_buf && >> + desc->mark == DESC_SUBMITTED) { >> cur_desc = desc; >> break; >> } >> } >> >> if (cur_desc) { >> + dev_dbg(sh_chan->dev, "done %p #%d start %u\n", >> + &cur_desc->async_tx, cur_desc->async_tx.cookie, >> + cur_desc->hw.sar); >> + >> switch (cur_desc->async_tx.cookie) { >> - case 0: /* other desc data */ >> + case -EINVAL: >> + case -ENOSPC: >> + /* other desc data */ >> break; >> - case -EBUSY: /* last desc */ >> - sh_chan->completed_cookie = >> - cur_desc->async_tx.cookie; >> + case -EBUSY: /* Cannot be */ >> + BUG_ON(1); >> break; >> - default: /* first desc ( 0 < )*/ >> + default: /* first desc: cookie > 0 */ >> sh_chan->completed_cookie = >> - cur_desc->async_tx.cookie - 1; >> + cur_desc->async_tx.cookie; >> break; >> } >> - cur_desc->mark = DESC_COMP; >> + cur_desc->mark = DESC_COMPLETED; >> } >> /* Next desc */ >> sh_chan_xfer_ld_queue(sh_chan); >> - sh_dmae_chan_ld_cleanup(sh_chan); >> + sh_dmae_chan_ld_cleanup(sh_chan, false); >> } >> >> static unsigned int get_dmae_irq(unsigned int id) >> diff --git a/drivers/dma/shdma.h b/drivers/dma/shdma.h >> index 2b4bc15..c93555c 100644 >> --- a/drivers/dma/shdma.h >> +++ b/drivers/dma/shdma.h >> @@ -26,7 +26,6 @@ struct sh_dmae_regs { >> }; >> >> struct sh_desc { >> - struct list_head tx_list; >> struct sh_dmae_regs hw; >> struct list_head node; >> struct dma_async_tx_descriptor async_tx; >> -- >> 1.6.2.4 >> >> -- >> 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/ >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-sh" in > the body of a message to majordomo@...r.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Nobuhiro Iwamatsu iwamatsu at {nigauri.org / debian.org} GPG ID: 40AD1FA6 -- 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