[PATCH] dmaengine: pl330: fix descriptor allocation fail

[PATCH] dmaengine: pl330: fix descriptor allocation fail

[Alexander Kochetkov]

Thread A calls pl330_get_desc() to get descriptor. If DMAC descriptor
pool is empty pl330_get_desc() allocates new descriptor using add_desc()
and then get newly allocated descriptor using pluck_desc().
It is possible that another concurrent thread B calls pluck_desc()
and catch newly allocated descriptor. In that case descriptor allocation
for thread A will fail with:

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

The commit fix the issue by calling _add_desc() to allocate new descriptor
to the local on stack pool and than get it from local pool. So the issue
described will nether happen.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/dma/pl330.c |   44 +++++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index f37f497..0e7f6c9 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2417,7 +2417,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;
@@ -2427,27 +2428,33 @@ 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 int add_desc(struct pl330_dmac *pl330, gfp_t flg, int count)
+{
+	return _add_desc(&pl330->desc_pool, &pl330->pool_lock, flg, count);
+}
+
+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);
@@ -2456,11 +2463,16 @@ 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;
 }
 
+static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330)
+{
+	return _pluck_desc(&pl330->desc_pool, &pl330->pool_lock);
+}
+
 static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
 {
 	struct pl330_dmac *pl330 = pch->dmac;
@@ -2472,16 +2484,14 @@ static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
 
 	/* 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 */
-- 
1.7.9.5

Re: [PATCH] dmaengine: pl330: fix descriptor allocation fail

[Vinod Koul]

On Fri, Sep 08, 2017 at 01:00:26PM +0300, Alexander Kochetkov wrote:
> Thread A calls pl330_get_desc() to get descriptor. If DMAC descriptor
> pool is empty pl330_get_desc() allocates new descriptor using add_desc()
> and then get newly allocated descriptor using pluck_desc().
> It is possible that another concurrent thread B calls pluck_desc()
> and catch newly allocated descriptor. In that case descriptor allocation
> for thread A will fail with:
> 
> kernel: dma-pl330 20078000.dma-controller: pl330_get_desc:2469 ALERT!
> 
> The commit fix the issue by calling _add_desc() to allocate new descriptor
> to the local on stack pool and than get it from local pool. So the issue
> described will nether happen.

Tested-by please...

> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> ---
>  drivers/dma/pl330.c |   44 +++++++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index f37f497..0e7f6c9 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2417,7 +2417,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)

right justifed please

>  {
>  	struct dma_pl330_desc *desc;
>  	unsigned long flags;
> @@ -2427,27 +2428,33 @@ 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 int add_desc(struct pl330_dmac *pl330, gfp_t flg, int count)
> +{
> +	return _add_desc(&pl330->desc_pool, &pl330->pool_lock, flg, count);

hmmm why add a wrapper?

> +}
> +
> +static struct dma_pl330_desc *_pluck_desc(struct list_head *pool,
> +	spinlock_t *lock)

here too, it helps in readability

>  {
>  	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);
> @@ -2456,11 +2463,16 @@ 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;
>  }
>  
> +static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330)
> +{
> +	return _pluck_desc(&pl330->desc_pool, &pl330->pool_lock);

one more wrapper why, we dont have any logic here!

> +}
> +
>  static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
>  {
>  	struct pl330_dmac *pl330 = pch->dmac;
> @@ -2472,16 +2484,14 @@ static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
>  
>  	/* 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 */
> -- 
> 1.7.9.5
> 

-- 
~Vinod

Re: [PATCH] dmaengine: pl330: fix descriptor allocation fail

[Alexander Kochetkov]

Hello Vinod! Thanks for review!

> 26 сент. 2017 г., в 20:37, Vinod Koul <vinod.koul@intel.com> написал(а):
> 
> Tested-by please...

In order to test the patch the driver should be rebuild with NR_DEFAULT_DESC defined to 1
and with some trace code included. Is it OK if I provide second patch I used for testing
with trace showing how change work?

> one more wrapper why, we dont have any logic here!
The idea was to keep rest of driver code intact. Ok, I’ll send v2 with no wrappers.

> right justifed please


Some functions has two tabs on second line, some has alignment to beginning of
argument declaration. How correct?

1) or like this (two tabs)
static int add_desc(struct list_head *pool, spinlock_t *lock,
                gfp_t flg, int count)

2) Like this:
static int add_desc(struct list_head *pool, spinlock_t *lock,
		               gfp_t flg, int count)

Regards,
Alexander.

Re: [PATCH] dmaengine: pl330: fix descriptor allocation fail

[Vinod Koul]

On Tue, Sep 26, 2017 at 09:03:50PM +0300, Alexander Kochetkov wrote:
> Hello Vinod! Thanks for review!
> 
> > 26 сент. 2017 г., в 20:37, Vinod Koul <vinod.koul@intel.com> написал(а):
> > 
> > Tested-by please...
> 
> In order to test the patch the driver should be rebuild with NR_DEFAULT_DESC defined to 1
> and with some trace code included. Is it OK if I provide second patch I used for testing
> with trace showing how change work?
> 
> > one more wrapper why, we dont have any logic here!
> The idea was to keep rest of driver code intact. Ok, I’ll send v2 with no wrappers.
> 
> > right justifed please
> 
> 
> Some functions has two tabs on second line, some has alignment to beginning of
> argument declaration. How correct?
> 
> 1) or like this (two tabs)
> static int add_desc(struct list_head *pool, spinlock_t *lock,
>                 gfp_t flg, int count)
> 
> 2) Like this:
> static int add_desc(struct list_head *pool, spinlock_t *lock,
> 		               gfp_t flg, int count)

Second one with one more tab :)

See Section 2  Breaking long lines and strings in
Documentation/process/coding-style.rst

-- 
~Vinod