* [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore
@ 2016-02-18 17:21 Anand Moon
2016-02-19 6:06 ` Krzysztof Kozlowski
0 siblings, 1 reply; 8+ messages in thread
From: Anand Moon @ 2016-02-18 17:21 UTC (permalink / raw)
To: Vinod Koul, Dan Williams, Anand Moon; +Cc: dmaengine, linux-kernel
From: Anand Moon <linux.amoon@gmail.com>
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 <linux.amoon@gmail.com>
---
drivers/dma/pl330.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 17ee758..df2cab1 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore
2016-02-18 17:21 [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore Anand Moon
@ 2016-02-19 6:06 ` Krzysztof Kozlowski
2016-02-19 6:39 ` Anand Moon
0 siblings, 1 reply; 8+ messages in thread
From: Krzysztof Kozlowski @ 2016-02-19 6:06 UTC (permalink / raw)
To: Anand Moon; +Cc: Vinod Koul, Dan Williams, dmaengine, linux-kernel
2016-02-19 2:21 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
> From: Anand Moon <linux.amoon@gmail.com>
>
> 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
>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> drivers/dma/pl330.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 17ee758..df2cab1 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore
2016-02-19 6:06 ` Krzysztof Kozlowski
@ 2016-02-19 6:39 ` Anand Moon
2016-02-19 7:20 ` Krzysztof Kozlowski
0 siblings, 1 reply; 8+ messages in thread
From: Anand Moon @ 2016-02-19 6:39 UTC (permalink / raw)
To: Krzysztof Kozlowski; +Cc: Vinod Koul, Dan Williams, dmaengine, Linux Kernel
Hi Krzysztof,
On 19 February 2016 at 11:36, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> 2016-02-19 2:21 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>> From: Anand Moon <linux.amoon@gmail.com>
>>
>> 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
>
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
>>
>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>> ---
>> drivers/dma/pl330.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index 17ee758..df2cab1 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -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 majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore
2016-02-19 6:39 ` Anand Moon
@ 2016-02-19 7:20 ` Krzysztof Kozlowski
2016-02-19 8:10 ` Anand Moon
0 siblings, 1 reply; 8+ messages in thread
From: Krzysztof Kozlowski @ 2016-02-19 7:20 UTC (permalink / raw)
To: Anand Moon; +Cc: Vinod Koul, Dan Williams, dmaengine, Linux Kernel
On 19.02.2016 15:39, Anand Moon wrote:
> Hi Krzysztof,
>
> On 19 February 2016 at 11:36, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
>> 2016-02-19 2:21 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>> From: Anand Moon <linux.amoon@gmail.com>
>>>
>>> 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
>>
>
> 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?
> So better release the pch->lock and then initialize the tasklet_init.
Why "better"?
Best regards,
Krzysztof
>
>>>
>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>> ---
>>> drivers/dma/pl330.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>>> index 17ee758..df2cab1 100644
>>> --- a/drivers/dma/pl330.c
>>> +++ b/drivers/dma/pl330.c
>>> @@ -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 majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore
2016-02-19 7:20 ` Krzysztof Kozlowski
@ 2016-02-19 8:10 ` Anand Moon
2016-02-19 8:15 ` Lars-Peter Clausen
0 siblings, 1 reply; 8+ messages in thread
From: Anand Moon @ 2016-02-19 8:10 UTC (permalink / raw)
To: Krzysztof Kozlowski; +Cc: Vinod Koul, Dan Williams, dmaengine, Linux Kernel
Hi Krzysztof,
On 19 February 2016 at 12:50, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> On 19.02.2016 15:39, Anand Moon wrote:
>> Hi Krzysztof,
>>
>> On 19 February 2016 at 11:36, Krzysztof Kozlowski
>> <k.kozlowski@samsung.com> wrote:
>>> 2016-02-19 2:21 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>>> From: Anand Moon <linux.amoon@gmail.com>
>>>>
>>>> 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
>>>
>>
>> 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?
>
>> 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
>>
>>>>
>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>> ---
>>>> drivers/dma/pl330.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>>>> index 17ee758..df2cab1 100644
>>>> --- a/drivers/dma/pl330.c
>>>> +++ b/drivers/dma/pl330.c
>>>> @@ -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 majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore
2016-02-19 8:10 ` Anand Moon
@ 2016-02-19 8:15 ` Lars-Peter Clausen
2016-02-19 8:46 ` Anand Moon
2016-02-21 17:34 ` Anand Moon
0 siblings, 2 replies; 8+ messages in thread
From: Lars-Peter Clausen @ 2016-02-19 8:15 UTC (permalink / raw)
To: Anand Moon, Krzysztof Kozlowski
Cc: Vinod Koul, Dan Williams, dmaengine, Linux Kernel
On 02/19/2016 09:10 AM, Anand Moon wrote:
> Hi Krzysztof,
>
> On 19 February 2016 at 12:50, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
>> On 19.02.2016 15:39, Anand Moon wrote:
>>> Hi Krzysztof,
>>>
>>> On 19 February 2016 at 11:36, Krzysztof Kozlowski
>>> <k.kozlowski@samsung.com> wrote:
>>>> 2016-02-19 2:21 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>>>> From: Anand Moon <linux.amoon@gmail.com>
>>>>>
>>>>> 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
>>>>
>>>
>>> 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?
>>
>>> 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.
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore
2016-02-19 8:15 ` Lars-Peter Clausen
@ 2016-02-19 8:46 ` Anand Moon
2016-02-21 17:34 ` Anand Moon
1 sibling, 0 replies; 8+ messages in thread
From: Anand Moon @ 2016-02-19 8:46 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Krzysztof Kozlowski, Vinod Koul, Dan Williams, dmaengine, Linux Kernel
hi Lars-Peter,
On 19 February 2016 at 13:45, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 02/19/2016 09:10 AM, Anand Moon wrote:
>> Hi Krzysztof,
>>
>> On 19 February 2016 at 12:50, Krzysztof Kozlowski
>> <k.kozlowski@samsung.com> wrote:
>>> On 19.02.2016 15:39, Anand Moon wrote:
>>>> Hi Krzysztof,
>>>>
>>>> On 19 February 2016 at 11:36, Krzysztof Kozlowski
>>>> <k.kozlowski@samsung.com> wrote:
>>>>> 2016-02-19 2:21 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>>>>> From: Anand Moon <linux.amoon@gmail.com>
>>>>>>
>>>>>> 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
>>>>>
>>>>
>>>> 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?
>>>
>>>> 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.
>
> 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
>
Thanks for clearing my miss-concept.
Best Regards.
-Anand moon
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore
2016-02-19 8:15 ` Lars-Peter Clausen
2016-02-19 8:46 ` Anand Moon
@ 2016-02-21 17:34 ` Anand Moon
1 sibling, 0 replies; 8+ messages in thread
From: Anand Moon @ 2016-02-21 17:34 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Krzysztof Kozlowski, Vinod Koul, Dan Williams, dmaengine, Linux Kernel
Hi Krzysztof,
On 19 February 2016 at 13:45, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 02/19/2016 09:10 AM, Anand Moon wrote:
>> Hi Krzysztof,
>>
>> On 19 February 2016 at 12:50, Krzysztof Kozlowski
>> <k.kozlowski@samsung.com> wrote:
>>> On 19.02.2016 15:39, Anand Moon wrote:
>>>> Hi Krzysztof,
>>>>
>>>> On 19 February 2016 at 11:36, Krzysztof Kozlowski
>>>> <k.kozlowski@samsung.com> wrote:
>>>>> 2016-02-19 2:21 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>>>>> From: Anand Moon <linux.amoon@gmail.com>
>>>>>>
>>>>>> 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
>>>>>
>>>>
>>>> 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?
>>>
>>>> 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.
>
> 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
>
After looking at the history of the change logs. I found below changes.
commit da331ba8e9c5de72a27e50f71105395bba6eebe0
Author: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-02-21 17:35 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-18 17:21 [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore Anand Moon
2016-02-19 6:06 ` Krzysztof Kozlowski
2016-02-19 6:39 ` Anand Moon
2016-02-19 7:20 ` Krzysztof Kozlowski
2016-02-19 8:10 ` Anand Moon
2016-02-19 8:15 ` Lars-Peter Clausen
2016-02-19 8:46 ` Anand Moon
2016-02-21 17:34 ` Anand Moon
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.