| Message ID | 20200424161147.16895-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v1,1/6] dmaengine: dmatest: Fix iteration non-stop logic | expand |
Andy, On 24/04/2020 at 18:11, Andy Shevchenko wrote: > Under some circumstances, i.e. when test is still running and about to > time out and user runs, for example, > > grep -H . /sys/module/dmatest/parameters/* > > the iterations parameter is not respected and test is going on and on until > user gives > > echo 0 > /sys/module/dmatest/parameters/run > > This is not what expected. > > The history of this bug is interesting. I though that the commit > 2d88ce76eb98 ("dmatest: add a 'wait' parameter") > is a culprit, but looking closer to the code I think it simple revealed the > broken logic from the day one, i.e. in the commit > 0a2ff57d6fba ("dmaengine: dmatest: add a maximum number of test iterations") > which adds iterations parameter. > > So, to the point, the conditional of checking the thread to be stopped being > first part of conjunction logic prevents to check iterations. Thus, we have to > always check both conditions to be able to stop after given iterations. > > Since it wasn't visible before second commit appeared, I add a respective > Fixes tag. > > Fixes: 2d88ce76eb98 ("dmatest: add a 'wait' parameter") > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Nicolas Ferre <nicolas.ferre@microchip.com> Yes, makes sense indeed: Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/dma/dmatest.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c > index a2cadfa2e6d78..4993e3e5c5b01 100644 > --- a/drivers/dma/dmatest.c > +++ b/drivers/dma/dmatest.c > @@ -662,8 +662,8 @@ static int dmatest_func(void *data) > flags = DMA_CTRL_ACK | DMA_PREP_INTERRUPT; > > ktime = ktime_get(); > - while (!kthread_should_stop() > - && !(params->iterations && total_tests >= params->iterations)) { > + while (!(kthread_should_stop() || > + (params->iterations && total_tests >= params->iterations))) { > struct dma_async_tx_descriptor *tx = NULL; > struct dmaengine_unmap_data *um; > dma_addr_t *dsts; > -- > 2.26.2 >
On 24-04-20, 19:11, Andy Shevchenko wrote: > Under some circumstances, i.e. when test is still running and about to > time out and user runs, for example, > > grep -H . /sys/module/dmatest/parameters/* > > the iterations parameter is not respected and test is going on and on until > user gives > > echo 0 > /sys/module/dmatest/parameters/run > > This is not what expected. > > The history of this bug is interesting. I though that the commit > 2d88ce76eb98 ("dmatest: add a 'wait' parameter") > is a culprit, but looking closer to the code I think it simple revealed the > broken logic from the day one, i.e. in the commit > 0a2ff57d6fba ("dmaengine: dmatest: add a maximum number of test iterations") > which adds iterations parameter. > > So, to the point, the conditional of checking the thread to be stopped being > first part of conjunction logic prevents to check iterations. Thus, we have to > always check both conditions to be able to stop after given iterations. > > Since it wasn't visible before second commit appeared, I add a respective > Fixes tag. Applied, thanks
diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c index a2cadfa2e6d78..4993e3e5c5b01 100644 --- a/drivers/dma/dmatest.c +++ b/drivers/dma/dmatest.c @@ -662,8 +662,8 @@ static int dmatest_func(void *data) flags = DMA_CTRL_ACK | DMA_PREP_INTERRUPT; ktime = ktime_get(); - while (!kthread_should_stop() - && !(params->iterations && total_tests >= params->iterations)) { + while (!(kthread_should_stop() || + (params->iterations && total_tests >= params->iterations))) { struct dma_async_tx_descriptor *tx = NULL; struct dmaengine_unmap_data *um; dma_addr_t *dsts;
Under some circumstances, i.e. when test is still running and about to time out and user runs, for example, grep -H . /sys/module/dmatest/parameters/* the iterations parameter is not respected and test is going on and on until user gives echo 0 > /sys/module/dmatest/parameters/run This is not what expected. The history of this bug is interesting. I though that the commit 2d88ce76eb98 ("dmatest: add a 'wait' parameter") is a culprit, but looking closer to the code I think it simple revealed the broken logic from the day one, i.e. in the commit 0a2ff57d6fba ("dmaengine: dmatest: add a maximum number of test iterations") which adds iterations parameter. So, to the point, the conditional of checking the thread to be stopped being first part of conjunction logic prevents to check iterations. Thus, we have to always check both conditions to be able to stop after given iterations. Since it wasn't visible before second commit appeared, I add a respective Fixes tag. Fixes: 2d88ce76eb98 ("dmatest: add a 'wait' parameter") Cc: Dan Williams <dan.j.williams@intel.com> Cc: Nicolas Ferre <nicolas.ferre@microchip.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/dma/dmatest.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)