[Sound-open-firmware] [PATCH v2] intel-ipc: fix host ring buffer size not page aligned issue
The host ring buffer size may be not page aligned, but the last page was utilized by host component wrongly, which may introduce beating noise.
Here change to correct size for the last element, which will fix the issue.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/ipc/intel-ipc.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/ipc/intel-ipc.c b/src/ipc/intel-ipc.c index 56bd8ba..7de533a 100644 --- a/src/ipc/intel-ipc.c +++ b/src/ipc/intel-ipc.c @@ -175,6 +175,12 @@ static int parse_page_descriptors(struct intel_ipc_data *iipc, host = (struct sof_ipc_comp_host *)&cd->comp; }
+ if (HOST_PAGE_SIZE * ring->pages < ring->size) { + /* host side passed in error ring buffer size */ + trace_ipc_error("eBs"); + return -EINVAL; + } + for (i = 0; i < ring->pages; i++) {
idx = (((i << 2) + i)) >> 1; @@ -192,6 +198,10 @@ static int parse_page_descriptors(struct intel_ipc_data *iipc, else elem.dest = phy_addr;
+ /* the last page may be not full used */ + if (i == (ring->pages - 1)) + elem.size = ring->size - HOST_PAGE_SIZE * i; + if (is_trace) err = dma_trace_host_buffer(d, &elem, ring->size); else
On 11/15/17 3:41 AM, Keyon Jie wrote:
The host ring buffer size may be not page aligned, but the last page was utilized by host component wrongly, which may introduce beating noise.
Here change to correct size for the last element, which will fix the issue.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com
src/ipc/intel-ipc.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/ipc/intel-ipc.c b/src/ipc/intel-ipc.c index 56bd8ba..7de533a 100644 --- a/src/ipc/intel-ipc.c +++ b/src/ipc/intel-ipc.c @@ -175,6 +175,12 @@ static int parse_page_descriptors(struct intel_ipc_data *iipc, host = (struct sof_ipc_comp_host *)&cd->comp; }
if (HOST_PAGE_SIZE * ring->pages < ring->size) {
/* host side passed in error ring buffer size */trace_ipc_error("eBs");return -EINVAL;}
for (i = 0; i < ring->pages; i++) {
idx = (((i << 2) + i)) >> 1;
@@ -192,6 +198,10 @@ static int parse_page_descriptors(struct intel_ipc_data *iipc, else elem.dest = phy_addr;
/* the last page may be not full used */if (i == (ring->pages - 1))elem.size = ring->size - HOST_PAGE_SIZE * i;
I was thrown off by the two changes, maybe document that the first case is indeed a bad configuration where the ring buffer uses too few pages and conversely in the second case that the ring buffer relies on an additional page which may not be completely used (not an error case).
In the first test, maybe you should also check there is only one page which isn't partially used, or the second case will fail. something like
if (HOST_PAGE_SIZE * ring->pages - ring->size >= HOST_PAGE_SIZE) error("too many pages");
- if (is_trace) err = dma_trace_host_buffer(d, &elem, ring->size); else
On Wed, 2017-11-15 at 08:55 -0600, Pierre-Louis Bossart wrote:
On 11/15/17 3:41 AM, Keyon Jie wrote:
The host ring buffer size may be not page aligned, but the last page was utilized by host component wrongly, which may introduce beating noise.
Here change to correct size for the last element, which will fix the issue.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com
src/ipc/intel-ipc.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/ipc/intel-ipc.c b/src/ipc/intel-ipc.c index 56bd8ba..7de533a 100644 --- a/src/ipc/intel-ipc.c +++ b/src/ipc/intel-ipc.c @@ -175,6 +175,12 @@ static int parse_page_descriptors(struct intel_ipc_data *iipc, host = (struct sof_ipc_comp_host *)&cd->comp; }
It's best to comment here what you are testing prior to the if() i.e.
/* is ring size a multiple of HOST_PAGE_SIZE ? */
- if (HOST_PAGE_SIZE * ring->pages < ring->size) {
Btw, what happens if PAGE * pages > ring->size ? We should reject this too.
Liam
On 11/15/2017 03:05 PM, Liam Girdwood wrote:
On Wed, 2017-11-15 at 08:55 -0600, Pierre-Louis Bossart wrote:
On 11/15/17 3:41 AM, Keyon Jie wrote:
The host ring buffer size may be not page aligned, but the last page was utilized by host component wrongly, which may introduce beating noise.
Here change to correct size for the last element, which will fix the issue.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com
src/ipc/intel-ipc.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/ipc/intel-ipc.c b/src/ipc/intel-ipc.c index 56bd8ba..7de533a 100644 --- a/src/ipc/intel-ipc.c +++ b/src/ipc/intel-ipc.c @@ -175,6 +175,12 @@ static int parse_page_descriptors(struct intel_ipc_data *iipc, host = (struct sof_ipc_comp_host *)&cd->comp; }
It's best to comment here what you are testing prior to the if() i.e.
/* is ring size a multiple of HOST_PAGE_SIZE ? */
- if (HOST_PAGE_SIZE * ring->pages < ring->size) {
Btw, what happens if PAGE * pages > ring->size ? We should reject this too.
it's ok if the last page is not fully used, if if there is an additional page used beyond what's necessary then there is a problem.
Liam
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
-----Original Message----- From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com] Sent: Wednesday, November 15, 2017 10:55 PM To: Keyon Jie yang.jie@linux.intel.com; sound-open-firmware@alsa- project.org; Girdwood, Liam R liam.r.girdwood@intel.com Cc: Jie, Yang yang.jie@intel.com Subject: Re: [Sound-open-firmware] [PATCH v2] intel-ipc: fix host ring buffer size not page aligned issue
On 11/15/17 3:41 AM, Keyon Jie wrote:
The host ring buffer size may be not page aligned, but the last page was utilized by host component wrongly, which may introduce beating noise.
Here change to correct size for the last element, which will fix the issue.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com
src/ipc/intel-ipc.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/ipc/intel-ipc.c b/src/ipc/intel-ipc.c index 56bd8ba..7de533a 100644 --- a/src/ipc/intel-ipc.c +++ b/src/ipc/intel-ipc.c @@ -175,6 +175,12 @@ static int parse_page_descriptors(struct intel_ipc_data
*iipc,
host = (struct sof_ipc_comp_host *)&cd->comp;}
if (HOST_PAGE_SIZE * ring->pages < ring->size) {
/* host side passed in error ring buffer size */trace_ipc_error("eBs");return -EINVAL;}
for (i = 0; i < ring->pages; i++) {
idx = (((i << 2) + i)) >> 1;
@@ -192,6 +198,10 @@ static int parse_page_descriptors(struct intel_ipc_data
*iipc,
else elem.dest = phy_addr;
/* the last page may be not full used */if (i == (ring->pages - 1))elem.size = ring->size - HOST_PAGE_SIZE * i;I was thrown off by the two changes, maybe document that the first case is indeed a bad configuration where the ring buffer uses too few pages and conversely in the second case that the ring buffer relies on an additional page which may not be completely used (not an error case).
In the first test, maybe you should also check there is only one page which isn't partially used, or the second case will fail. something like
if (HOST_PAGE_SIZE * ring->pages - ring->size >= HOST_PAGE_SIZE) error("too many pages");
Good point, let me fix it and make it more clear.
Thanks, ~Keyon
- if (is_trace) err = dma_trace_host_buffer(d, &elem, ring->size); else
participants (4)
-
Jie, Yang -
Keyon Jie -
Liam Girdwood -
Pierre-Louis Bossart