diff mbox series

[v2,08/13] dmaengine: at_xdmac: Move the free desc to the tail of the desc list

Message ID 20211125090028.786832-9-tudor.ambarus@microchip.com (mailing list archive)
State Superseded
Headers show
Series dmaengine: at_xdmac: Various fixes | expand

Commit Message

Tudor Ambarus Nov. 25, 2021, 9 a.m. UTC
So that we don't use the same desc over and over again.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/dma/at_xdmac.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Comments

Vinod Koul Dec. 13, 2021, 8:07 a.m. UTC | #1
On 25-11-21, 11:00, Tudor Ambarus wrote:
> So that we don't use the same desc over and over again.

Please use full para in the changelog and not a continuation of the
patch title!

and why is wrong with using same desc over and over? Any benefits of not
doing so?


> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/dma/at_xdmac.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
> index 2cc9af222681..8804a86a9bcc 100644
> --- a/drivers/dma/at_xdmac.c
> +++ b/drivers/dma/at_xdmac.c
> @@ -729,7 +729,8 @@ at_xdmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  		if (!desc) {
>  			dev_err(chan2dev(chan), "can't get descriptor\n");
>  			if (first)
> -				list_splice_init(&first->descs_list, &atchan->free_descs_list);
> +				list_splice_tail_init(&first->descs_list,
> +						      &atchan->free_descs_list);
>  			goto spin_unlock;
>  		}
>  
> @@ -817,7 +818,8 @@ at_xdmac_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr,
>  		if (!desc) {
>  			dev_err(chan2dev(chan), "can't get descriptor\n");
>  			if (first)
> -				list_splice_init(&first->descs_list, &atchan->free_descs_list);
> +				list_splice_tail_init(&first->descs_list,
> +						      &atchan->free_descs_list);
>  			spin_unlock_irqrestore(&atchan->lock, irqflags);
>  			return NULL;
>  		}
> @@ -1051,8 +1053,8 @@ at_xdmac_prep_interleaved(struct dma_chan *chan,
>  							       src_addr, dst_addr,
>  							       xt, chunk);
>  			if (!desc) {
> -				list_splice_init(&first->descs_list,
> -						 &atchan->free_descs_list);
> +				list_splice_tail_init(&first->descs_list,
> +						      &atchan->free_descs_list);
>  				return NULL;
>  			}
>  
> @@ -1132,7 +1134,8 @@ at_xdmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
>  		if (!desc) {
>  			dev_err(chan2dev(chan), "can't get descriptor\n");
>  			if (first)
> -				list_splice_init(&first->descs_list, &atchan->free_descs_list);
> +				list_splice_tail_init(&first->descs_list,
> +						      &atchan->free_descs_list);
>  			return NULL;
>  		}
>  
> @@ -1308,8 +1311,8 @@ at_xdmac_prep_dma_memset_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  						   sg_dma_len(sg),
>  						   value);
>  		if (!desc && first)
> -			list_splice_init(&first->descs_list,
> -					 &atchan->free_descs_list);
> +			list_splice_tail_init(&first->descs_list,
> +					      &atchan->free_descs_list);
>  
>  		if (!first)
>  			first = desc;
> @@ -1701,7 +1704,8 @@ static void at_xdmac_tasklet(struct tasklet_struct *t)
>  
>  		spin_lock_irq(&atchan->lock);
>  		/* Move the xfer descriptors into the free descriptors list. */
> -		list_splice_init(&desc->descs_list, &atchan->free_descs_list);
> +		list_splice_tail_init(&desc->descs_list,
> +				      &atchan->free_descs_list);
>  		at_xdmac_advance_work(atchan);
>  		spin_unlock_irq(&atchan->lock);
>  	}
> @@ -1850,7 +1854,8 @@ static int at_xdmac_device_terminate_all(struct dma_chan *chan)
>  	/* Cancel all pending transfers. */
>  	list_for_each_entry_safe(desc, _desc, &atchan->xfers_list, xfer_node) {
>  		list_del(&desc->xfer_node);
> -		list_splice_init(&desc->descs_list, &atchan->free_descs_list);
> +		list_splice_tail_init(&desc->descs_list,
> +				      &atchan->free_descs_list);
>  	}
>  
>  	clear_bit(AT_XDMAC_CHAN_IS_PAUSED, &atchan->status);
> -- 
> 2.25.1
Tudor Ambarus Dec. 13, 2021, 8:51 a.m. UTC | #2
Hi, Vinod,

On 12/13/21 10:07 AM, Vinod Koul wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 25-11-21, 11:00, Tudor Ambarus wrote:
>> So that we don't use the same desc over and over again.
> 
> Please use full para in the changelog and not a continuation of the
> patch title!

Ok, will add a better commit description. Here and in other patches where
your comment applies.

> 
> and why is wrong with using same desc over and over? Any benefits of not
> doing so?

Not wrong, but if we move the free desc to the tail of the list, then the
sequence of descriptors is more track-able in case of debug. You would
know which descriptor should come next and you could easier catch
concurrency over descriptors for example. I saw virt-dma uses
list_splice_tail_init() as well, I found it a good idea, so I thought to
follow the core driver.

Cheers,
ta
Vinod Koul Dec. 13, 2021, 8:59 a.m. UTC | #3
On 13-12-21, 08:51, Tudor.Ambarus@microchip.com wrote:
> Hi, Vinod,
> 
> On 12/13/21 10:07 AM, Vinod Koul wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On 25-11-21, 11:00, Tudor Ambarus wrote:
> >> So that we don't use the same desc over and over again.
> > 
> > Please use full para in the changelog and not a continuation of the
> > patch title!
> 
> Ok, will add a better commit description. Here and in other patches where
> your comment applies.

Great!

> > 
> > and why is wrong with using same desc over and over? Any benefits of not
> > doing so?
> 
> Not wrong, but if we move the free desc to the tail of the list, then the
> sequence of descriptors is more track-able in case of debug. You would
> know which descriptor should come next and you could easier catch
> concurrency over descriptors for example. I saw virt-dma uses
> list_splice_tail_init() as well, I found it a good idea, so I thought to
> follow the core driver.

Okay, I would be good to add this motivation in the change log. I am
sure after few you would also wonder why you did this change :)
Vinod Koul Dec. 13, 2021, 9 a.m. UTC | #4
On 13-12-21, 14:29, Vinod Koul wrote:
> On 13-12-21, 08:51, Tudor.Ambarus@microchip.com wrote:
> > Hi, Vinod,
> > 
> > On 12/13/21 10:07 AM, Vinod Koul wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > > 
> > > On 25-11-21, 11:00, Tudor Ambarus wrote:
> > >> So that we don't use the same desc over and over again.
> > > 
> > > Please use full para in the changelog and not a continuation of the
> > > patch title!
> > 
> > Ok, will add a better commit description. Here and in other patches where
> > your comment applies.
> 
> Great!
> 
> > > 
> > > and why is wrong with using same desc over and over? Any benefits of not
> > > doing so?
> > 
> > Not wrong, but if we move the free desc to the tail of the list, then the
> > sequence of descriptors is more track-able in case of debug. You would
> > know which descriptor should come next and you could easier catch
> > concurrency over descriptors for example. I saw virt-dma uses
> > list_splice_tail_init() as well, I found it a good idea, so I thought to
> > follow the core driver.
> 
> Okay, I would be good to add this motivation in the change log. I am
> sure after few you would also wonder why you did this change :)

Also, pls submit serial patches to Greg separately. I guess he saw the
title and overlooked those...
Tudor Ambarus Dec. 13, 2021, 9:22 a.m. UTC | #5
On 12/13/21 11:00 AM, Vinod Koul wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 13-12-21, 14:29, Vinod Koul wrote:
>> On 13-12-21, 08:51, Tudor.Ambarus@microchip.com wrote:
>>> Hi, Vinod,
>>>
>>> On 12/13/21 10:07 AM, Vinod Koul wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On 25-11-21, 11:00, Tudor Ambarus wrote:
>>>>> So that we don't use the same desc over and over again.
>>>>
>>>> Please use full para in the changelog and not a continuation of the
>>>> patch title!
>>>
>>> Ok, will add a better commit description. Here and in other patches where
>>> your comment applies.
>>
>> Great!
>>
>>>>
>>>> and why is wrong with using same desc over and over? Any benefits of not
>>>> doing so?
>>>
>>> Not wrong, but if we move the free desc to the tail of the list, then the
>>> sequence of descriptors is more track-able in case of debug. You would
>>> know which descriptor should come next and you could easier catch
>>> concurrency over descriptors for example. I saw virt-dma uses
>>> list_splice_tail_init() as well, I found it a good idea, so I thought to
>>> follow the core driver.
>>
>> Okay, I would be good to add this motivation in the change log. I am
>> sure after few you would also wonder why you did this change :)

Sure.

> 
> Also, pls submit serial patches to Greg separately. I guess he saw the
> title and overlooked those...

I received a private message from Greg informing me that he applied the
patches to tty-next and that they will be merged during the merge window.
So I'll drop the tty patches in v3.

Cheers,
ta
Tudor Ambarus Dec. 13, 2021, 9:27 a.m. UTC | #6
On 12/13/21 11:22 AM, Tudor Ambarus wrote:
> On 12/13/21 11:00 AM, Vinod Koul wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 13-12-21, 14:29, Vinod Koul wrote:
>>> On 13-12-21, 08:51, Tudor.Ambarus@microchip.com wrote:
>>>> Hi, Vinod,
>>>>
>>>> On 12/13/21 10:07 AM, Vinod Koul wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On 25-11-21, 11:00, Tudor Ambarus wrote:
>>>>>> So that we don't use the same desc over and over again.
>>>>>
>>>>> Please use full para in the changelog and not a continuation of the
>>>>> patch title!
>>>>
>>>> Ok, will add a better commit description. Here and in other patches where
>>>> your comment applies.
>>>
>>> Great!
>>>
>>>>>
>>>>> and why is wrong with using same desc over and over? Any benefits of not
>>>>> doing so?
>>>>
>>>> Not wrong, but if we move the free desc to the tail of the list, then the
>>>> sequence of descriptors is more track-able in case of debug. You would
>>>> know which descriptor should come next and you could easier catch
>>>> concurrency over descriptors for example. I saw virt-dma uses
>>>> list_splice_tail_init() as well, I found it a good idea, so I thought to
>>>> follow the core driver.
>>>
>>> Okay, I would be good to add this motivation in the change log. I am
>>> sure after few you would also wonder why you did this change :)
> 
> Sure.
> 
>>
>> Also, pls submit serial patches to Greg separately. I guess he saw the
>> title and overlooked those...
> 
> I received a private message from Greg informing me that he applied the

for clarity: Greg applied just the 2 tty patches, not the entire series.

> patches to tty-next and that they will be merged during the merge window.
> So I'll drop the tty patches in v3.

v3 will follow and it will contain just the at_xdmac patches.
diff mbox series

Patch

diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
index 2cc9af222681..8804a86a9bcc 100644
--- a/drivers/dma/at_xdmac.c
+++ b/drivers/dma/at_xdmac.c
@@ -729,7 +729,8 @@  at_xdmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		if (!desc) {
 			dev_err(chan2dev(chan), "can't get descriptor\n");
 			if (first)
-				list_splice_init(&first->descs_list, &atchan->free_descs_list);
+				list_splice_tail_init(&first->descs_list,
+						      &atchan->free_descs_list);
 			goto spin_unlock;
 		}
 
@@ -817,7 +818,8 @@  at_xdmac_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr,
 		if (!desc) {
 			dev_err(chan2dev(chan), "can't get descriptor\n");
 			if (first)
-				list_splice_init(&first->descs_list, &atchan->free_descs_list);
+				list_splice_tail_init(&first->descs_list,
+						      &atchan->free_descs_list);
 			spin_unlock_irqrestore(&atchan->lock, irqflags);
 			return NULL;
 		}
@@ -1051,8 +1053,8 @@  at_xdmac_prep_interleaved(struct dma_chan *chan,
 							       src_addr, dst_addr,
 							       xt, chunk);
 			if (!desc) {
-				list_splice_init(&first->descs_list,
-						 &atchan->free_descs_list);
+				list_splice_tail_init(&first->descs_list,
+						      &atchan->free_descs_list);
 				return NULL;
 			}
 
@@ -1132,7 +1134,8 @@  at_xdmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 		if (!desc) {
 			dev_err(chan2dev(chan), "can't get descriptor\n");
 			if (first)
-				list_splice_init(&first->descs_list, &atchan->free_descs_list);
+				list_splice_tail_init(&first->descs_list,
+						      &atchan->free_descs_list);
 			return NULL;
 		}
 
@@ -1308,8 +1311,8 @@  at_xdmac_prep_dma_memset_sg(struct dma_chan *chan, struct scatterlist *sgl,
 						   sg_dma_len(sg),
 						   value);
 		if (!desc && first)
-			list_splice_init(&first->descs_list,
-					 &atchan->free_descs_list);
+			list_splice_tail_init(&first->descs_list,
+					      &atchan->free_descs_list);
 
 		if (!first)
 			first = desc;
@@ -1701,7 +1704,8 @@  static void at_xdmac_tasklet(struct tasklet_struct *t)
 
 		spin_lock_irq(&atchan->lock);
 		/* Move the xfer descriptors into the free descriptors list. */
-		list_splice_init(&desc->descs_list, &atchan->free_descs_list);
+		list_splice_tail_init(&desc->descs_list,
+				      &atchan->free_descs_list);
 		at_xdmac_advance_work(atchan);
 		spin_unlock_irq(&atchan->lock);
 	}
@@ -1850,7 +1854,8 @@  static int at_xdmac_device_terminate_all(struct dma_chan *chan)
 	/* Cancel all pending transfers. */
 	list_for_each_entry_safe(desc, _desc, &atchan->xfers_list, xfer_node) {
 		list_del(&desc->xfer_node);
-		list_splice_init(&desc->descs_list, &atchan->free_descs_list);
+		list_splice_tail_init(&desc->descs_list,
+				      &atchan->free_descs_list);
 	}
 
 	clear_bit(AT_XDMAC_CHAN_IS_PAUSED, &atchan->status);