Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1337558 > unrolled thread
| Started by | Anand Moon <linux.amoon@gmail.com> |
|---|---|
| First post | 2016-02-18 18:30 +0100 |
| Last post | 2016-02-19 09:20 +0100 |
| Articles | 8 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore Anand Moon <linux.amoon@gmail.com> - 2016-02-18 18:30 +0100
Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-02-19 07:10 +0100
Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore Anand Moon <linux.amoon@gmail.com> - 2016-02-19 07:50 +0100
Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-02-19 08:30 +0100
Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore Lars-Peter Clausen <lars@metafoo.de> - 2016-02-19 09:20 +0100
Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore Anand Moon <linux.amoon@gmail.com> - 2016-02-19 09:50 +0100
Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore Anand Moon <linux.amoon@gmail.com> - 2016-02-21 18:40 +0100
Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore Anand Moon <linux.amoon@gmail.com> - 2016-02-19 09:20 +0100
| From | Anand Moon <linux.amoon@gmail.com> |
|---|---|
| Date | 2016-02-18 18:30 +0100 |
| Subject | [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore |
| Message-ID | <r3AyU-uN-43@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Krzysztof Kozlowski <k.kozlowski@samsung.com> |
|---|---|
| Date | 2016-02-19 07:10 +0100 |
| Message-ID | <r3Mqn-MH-7@gated-at.bofh.it> |
| In reply to | #1337558 |
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
[toc] | [prev] | [next] | [standalone]
| From | Anand Moon <linux.amoon@gmail.com> |
|---|---|
| Date | 2016-02-19 07:50 +0100 |
| Message-ID | <r3N34-13P-7@gated-at.bofh.it> |
| In reply to | #1337900 |
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
[toc] | [prev] | [next] | [standalone]
| From | Krzysztof Kozlowski <k.kozlowski@samsung.com> |
|---|---|
| Date | 2016-02-19 08:30 +0100 |
| Subject | Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore |
| Message-ID | <r3NFM-1zJ-7@gated-at.bofh.it> |
| In reply to | #1337924 |
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 > >
[toc] | [prev] | [next] | [standalone]
| From | Lars-Peter Clausen <lars@metafoo.de> |
|---|---|
| Date | 2016-02-19 09:20 +0100 |
| Subject | Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore |
| Message-ID | <r3Osa-295-13@gated-at.bofh.it> |
| In reply to | #1337941 |
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
[toc] | [prev] | [next] | [standalone]
| From | Anand Moon <linux.amoon@gmail.com> |
|---|---|
| Date | 2016-02-19 09:50 +0100 |
| Message-ID | <r3OVe-2o2-49@gated-at.bofh.it> |
| In reply to | #1337954 |
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
[toc] | [prev] | [next] | [standalone]
| From | Anand Moon <linux.amoon@gmail.com> |
|---|---|
| Date | 2016-02-21 18:40 +0100 |
| Message-ID | <r4G9d-XY-11@gated-at.bofh.it> |
| In reply to | #1337954 |
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
[toc] | [prev] | [next] | [standalone]
| From | Anand Moon <linux.amoon@gmail.com> |
|---|---|
| Date | 2016-02-19 09:20 +0100 |
| Message-ID | <r3Osa-295-15@gated-at.bofh.it> |
| In reply to | #1337941 |
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 >> >> >
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web