[Sound-open-firmware] [PATCH v2] intel-ipc: fix host ring buffer size not page aligned issue

[Jie, Yang]

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart at linux.intel.com]
>Sent: Wednesday, November 15, 2017 10:55 PM
>To: Keyon Jie <yang.jie at linux.intel.com>; sound-open-firmware at alsa-
>project.org; Girdwood, Liam R <liam.r.girdwood at intel.com>
>Cc: Jie, Yang <yang.jie at 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 at 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
>>