[Sound-open-firmware] [PATCH 4/4] Fix DMA host offset calculation.
![](https://secure.gravatar.com/avatar/c72d9b14c51ed08a3d038871682ea7a0.jpg?s=120&d=mm&r=g)
From: Yan Wang yan.wang@linux.intel.com
1. "size" should not be considered twice. 2. Change host_offset to uint32_t type.
Signed-off-by: Yan Wang yan.wang@linux.intel.com --- src/include/reef/dma-trace.h | 2 +- src/lib/dma-trace.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/include/reef/dma-trace.h b/src/include/reef/dma-trace.h index 12d0956..641a4dc 100644 --- a/src/include/reef/dma-trace.h +++ b/src/include/reef/dma-trace.h @@ -56,7 +56,7 @@ struct dma_trace_data { struct dma_sg_config config; struct dma_trace_buf dmatb; struct dma_copy dc; - int32_t host_offset; + uint32_t host_offset; uint32_t host_size; struct work dmat_work; uint32_t enabled; diff --git a/src/lib/dma-trace.c b/src/lib/dma-trace.c index 5885037..249e18b 100644 --- a/src/lib/dma-trace.c +++ b/src/lib/dma-trace.c @@ -90,7 +90,7 @@ static uint64_t trace_work(void *data, uint64_t delay)
/* update host pointer and check for wrap */ d->host_offset += size; - if (d->host_offset + size >= d->host_size) + if (d->host_offset >= d->host_size) d->host_offset = 0;
/* update local pointer and check for wrap */
![](https://secure.gravatar.com/avatar/59bffa8d29505a5951521a2fd09a4fd9.jpg?s=120&d=mm&r=g)
On 11/09/2017 12:17 AM, yan.wang@linux.intel.com wrote:
From: Yan Wang yan.wang@linux.intel.com
- "size" should not be considered twice.
- Change host_offset to uint32_t type.
Signed-off-by: Yan Wang yan.wang@linux.intel.com
src/include/reef/dma-trace.h | 2 +- src/lib/dma-trace.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/include/reef/dma-trace.h b/src/include/reef/dma-trace.h index 12d0956..641a4dc 100644 --- a/src/include/reef/dma-trace.h +++ b/src/include/reef/dma-trace.h @@ -56,7 +56,7 @@ struct dma_trace_data { struct dma_sg_config config; struct dma_trace_buf dmatb; struct dma_copy dc;
- int32_t host_offset;
- uint32_t host_offset; uint32_t host_size; struct work dmat_work; uint32_t enabled;
diff --git a/src/lib/dma-trace.c b/src/lib/dma-trace.c index 5885037..249e18b 100644 --- a/src/lib/dma-trace.c +++ b/src/lib/dma-trace.c @@ -90,7 +90,7 @@ static uint64_t trace_work(void *data, uint64_t delay)
/* update host pointer and check for wrap */ d->host_offset += size;
- if (d->host_offset + size >= d->host_size)
- if (d->host_offset >= d->host_size) d->host_offset = 0;
this code pattern is present multiple times in the DMA handling code. I wonder if this is correct when the > condition is true? That'd mean some overrun happened or that some bytes have been lost. Shouldn't this be an exact == condition in there is a true wrap?
/* update local pointer and check for wrap */
![](https://secure.gravatar.com/avatar/572dad3fc66054794c0609562c04b630.jpg?s=120&d=mm&r=g)
On 2017年11月10日 05:21, Pierre-Louis Bossart wrote:
On 11/09/2017 12:17 AM, yan.wang@linux.intel.com wrote:
From: Yan Wang yan.wang@linux.intel.com
- "size" should not be considered twice.
- Change host_offset to uint32_t type.
Signed-off-by: Yan Wang yan.wang@linux.intel.com
src/include/reef/dma-trace.h | 2 +- src/lib/dma-trace.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/include/reef/dma-trace.h b/src/include/reef/dma-trace.h index 12d0956..641a4dc 100644 --- a/src/include/reef/dma-trace.h +++ b/src/include/reef/dma-trace.h @@ -56,7 +56,7 @@ struct dma_trace_data { struct dma_sg_config config; struct dma_trace_buf dmatb; struct dma_copy dc;
- int32_t host_offset;
- uint32_t host_offset; uint32_t host_size; struct work dmat_work; uint32_t enabled;
diff --git a/src/lib/dma-trace.c b/src/lib/dma-trace.c index 5885037..249e18b 100644 --- a/src/lib/dma-trace.c +++ b/src/lib/dma-trace.c @@ -90,7 +90,7 @@ static uint64_t trace_work(void *data, uint64_t delay) /* update host pointer and check for wrap */ d->host_offset += size;
- if (d->host_offset + size >= d->host_size)
- if (d->host_offset >= d->host_size) d->host_offset = 0;
this code pattern is present multiple times in the DMA handling code. I wonder if this is correct when the > condition is true? That'd mean some overrun happened or that some bytes have been lost. Shouldn't this be an exact == condition in there is a true wrap?
actually we should make it to something like this for sake of safe: d->host_offset -= d->host_size;
but considering that we have require the host_size to be multiple of period size, the '>' case won't happen, so here it is simplified to be d->host_offset = 0;
I agree that we should make it more robust.
Thanks, ~Keyon
/* update local pointer and check for wrap */
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
![](https://secure.gravatar.com/avatar/59bffa8d29505a5951521a2fd09a4fd9.jpg?s=120&d=mm&r=g)
@@ -90,7 +90,7 @@ static uint64_t trace_work(void *data, uint64_t delay) /* update host pointer and check for wrap */ d->host_offset += size; - if (d->host_offset + size >= d->host_size) + if (d->host_offset >= d->host_size) d->host_offset = 0;
this code pattern is present multiple times in the DMA handling code. I wonder if this is correct when the > condition is true? That'd mean some overrun happened or that some bytes have been lost. Shouldn't this be an exact == condition in there is a true wrap?
actually we should make it to something like this for sake of safe: d->host_offset -= d->host_size;
but considering that we have require the host_size to be multiple of period size, the '>' case won't happen, so here it is simplified to be d->host_offset = 0;
I agree that we should make it more robust.
Yes, my point exactly. Either the > condition happens and we have to handle it, or it doesn't by construction due to other restrictions and the test can be simplified.
![](https://secure.gravatar.com/avatar/c72d9b14c51ed08a3d038871682ea7a0.jpg?s=120&d=mm&r=g)
Hi, Peierre and Keyon, Thanks for your comments. Because size = min(hsize, lsize), the updated host_offset should not > host_size. So "==" is enough. I will submit it again after modification. Thanks again.
Yan Wang On Thu, 2017-11-09 at 20:31 -0600, Pierre-Louis Bossart wrote:
@@ -90,7 +90,7 @@ static uint64_t trace_work(void *data, uint64_t delay) /* update host pointer and check for wrap */ d->host_offset += size; - if (d->host_offset + size >= d->host_size) + if (d->host_offset >= d->host_size) d->host_offset = 0;
this code pattern is present multiple times in the DMA handling code. I wonder if this is correct when the > condition is true? That'd mean some overrun happened or that some bytes have been lost. Shouldn't this be an exact == condition in there is a true wrap?
actually we should make it to something like this for sake of safe: d->host_offset -= d->host_size;
but considering that we have require the host_size to be multiple of period size, the '>' case won't happen, so here it is simplified to be d->host_offset = 0;
I agree that we should make it more robust.
Yes, my point exactly. Either the > condition happens and we have to handle it, or it doesn't by construction due to other restrictions and the test can be simplified.
participants (4)
-
Keyon Jie
-
Pierre-Louis Bossart
-
yan.wang@linux.intel.com
-
yanwang