linux-kernel - Re: [PATCH v2 4/4] dw_dmac: return proper residue value

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 24 Jan 2013 10:37:03 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:	Vinod Koul <vinod.koul@...el.com>, linux-kernel@...r.kernel.org,
	spear-devel <spear-devel@...t.st.com>
Subject: Re: [PATCH v2 4/4] dw_dmac: return proper residue value

On Wed, Jan 23, 2013 at 9:07 PM, Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c

>  static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
>  {
>         dma_addr_t llp;
> @@ -410,6 +441,8 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
>                          */
>                         desc = dwc_first_active(dwc);
>
> +                       dwc_update_residue(dwc, desc);
> +
>                         if (dwc->tx_node_active != &desc->tx_list) {
>                                 child = to_dw_desc(dwc->tx_node_active);

Is there a point updating residue here? I don't have a very good knowledge of
nollp transfers but this is what i know...

The above "if" will pass if we are still doing transfers and fail if
all transfers are done.
After the end of each LLI we receive an interrupt, where we queue next
LLI. Better
would be to initialize dwc->residue at dwc_dostart() with total
length, start decrementing
it with desc->len for every lli interrupt we get and if call for
getting residue comes in
middle of transfer, simple return residue - dwc_get_sent(desc) without
updating residue
field...

> @@ -436,6 +469,9 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
>
>         if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags)) {
>                 dev_vdbg(chan2dev(&dwc->chan), "%s: soft LLP mode\n", __func__);
> +
> +               dwc_update_residue(dwc, dwc_first_active(dwc));
> +

same is applicable here too and so you can get rid of
dwc_update_residue() routine.

>                 spin_unlock_irqrestore(&dwc->lock, flags);
>                 return;
>         }
> @@ -444,6 +480,9 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
>                         (unsigned long long)llp);
>
>         list_for_each_entry_safe(desc, _desc, &dwc->active_list, desc_node) {
> +               /* initial residue value */
> +               dwc->residue = desc->total_len;
> +
>                 /* check first descriptors addr */
>                 if (desc->txd.phys == llp) {
>                         spin_unlock_irqrestore(&dwc->lock, flags);
> @@ -453,16 +492,21 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
>                 /* check first descriptors llp */
>                 if (desc->lli.llp == llp) {
>                         /* This one is currently in progress */
> +                       dwc->residue -= dwc_get_sent(dwc);
>                         spin_unlock_irqrestore(&dwc->lock, flags);
>                         return;
>                 }
>
> -               list_for_each_entry(child, &desc->tx_list, desc_node)
> +               dwc->residue -= desc->len;
> +               list_for_each_entry(child, &desc->tx_list, desc_node) {
>                         if (child->lli.llp == llp) {
>                                 /* Currently in progress */
> +                               dwc->residue -= dwc_get_sent(dwc);
>                                 spin_unlock_irqrestore(&dwc->lock, flags);
>                                 return;
>                         }
> +                       dwc->residue -= child->len;
> +               }
>
>                 /*
>                  * No descriptors so far seem to be in progress, i.e.
> @@ -1058,6 +1102,7 @@ dwc_tx_status(struct dma_chan *chan,
>               struct dma_tx_state *txstate)
>  {
>         struct dw_dma_chan      *dwc = to_dw_dma_chan(chan);
> +       unsigned long           flags;
>         enum dma_status         ret;
>
>         ret = dma_cookie_status(chan, cookie, txstate);
> @@ -1067,8 +1112,12 @@ dwc_tx_status(struct dma_chan *chan,
>                 ret = dma_cookie_status(chan, cookie, txstate);
>         }
>
> +       spin_lock_irqsave(&dwc->lock, flags);
> +

why do you need locking here?

>         if (ret != DMA_SUCCESS)
> -               dma_set_residue(txstate, dwc_first_active(dwc)->len);
> +               dma_set_residue(txstate, dwc->residue);
> +
> +       spin_unlock_irqrestore(&dwc->lock, flags);
>
>         if (dwc->paused)
>                 return DMA_PAUSED;
> diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
> index 833b4cf..88dd8eb 100644
> --- a/drivers/dma/dw_dmac_regs.h
> +++ b/drivers/dma/dw_dmac_regs.h
> @@ -203,6 +203,7 @@ struct dw_dma_chan {
>         struct list_head        active_list;
>         struct list_head        queue;
>         struct list_head        free_list;
> +       u32                     residue;
>         struct dw_cyclic_desc   *cdesc;
>
>         unsigned int            descs_allocated;
--
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/