Re: [PATCH] dmaengine: pl330: fix descriptor allocation fail
On Tue, Sep 26, 2017 at 09:03:50PM +0300, Alexander Kochetkov wrote: > Hello Vinod! Thanks for review! > > > 26 сент. 2017 г., в 20:37, Vinod Koulнаписал(а): > > > > 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
Re: [PATCH] dmaengine: pl330: fix descriptor allocation fail
On Tue, Sep 26, 2017 at 09:03:50PM +0300, Alexander Kochetkov wrote: > Hello Vinod! Thanks for review! > > > 26 сент. 2017 г., в 20:37, Vinod Koul написал(а): > > > > 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
Re: [PATCH] dmaengine: pl330: fix descriptor allocation fail
Hello Vinod! Thanks for review! > 26 сент. 2017 г., в 20:37, Vinod Koulнаписал(а): > > 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
Hello Vinod! Thanks for review! > 26 сент. 2017 г., в 20:37, Vinod Koul написал(а): > > 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
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> --- > 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(>pool_lock, flags); > + spin_lock_irqsave(lock, flags); > > for (i = 0; i < count; i++) { > _init_desc([i]); > - list_add_tail([i].node, >desc_pool); > + list_add_tail([i].node, pool); > } > > - spin_unlock_irqrestore(>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(>desc_pool, >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(>pool_lock, flags); > + spin_lock_irqsave(lock, flags); > > - if (!list_empty(>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(>node); > @@ -2456,11 +2463,16 @@ static struct dma_pl330_desc *pluck_desc(struct > pl330_dmac *pl330) > desc->txd.callback = NULL; > } > > - spin_unlock_irqrestore(>pool_lock, flags); > + spin_unlock_irqrestore(lock, flags); > > return desc; > } > > +static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330) > +{ > + return _pluck_desc(>desc_pool, >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(, , GFP_ATOMIC, 1)) > return NULL; > - } > + > + desc = _pluck_desc(, ); > + WARN_ON(!desc || !list_empty()); > } > > /* Initialize the descriptor */ > -- > 1.7.9.5 > -- ~Vinod
Re: [PATCH] dmaengine: pl330: fix descriptor allocation fail
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 > --- > 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(>pool_lock, flags); > + spin_lock_irqsave(lock, flags); > > for (i = 0; i < count; i++) { > _init_desc([i]); > - list_add_tail([i].node, >desc_pool); > + list_add_tail([i].node, pool); > } > > - spin_unlock_irqrestore(>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(>desc_pool, >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(>pool_lock, flags); > + spin_lock_irqsave(lock, flags); > > - if (!list_empty(>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(>node); > @@ -2456,11 +2463,16 @@ static struct dma_pl330_desc *pluck_desc(struct > pl330_dmac *pl330) > desc->txd.callback = NULL; > } > > - spin_unlock_irqrestore(>pool_lock, flags); > + spin_unlock_irqrestore(lock, flags); > > return desc; > } > > +static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330) > +{ > + return _pluck_desc(>desc_pool, >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(, , GFP_ATOMIC, 1)) > return NULL; > - } > + > + desc = _pluck_desc(, ); > + WARN_ON(!desc || !list_empty()); > } > > /* Initialize the descriptor */ > -- > 1.7.9.5 > -- ~Vinod