* [PATCH] dmaengine: ptdma: fix concurrency issue with multiple dma transfer
@ 2021-12-17 9:58 Sanjay R Mehta
2022-01-03 11:34 ` Vinod Koul
0 siblings, 1 reply; 5+ messages in thread
From: Sanjay R Mehta @ 2021-12-17 9:58 UTC (permalink / raw)
To: vkoul
Cc: gregkh, dan.j.williams, Thomas.Lendacky, robh, mchehab+samsung,
davem, linux-kernel, dmaengine, Sanjay R Mehta
From: Sanjay R Mehta <sanju.mehta@amd.com>
The command should be submitted only if the engine is idle,
for this, the next available descriptor is checked and set the flag
to false in case the descriptor is non-empty.
Also need to segregate the cases when DMA is complete or not.
In case if DMA is already complete there is no need to handle it
again and gracefully exit from the function.
Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
---
drivers/dma/ptdma/ptdma-dmaengine.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/dma/ptdma/ptdma-dmaengine.c b/drivers/dma/ptdma/ptdma-dmaengine.c
index c9e52f6..91b93e8 100644
--- a/drivers/dma/ptdma/ptdma-dmaengine.c
+++ b/drivers/dma/ptdma/ptdma-dmaengine.c
@@ -100,12 +100,17 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
spin_lock_irqsave(&chan->vc.lock, flags);
if (desc) {
- if (desc->status != DMA_ERROR)
- desc->status = DMA_COMPLETE;
-
- dma_cookie_complete(tx_desc);
- dma_descriptor_unmap(tx_desc);
- list_del(&desc->vd.node);
+ if (desc->status != DMA_COMPLETE) {
+ if (desc->status != DMA_ERROR)
+ desc->status = DMA_COMPLETE;
+
+ dma_cookie_complete(tx_desc);
+ dma_descriptor_unmap(tx_desc);
+ list_del(&desc->vd.node);
+ } else {
+ /* Don't handle it twice */
+ tx_desc = NULL;
+ }
}
desc = pt_next_dma_desc(chan);
@@ -233,9 +238,14 @@ static void pt_issue_pending(struct dma_chan *dma_chan)
struct pt_dma_chan *chan = to_pt_chan(dma_chan);
struct pt_dma_desc *desc;
unsigned long flags;
+ bool engine_is_idle = true;
spin_lock_irqsave(&chan->vc.lock, flags);
+ desc = pt_next_dma_desc(chan);
+ if (desc)
+ engine_is_idle = false;
+
vchan_issue_pending(&chan->vc);
desc = pt_next_dma_desc(chan);
@@ -243,7 +253,7 @@ static void pt_issue_pending(struct dma_chan *dma_chan)
spin_unlock_irqrestore(&chan->vc.lock, flags);
/* If there was nothing active, start processing */
- if (desc)
+ if (engine_is_idle)
pt_cmd_callback(desc, 0);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] dmaengine: ptdma: fix concurrency issue with multiple dma transfer
2021-12-17 9:58 [PATCH] dmaengine: ptdma: fix concurrency issue with multiple dma transfer Sanjay R Mehta
@ 2022-01-03 11:34 ` Vinod Koul
2022-01-10 7:57 ` Sanjay R Mehta
0 siblings, 1 reply; 5+ messages in thread
From: Vinod Koul @ 2022-01-03 11:34 UTC (permalink / raw)
To: Sanjay R Mehta
Cc: gregkh, dan.j.williams, Thomas.Lendacky, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On 17-12-21, 03:58, Sanjay R Mehta wrote:
> From: Sanjay R Mehta <sanju.mehta@amd.com>
>
> The command should be submitted only if the engine is idle,
> for this, the next available descriptor is checked and set the flag
> to false in case the descriptor is non-empty.
>
> Also need to segregate the cases when DMA is complete or not.
> In case if DMA is already complete there is no need to handle it
> again and gracefully exit from the function.
>
> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
> ---
> drivers/dma/ptdma/ptdma-dmaengine.c | 24 +++++++++++++++++-------
> 1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma/ptdma/ptdma-dmaengine.c b/drivers/dma/ptdma/ptdma-dmaengine.c
> index c9e52f6..91b93e8 100644
> --- a/drivers/dma/ptdma/ptdma-dmaengine.c
> +++ b/drivers/dma/ptdma/ptdma-dmaengine.c
> @@ -100,12 +100,17 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
> spin_lock_irqsave(&chan->vc.lock, flags);
>
> if (desc) {
> - if (desc->status != DMA_ERROR)
> - desc->status = DMA_COMPLETE;
> -
> - dma_cookie_complete(tx_desc);
> - dma_descriptor_unmap(tx_desc);
> - list_del(&desc->vd.node);
> + if (desc->status != DMA_COMPLETE) {
> + if (desc->status != DMA_ERROR)
> + desc->status = DMA_COMPLETE;
> +
> + dma_cookie_complete(tx_desc);
> + dma_descriptor_unmap(tx_desc);
> + list_del(&desc->vd.node);
> + } else {
> + /* Don't handle it twice */
> + tx_desc = NULL;
> + }
> }
>
> desc = pt_next_dma_desc(chan);
> @@ -233,9 +238,14 @@ static void pt_issue_pending(struct dma_chan *dma_chan)
> struct pt_dma_chan *chan = to_pt_chan(dma_chan);
> struct pt_dma_desc *desc;
> unsigned long flags;
> + bool engine_is_idle = true;
>
> spin_lock_irqsave(&chan->vc.lock, flags);
>
> + desc = pt_next_dma_desc(chan);
> + if (desc)
> + engine_is_idle = false;
> +
> vchan_issue_pending(&chan->vc);
>
> desc = pt_next_dma_desc(chan);
> @@ -243,7 +253,7 @@ static void pt_issue_pending(struct dma_chan *dma_chan)
> spin_unlock_irqrestore(&chan->vc.lock, flags);
>
> /* If there was nothing active, start processing */
> - if (desc)
> + if (engine_is_idle)
Can you explain why do you need this flag and why desc is not
sufficient..
It also sounds like 2 patches to me...
> pt_cmd_callback(desc, 0);
> }
>
> --
> 2.7.4
--
~Vinod
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dmaengine: ptdma: fix concurrency issue with multiple dma transfer
2022-01-03 11:34 ` Vinod Koul
@ 2022-01-10 7:57 ` Sanjay R Mehta
2022-01-18 12:05 ` Sanjay R Mehta
2022-01-19 4:23 ` Vinod Koul
0 siblings, 2 replies; 5+ messages in thread
From: Sanjay R Mehta @ 2022-01-10 7:57 UTC (permalink / raw)
To: Vinod Koul, Sanjay R Mehta
Cc: gregkh, dan.j.williams, Thomas.Lendacky, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On 1/3/2022 5:04 PM, Vinod Koul wrote:
> On 17-12-21, 03:58, Sanjay R Mehta wrote:
>> From: Sanjay R Mehta <sanju.mehta@amd.com>
>>
>> The command should be submitted only if the engine is idle,
>> for this, the next available descriptor is checked and set the flag
>> to false in case the descriptor is non-empty.
>>
>> Also need to segregate the cases when DMA is complete or not.
>> In case if DMA is already complete there is no need to handle it
>> again and gracefully exit from the function.
>>
>> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
>> ---
>> drivers/dma/ptdma/ptdma-dmaengine.c | 24 +++++++++++++++++-------
>> 1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma/ptdma/ptdma-dmaengine.c b/drivers/dma/ptdma/ptdma-dmaengine.c
>> index c9e52f6..91b93e8 100644
>> --- a/drivers/dma/ptdma/ptdma-dmaengine.c
>> +++ b/drivers/dma/ptdma/ptdma-dmaengine.c
>> @@ -100,12 +100,17 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
>> spin_lock_irqsave(&chan->vc.lock, flags);
>>
>> if (desc) {
>> - if (desc->status != DMA_ERROR)
>> - desc->status = DMA_COMPLETE;
>> -
>> - dma_cookie_complete(tx_desc);
>> - dma_descriptor_unmap(tx_desc);
>> - list_del(&desc->vd.node);
>> + if (desc->status != DMA_COMPLETE) {
>> + if (desc->status != DMA_ERROR)
>> + desc->status = DMA_COMPLETE;
>> +
>> + dma_cookie_complete(tx_desc);
>> + dma_descriptor_unmap(tx_desc);
>> + list_del(&desc->vd.node);
>> + } else {
>> + /* Don't handle it twice */
>> + tx_desc = NULL;
>> + }
>> }
>>
>> desc = pt_next_dma_desc(chan);
>> @@ -233,9 +238,14 @@ static void pt_issue_pending(struct dma_chan *dma_chan)
>> struct pt_dma_chan *chan = to_pt_chan(dma_chan);
>> struct pt_dma_desc *desc;
>> unsigned long flags;
>> + bool engine_is_idle = true;
>>
>> spin_lock_irqsave(&chan->vc.lock, flags);
>>
>> + desc = pt_next_dma_desc(chan);
>> + if (desc)
>> + engine_is_idle = false;
>> +
>> vchan_issue_pending(&chan->vc);
>>
>> desc = pt_next_dma_desc(chan);
>> @@ -243,7 +253,7 @@ static void pt_issue_pending(struct dma_chan *dma_chan)
>> spin_unlock_irqrestore(&chan->vc.lock, flags);
>>
>> /* If there was nothing active, start processing */
>> - if (desc)
>> + if (engine_is_idle)
>
> Can you explain why do you need this flag and why desc is not
> sufficient..
Here it is required to know if the engine was idle or not before
submitting new desc to the active list (i.e, before calling
"vchan_issue_pending()" API). So that if there was nothing active then
start processing this desc otherwise later.
Here desc is submitted to the engine after vchan_issue_pending() API
called which will actually put the desc into the active list and then if
I get the next desc, the condition will always be true. Therefore used
this flag here to solve this issue.
>
> It also sounds like 2 patches to me...
Once the desc is submitted to the engine that will be handled by
pt_handle_active_desc() function. This issue was resolved by making
these changes together. Hence kept into the single patch.
Please suggest to me, if this still needs to be split. I'll make the
changes accordingly.
- Sanjay
>
>> pt_cmd_callback(desc, 0);
>> }
>>
>> --
>> 2.7.4
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dmaengine: ptdma: fix concurrency issue with multiple dma transfer
2022-01-10 7:57 ` Sanjay R Mehta
@ 2022-01-18 12:05 ` Sanjay R Mehta
2022-01-19 4:23 ` Vinod Koul
1 sibling, 0 replies; 5+ messages in thread
From: Sanjay R Mehta @ 2022-01-18 12:05 UTC (permalink / raw)
To: Vinod Koul, Sanjay R Mehta
Cc: gregkh, dan.j.williams, Thomas.Lendacky, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On 1/10/2022 1:27 PM, Sanjay R Mehta wrote:
>
>
> On 1/3/2022 5:04 PM, Vinod Koul wrote:
>> On 17-12-21, 03:58, Sanjay R Mehta wrote:
>>> From: Sanjay R Mehta <sanju.mehta@amd.com>
>>>
>>> The command should be submitted only if the engine is idle,
>>> for this, the next available descriptor is checked and set the flag
>>> to false in case the descriptor is non-empty.
>>>
>>> Also need to segregate the cases when DMA is complete or not.
>>> In case if DMA is already complete there is no need to handle it
>>> again and gracefully exit from the function.
>>>
>>> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
>>> ---
>>> drivers/dma/ptdma/ptdma-dmaengine.c | 24 +++++++++++++++++-------
>>> 1 file changed, 17 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/dma/ptdma/ptdma-dmaengine.c b/drivers/dma/ptdma/ptdma-dmaengine.c
>>> index c9e52f6..91b93e8 100644
>>> --- a/drivers/dma/ptdma/ptdma-dmaengine.c
>>> +++ b/drivers/dma/ptdma/ptdma-dmaengine.c
>>> @@ -100,12 +100,17 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
>>> spin_lock_irqsave(&chan->vc.lock, flags);
>>>
>>> if (desc) {
>>> - if (desc->status != DMA_ERROR)
>>> - desc->status = DMA_COMPLETE;
>>> -
>>> - dma_cookie_complete(tx_desc);
>>> - dma_descriptor_unmap(tx_desc);
>>> - list_del(&desc->vd.node);
>>> + if (desc->status != DMA_COMPLETE) {
>>> + if (desc->status != DMA_ERROR)
>>> + desc->status = DMA_COMPLETE;
>>> +
>>> + dma_cookie_complete(tx_desc);
>>> + dma_descriptor_unmap(tx_desc);
>>> + list_del(&desc->vd.node);
>>> + } else {
>>> + /* Don't handle it twice */
>>> + tx_desc = NULL;
>>> + }
>>> }
>>>
>>> desc = pt_next_dma_desc(chan);
>>> @@ -233,9 +238,14 @@ static void pt_issue_pending(struct dma_chan *dma_chan)
>>> struct pt_dma_chan *chan = to_pt_chan(dma_chan);
>>> struct pt_dma_desc *desc;
>>> unsigned long flags;
>>> + bool engine_is_idle = true;
>>>
>>> spin_lock_irqsave(&chan->vc.lock, flags);
>>>
>>> + desc = pt_next_dma_desc(chan);
>>> + if (desc)
>>> + engine_is_idle = false;
>>> +
>>> vchan_issue_pending(&chan->vc);
>>>
>>> desc = pt_next_dma_desc(chan);
>>> @@ -243,7 +253,7 @@ static void pt_issue_pending(struct dma_chan *dma_chan)
>>> spin_unlock_irqrestore(&chan->vc.lock, flags);
>>>
>>> /* If there was nothing active, start processing */
>>> - if (desc)
>>> + if (engine_is_idle)
>>
>> Can you explain why do you need this flag and why desc is not
>> sufficient..
>
> Here it is required to know if the engine was idle or not before
> submitting new desc to the active list (i.e, before calling
> "vchan_issue_pending()" API). So that if there was nothing active then
> start processing this desc otherwise later.
>
> Here desc is submitted to the engine after vchan_issue_pending() API
> called which will actually put the desc into the active list and then if
> I get the next desc, the condition will always be true. Therefore used
> this flag here to solve this issue.
>
>>
>> It also sounds like 2 patches to me...
>
> Once the desc is submitted to the engine that will be handled by
> pt_handle_active_desc() function. This issue was resolved by making
> these changes together. Hence kept into the single patch.
>
> Please suggest to me, if this still needs to be split. I'll make the
> changes accordingly.
>
Hi Vinod,
Any further comments for this patch? Need your help to get this upstreamed.
> - Sanjay
>
>>
>>> pt_cmd_callback(desc, 0);
>>> }
>>>
>>> --
>>> 2.7.4
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dmaengine: ptdma: fix concurrency issue with multiple dma transfer
2022-01-10 7:57 ` Sanjay R Mehta
2022-01-18 12:05 ` Sanjay R Mehta
@ 2022-01-19 4:23 ` Vinod Koul
1 sibling, 0 replies; 5+ messages in thread
From: Vinod Koul @ 2022-01-19 4:23 UTC (permalink / raw)
To: Sanjay R Mehta
Cc: Sanjay R Mehta, gregkh, dan.j.williams, Thomas.Lendacky, robh,
mchehab+samsung, davem, linux-kernel, dmaengine
On 10-01-22, 13:27, Sanjay R Mehta wrote:
> On 1/3/2022 5:04 PM, Vinod Koul wrote:
> > On 17-12-21, 03:58, Sanjay R Mehta wrote:
> >> From: Sanjay R Mehta <sanju.mehta@amd.com>
> >>
> >> The command should be submitted only if the engine is idle,
> >> for this, the next available descriptor is checked and set the flag
> >> to false in case the descriptor is non-empty.
> >>
> >> Also need to segregate the cases when DMA is complete or not.
> >> In case if DMA is already complete there is no need to handle it
> >> again and gracefully exit from the function.
> >>
> >> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
> >> ---
> >> drivers/dma/ptdma/ptdma-dmaengine.c | 24 +++++++++++++++++-------
> >> 1 file changed, 17 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/dma/ptdma/ptdma-dmaengine.c b/drivers/dma/ptdma/ptdma-dmaengine.c
> >> index c9e52f6..91b93e8 100644
> >> --- a/drivers/dma/ptdma/ptdma-dmaengine.c
> >> +++ b/drivers/dma/ptdma/ptdma-dmaengine.c
> >> @@ -100,12 +100,17 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
> >> spin_lock_irqsave(&chan->vc.lock, flags);
> >>
> >> if (desc) {
> >> - if (desc->status != DMA_ERROR)
> >> - desc->status = DMA_COMPLETE;
> >> -
> >> - dma_cookie_complete(tx_desc);
> >> - dma_descriptor_unmap(tx_desc);
> >> - list_del(&desc->vd.node);
> >> + if (desc->status != DMA_COMPLETE) {
> >> + if (desc->status != DMA_ERROR)
> >> + desc->status = DMA_COMPLETE;
> >> +
> >> + dma_cookie_complete(tx_desc);
> >> + dma_descriptor_unmap(tx_desc);
> >> + list_del(&desc->vd.node);
> >> + } else {
> >> + /* Don't handle it twice */
> >> + tx_desc = NULL;
> >> + }
> >> }
> >>
> >> desc = pt_next_dma_desc(chan);
> >> @@ -233,9 +238,14 @@ static void pt_issue_pending(struct dma_chan *dma_chan)
> >> struct pt_dma_chan *chan = to_pt_chan(dma_chan);
> >> struct pt_dma_desc *desc;
> >> unsigned long flags;
> >> + bool engine_is_idle = true;
> >>
> >> spin_lock_irqsave(&chan->vc.lock, flags);
> >>
> >> + desc = pt_next_dma_desc(chan);
> >> + if (desc)
> >> + engine_is_idle = false;
> >> +
> >> vchan_issue_pending(&chan->vc);
> >>
> >> desc = pt_next_dma_desc(chan);
> >> @@ -243,7 +253,7 @@ static void pt_issue_pending(struct dma_chan *dma_chan)
> >> spin_unlock_irqrestore(&chan->vc.lock, flags);
> >>
> >> /* If there was nothing active, start processing */
> >> - if (desc)
> >> + if (engine_is_idle)
> >
> > Can you explain why do you need this flag and why desc is not
> > sufficient..
>
> Here it is required to know if the engine was idle or not before
> submitting new desc to the active list (i.e, before calling
> "vchan_issue_pending()" API). So that if there was nothing active then
> start processing this desc otherwise later.
>
> Here desc is submitted to the engine after vchan_issue_pending() API
> called which will actually put the desc into the active list and then if
> I get the next desc, the condition will always be true. Therefore used
> this flag here to solve this issue.
ok
>
> >
> > It also sounds like 2 patches to me...
>
> Once the desc is submitted to the engine that will be handled by
> pt_handle_active_desc() function. This issue was resolved by making
> these changes together. Hence kept into the single patch.
>
> Please suggest to me, if this still needs to be split. I'll make the
> changes accordingly.
2 patches please
--
~Vinod
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-01-19 4:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-17 9:58 [PATCH] dmaengine: ptdma: fix concurrency issue with multiple dma transfer Sanjay R Mehta
2022-01-03 11:34 ` Vinod Koul
2022-01-10 7:57 ` Sanjay R Mehta
2022-01-18 12:05 ` Sanjay R Mehta
2022-01-19 4:23 ` Vinod Koul
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).