Most of the logic here is try and error since what actually happens does
not match the trm or I miss read it.
My first assumption was that the queue on which the tear-down descriptor
completes (their own complete queue vs "active descriptor" complete
queue) depends on the transfer direction. This seems not to be true
because I manage to trigger
| WARN_ON(c->desc_phys != desc_phys);
and the other few were fine means the tear-down descriptor was valid but
on different queue.
This patch changes the logic here to look on both queues for the
descriptor.
Cc: Daniel Mack <[email protected]>
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
drivers/dma/cppi41.c | 41 ++++++++++++++++-------------------------
1 file changed, 16 insertions(+), 25 deletions(-)
diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
index 42d1c58..272969c 100644
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -563,36 +563,27 @@ static int cppi41_tear_down_chan(struct cppi41_channel *c)
c->td_retry = 100;
}
- if (!c->td_seen) {
+ if (!c->td_seen || !c->td_desc_seen) {
unsigned td_comp_queue;
- if (c->is_tx)
- td_comp_queue = cdd->td_queue.complete;
- else
- td_comp_queue = c->q_comp_num;
+ desc_phys = cppi41_pop_desc(cdd, cdd->td_queue.complete);
+ if (!desc_phys)
+ desc_phys = cppi41_pop_desc(cdd, c->q_comp_num);
- desc_phys = cppi41_pop_desc(cdd, td_comp_queue);
- if (desc_phys) {
- __iormb();
+ if (desc_phys == c->desc_phys) {
+ c->td_desc_seen = 1;
+
+ } else if (desc_phys == td_desc_phys) {
+ u32 pd0;
- if (desc_phys == td_desc_phys) {
- u32 pd0;
- pd0 = td->pd0;
- WARN_ON((pd0 >> DESC_TYPE) != DESC_TYPE_TEARD);
- WARN_ON(!c->is_tx && !(pd0 & TD_DESC_IS_RX));
- WARN_ON((pd0 & 0x1f) != c->port_num);
- } else {
- WARN_ON_ONCE(1);
- }
- c->td_seen = 1;
- }
- }
- if (!c->td_desc_seen) {
- desc_phys = cppi41_pop_desc(cdd, c->q_comp_num);
- if (desc_phys) {
__iormb();
- WARN_ON(c->desc_phys != desc_phys);
- c->td_desc_seen = 1;
+ pd0 = td->pd0;
+ WARN_ON((pd0 >> DESC_TYPE) != DESC_TYPE_TEARD);
+ WARN_ON(!c->is_tx && !(pd0 & TD_DESC_IS_RX));
+ WARN_ON((pd0 & 0x1f) != c->port_num);
+ c->td_seen = 1;
+ } else if (desc_phys) {
+ WARN_ON_ONCE(1);
}
}
c->td_retry--;
--
1.8.4.rc3
Return code of pm_runtime_get_sync() > 0 is not an error and may happen.
Noticed during rmmod & modprobe testing.
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
drivers/dma/cppi41.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
index 272969c..42134f9 100644
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -957,7 +957,7 @@ static int cppi41_dma_probe(struct platform_device *pdev)
pm_runtime_enable(dev);
ret = pm_runtime_get_sync(dev);
- if (ret)
+ if (ret < 0)
goto err_get_sync;
cdd->queues_rx = glue_info->queues_rx;
--
1.8.4.rc3
On 10/17/2013 04:19 PM, Sebastian Andrzej Siewior wrote:
> This patch changes the logic here to look on both queues for the
> descriptor.
Daniel, could please look if this solves your suspend / resume warnings?
Sebastian
On 10/17/2013 04:20 PM, Sebastian Andrzej Siewior wrote:
> On 10/17/2013 04:19 PM, Sebastian Andrzej Siewior wrote:
>> This patch changes the logic here to look on both queues for the
>> descriptor.
>
> Daniel, could please look if this solves your suspend / resume warnings?
Will do (hopefully) tomorrow. So this is a replacement for my "dma:
cppi41: move -EAGAIN in tear_down" patch, or does it go on top of it?
How does your patch queue look like? Someone should probably re-collect
all necessary patches for the next merge window eventually, so Vinod
knows what to apply :)
Thanks,
Daniel
On 10/17/2013 04:23 PM, Daniel Mack wrote:
>> Daniel, could please look if this solves your suspend / resume warnings?
>
> Will do (hopefully) tomorrow. So this is a replacement for my "dma:
> cppi41: move -EAGAIN in tear_down" patch, or does it go on top of it?
I applied your three patches and manage to break 2 of 4 tests I had. It
was still the same without your patch. So I made this change and merged
it back into your patch so this patch should apply ontop of your 2/3.
> How does your patch queue look like? Someone should probably re-collect
> all necessary patches for the next merge window eventually, so Vinod
> knows what to apply :)
dma: cppi41: add support for suspend and resume
dma: cppi41: restore more registers
dma: cppi41: use cppi41_pop_desc() where possible
dma: cppi41: redo descriptor collection in abort case
dma: cppi41: return code > 0 of pm_runtime_get_sync() is not an error
The first one is in Vinod's tree.
>
> Thanks,
> Daniel
Sebastian
On 10/18/2013 11:03 AM, Daniel Mack wrote:
> Hi Sebastian,
>
> On 10/17/2013 04:36 PM, Sebastian Andrzej Siewior wrote:
>> On 10/17/2013 04:23 PM, Daniel Mack wrote:
>>>> Daniel, could please look if this solves your suspend / resume warnings?
>
> Looking very good :)
>
>> dma: cppi41: add support for suspend and resume
>> dma: cppi41: restore more registers
>> dma: cppi41: use cppi41_pop_desc() where possible
>> dma: cppi41: redo descriptor collection in abort case
>> dma: cppi41: return code > 0 of pm_runtime_get_sync() is not an error
>
> Tested with those patches applied, and did dozens of sleep/resume cycley
> - no problem seen.
>
> Thanks a bunch again. Vinod, you can add my
>
> Tested-by: Daniel Mack <[email protected]>
>
> if you want.
Vinod, in case you're ok with these changes, it would be good to have
them for 3.13. Sebastian recently resent them all again (3 from me, 2
from him).
Thanks,
Daniel