lkml.org 
[lkml]   [2017]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/2] dmaengine: pl330: fix descriptor allocation fail
From
Date
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.kochet@gmail.com>

Tested-by: Marek Szyprowski <m.szyprowski@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

\
 
 \ /
  Last update: 2017-10-17 13:27    [W:0.101 / U:0.192 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site