Re: [PATCH] dmaengine: pl330: initialize tasklet after spin_unlock_irqrestore

[Anand Moon]

Hi Krzysztof,

On 19 February 2016 at 12:50, Krzysztof Kozlowski
<k.kozlowski@xxxxxxxxxxx> wrote:
> On 19.02.2016 15:39, Anand Moon wrote:
>> Hi Krzysztof,
>>
>> On 19 February 2016 at 11:36, Krzysztof Kozlowski
>> <k.kozlowski@xxxxxxxxxxx> wrote:
>>> 2016-02-19 2:21 GMT+09:00 Anand Moon <linux.amoon@xxxxxxxxx>:
>>>> From: Anand Moon <linux.amoon@xxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxx>
>>>> ---
>>>>  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@xxxxxxxxxxxxxxx
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>