Re: [PATCH v2 1/2] dmaengine: pl330: fix descriptor allocation fail

[Marek Szyprowski]

Hi Alexander,

It took me a while to test this patch, I've been busy with other stuff...

On 2017-10-04 13:37, Alexander Kochetkov wrote:

If two concurrent threads call pl330_get_desc() when DMAC descriptor
pool is empty it is possible that allocation for one of threads will fail
with message:

kernel: dma-pl330 20078000.dma-controller: pl330_get_desc:2469 ALERT!

Here how that can happen. Thread A calls pl330_get_desc() to get
descriptor. If DMAC descriptor pool is empty pl330_get_desc() allocates
new descriptor on shared pool using add_desc() and then get newly
allocated descriptor using pluck_desc(). At the same time thread B calls
pluck_desc() and take newly allocated descriptor. In that case descriptor
allocation for thread A will fail.

Using on-stack pool for new descriptor allow avoid the issue described.
The patch modify pl330_get_desc() to use on-stack pool for allocation
new descriptors.

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>

Tested-by: Marek Szyprowski <m.szyprow...@samsung.com>

---
  drivers/dma/pl330.c |   39 ++++++++++++++++++++-------------------
  1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index b19ee04..deec4a4 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2390,7 +2390,8 @@ static inline void _init_desc(struct dma_pl330_desc *desc)
  }

/* Returns the number of descriptors added to the DMAC pool */

-static int add_desc(struct pl330_dmac *pl330, gfp_t flg, int count)
+static int add_desc(struct list_head *pool, spinlock_t *lock,
+                   gfp_t flg, int count)
  {
        struct dma_pl330_desc *desc;
        unsigned long flags;
@@ -2400,27 +2401,28 @@ static int add_desc(struct pl330_dmac *pl330, gfp_t 
flg, int count)
        if (!desc)
                return 0;

- spin_lock_irqsave(&pl330->pool_lock, flags);

+       spin_lock_irqsave(lock, flags);

for (i = 0; i < count; i++) {

                _init_desc(&desc[i]);
-               list_add_tail(&desc[i].node, &pl330->desc_pool);
+               list_add_tail(&desc[i].node, pool);
        }

- spin_unlock_irqrestore(&pl330->pool_lock, flags);

+       spin_unlock_irqrestore(lock, flags);

return count;

  }

-static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330)

+static struct dma_pl330_desc *pluck_desc(struct list_head *pool,
+                                        spinlock_t *lock)
  {
        struct dma_pl330_desc *desc = NULL;
        unsigned long flags;

- spin_lock_irqsave(&pl330->pool_lock, flags);

+       spin_lock_irqsave(lock, flags);

- if (!list_empty(&pl330->desc_pool)) {

-               desc = list_entry(pl330->desc_pool.next,
+       if (!list_empty(pool)) {
+               desc = list_entry(pool->next,
                                struct dma_pl330_desc, node);

list_del_init(&desc->node);

@@ -2429,7 +2431,7 @@ static struct dma_pl330_desc *pluck_desc(struct 
pl330_dmac *pl330)
                desc->txd.callback = NULL;
        }

- spin_unlock_irqrestore(&pl330->pool_lock, flags);

+       spin_unlock_irqrestore(lock, flags);

return desc;

  }
@@ -2441,20 +2443,18 @@ static struct dma_pl330_desc *pl330_get_desc(struct 
dma_pl330_chan *pch)
        struct dma_pl330_desc *desc;

/* Pluck one desc from the pool of DMAC */

-       desc = pluck_desc(pl330);
+       desc = pluck_desc(&pl330->desc_pool, &pl330->pool_lock);

/* If the DMAC pool is empty, alloc new */

        if (!desc) {
-               if (!add_desc(pl330, GFP_ATOMIC, 1))
-                       return NULL;
+               DEFINE_SPINLOCK(lock);
+               LIST_HEAD(pool);

- /* Try again */

-               desc = pluck_desc(pl330);
-               if (!desc) {
-                       dev_err(pch->dmac->ddma.dev,
-                               "%s:%d ALERT!\n", __func__, __LINE__);
+               if (!add_desc(&pool, &lock, GFP_ATOMIC, 1))
                        return NULL;
-               }
+
+               desc = pluck_desc(&pool, &lock);
+               WARN_ON(!desc || !list_empty(&pool));
        }

/* Initialize the descriptor */

@@ -2868,7 +2868,8 @@ static int __maybe_unused pl330_resume(struct device *dev)
        spin_lock_init(&pl330->pool_lock);

/* Create a descriptor pool of default size */

-       if (!add_desc(pl330, GFP_KERNEL, NR_DEFAULT_DESC))
+       if (!add_desc(&pl330->desc_pool, &pl330->pool_lock,
+                     GFP_KERNEL, NR_DEFAULT_DESC))
                dev_warn(&adev->dev, "unable to allocate desc\n");

INIT_LIST_HEAD(&pd->channels);

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland