All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 08/18] DMAENGINE: ste_dma40: lock fix
@ 2010-06-01 12:21 Linus Walleij
  2010-06-11 22:38 ` Dan Williams
  0 siblings, 1 reply; 4+ messages in thread
From: Linus Walleij @ 2010-06-01 12:21 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-kernel, Jonas Aaberg, Linus Walleij

From: Jonas Aaberg <jonas.aberg@stericsson.com>

Fix up some locking issues found by enabling lock debugging.

Signed-off-by: Jonas Aaberg <jonas.aberg@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/dma/ste_dma40.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
index aba86bb..c4b0d37 100644
--- a/drivers/dma/ste_dma40.c
+++ b/drivers/dma/ste_dma40.c
@@ -441,6 +441,7 @@ static int d40_lcla_id_get(struct d40_chan *d40c,
 		pool->base + d40c->phy_chan->num * 1024;
 	int i;
 	int lli_per_log = d40c->base->plat_data->llis_per_log;
+	unsigned long flags;
 
 	if (d40c->lcla.src_id >= 0 && d40c->lcla.dst_id >= 0)
 		return 0;
@@ -448,7 +449,7 @@ static int d40_lcla_id_get(struct d40_chan *d40c,
 	if (pool->num_blocks > 32)
 		return -EINVAL;
 
-	spin_lock(&pool->lock);
+	spin_lock_irqsave(&pool->lock, flags);
 
 	for (i = 0; i < pool->num_blocks; i++) {
 		if (!(pool->alloc_map[d40c->phy_chan->num] & (0x1 << i))) {
@@ -477,10 +478,10 @@ static int d40_lcla_id_get(struct d40_chan *d40c,
 	d40c->lcla.src = lcla_lidx_base + src_id * lli_per_log + 1;
 
 
-	spin_unlock(&pool->lock);
+	spin_unlock_irqrestore(&pool->lock, flags);
 	return 0;
 err:
-	spin_unlock(&pool->lock);
+	spin_unlock_irqrestore(&pool->lock, flags);
 	return -EINVAL;
 }
 
@@ -488,15 +489,16 @@ static void d40_lcla_id_put(struct d40_chan *d40c,
 			    struct d40_lcla_pool *pool,
 			    int id)
 {
+	unsigned long flags;
 	if (id < 0)
 		return;
 
 	d40c->lcla.src_id = -1;
 	d40c->lcla.dst_id = -1;
 
-	spin_lock(&pool->lock);
+	spin_lock_irqsave(&pool->lock, flags);
 	pool->alloc_map[d40c->phy_chan->num] &= (~(0x1 << id));
-	spin_unlock(&pool->lock);
+	spin_unlock_irqrestore(&pool->lock, flags);
 }
 
 static int d40_channel_execute_command(struct d40_chan *d40c,
@@ -1990,8 +1992,6 @@ static struct dma_async_tx_descriptor *d40_prep_slave_sg(struct dma_chan *chan,
 	if (d40d == NULL)
 		return NULL;
 
-	memset(d40d, 0, sizeof(struct d40_desc));
-
 	if (d40c->log_num != D40_PHY_CHAN)
 		err = d40_prep_slave_sg_log(d40d, d40c, sgl, sg_len,
 					    direction, dma_flags);
-- 
1.6.3.3


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 08/18] DMAENGINE: ste_dma40: lock fix
  2010-06-01 12:21 [PATCH 08/18] DMAENGINE: ste_dma40: lock fix Linus Walleij
@ 2010-06-11 22:38 ` Dan Williams
  2010-06-14 16:41   ` Linus WALLEIJ
  0 siblings, 1 reply; 4+ messages in thread
From: Dan Williams @ 2010-06-11 22:38 UTC (permalink / raw)
  To: Linus Walleij; +Cc: linux-kernel, Jonas Aaberg

On Tue, Jun 1, 2010 at 5:21 AM, Linus Walleij
<linus.walleij@stericsson.com> wrote:
> From: Jonas Aaberg <jonas.aberg@stericsson.com>
>
> Fix up some locking issues found by enabling lock debugging.
>
> Signed-off-by: Jonas Aaberg <jonas.aberg@stericsson.com>
> Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
> ---
>  drivers/dma/ste_dma40.c |   14 +++++++-------
>  1 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> index aba86bb..c4b0d37 100644
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c
[..]
>  static int d40_channel_execute_command(struct d40_chan *d40c,
> @@ -1990,8 +1992,6 @@ static struct dma_async_tx_descriptor *d40_prep_slave_sg(struct dma_chan *chan,
>        if (d40d == NULL)
>                return NULL;
>
> -       memset(d40d, 0, sizeof(struct d40_desc));
> -

This one isn't a locking bug fix.

--
Dan

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH 08/18] DMAENGINE: ste_dma40: lock fix
  2010-06-11 22:38 ` Dan Williams
@ 2010-06-14 16:41   ` Linus WALLEIJ
  2010-06-15  4:53     ` Dan Williams
  0 siblings, 1 reply; 4+ messages in thread
From: Linus WALLEIJ @ 2010-06-14 16:41 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-kernel, Jonas ABERG

On 06/12/2010 12:38 AM, Dan Williams wrote:

> On Tue, Jun 1, 2010 at 5:21 AM, Linus Walleij 
> <linus.walleij@stericsson.com> wrote:
>> From: Jonas Aaberg <jonas.aberg@stericsson.com>
>>
>> Fix up some locking issues found by enabling lock debugging.
>>
>> Signed-off-by: Jonas Aaberg <jonas.aberg@stericsson.com>
>> Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
>> ---
>>  drivers/dma/ste_dma40.c |   14 +++++++-------
>>  1 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c index 
>> aba86bb..c4b0d37 100644
>> --- a/drivers/dma/ste_dma40.c
>> +++ b/drivers/dma/ste_dma40.c
> [..]
>>  static int d40_channel_execute_command(struct d40_chan *d40c, @@ 
>> -1990,8 +1992,6 @@ static struct dma_async_tx_descriptor *d40_prep_slave_sg(struct dma_chan *chan,
>>        if (d40d == NULL)
>>                return NULL;
>>
>> -       memset(d40d, 0, sizeof(struct d40_desc));
>> -
> 
> This one isn't a locking bug fix.

Well, actually it was overwriting a spinlock, so I would considering
a locking bug area atleast.

But if you prefer we split this one-liner into a separate patch.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 08/18] DMAENGINE: ste_dma40: lock fix
  2010-06-14 16:41   ` Linus WALLEIJ
@ 2010-06-15  4:53     ` Dan Williams
  0 siblings, 0 replies; 4+ messages in thread
From: Dan Williams @ 2010-06-15  4:53 UTC (permalink / raw)
  To: Linus WALLEIJ; +Cc: linux-kernel, Jonas ABERG

On Mon, Jun 14, 2010 at 9:41 AM, Linus WALLEIJ
<linus.walleij@stericsson.com> wrote:
> On 06/12/2010 12:38 AM, Dan Williams wrote:
>
>> On Tue, Jun 1, 2010 at 5:21 AM, Linus Walleij
>> <linus.walleij@stericsson.com> wrote:
>>> From: Jonas Aaberg <jonas.aberg@stericsson.com>
>>>
>>> Fix up some locking issues found by enabling lock debugging.
>>>
>>> Signed-off-by: Jonas Aaberg <jonas.aberg@stericsson.com>
>>> Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
>>> ---
>>>  drivers/dma/ste_dma40.c |   14 +++++++-------
>>>  1 files changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c index
>>> aba86bb..c4b0d37 100644
>>> --- a/drivers/dma/ste_dma40.c
>>> +++ b/drivers/dma/ste_dma40.c
>> [..]
>>>  static int d40_channel_execute_command(struct d40_chan *d40c, @@
>>> -1990,8 +1992,6 @@ static struct dma_async_tx_descriptor *d40_prep_slave_sg(struct dma_chan *chan,
>>>        if (d40d == NULL)
>>>                return NULL;
>>>
>>> -       memset(d40d, 0, sizeof(struct d40_desc));
>>> -
>>
>> This one isn't a locking bug fix.
>
> Well, actually it was overwriting a spinlock, so I would considering
> a locking bug area atleast.

Ah, I missed that you call dma_async_tx_descriptor_init() every prep.
It may be worthwhile to select ASYNC_TX_DISABLE_CHANNEL_SWITCH to get
rid of the unused fields for async_tx channel switching *.

> But if you prefer we split this one-liner into a separate patch.

You never take the embedded lock so there was not a problem, but you
can leave it in as an innocuous cleanup.

Thanks,
Dan

* I'm now thinking that channel switching should be an opt-in request
rather than opt-out, because I can foresee a driver relying on the
size reductions it affords.  With the logic flipped we can disable the
driver that has this dependence.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2010-06-15  4:53 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-06-01 12:21 [PATCH 08/18] DMAENGINE: ste_dma40: lock fix Linus Walleij
2010-06-11 22:38 ` Dan Williams
2010-06-14 16:41   ` Linus WALLEIJ
2010-06-15  4:53     ` Dan Williams

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.