dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore

PATCH In review Anand Moon 3 participants 8 messages Feb 18, 2016 – Feb 21, 2016 dmaengine

lore

Share & tools

lore

Anand Moon

author

2016-02-18 17:22 UTC #0 lore raw

PATCH 1/1 dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore

From: Anand Moon <[email protected]>

pl330_tasklet tasklet uses the same spinlock pch->lock for safe IRQ locking. It's safe to initialize pl330_tasklet tasklet after release of the locking.

Signed-off-by Anand Moon <[email protected]>

---
 drivers/dma/pl330.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/dma/pl330.c +2 -2

index 17ee758..df2cab1 100644
@@ -2091,10 +2091,10 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan)
 		return -ENOMEM;
 	}
 
-	tasklet_init(&pch->task, pl330_tasklet, (unsigned long) pch);
-
 	spin_unlock_irqrestore(&pch->lock, flags);
 
+	tasklet_init(&pch->task, pl330_tasklet, (unsigned long) pch);
+
 	return 1;
 }

Signature · 1 line

1.9.1

Krzysztof Kozlowski

2016-02-19 06:07 UTC #1 lore raw

2016-02-19 2:21 GMT+09:00 Anand Moon <[email protected]>:

Quoted 4 lines

From: Anand Moon <[email protected]>
pl330_tasklet tasklet uses the same spinlock pch->lock for safe IRQ locking. It's safe to initialize pl330_tasklet tasklet after release of the locking.

This is tasklet init, not tasklet execution (which you are referring to in first sentence). I don't get how usage of spinlock during execution guarantees the safeness during init... Please describe why this is safe.

Best regards,
Krzysztof

Quoted 30 lines

Signed-off-by Anand Moon <[email protected]>

---
 drivers/dma/pl330.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/dma/pl330.c +2 -3

index 17ee758..df2cab1 100644
@@ -2091,10 +2091,10 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan)
                return -ENOMEM;
        }
 
-       tasklet_init(&pch->task, pl330_tasklet, (unsigned long) pch);
-
        spin_unlock_irqrestore(&pch->lock, flags);
 
+       tasklet_init(&pch->task, pl330_tasklet, (unsigned long) pch);
+
        return 1;
 }
 
--

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Anand Moon

author

2016-02-19 06:40 UTC #2 lore raw

Hi Krzysztof,

Quoted 6 lines · Krzysztof Kozlowski

2016-02-19 2:21 GMT+09:00 Anand Moon <[email protected]>:
Quoted 4 lines
From: Anand Moon <[email protected]>
pl330_tasklet tasklet uses the same spinlock pch->lock for safe IRQ locking. It's safe to initialize pl330_tasklet tasklet after release of the locking.

Quoted 8 lines

This is tasklet init, not tasklet execution (which you are referring to in first sentence). I don't get how usage of spinlock during execution guarantees the safeness during init... Please describe why this is safe.
Best regards,
Krzysztof

http://lxr.free-electrons.com/source/drivers/dma/pl330.c#L1972

pl330_tasklet function which is initiated by tasklet_init is trying to lock using same spin_unlock_irqsave/restore pch->lock.
So better release the pch->lock and then initialize the tasklet_init.

Best Regards,
-Anand Moon

Quoted 30 lines

Signed-off-by Anand Moon <[email protected]>

---
 drivers/dma/pl330.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/dma/pl330.c +2 -3

index 17ee758..df2cab1 100644
@@ -2091,10 +2091,10 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan)
                return -ENOMEM;
        }
 
-       tasklet_init(&pch->task, pl330_tasklet, (unsigned long) pch);
-
        spin_unlock_irqrestore(&pch->lock, flags);
 
+       tasklet_init(&pch->task, pl330_tasklet, (unsigned long) pch);
+
        return 1;
 }
 
--

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Krzysztof Kozlowski

2016-02-19 07:20 UTC #3 lore raw

Quoted 15 lines · Anand Moon

Hi Krzysztof,
Quoted 11 lines · Krzysztof Kozlowski
2016-02-19 2:21 GMT+09:00 Anand Moon <[email protected]>:
Quoted 4 lines
From: Anand Moon <[email protected]>
pl330_tasklet tasklet uses the same spinlock pch->lock for safe IRQ locking. It's safe to initialize pl330_tasklet tasklet after release of the locking.
This is tasklet init, not tasklet execution (which you are referring to in first sentence). I don't get how usage of spinlock during execution guarantees the safeness during init... Please describe why this is safe.

Quoted 8 lines

Quoted 3 lines
Best regards,
Krzysztof
http://lxr.free-electrons.com/source/drivers/dma/pl330.c#L1972
pl330_tasklet function which is initiated by tasklet_init is trying to lock using same spin_unlock_irqsave/restore pch->lock.

tasklet_init does not call pl330_tasklet (if this is what you mean by "initiated"). What is the correlation? Why are you referring to the locks in pl330_tasklet?

Quoted 1 line

So better release the pch->lock and then initialize the tasklet_init.

Why "better"?

Best regards,
Krzysztof

Quoted 33 lines

Quoted 31 lines
Quoted 31 lines
Signed-off-by Anand Moon <[email protected]>
---
 drivers/dma/pl330.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/dma/pl330.c +2 -3
index 17ee758..df2cab1 100644
@@ -2091,10 +2091,10 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan)
                return -ENOMEM;
        }
 
-       tasklet_init(&pch->task, pl330_tasklet, (unsigned long) pch);
-
        spin_unlock_irqrestore(&pch->lock, flags);
 
+       tasklet_init(&pch->task, pl330_tasklet, (unsigned long) pch);
+
        return 1;
 }
 
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Anand Moon

author

2016-02-19 08:11 UTC #4 lore raw

Hi Krzysztof,

Quoted 29 lines · Krzysztof Kozlowski

Quoted 15 lines · Anand Moon
Hi Krzysztof,
Quoted 11 lines · Krzysztof Kozlowski
2016-02-19 2:21 GMT+09:00 Anand Moon <[email protected]>:
Quoted 4 lines
From: Anand Moon <[email protected]>
pl330_tasklet tasklet uses the same spinlock pch->lock for safe IRQ locking. It's safe to initialize pl330_tasklet tasklet after release of the locking.
This is tasklet init, not tasklet execution (which you are referring to in first sentence). I don't get how usage of spinlock during execution guarantees the safeness during init... Please describe why this is safe.
Quoted 8 lines
Quoted 3 lines
Best regards,
Krzysztof
http://lxr.free-electrons.com/source/drivers/dma/pl330.c#L1972
pl330_tasklet function which is initiated by tasklet_init is trying to lock using same spin_unlock_irqsave/restore pch->lock.
tasklet_init does not call pl330_tasklet (if this is what you mean by "initiated"). What is the correlation? Why are you referring to the locks in pl330_tasklet?

Quoted 7 lines

Quoted 1 line
So better release the pch->lock and then initialize the tasklet_init.
Why "better"?
Best regards,
Krzysztof

On SMP arch, tasklet_init could spawn the pl330_tasklet routine, it could be any CPU which could take up this task.
So just for good timing of Initialization of the pl330_tasklet after spin_unlock_irqrestore.
That is what I can figure out.

My choice of words could be a problem.

Best regards,
-Anand Moon

Quoted 34 lines

Quoted 33 lines
Quoted 31 lines
Quoted 31 lines
Signed-off-by Anand Moon <[email protected]>
---
 drivers/dma/pl330.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/dma/pl330.c +2 -3
index 17ee758..df2cab1 100644
@@ -2091,10 +2091,10 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan)
                return -ENOMEM;
        }
 
-       tasklet_init(&pch->task, pl330_tasklet, (unsigned long) pch);
-
        spin_unlock_irqrestore(&pch->lock, flags);
 
+       tasklet_init(&pch->task, pl330_tasklet, (unsigned long) pch);
+
        return 1;
 }
 
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lars-Peter Clausen

2016-02-19 08:15 UTC #5 lore raw

Quoted 41 lines · Anand Moon

Hi Krzysztof,
Quoted 29 lines · Krzysztof Kozlowski
Quoted 15 lines · Anand Moon
Hi Krzysztof,
Quoted 11 lines · Krzysztof Kozlowski
2016-02-19 2:21 GMT+09:00 Anand Moon <[email protected]>:
Quoted 4 lines
From: Anand Moon <[email protected]>
pl330_tasklet tasklet uses the same spinlock pch->lock for safe IRQ locking. It's safe to initialize pl330_tasklet tasklet after release of the locking.
This is tasklet init, not tasklet execution (which you are referring to in first sentence). I don't get how usage of spinlock during execution guarantees the safeness during init... Please describe why this is safe.
Quoted 8 lines
Quoted 3 lines
Best regards,
Krzysztof
http://lxr.free-electrons.com/source/drivers/dma/pl330.c#L1972
pl330_tasklet function which is initiated by tasklet_init is trying to lock using same spin_unlock_irqsave/restore pch->lock.
tasklet_init does not call pl330_tasklet (if this is what you mean by "initiated"). What is the correlation? Why are you referring to the locks in pl330_tasklet?
Quoted 7 lines
Quoted 1 line
So better release the pch->lock and then initialize the tasklet_init.
Why "better"?
Best regards,
Krzysztof

Quoted 5 lines

On SMP arch, tasklet_init could spawn the pl330_tasklet routine, it could be any CPU which could take up this task.
So just for good timing of Initialization of the pl330_tasklet after spin_unlock_irqrestore.
That is what I can figure out.

Hi,

tasklet_init() does not spwan the tasklet function, tasklet_schedule() does that.

But there is still room for optimization here. If you want to move the tasklet_init() call please move it into pl330_probe() next to where the channel is allocated. There is no need to re-initialize the tasklet each time the channel is requested.

- Lars

Anand Moon

author

2016-02-19 08:46 UTC #6 lore raw

hi Lars-Peter,

Quoted 53 lines · Lars-Peter Clausen

Quoted 41 lines · Anand Moon
Hi Krzysztof,
Quoted 29 lines · Krzysztof Kozlowski
Quoted 15 lines · Anand Moon
Hi Krzysztof,
Quoted 11 lines · Krzysztof Kozlowski
2016-02-19 2:21 GMT+09:00 Anand Moon <[email protected]>:
Quoted 4 lines
From: Anand Moon <[email protected]>
pl330_tasklet tasklet uses the same spinlock pch->lock for safe IRQ locking. It's safe to initialize pl330_tasklet tasklet after release of the locking.
This is tasklet init, not tasklet execution (which you are referring to in first sentence). I don't get how usage of spinlock during execution guarantees the safeness during init... Please describe why this is safe.
Quoted 8 lines
Quoted 3 lines
Best regards,
Krzysztof
http://lxr.free-electrons.com/source/drivers/dma/pl330.c#L1972
pl330_tasklet function which is initiated by tasklet_init is trying to lock using same spin_unlock_irqsave/restore pch->lock.
tasklet_init does not call pl330_tasklet (if this is what you mean by "initiated"). What is the correlation? Why are you referring to the locks in pl330_tasklet?
Quoted 7 lines
Quoted 1 line
So better release the pch->lock and then initialize the tasklet_init.
Why "better"?
Best regards,
Krzysztof
Quoted 5 lines
On SMP arch, tasklet_init could spawn the pl330_tasklet routine, it could be any CPU which could take up this task.
So just for good timing of Initialization of the pl330_tasklet after spin_unlock_irqrestore.
That is what I can figure out.
Hi,
tasklet_init() does not spwan the tasklet function, tasklet_schedule() does that.

Quoted 7 lines

But there is still room for optimization here. If you want to move the tasklet_init() call please move it into pl330_probe() next to where the channel is allocated. There is no need to re-initialize the tasklet each time the channel is requested.
- Lars

Thanks for clearing my miss-concept.

Best Regards.
-Anand moon

Anand Moon

author

2016-02-21 17:35 UTC #7 lore raw

In reply to Lars-Peter Clausen

Hi Krzysztof,

Quoted 53 lines · Lars-Peter Clausen

Quoted 41 lines · Anand Moon
Hi Krzysztof,
Quoted 29 lines · Krzysztof Kozlowski
Quoted 15 lines · Anand Moon
Hi Krzysztof,
Quoted 11 lines · Krzysztof Kozlowski
2016-02-19 2:21 GMT+09:00 Anand Moon <[email protected]>:
Quoted 4 lines
From: Anand Moon <[email protected]>
pl330_tasklet tasklet uses the same spinlock pch->lock for safe IRQ locking. It's safe to initialize pl330_tasklet tasklet after release of the locking.
This is tasklet init, not tasklet execution (which you are referring to in first sentence). I don't get how usage of spinlock during execution guarantees the safeness during init... Please describe why this is safe.
Quoted 8 lines
Quoted 3 lines
Best regards,
Krzysztof
http://lxr.free-electrons.com/source/drivers/dma/pl330.c#L1972
pl330_tasklet function which is initiated by tasklet_init is trying to lock using same spin_unlock_irqsave/restore pch->lock.
tasklet_init does not call pl330_tasklet (if this is what you mean by "initiated"). What is the correlation? Why are you referring to the locks in pl330_tasklet?
Quoted 7 lines
Quoted 1 line
So better release the pch->lock and then initialize the tasklet_init.
Why "better"?
Best regards,
Krzysztof
Quoted 5 lines
On SMP arch, tasklet_init could spawn the pl330_tasklet routine, it could be any CPU which could take up this task.
So just for good timing of Initialization of the pl330_tasklet after spin_unlock_irqrestore.
That is what I can figure out.
Hi,
tasklet_init() does not spwan the tasklet function, tasklet_schedule() does that.

Quoted 7 lines

But there is still room for optimization here. If you want to move the tasklet_init() call please move it into pl330_probe() next to where the channel is allocated. There is no need to re-initialize the tasklet each time the channel is requested.
- Lars

After looking at the history of the change logs. I found below changes.

commit da331ba8e9c5de72a27e50f71105395bba6eebe0

Author: Bartlomiej Zolnierkiewicz <[email protected]>
Date:   Wed Jul 3 15:00:43 2013 -0700

drivers/dma/pl330.c: fix locking in pl330_free_chan_resources()

tasklet_kill() may sleep so call it before taking pch->lock.

---------------------------------------------
sorry for the noise.
Next time I will be more careful.

-Anand Moon