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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
Hello Vinod! Thanks for review!
> 26 сент. 2017 г., в 20:37, Vinod Koul <[email protected]> написал(а):
>
> 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.
On Tue, Sep 26, 2017 at 09:03:50PM +0300, Alexander Kochetkov wrote:
> Hello Vinod! Thanks for review!
>
> > 26 сент. 2017 г., в 20:37, Vinod Koul <[email protected]> написал(а):
> >
> > 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