Revert "dmaengine: dmatest: Fix dmatest waiting less when interrupted"

[PATCH] Revert "dmaengine: dmatest: Fix dmatest waiting less when interrupted"

Posted by Nathan Lynch via B4 Relay 10 months, 1 week ago

From: Nathan Lynch <nathan.lynch@amd.com>

Several issues with this change:

* The analysis is flawed and it's unclear what problem is being
  fixed. There is no difference between wait_event_freezable_timeout()
  and wait_event_timeout() with respect to device interrupts. And of
  course "the interrupt notifying the finish of an operation happens
  during wait_event_freezable_timeout()" -- that's how it's supposed
  to work.

* The link at the "Closes:" tag appears to be an unrelated
  use-after-free in idxd.

* It introduces a regression: dmatest threads are meant to be
  freezable and this change breaks that.

See discussion here:
https://lore.kernel.org/dmaengine/878qpa13fe.fsf@AUSNATLYNCH.amd.com/

Fixes: e87ca16e9911 ("dmaengine: dmatest: Fix dmatest waiting less when interrupted")
Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
---
 drivers/dma/dmatest.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index d891dfca358e209c9a5c2d8b49e3e539130e53e8..91b2fbc0b8647127c7753669fa21c84d300764b9 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -841,9 +841,9 @@ static int dmatest_func(void *data)
 		} else {
 			dma_async_issue_pending(chan);
 
-			wait_event_timeout(thread->done_wait,
-					   done->done,
-					   msecs_to_jiffies(params->timeout));
+			wait_event_freezable_timeout(thread->done_wait,
+					done->done,
+					msecs_to_jiffies(params->timeout));
 
 			status = dma_async_is_tx_complete(chan, cookie, NULL,
 							  NULL);

---
base-commit: 6565439894570a07b00dba0b739729fe6b56fba4
change-id: 20250403-dmaengine-dmatest-revert-waiting-less-a06639d0d1c3

Best regards,
-- 
Nathan Lynch <nathan.lynch@amd.com>

Re: [PATCH] Revert "dmaengine: dmatest: Fix dmatest waiting less when interrupted"

Posted by Vinod Koul 9 months, 3 weeks ago

On Thu, 03 Apr 2025 11:24:19 -0500, Nathan Lynch wrote:
> Several issues with this change:
> 
> * The analysis is flawed and it's unclear what problem is being
>   fixed. There is no difference between wait_event_freezable_timeout()
>   and wait_event_timeout() with respect to device interrupts. And of
>   course "the interrupt notifying the finish of an operation happens
>   during wait_event_freezable_timeout()" -- that's how it's supposed
>   to work.
> 
> [...]

Applied, thanks!

[1/1] Revert "dmaengine: dmatest: Fix dmatest waiting less when interrupted"
      commit: df180e65305f8c1e020d54bfc2132349fd693de1

Best regards,
-- 
~Vinod

Re: [PATCH] Revert "dmaengine: dmatest: Fix dmatest waiting less when interrupted"

Posted by Nathan Lynch 9 months, 4 weeks ago

Nathan Lynch <nathan.lynch@amd.com> writes:
> Several issues with this change:
>
> * The analysis is flawed and it's unclear what problem is being
>   fixed. There is no difference between wait_event_freezable_timeout()
>   and wait_event_timeout() with respect to device interrupts. And of
>   course "the interrupt notifying the finish of an operation happens
>   during wait_event_freezable_timeout()" -- that's how it's supposed
>   to work.
>
> * The link at the "Closes:" tag appears to be an unrelated
>   use-after-free in idxd.
>
> * It introduces a regression: dmatest threads are meant to be
>   freezable and this change breaks that.
>
> See discussion here:
> https://lore.kernel.org/dmaengine/878qpa13fe.fsf@AUSNATLYNCH.amd.com/
>
> Fixes: e87ca16e9911 ("dmaengine: dmatest: Fix dmatest waiting less when interrupted")
> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>

I'm puzzled by the silence here. This is a clear regression fix and
Vinicius agreed that the change should be reverted in the original
thread.