dmaengine: dmatest: move callback wait queue to thread context
PATCH v3 1/1 dmaengine: dmatest: move callback wait queue to thread context
Commit adfa543e7314 ("dmatest: don't use set_freezable_with_signal()") introduced a bug (that is in fact documented by the patch commit text) that leaves behind a dangling pointer. Since the done_wait structure is allocated on the stack, future invocations to the DMATEST can produce undesirable results (e.g., corrupted spinlocks).
Commit a9df21e34b42 ("dmaengine: dmatest: warn user when dma test times out") attempted to WARN the user that the stack was likely corrupted but did not fix the actual issue.
This patch fixes the issue by pushing the wait queue and callback structs into the the thread structure. If a failure occurs due to time, dmaengine_terminate_all will force the callback to safely call wake_up_all() without possibility of using a freed pointer.
---
changes from v2: Added "Fixes" tag
changes from v1: Added pre-req patches for stable
drivers/dma/dmatest.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)
drivers/dma/dmatest.c +16 -21
@@ -155,6 +155,12 @@ struct dmatest_params {#define PATTERN_COUNT_MASK 0x1f#define PATTERN_MEMSET_IDX 0x01 +/* poor man's completion - we want to use wait_event_freezable() on it */ +struct dmatest_done { +bool done; +wait_queue_head_t *wait; +}; +struct dmatest_thread {struct list_head node;struct dmatest_info *info;
@@ -165,6 +171,8 @@ struct dmatest_thread {u8 **dsts;u8 **udsts;enum dma_transaction_type type; +wait_queue_head_t done_wait; +struct dmatest_done test_done;bool done; };
@@ -342,11 +350,6 @@ static unsigned int dmatest_verify(u8 **bufs, unsigned int start,return error_count; } -/* poor man's completion - we want to use wait_event_freezable() on it */ -struct dmatest_done { -bool done; -wait_queue_head_t *wait; -};static void dmatest_callback (void *arg) {
@@ -424,9 +427,8 @@ static unsigned long long dmatest_KBs(s64 runtime, unsigned long long len)*/ static int dmatest_func (void *data) { -DECLARE_WAIT_QUEUE_HEAD_ONSTACK (done_wait);struct dmatest_thread *thread = data; -struct dmatest_done done = { .wait = &done_wait }; +struct dmatest_done *done = &thread->test_done;struct dmatest_info *info;struct dmatest_params *params;struct dma_chan *chan;
@@ -673,9 +675,9 @@ static int dmatest_func(void *data)continue ; } - done.done = false; + done->done = false; tx->callback = dmatest_callback; - tx->callback_param = &done; + tx->callback_param = done; cookie = tx->tx_submit (tx);if (dma_submit_error (cookie)) {
@@ -688,21 +690,12 @@ static int dmatest_func(void *data) }dma_async_issue_pending (chan); -wait_event_freezable_timeout (done_wait, done.done, +wait_event_freezable_timeout (thread->done_wait, done->done,msecs_to_jiffies (params->timeout)); status =dma_async_is_tx_complete (chan, cookie,NULL ,NULL ); -if (!done.done) { -/* -* We're leaving the timed out dma operation with -* dangling pointer to done_wait. To make this -* correct, we'll need to allocate wait_done for -* each test iteration and perform "who's gonna -* free it this time?" dancing. For now, just -* leave it dangling. -*/ -WARN (1 ,"dmatest: Kernel stack may be corrupted!!\n" ); +if (!done->done) {dmaengine_unmap_put (um);result ("test timed out" , total_tests, src_off, dst_off, len,0 );
@@ -789,7 +782,7 @@ static int dmatest_func(void *data) dmatest_KBs(runtime, total_len), ret);/* terminate all transfers on specified channels */ -if (ret) +if (ret || failed_tests)dmaengine_terminate_all (chan); thread->done = true;
@@ -849,6 +842,8 @@ static int dmatest_add_threads(struct dmatest_info *info, thread->info = info; thread->chan = dtc->chan; thread->type = type; + thread->test_done.wait = &thread->done_wait; +init_waitqueue_head (&thread->done_wait);smp_wmb (); thread->task =kthread_create (dmatest_func, thread,"%s-%s%u" ,dma_chan_name (chan), op, i);
Signature · 3 lines
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v3 1/1 dmaengine: dmatest: move callback wait queue to thread context
Quoted 10 lines · Adam Wallis
Commit
adfa543e7314("dmatest: don't useset_freezable_with_signal()") introduced a bug (that is in fact documented by the patch commit text) that leaves behind a dangling pointer. Since thedone_waitstructure is allocated on the stack, future invocations to the DMATEST can produce undesirable results (e.g., corrupted spinlocks).Commit
a9df21e34b42("dmaengine: dmatest: warn user when dma test times out") attempted to WARN the user that the stack was likely corrupted but did not fix the actual issue.
Quoted 8 lines
This patch fixes the issue by pushing the wait queue and callback structs into the the thread structure. If a failure occurs due to time,
dmaengine_terminate_allwill force the callback to safely callwake_up_all()without possibility of using a freed pointer.Cc stable at vger.kernel.org # 4.13.x: a9df21e: dmatest: Warn UserCc stable at vger.kernel.org # 4.13.xCc stable at vger.kernel.org # 4.14.x
You don't need 3 cc stables, you don't even need the "# kernel-version". Since you have the "Fixes:" line the target kernel(s) for the backport can be auto-determined. I should go update Documentation/process/stable-kernel-rules.rst to mention this.
Quoted 30 lines
Fixes adfa543e7314 ("dmatest: don't use set_freezable_with_signal()")Reviewed-by Sinan Kaya <[email protected]>Suggested-by Shunyong Yang <[email protected]>Signed-off-by Adam Wallis <[email protected]>---
changes from v2: Added "Fixes" tag
changes from v1: Added pre-req patches for stabledrivers/dma/dmatest.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)drivers/dma/dmatest.c +6
-0@@ -155,6 +155,12 @@ struct dmatest_params {#define PATTERN_COUNT_MASK 0x1f#define PATTERN_MEMSET_IDX 0x01 +/* poor man's completion - we want to use wait_event_freezable() on it */ +struct dmatest_done { +bool done; +wait_queue_head_t *wait; +}; +struct dmatest_thread {struct list_head node;struct dmatest_info *info;
drivers/dma/dmatest.c struct dmatest_thread { · line 174
Quoted 4 lines
drivers/dma/dmatest.c +1
-0@@ -165,6 +171,8 @@ struct dmatest_thread {u8 **dsts;u8 **udsts;enum dma_transaction_type type; +wait_queue_head_t done_wait;
Why are we defining a waitquehead per thread vs defining one globally for the whole module with "static DECLARE_WAIT_QUEUE_HEAD(x);"?
PATCH v3 1/1 dmaengine: dmatest: move callback wait queue to thread context
Quoted 11 lines · Dan Williams
Quoted 10 lines · Adam Wallis
Commit
adfa543e7314("dmatest: don't useset_freezable_with_signal()") introduced a bug (that is in fact documented by the patch commit text) that leaves behind a dangling pointer. Since thedone_waitstructure is allocated on the stack, future invocations to the DMATEST can produce undesirable results (e.g., corrupted spinlocks).Commit
a9df21e34b42("dmaengine: dmatest: warn user when dma test times out") attempted to WARN the user that the stack was likely corrupted but did not fix the actual issue.
Quoted 9 lines
Quoted 8 lines
This patch fixes the issue by pushing the wait queue and callback structs into the the thread structure. If a failure occurs due to time,
dmaengine_terminate_allwill force the callback to safely callwake_up_all()without possibility of using a freed pointer.Cc stable at vger.kernel.org # 4.13.x: a9df21e: dmatest: Warn UserCc stable at vger.kernel.org # 4.13.xCc stable at vger.kernel.org # 4.14.x
Sure - do you want me to remove them? I was just following the instructions on stable.
Quoted 34 lines
You don't need 3 cc stables, you don't even need the "# kernel-version". Since you have the "Fixes:" line the target kernel(s) for the backport can be auto-determined. I should go update
Documentation/process/stable-kernel-rules.rstto mention this.Quoted 29 lines
Fixes adfa543e7314 ("dmatest: don't use set_freezable_with_signal()")Reviewed-by Sinan Kaya <[email protected]>Suggested-by Shunyong Yang <[email protected]>Signed-off-by Adam Wallis <[email protected]>---
changes from v2: Added "Fixes" tag
changes from v1: Added pre-req patches for stabledrivers/dma/dmatest.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)drivers/dma/dmatest.c +6
-0@@ -155,6 +155,12 @@ struct dmatest_params {#define PATTERN_COUNT_MASK 0x1f#define PATTERN_MEMSET_IDX 0x01 +/* poor man's completion - we want to use wait_event_freezable() on it */ +struct dmatest_done { +bool done; +wait_queue_head_t *wait; +}; +struct dmatest_thread {struct list_head node;struct dmatest_info *info;
Quoted 8 lines
drivers/dma/dmatest.c struct dmatest_thread { · line 174
Quoted 5 lines
drivers/dma/dmatest.c +1
-0@@ -165,6 +171,8 @@ struct dmatest_thread {u8 **dsts;u8 **udsts;enum dma_transaction_type type; +wait_queue_head_t done_wait;Why are we defining a waitquehead per thread vs defining one globally for the whole module with "static
DECLARE_WAIT_QUEUE_HEAD(x);"?
This is how the original dmatest functions. Each thread had a wait queue that it created so that it could go to sleep while the DMA transfer occurred. Each thread is dependent on its own DMA transaction for the wakeup call. Again, this is how the test originally worked. I just moved the wait queue from the stack (which was getting corrupted) to the thread context to allow for safe cleanup. In other words, I haven't really changed how the test works...just fixing a bug with the current implementation.
Quoted 5 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 4 lines
Adam Wallis Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v3 1/1 dmaengine: dmatest: move callback wait queue to thread context
Quoted 17 lines · Adam Wallis
Quoted 16 lines · Dan Williams
Quoted 15 lines · Adam Wallis
Commit
adfa543e7314("dmatest: don't useset_freezable_with_signal()") introduced a bug (that is in fact documented by the patch commit text) that leaves behind a dangling pointer. Since thedone_waitstructure is allocated on the stack, future invocations to the DMATEST can produce undesirable results (e.g., corrupted spinlocks).Commit
a9df21e34b42("dmaengine: dmatest: warn user when dma test times out") attempted to WARN the user that the stack was likely corrupted but did not fix the actual issue.This patch fixes the issue by pushing the wait queue and callback structs into the the thread structure. If a failure occurs due to time,
dmaengine_terminate_allwill force the callback to safely callwake_up_all()without possibility of using a freed pointer.
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
Cc stable at vger.kernel.org # 4.13.x: a9df21e: dmatest: Warn UserCc stable at vger.kernel.org # 4.13.xCc stable at vger.kernel.org # 4.14.xSure - do you want me to remove them? I was just following the instructions on stable.
It's not broken, just a note for next time.
Quoted 44 lines
Quoted 35 lines
You don't need 3 cc stables, you don't even need the "# kernel-version". Since you have the "Fixes:" line the target kernel(s) for the backport can be auto-determined. I should go update
Documentation/process/stable-kernel-rules.rstto mention this.Quoted 29 lines
Fixes adfa543e7314 ("dmatest: don't use set_freezable_with_signal()")Reviewed-by Sinan Kaya <[email protected]>Suggested-by Shunyong Yang <[email protected]>Signed-off-by Adam Wallis <[email protected]>---
changes from v2: Added "Fixes" tag
changes from v1: Added pre-req patches for stabledrivers/dma/dmatest.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)drivers/dma/dmatest.c +6
-0@@ -155,6 +155,12 @@ struct dmatest_params {#define PATTERN_COUNT_MASK 0x1f#define PATTERN_MEMSET_IDX 0x01 +/* poor man's completion - we want to use wait_event_freezable() on it */ +struct dmatest_done { +bool done; +wait_queue_head_t *wait; +}; +struct dmatest_thread {struct list_head node;struct dmatest_info *info;Quoted 8 lines
drivers/dma/dmatest.c struct dmatest_thread { · line 174
Quoted 5 lines
drivers/dma/dmatest.c +1
-0@@ -165,6 +171,8 @@ struct dmatest_thread {u8 **dsts;u8 **udsts;enum dma_transaction_type type; +wait_queue_head_t done_wait;Why are we defining a waitquehead per thread vs defining one globally for the whole module with "static
DECLARE_WAIT_QUEUE_HEAD(x);"?
Quoted 7 lines
This is how the original dmatest functions. Each thread had a wait queue that it created so that it could go to sleep while the DMA transfer occurred. Each thread is dependent on its own DMA transaction for the wakeup call. Again, this is how the test originally worked. I just moved the wait queue from the stack (which was getting corrupted) to the thread context to allow for safe cleanup. In other words, I haven't really changed how the test works...just fixing a bug with the current implementation.
Ok, always takes me a bit to re-orient myself to this file since I only look at it once a year.
This fix seems incomplete. The next test iteration after a timeout will now reuse the per-thread 'done' notification. If the engine that timed out still completes its dma it will collide with the next operation that is using the same 'done' variable. So it seems to me that the wait_queue_head should be global, and the 'done' variable should be either allocated per-operation or we should call dmaengine_terminate_all() after a timeout. Since not all engines implement a terminate I think the potential memory leak of a few 'done' variables is a better option.
PATCH v3 1/1 dmaengine: dmatest: move callback wait queue to thread context
Quoted 83 lines · Dan Williams
Quoted 17 lines · Adam Wallis
Quoted 16 lines · Dan Williams
Quoted 15 lines · Adam Wallis
Commit
adfa543e7314("dmatest: don't useset_freezable_with_signal()") introduced a bug (that is in fact documented by the patch commit text) that leaves behind a dangling pointer. Since thedone_waitstructure is allocated on the stack, future invocations to the DMATEST can produce undesirable results (e.g., corrupted spinlocks).Commit
a9df21e34b42("dmaengine: dmatest: warn user when dma test times out") attempted to WARN the user that the stack was likely corrupted but did not fix the actual issue.This patch fixes the issue by pushing the wait queue and callback structs into the the thread structure. If a failure occurs due to time,
dmaengine_terminate_allwill force the callback to safely callwake_up_all()without possibility of using a freed pointer.Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
Cc stable at vger.kernel.org # 4.13.x: a9df21e: dmatest: Warn UserCc stable at vger.kernel.org # 4.13.xCc stable at vger.kernel.org # 4.14.xSure - do you want me to remove them? I was just following the instructions on stable.
It's not broken, just a note for next time.
Quoted 44 lines
Quoted 35 lines
You don't need 3 cc stables, you don't even need the "# kernel-version". Since you have the "Fixes:" line the target kernel(s) for the backport can be auto-determined. I should go update
Documentation/process/stable-kernel-rules.rstto mention this.Quoted 29 lines
Fixes adfa543e7314 ("dmatest: don't use set_freezable_with_signal()")Reviewed-by Sinan Kaya <[email protected]>Suggested-by Shunyong Yang <[email protected]>Signed-off-by Adam Wallis <[email protected]>---
changes from v2: Added "Fixes" tag
changes from v1: Added pre-req patches for stabledrivers/dma/dmatest.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)drivers/dma/dmatest.c +6
-0@@ -155,6 +155,12 @@ struct dmatest_params {#define PATTERN_COUNT_MASK 0x1f#define PATTERN_MEMSET_IDX 0x01 +/* poor man's completion - we want to use wait_event_freezable() on it */ +struct dmatest_done { +bool done; +wait_queue_head_t *wait; +}; +struct dmatest_thread {struct list_head node;struct dmatest_info *info;Quoted 8 lines
drivers/dma/dmatest.c struct dmatest_thread { · line 174
Quoted 5 lines
drivers/dma/dmatest.c +1
-0@@ -165,6 +171,8 @@ struct dmatest_thread {u8 **dsts;u8 **udsts;enum dma_transaction_type type; +wait_queue_head_t done_wait;Why are we defining a waitquehead per thread vs defining one globally for the whole module with "static
DECLARE_WAIT_QUEUE_HEAD(x);"?Quoted 7 lines
This is how the original dmatest functions. Each thread had a wait queue that it created so that it could go to sleep while the DMA transfer occurred. Each thread is dependent on its own DMA transaction for the wakeup call. Again, this is how the test originally worked. I just moved the wait queue from the stack (which was getting corrupted) to the thread context to allow for safe cleanup. In other words, I haven't really changed how the test works...just fixing a bug with the current implementation.
Ok, always takes me a bit to re-orient myself to this file since I only look at it once a year.
Quoted 10 lines
This fix seems incomplete. The next test iteration after a timeout will now reuse the per-thread 'done' notification. If the engine that timed out still completes its dma it will collide with the next operation that is using the same 'done' variable. So it seems to me that the
wait_queue_headshould be global, and the 'done' variable should be either allocated per-operation or we should calldmaengine_terminate_all()after a timeout. Since not all engines implement a terminate I think the potential memory leak of a few 'done' variables is a better option.
Dan
An important part of my patch was severed in this v3 submission. My apologies.
There is a change that addresses, I believe, your concern that was in v2
/* terminate all transfers on specified channels */
- if (ret)
+ if (ret || failed_tests)
dmaengine_terminate_all(chan);
Will clean up again, retest, and resubmit. Thanks for your patience and instruction.
Adam
Signature · 4 lines
Adam Wallis Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v3 1/1 dmaengine: dmatest: move callback wait queue to thread context
Quoted 24 lines · Adam Wallis
Quoted 23 lines · Dan Williams
Quoted 22 lines · Adam Wallis
Quoted 20 lines · Dan Williams
Quoted 18 lines · Adam Wallis
Commit
adfa543e7314("dmatest: don't useset_freezable_with_signal()") introduced a bug (that is in fact documented by the patch commit text) that leaves behind a dangling pointer. Since thedone_waitstructure is allocated on the stack, future invocations to the DMATEST can produce undesirable results (e.g., corrupted spinlocks).Commit
a9df21e34b42("dmaengine: dmatest: warn user when dma test times out") attempted to WARN the user that the stack was likely corrupted but did not fix the actual issue.This patch fixes the issue by pushing the wait queue and callback structs into the the thread structure. If a failure occurs due to time,
dmaengine_terminate_allwill force the callback to safely callwake_up_all()without possibility of using a freed pointer.Cc stable at vger.kernel.org # 4.13.x: a9df21e: dmatest: Warn UserCc stable at vger.kernel.org # 4.13.xCc stable at vger.kernel.org # 4.14.x
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Sure - do you want me to remove them? I was just following the instructions on stable.
It's not broken, just a note for next time.
One last question - if all we need is Fixes and CC stable...how do we indicate a dependency (for a cherry pick)?
Thanks
Signature · 4 lines
Adam Wallis Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v3 1/1 dmaengine: dmatest: move callback wait queue to thread context
Quoted 25 lines · Adam Wallis
Quoted 24 lines · Adam Wallis
Quoted 23 lines · Dan Williams
Quoted 22 lines · Adam Wallis
Quoted 20 lines · Dan Williams
Quoted 18 lines · Adam Wallis
Commit
adfa543e7314("dmatest: don't useset_freezable_with_signal()") introduced a bug (that is in fact documented by the patch commit text) that leaves behind a dangling pointer. Since thedone_waitstructure is allocated on the stack, future invocations to the DMATEST can produce undesirable results (e.g., corrupted spinlocks).Commit
a9df21e34b42("dmaengine: dmatest: warn user when dma test times out") attempted to WARN the user that the stack was likely corrupted but did not fix the actual issue.This patch fixes the issue by pushing the wait queue and callback structs into the the thread structure. If a failure occurs due to time,
dmaengine_terminate_allwill force the callback to safely callwake_up_all()without possibility of using a freed pointer.Cc stable at vger.kernel.org # 4.13.x: a9df21e: dmatest: Warn UserCc stable at vger.kernel.org # 4.13.xCc stable at vger.kernel.org # 4.14.x
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Sure - do you want me to remove them? I was just following the instructions on stable.
It's not broken, just a note for next time.
One last question - if all we need is Fixes and CC stable...how do we indicate a dependency (for a cherry pick)?
For cherry picks the document does say to include multiple "Cc:
stable" lines. So ignore what I said you're following the proper process.
The way I've handled this is to just wait for the -stable team to notify that the patch does not apply and send a custom backport at that point.
PATCH v3 1/1 dmaengine: dmatest: move callback wait queue to thread context
In reply to Adam WallisQuoted 104 lines · Adam Wallis
Quoted 83 lines · Dan Williams
Quoted 17 lines · Adam Wallis
Quoted 16 lines · Dan Williams
Quoted 15 lines · Adam Wallis
Commit
adfa543e7314("dmatest: don't useset_freezable_with_signal()") introduced a bug (that is in fact documented by the patch commit text) that leaves behind a dangling pointer. Since thedone_waitstructure is allocated on the stack, future invocations to the DMATEST can produce undesirable results (e.g., corrupted spinlocks).Commit
a9df21e34b42("dmaengine: dmatest: warn user when dma test times out") attempted to WARN the user that the stack was likely corrupted but did not fix the actual issue.This patch fixes the issue by pushing the wait queue and callback structs into the the thread structure. If a failure occurs due to time,
dmaengine_terminate_allwill force the callback to safely callwake_up_all()without possibility of using a freed pointer.Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
Cc stable at vger.kernel.org # 4.13.x: a9df21e: dmatest: Warn UserCc stable at vger.kernel.org # 4.13.xCc stable at vger.kernel.org # 4.14.xSure - do you want me to remove them? I was just following the instructions on stable.
It's not broken, just a note for next time.
Quoted 44 lines
Quoted 35 lines
You don't need 3 cc stables, you don't even need the "# kernel-version". Since you have the "Fixes:" line the target kernel(s) for the backport can be auto-determined. I should go update
Documentation/process/stable-kernel-rules.rstto mention this.Quoted 29 lines
Fixes adfa543e7314 ("dmatest: don't use set_freezable_with_signal()")Reviewed-by Sinan Kaya <[email protected]>Suggested-by Shunyong Yang <[email protected]>Signed-off-by Adam Wallis <[email protected]>---
changes from v2: Added "Fixes" tag
changes from v1: Added pre-req patches for stabledrivers/dma/dmatest.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)drivers/dma/dmatest.c +6
-0@@ -155,6 +155,12 @@ struct dmatest_params {#define PATTERN_COUNT_MASK 0x1f#define PATTERN_MEMSET_IDX 0x01 +/* poor man's completion - we want to use wait_event_freezable() on it */ +struct dmatest_done { +bool done; +wait_queue_head_t *wait; +}; +struct dmatest_thread {struct list_head node;struct dmatest_info *info;Quoted 8 lines
drivers/dma/dmatest.c struct dmatest_thread { · line 174
Quoted 5 lines
drivers/dma/dmatest.c +1
-0@@ -165,6 +171,8 @@ struct dmatest_thread {u8 **dsts;u8 **udsts;enum dma_transaction_type type; +wait_queue_head_t done_wait;Why are we defining a waitquehead per thread vs defining one globally for the whole module with "static
DECLARE_WAIT_QUEUE_HEAD(x);"?Quoted 7 lines
This is how the original dmatest functions. Each thread had a wait queue that it created so that it could go to sleep while the DMA transfer occurred. Each thread is dependent on its own DMA transaction for the wakeup call. Again, this is how the test originally worked. I just moved the wait queue from the stack (which was getting corrupted) to the thread context to allow for safe cleanup. In other words, I haven't really changed how the test works...just fixing a bug with the current implementation.
Ok, always takes me a bit to re-orient myself to this file since I only look at it once a year.
Quoted 10 lines
This fix seems incomplete. The next test iteration after a timeout will now reuse the per-thread 'done' notification. If the engine that timed out still completes its dma it will collide with the next operation that is using the same 'done' variable. So it seems to me that the
wait_queue_headshould be global, and the 'done' variable should be either allocated per-operation or we should calldmaengine_terminate_all()after a timeout. Since not all engines implement a terminate I think the potential memory leak of a few 'done' variables is a better option.Dan
An important part of my patch was severed in this v3 submission. My apologies.There is a change that addresses, I believe, your concern that was in v2
drivers/dma/dmatest.c +1
-1/* terminate all transfers on specified channels */ -if (ret) +if (ret || failed_tests)dmaengine_terminate_all (chan);
Quoted 1 line
Will clean up again, retest, and resubmit. Thanks for your patience and instruction.
Dan, I thought the patch was truncated, but it's all there in V3. I should have finished my coffee before responding. You are absolutely right that in the timed out case that dmaengine_terminate_all(chan) should be called, and that change is in fact already included in this patch set
@@ -789,7 +782,7 @@ static int dmatest_func(void *data)
dmatest_KBs(runtime, total_len), ret);
/* terminate all transfers on specified channels */
- if (ret)
+ if (ret || failed_tests)
dmaengine_terminate_all(chan);
Would you prefer that I add a better description in the commit text to address the fact this was in fact added?
Quoted 3 lines
Adam
Signature · 4 lines
Adam Wallis Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v3 1/1 dmaengine: dmatest: move callback wait queue to thread context
Quoted 1 line · Adam Wallis
On 11/17/2017 12:01 PM, Adam Wallis wrote:
[..]
Quoted 25 lines
Quoted 11 lines
Dan
An important part of my patch was severed in this v3 submission. My apologies.There is a change that addresses, I believe, your concern that was in v2
/* terminate all transfers on specified channels */drivers/dma/dmatest.c +1
-1-if (ret) +if (ret || failed_tests)dmaengine_terminate_all (chan);Will clean up again, retest, and resubmit. Thanks for your patience and instruction.
Dan, I thought the patch was truncated, but it's all there in V3. I should have finished my coffee before responding. You are absolutely right that in the timed out case that
dmaengine_terminate_all(chan)should be called, and that change is in fact already included in this patch setdrivers/dma/dmatest.c +1
-1@@ -789,7 +782,7 @@ static int dmatest_func(void *data) dmatest_KBs(runtime, total_len), ret);/* terminate all transfers on specified channels */ -if (ret) +if (ret || failed_tests)dmaengine_terminate_all (chan);
Quoted 2 lines
Would you prefer that I add a better description in the commit text to address the fact this was in fact added?
Ah, sorry I overlooked that.
What about the case where the dmaengine does not support the terminate_all operation? I think we should WARN in that case, but that's also where the per-operation allocation of a done variable can prevent done notifications leaking between operations.
PATCH v3 1/1 dmaengine: dmatest: move callback wait queue to thread context
Quoted 33 lines · Dan Williams
Quoted 1 line · Adam Wallis
On 11/17/2017 12:01 PM, Adam Wallis wrote:
[..]
Quoted 25 lines
Quoted 11 lines
Dan
An important part of my patch was severed in this v3 submission. My apologies.There is a change that addresses, I believe, your concern that was in v2
/* terminate all transfers on specified channels */(quoted diff) +1
-1- if (ret) + if (ret || failed_tests) dmaengine_terminate_all(chan);Will clean up again, retest, and resubmit. Thanks for your patience and instruction.
Dan, I thought the patch was truncated, but it's all there in V3. I should have finished my coffee before responding. You are absolutely right that in the timed out case that
dmaengine_terminate_all(chan)should be called, and that change is in fact already included in this patch set(quoted diff) +1
-1@@ -789,7 +782,7 @@ static int dmatest_func(void *data) dmatest_KBs(runtime, total_len), ret); /* terminate all transfers on specified channels */ - if (ret) + if (ret || failed_tests) dmaengine_terminate_all(chan);Quoted 2 lines
Would you prefer that I add a better description in the commit text to address the fact this was in fact added?
Ah, sorry I overlooked that.
Quoted 5 lines
What about the case where the dmaengine does not support the
terminate_alloperation? I think we should WARN in that case, but that's also where the per-operation allocation of a done variable can prevent done notifications leaking between operations.
Dan, thanks for your comments. I just released v4 of this patch. Let me know if I misunderstood what you said above. I do acknowledge that container_of might be less desirable than just changing the argument that we pass to the callback. Open to your suggestions!
Quoted 5 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 4 lines
Adam Wallis Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.