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

[Alexander Kochetkov]

Hello Vinod!

It looks like the patch did not get enough attention. If no one except me will test the patch,
how do we proceed? This patch does not depend on the SOC implementation and the
runtime environment. For me the patch looks fine and will not do harm if it will be committed.

Regards,
Alexander.

> 4 ÐÐÑ. 2017 Ð., Ð 14:37, Alexander Kochetkov <al.kochet@xxxxxxxxx> ÐÐÐÐÑÐÐ(Ð):
> 
> 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@xxxxxxxxx>
> ---
> 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);
> -- 
> 1.7.9.5
>