dmaengine Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] dmaengine: qcom-bam: fix circular buffer handling
@ 2019-06-14 14:20 Srinivas Kandagatla
  2019-06-14 19:43 ` Andy Gross
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Srinivas Kandagatla @ 2019-06-14 14:20 UTC (permalink / raw)
  To: vkoul
  Cc: dmaengine, linux-kernel, linux-arm-msm, sricharan, Srinivas Kandagatla

For some reason arguments to most of the circular buffers
macros are used in reverse, tail is used for head and vice versa.

This leads to bam thinking that there is an extra descriptor at the
end and leading to retransmitting descriptor which was not scheduled
by any driver. This happens after MAX_DESCRIPTORS (4096) are scheduled
and done, so most of the drivers would not notice this, unless they are
heavily using bam dma. Originally found this issue while testing
SoundWire over SlimBus on DB845c which uses DMA very heavily for
read/writes.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/dma/qcom/bam_dma.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index cb860cb53c27..43d7b0a9713a 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -350,8 +350,8 @@ static const struct reg_offset_data bam_v1_7_reg_info[] = {
 #define BAM_DESC_FIFO_SIZE	SZ_32K
 #define MAX_DESCRIPTORS (BAM_DESC_FIFO_SIZE / sizeof(struct bam_desc_hw) - 1)
 #define BAM_FIFO_SIZE	(SZ_32K - 8)
-#define IS_BUSY(chan)	(CIRC_SPACE(bchan->tail, bchan->head,\
-			 MAX_DESCRIPTORS + 1) == 0)
+#define IS_BUSY(chan)	(CIRC_SPACE(bchan->head, bchan->tail,\
+			 MAX_DESCRIPTORS) == 0)
 
 struct bam_chan {
 	struct virt_dma_chan vc;
@@ -806,7 +806,7 @@ static u32 process_channel_irqs(struct bam_device *bdev)
 		offset /= sizeof(struct bam_desc_hw);
 
 		/* Number of bytes available to read */
-		avail = CIRC_CNT(offset, bchan->head, MAX_DESCRIPTORS + 1);
+		avail = CIRC_CNT(bchan->head, offset, MAX_DESCRIPTORS);
 
 		list_for_each_entry_safe(async_desc, tmp,
 					 &bchan->desc_list, desc_node) {
@@ -997,8 +997,7 @@ static void bam_start_dma(struct bam_chan *bchan)
 			bam_apply_new_config(bchan, async_desc->dir);
 
 		desc = async_desc->curr_desc;
-		avail = CIRC_SPACE(bchan->tail, bchan->head,
-				   MAX_DESCRIPTORS + 1);
+		avail = CIRC_SPACE(bchan->head, bchan->tail, MAX_DESCRIPTORS);
 
 		if (async_desc->num_desc > avail)
 			async_desc->xfer_len = avail;
-- 
2.21.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dmaengine: qcom-bam: fix circular buffer handling
  2019-06-14 14:20 [PATCH] dmaengine: qcom-bam: fix circular buffer handling Srinivas Kandagatla
@ 2019-06-14 19:43 ` Andy Gross
  2019-06-18  4:53 ` Vinod Koul
  2019-06-18  7:13 ` Sricharan R
  2 siblings, 0 replies; 11+ messages in thread
From: Andy Gross @ 2019-06-14 19:43 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: vkoul, dmaengine, linux-kernel, linux-arm-msm, sricharan

On Fri, Jun 14, 2019 at 03:20:12PM +0100, Srinivas Kandagatla wrote:
> For some reason arguments to most of the circular buffers
> macros are used in reverse, tail is used for head and vice versa.
> 
> This leads to bam thinking that there is an extra descriptor at the
> end and leading to retransmitting descriptor which was not scheduled
> by any driver. This happens after MAX_DESCRIPTORS (4096) are scheduled
> and done, so most of the drivers would not notice this, unless they are
> heavily using bam dma. Originally found this issue while testing
> SoundWire over SlimBus on DB845c which uses DMA very heavily for
> read/writes.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Reviewed-by: Andy Gross <agross@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dmaengine: qcom-bam: fix circular buffer handling
  2019-06-14 14:20 [PATCH] dmaengine: qcom-bam: fix circular buffer handling Srinivas Kandagatla
  2019-06-14 19:43 ` Andy Gross
@ 2019-06-18  4:53 ` Vinod Koul
  2019-06-18  7:13 ` Sricharan R
  2 siblings, 0 replies; 11+ messages in thread
From: Vinod Koul @ 2019-06-18  4:53 UTC (permalink / raw)
  To: Srinivas Kandagatla; +Cc: dmaengine, linux-kernel, linux-arm-msm, sricharan

On 14-06-19, 15:20, Srinivas Kandagatla wrote:
> For some reason arguments to most of the circular buffers
> macros are used in reverse, tail is used for head and vice versa.
> 
> This leads to bam thinking that there is an extra descriptor at the
> end and leading to retransmitting descriptor which was not scheduled
> by any driver. This happens after MAX_DESCRIPTORS (4096) are scheduled
> and done, so most of the drivers would not notice this, unless they are
> heavily using bam dma. Originally found this issue while testing
> SoundWire over SlimBus on DB845c which uses DMA very heavily for
> read/writes.

Applied and copied stable, thanks
-- 
~Vinod

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dmaengine: qcom-bam: fix circular buffer handling
  2019-06-14 14:20 [PATCH] dmaengine: qcom-bam: fix circular buffer handling Srinivas Kandagatla
  2019-06-14 19:43 ` Andy Gross
  2019-06-18  4:53 ` Vinod Koul
@ 2019-06-18  7:13 ` Sricharan R
  2019-06-18 14:49   ` Srinivas Kandagatla
  2019-06-18 14:50   ` Srinivas Kandagatla
  2 siblings, 2 replies; 11+ messages in thread
From: Sricharan R @ 2019-06-18  7:13 UTC (permalink / raw)
  To: Srinivas Kandagatla, vkoul; +Cc: dmaengine, linux-kernel, linux-arm-msm

Hi Srini,

On 6/14/2019 7:50 PM, Srinivas Kandagatla wrote:
> For some reason arguments to most of the circular buffers
> macros are used in reverse, tail is used for head and vice versa.
> 
> This leads to bam thinking that there is an extra descriptor at the
> end and leading to retransmitting descriptor which was not scheduled
> by any driver. This happens after MAX_DESCRIPTORS (4096) are scheduled
> and done, so most of the drivers would not notice this, unless they are
> heavily using bam dma. Originally found this issue while testing
> SoundWire over SlimBus on DB845c which uses DMA very heavily for
> read/writes.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/dma/qcom/bam_dma.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index cb860cb53c27..43d7b0a9713a 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -350,8 +350,8 @@ static const struct reg_offset_data bam_v1_7_reg_info[] = {
>  #define BAM_DESC_FIFO_SIZE	SZ_32K
>  #define MAX_DESCRIPTORS (BAM_DESC_FIFO_SIZE / sizeof(struct bam_desc_hw) - 1)
>  #define BAM_FIFO_SIZE	(SZ_32K - 8)
> -#define IS_BUSY(chan)	(CIRC_SPACE(bchan->tail, bchan->head,\
> -			 MAX_DESCRIPTORS + 1) == 0)
> +#define IS_BUSY(chan)	(CIRC_SPACE(bchan->head, bchan->tail,\
> +			 MAX_DESCRIPTORS) == 0)
>  
>  struct bam_chan {
>  	struct virt_dma_chan vc;
> @@ -806,7 +806,7 @@ static u32 process_channel_irqs(struct bam_device *bdev)
>  		offset /= sizeof(struct bam_desc_hw);
>  
>  		/* Number of bytes available to read */
> -		avail = CIRC_CNT(offset, bchan->head, MAX_DESCRIPTORS + 1);
> +		avail = CIRC_CNT(bchan->head, offset, MAX_DESCRIPTORS);
>
 one question, so MAX_DESCRIPTORS is already a mask,
    #define MAX_DESCRIPTORS (BAM_DESC_FIFO_SIZE / sizeof(struct bam_desc_hw) - 1)

 CIRC_CNT/SPACE macros also does a size - 1, so would it not be a problem if we
 just pass MAX_DESCRIPTORS ?

Regards,
 Sricharan
  
>  		list_for_each_entry_safe(async_desc, tmp,
>  					 &bchan->desc_list, desc_node) {
> @@ -997,8 +997,7 @@ static void bam_start_dma(struct bam_chan *bchan)
>  			bam_apply_new_config(bchan, async_desc->dir);
>  
>  		desc = async_desc->curr_desc;
> -		avail = CIRC_SPACE(bchan->tail, bchan->head,
> -				   MAX_DESCRIPTORS + 1);
> +		avail = CIRC_SPACE(bchan->head, bchan->tail, MAX_DESCRIPTORS);
>  
>  		if (async_desc->num_desc > avail)
>  			async_desc->xfer_len = avail;
> 

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dmaengine: qcom-bam: fix circular buffer handling
  2019-06-18  7:13 ` Sricharan R
@ 2019-06-18 14:49   ` Srinivas Kandagatla
  2019-06-18 14:50   ` Srinivas Kandagatla
  1 sibling, 0 replies; 11+ messages in thread
From: Srinivas Kandagatla @ 2019-06-18 14:49 UTC (permalink / raw)
  To: Sricharan R, vkoul; +Cc: dmaengine, linux-kernel, linux-arm-msm

Hi Sricharan,

On 18/06/2019 08:13, Sricharan R wrote:
> Hi Srini,
> 
> On 6/14/2019 7:50 PM, Srinivas Kandagatla wrote:
>> For some reason arguments to most of the circular buffers
>> macros are used in reverse, tail is used for head and vice versa.
>>
>> This leads to bam thinking that there is an extra descriptor at the
>> end and leading to retransmitting descriptor which was not scheduled
>> by any driver. This happens after MAX_DESCRIPTORS (4096) are scheduled
>> and done, so most of the drivers would not notice this, unless they are
>> heavily using bam dma. Originally found this issue while testing
>> SoundWire over SlimBus on DB845c which uses DMA very heavily for
>> read/writes.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/dma/qcom/bam_dma.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
>> index cb860cb53c27..43d7b0a9713a 100644
>> --- a/drivers/dma/qcom/bam_dma.c
>> +++ b/drivers/dma/qcom/bam_dma.c
>> @@ -350,8 +350,8 @@ static const struct reg_offset_data bam_v1_7_reg_info[] = {
>>   #define BAM_DESC_FIFO_SIZE	SZ_32K
>>   #define MAX_DESCRIPTORS (BAM_DESC_FIFO_SIZE / sizeof(struct bam_desc_hw) - 1)
>>   #define BAM_FIFO_SIZE	(SZ_32K - 8)
>> -#define IS_BUSY(chan)	(CIRC_SPACE(bchan->tail, bchan->head,\
>> -			 MAX_DESCRIPTORS + 1) == 0)
>> +#define IS_BUSY(chan)	(CIRC_SPACE(bchan->head, bchan->tail,\
>> +			 MAX_DESCRIPTORS) == 0)
>>   
>>   struct bam_chan {
>>   	struct virt_dma_chan vc;
>> @@ -806,7 +806,7 @@ static u32 process_channel_irqs(struct bam_device *bdev)
>>   		offset /= sizeof(struct bam_desc_hw);
>>   
>>   		/* Number of bytes available to read */
>> -		avail = CIRC_CNT(offset, bchan->head, MAX_DESCRIPTORS + 1);
>> +		avail = CIRC_CNT(bchan->head, offset, MAX_DESCRIPTORS);
>>
>   one question, so MAX_DESCRIPTORS is already a mask,
>      #define MAX_DESCRIPTORS (BAM_DESC_FIFO_SIZE / sizeof(struct bam_desc_hw) - 1)
> 
>   CIRC_CNT/SPACE macros also does a size - 1, so would it not be a problem if we
>   just pass MAX_DESCRIPTORS ?

Thanks for looking at this,
TBH, usage of CIRC_* macros is only valid for power-of-2 buffers,
In bam case MAX_DESCRIPTORS is 4095.
Am really not sure why 8 bytes have been removed from fifo data buffer size.
So basically usage of these macros is incorrect in bam case, this need 
to be fixed properly.

Do you agree?

Vinod, can you hold off with this patch, I will try to find some time 
this week to cook up a better patch removing the usage of these macros.



thanks,
srini

> 
> Regards,
>   Sricharan
>    
>>   		list_for_each_entry_safe(async_desc, tmp,
>>   					 &bchan->desc_list, desc_node) {
>> @@ -997,8 +997,7 @@ static void bam_start_dma(struct bam_chan *bchan)
>>   			bam_apply_new_config(bchan, async_desc->dir);
>>   
>>   		desc = async_desc->curr_desc;
>> -		avail = CIRC_SPACE(bchan->tail, bchan->head,
>> -				   MAX_DESCRIPTORS + 1);
>> +		avail = CIRC_SPACE(bchan->head, bchan->tail, MAX_DESCRIPTORS);
>>   
>>   		if (async_desc->num_desc > avail)
>>   			async_desc->xfer_len = avail;
>>
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dmaengine: qcom-bam: fix circular buffer handling
  2019-06-18  7:13 ` Sricharan R
  2019-06-18 14:49   ` Srinivas Kandagatla
@ 2019-06-18 14:50   ` Srinivas Kandagatla
  2019-06-18 14:56     ` Sricharan R
  1 sibling, 1 reply; 11+ messages in thread
From: Srinivas Kandagatla @ 2019-06-18 14:50 UTC (permalink / raw)
  To: Sricharan R, vkoul; +Cc: dmaengine, linux-kernel, linux-arm-msm

Hi Sricharan,

On 18/06/2019 08:13, Sricharan R wrote:
> Hi Srini,
> 
> On 6/14/2019 7:50 PM, Srinivas Kandagatla wrote:
>> For some reason arguments to most of the circular buffers
>> macros are used in reverse, tail is used for head and vice versa.
>>
>> This leads to bam thinking that there is an extra descriptor at the
>> end and leading to retransmitting descriptor which was not scheduled
>> by any driver. This happens after MAX_DESCRIPTORS (4096) are scheduled
>> and done, so most of the drivers would not notice this, unless they are
>> heavily using bam dma. Originally found this issue while testing
>> SoundWire over SlimBus on DB845c which uses DMA very heavily for
>> read/writes.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/dma/qcom/bam_dma.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
>> index cb860cb53c27..43d7b0a9713a 100644
>> --- a/drivers/dma/qcom/bam_dma.c
>> +++ b/drivers/dma/qcom/bam_dma.c
>> @@ -350,8 +350,8 @@ static const struct reg_offset_data bam_v1_7_reg_info[] = {
>>   #define BAM_DESC_FIFO_SIZE	SZ_32K
>>   #define MAX_DESCRIPTORS (BAM_DESC_FIFO_SIZE / sizeof(struct bam_desc_hw) - 1)
>>   #define BAM_FIFO_SIZE	(SZ_32K - 8)
>> -#define IS_BUSY(chan)	(CIRC_SPACE(bchan->tail, bchan->head,\
>> -			 MAX_DESCRIPTORS + 1) == 0)
>> +#define IS_BUSY(chan)	(CIRC_SPACE(bchan->head, bchan->tail,\
>> +			 MAX_DESCRIPTORS) == 0)
>>   
>>   struct bam_chan {
>>   	struct virt_dma_chan vc;
>> @@ -806,7 +806,7 @@ static u32 process_channel_irqs(struct bam_device *bdev)
>>   		offset /= sizeof(struct bam_desc_hw);
>>   
>>   		/* Number of bytes available to read */
>> -		avail = CIRC_CNT(offset, bchan->head, MAX_DESCRIPTORS + 1);
>> +		avail = CIRC_CNT(bchan->head, offset, MAX_DESCRIPTORS);
>>
>   one question, so MAX_DESCRIPTORS is already a mask,
>      #define MAX_DESCRIPTORS (BAM_DESC_FIFO_SIZE / sizeof(struct bam_desc_hw) - 1)
> 
>   CIRC_CNT/SPACE macros also does a size - 1, so would it not be a problem if we
>   just pass MAX_DESCRIPTORS ?

Thanks for looking at this,
TBH, usage of CIRC_* macros is only valid for power-of-2 buffers,
In bam case MAX_DESCRIPTORS is 4095.
Am really not sure why 8 bytes have been removed from fifo data buffer size.
So basically usage of these macros is incorrect in bam case, this need 
to be fixed properly.

Do you agree?

Vinod, can you hold off with this patch, I will try to find some time 
this week to cook up a better patch removing the usage of these macros.



thanks,
srini

> 
> Regards,
>   Sricharan
>    
>>   		list_for_each_entry_safe(async_desc, tmp,
>>   					 &bchan->desc_list, desc_node) {
>> @@ -997,8 +997,7 @@ static void bam_start_dma(struct bam_chan *bchan)
>>   			bam_apply_new_config(bchan, async_desc->dir);
>>   
>>   		desc = async_desc->curr_desc;
>> -		avail = CIRC_SPACE(bchan->tail, bchan->head,
>> -				   MAX_DESCRIPTORS + 1);
>> +		avail = CIRC_SPACE(bchan->head, bchan->tail, MAX_DESCRIPTORS);
>>   
>>   		if (async_desc->num_desc > avail)
>>   			async_desc->xfer_len = avail;
>>
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dmaengine: qcom-bam: fix circular buffer handling
  2019-06-18 14:50   ` Srinivas Kandagatla
@ 2019-06-18 14:56     ` Sricharan R
  2019-06-18 15:12       ` Srinivas Kandagatla
  0 siblings, 1 reply; 11+ messages in thread
From: Sricharan R @ 2019-06-18 14:56 UTC (permalink / raw)
  To: Srinivas Kandagatla, vkoul; +Cc: dmaengine, linux-kernel, linux-arm-msm

Hi Srini,

On 6/18/2019 8:20 PM, Srinivas Kandagatla wrote:
> Hi Sricharan,
> 
> On 18/06/2019 08:13, Sricharan R wrote:
>> Hi Srini,
>>
>> On 6/14/2019 7:50 PM, Srinivas Kandagatla wrote:
>>> For some reason arguments to most of the circular buffers
>>> macros are used in reverse, tail is used for head and vice versa.
>>>
>>> This leads to bam thinking that there is an extra descriptor at the
>>> end and leading to retransmitting descriptor which was not scheduled
>>> by any driver. This happens after MAX_DESCRIPTORS (4096) are scheduled
>>> and done, so most of the drivers would not notice this, unless they are
>>> heavily using bam dma. Originally found this issue while testing
>>> SoundWire over SlimBus on DB845c which uses DMA very heavily for
>>> read/writes.
>>>
>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>> ---
>>>   drivers/dma/qcom/bam_dma.c | 9 ++++-----
>>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
>>> index cb860cb53c27..43d7b0a9713a 100644
>>> --- a/drivers/dma/qcom/bam_dma.c
>>> +++ b/drivers/dma/qcom/bam_dma.c
>>> @@ -350,8 +350,8 @@ static const struct reg_offset_data bam_v1_7_reg_info[] = {
>>>   #define BAM_DESC_FIFO_SIZE    SZ_32K
>>>   #define MAX_DESCRIPTORS (BAM_DESC_FIFO_SIZE / sizeof(struct bam_desc_hw) - 1)
>>>   #define BAM_FIFO_SIZE    (SZ_32K - 8)
>>> -#define IS_BUSY(chan)    (CIRC_SPACE(bchan->tail, bchan->head,\
>>> -             MAX_DESCRIPTORS + 1) == 0)
>>> +#define IS_BUSY(chan)    (CIRC_SPACE(bchan->head, bchan->tail,\
>>> +             MAX_DESCRIPTORS) == 0)
>>>     struct bam_chan {
>>>       struct virt_dma_chan vc;
>>> @@ -806,7 +806,7 @@ static u32 process_channel_irqs(struct bam_device *bdev)
>>>           offset /= sizeof(struct bam_desc_hw);
>>>             /* Number of bytes available to read */
>>> -        avail = CIRC_CNT(offset, bchan->head, MAX_DESCRIPTORS + 1);
>>> +        avail = CIRC_CNT(bchan->head, offset, MAX_DESCRIPTORS);
>>>
>>   one question, so MAX_DESCRIPTORS is already a mask,
>>      #define MAX_DESCRIPTORS (BAM_DESC_FIFO_SIZE / sizeof(struct bam_desc_hw) - 1)
>>
>>   CIRC_CNT/SPACE macros also does a size - 1, so would it not be a problem if we
>>   just pass MAX_DESCRIPTORS ?
> 
> Thanks for looking at this,
> TBH, usage of CIRC_* macros is only valid for power-of-2 buffers,
> In bam case MAX_DESCRIPTORS is 4095.
> Am really not sure why 8 bytes have been removed from fifo data buffer size.
> So basically usage of these macros is incorrect in bam case, this need to be fixed properly.
> 
> Do you agree?
> 
  So MAX_DESCRIPTORS is used in driver for masking head/tail pointers.
  That's why we have to pass MAX_DESCRIPTORS + 1 so that it works
  when the Macros does a size - 1

Regards,
 Sricharan

> Vinod, can you hold off with this patch, I will try to find some time this week to cook up a better patch removing the usage of these macros.
> 
> 
> 
> thanks,
> srini
> 
>>
>> Regards,
>>   Sricharan
>>   
>>>           list_for_each_entry_safe(async_desc, tmp,
>>>                        &bchan->desc_list, desc_node) {
>>> @@ -997,8 +997,7 @@ static void bam_start_dma(struct bam_chan *bchan)
>>>               bam_apply_new_config(bchan, async_desc->dir);
>>>             desc = async_desc->curr_desc;
>>> -        avail = CIRC_SPACE(bchan->tail, bchan->head,
>>> -                   MAX_DESCRIPTORS + 1);
>>> +        avail = CIRC_SPACE(bchan->head, bchan->tail, MAX_DESCRIPTORS);
>>>             if (async_desc->num_desc > avail)
>>>               async_desc->xfer_len = avail;
>>>
>>

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dmaengine: qcom-bam: fix circular buffer handling
  2019-06-18 14:56     ` Sricharan R
@ 2019-06-18 15:12       ` Srinivas Kandagatla
  2019-06-18 16:27         ` Sricharan R
  0 siblings, 1 reply; 11+ messages in thread
From: Srinivas Kandagatla @ 2019-06-18 15:12 UTC (permalink / raw)
  To: Sricharan R, vkoul; +Cc: dmaengine, linux-kernel, linux-arm-msm



On 18/06/2019 15:56, Sricharan R wrote:
>    So MAX_DESCRIPTORS is used in driver for masking head/tail pointers.
>    That's why we have to pass MAX_DESCRIPTORS + 1 so that it works
>    when the Macros does a size - 1
Isn't that incorrect to do that, pretending to have more descriptors 
than we actually have?

--srini

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dmaengine: qcom-bam: fix circular buffer handling
  2019-06-18 15:12       ` Srinivas Kandagatla
@ 2019-06-18 16:27         ` Sricharan R
  2019-06-18 16:50           ` Srinivas Kandagatla
  0 siblings, 1 reply; 11+ messages in thread
From: Sricharan R @ 2019-06-18 16:27 UTC (permalink / raw)
  To: Srinivas Kandagatla, vkoul; +Cc: dmaengine, linux-kernel, linux-arm-msm



On 6/18/2019 8:42 PM, Srinivas Kandagatla wrote:
> 
> 
> On 18/06/2019 15:56, Sricharan R wrote:
>>    So MAX_DESCRIPTORS is used in driver for masking head/tail pointers.
>>    That's why we have to pass MAX_DESCRIPTORS + 1 so that it works
>>    when the Macros does a size - 1
> Isn't that incorrect to do that, pretending to have more descriptors than we actually have?
> 

 The Macro's expect that buffer size is power of 2. So we are infact passing the actual correct
 size ( MAX_DESCRIPTORS + 1 = 4096)

Regards,
 Sricharan

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dmaengine: qcom-bam: fix circular buffer handling
  2019-06-18 16:27         ` Sricharan R
@ 2019-06-18 16:50           ` Srinivas Kandagatla
  2019-06-19 17:50             ` Sricharan R
  0 siblings, 1 reply; 11+ messages in thread
From: Srinivas Kandagatla @ 2019-06-18 16:50 UTC (permalink / raw)
  To: Sricharan R, vkoul; +Cc: dmaengine, linux-kernel, linux-arm-msm



On 18/06/2019 17:27, Sricharan R wrote:
>   The Macro's expect that buffer size is power of 2. So we are infact passing the actual correct
>   size ( MAX_DESCRIPTORS + 1 = 4096)
This will make the circular buffer macros happy but question is that do 
we actually have that many descriptor buffers?

This is what is in the driver:

#define BAM_DESC_FIFO_SIZE	SZ_32K
#define MAX_DESCRIPTORS (BAM_DESC_FIFO_SIZE / sizeof(struct bam_desc_hw) 
- 1)
#define BAM_FIFO_SIZE	(SZ_32K - 8)

Wouldn't having MAX_DESCRIPTORS + 1 = 4096  lead to overflow the actual 
descriptor memory size of (SZ_32K - 8) ?

--srini


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dmaengine: qcom-bam: fix circular buffer handling
  2019-06-18 16:50           ` Srinivas Kandagatla
@ 2019-06-19 17:50             ` Sricharan R
  0 siblings, 0 replies; 11+ messages in thread
From: Sricharan R @ 2019-06-19 17:50 UTC (permalink / raw)
  To: Srinivas Kandagatla, vkoul; +Cc: dmaengine, linux-kernel, linux-arm-msm

Hi Srini,

On 6/18/2019 10:20 PM, Srinivas Kandagatla wrote:
> 
> 
> On 18/06/2019 17:27, Sricharan R wrote:
>>   The Macro's expect that buffer size is power of 2. So we are infact passing the actual correct
>>   size ( MAX_DESCRIPTORS + 1 = 4096)
> This will make the circular buffer macros happy but question is that do we actually have that many descriptor buffers?
> 
> This is what is in the driver:
> 
> #define BAM_DESC_FIFO_SIZE    SZ_32K
> #define MAX_DESCRIPTORS (BAM_DESC_FIFO_SIZE / sizeof(struct bam_desc_hw) - 1)
> #define BAM_FIFO_SIZE    (SZ_32K - 8)
> 
> Wouldn't having MAX_DESCRIPTORS + 1 = 4096  lead to overflow the actual descriptor memory size of (SZ_32K - 8) ?
> 

Right, but the CIRC_SPACE macro assumes there is 1 space less than the actual size.
That said, agree there is an issue on the boundary. I will also do some testing tomorrow
on this and get back.

Regards,
 Sricharan
-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-14 14:20 [PATCH] dmaengine: qcom-bam: fix circular buffer handling Srinivas Kandagatla
2019-06-14 19:43 ` Andy Gross
2019-06-18  4:53 ` Vinod Koul
2019-06-18  7:13 ` Sricharan R
2019-06-18 14:49   ` Srinivas Kandagatla
2019-06-18 14:50   ` Srinivas Kandagatla
2019-06-18 14:56     ` Sricharan R
2019-06-18 15:12       ` Srinivas Kandagatla
2019-06-18 16:27         ` Sricharan R
2019-06-18 16:50           ` Srinivas Kandagatla
2019-06-19 17:50             ` Sricharan R

dmaengine Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dmaengine/0 dmaengine/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dmaengine dmaengine/ https://lore.kernel.org/dmaengine \
		dmaengine@vger.kernel.org dmaengine@archiver.kernel.org
	public-inbox-index dmaengine


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.dmaengine


AGPL code for this site: git clone https://public-inbox.org/ public-inbox