[PATCH v2 00/13] dmaengine: at_xdmac: Various fixes

[PATCH v2 00/13] dmaengine: at_xdmac: Various fixes

[Tudor Ambarus]

Bugs identified when debugging a hang encountered when operating
an octal DTR SPI NOR memory. The culprit was the flash, not the
DMA driver, so all these bugs are not experienced in real life,
they are all theoretical fixes. Nevertheless the bugs are there
and I think they should be squashed.

If all of you consider that all these are worthy to be applied,
I would suggest that all the patches to be taken via an immutable
branch of the DMA tree. The serial patches depend on the first patch
in the series. The DMA transactions are no longer started at tx_submit()
level, but at device_issue_pending() level, as the DMA API requires.
The atmel serial driver wrongly assumed that the DMA transactions
are started at tx_submit() level and never called
dma_async_issue_pending(). Applying first patch, but not the atmel_serial
patches will break atmel_serial when using DMA.

Tested the serial with DMA on sama5d2_xplained. Tested QSPI with DMA on
sama7g5ek. All went well.

v2:
- drop local chan_rx local variable in patch 3/13, focus just on fixes
for now.
- collect Richard's Acked-by tag.
- add details in the cover letter about what tests were performed.

Tudor Ambarus (13):
  dmaengine: at_xdmac: Don't start transactions at tx_submit level
  tty: serial: atmel: Check return code of dmaengine_submit()
  tty: serial: atmel: Call dma_async_issue_pending()
  dmaengine: at_xdmac: Start transfer for cyclic channels in
    issue_pending
  dmaengine: at_xdmac: Print debug message after realeasing the lock
  dmaengine: at_xdmac: Fix concurrency over chan's completed_cookie
  dmaengine: at_xdmac: Fix race for the tx desc callback
  dmaengine: at_xdmac: Move the free desc to the tail of the desc list
  dmaengine: at_xdmac: Fix concurrency over xfers_list
  dmaengine: at_xdmac: Remove a level of indentation in
    at_xdmac_advance_work()
  dmaengine: at_xdmac: Fix lld view setting
  dmaengine: at_xdmac: Fix at_xdmac_lld struct definition
  dmaengine: at_xdmac: Fix race over irq_status

 drivers/dma/at_xdmac.c            | 186 ++++++++++++++----------------
 drivers/tty/serial/atmel_serial.c |  14 +++
 2 files changed, 101 insertions(+), 99 deletions(-)

-- 
2.25.1

[PATCH v2 01/13] dmaengine: at_xdmac: Don't start transactions at tx_submit level

[Tudor Ambarus]

tx_submit is supposed to push the current transaction descriptor
to a pending queue, waiting for issue_pending() to be called.
issue_pending() must start the transfer, not tx_submit().
As the at_xdmac_start_xfer() is now called only from
at_xdmac_advance_work() when !at_xdmac_chan_is_enabled(),
the at_xdmac_chan_is_enabled() check is no longer needed in
at_xdmac_start_xfer(), thus remove it.

Clients of at_xdmac that assume that tx_submit() starts the transfer
must be updated and call dma_async_issue_pending() if they miss to
call it (one example is atmel_serial).

Fixes: e1f7c9eee707 ("dmaengine: at_xdmac: creation of the atmel eXtended DMA Controller driver")
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/dma/at_xdmac.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
index 275a76f188ae..ccf796a3b9f3 100644
--- a/drivers/dma/at_xdmac.c
+++ b/drivers/dma/at_xdmac.c
@@ -385,9 +385,6 @@ static void at_xdmac_start_xfer(struct at_xdmac_chan *atchan,
 
 	dev_vdbg(chan2dev(&atchan->chan), "%s: desc 0x%p\n", __func__, first);
 
-	if (at_xdmac_chan_is_enabled(atchan))
-		return;
-
 	/* Set transfer as active to not try to start it again. */
 	first->active_xfer = true;
 
@@ -479,9 +476,6 @@ static dma_cookie_t at_xdmac_tx_submit(struct dma_async_tx_descriptor *tx)
 	dev_vdbg(chan2dev(tx->chan), "%s: atchan 0x%p, add desc 0x%p to xfers_list\n",
 		 __func__, atchan, desc);
 	list_add_tail(&desc->xfer_node, &atchan->xfers_list);
-	if (list_is_singular(&atchan->xfers_list))
-		at_xdmac_start_xfer(atchan, desc);
-
 	spin_unlock_irqrestore(&atchan->lock, irqflags);
 	return cookie;
 }
-- 
2.25.1

[PATCH v2 02/13] tty: serial: atmel: Check return code of dmaengine_submit()

[Tudor Ambarus]

The tx_submit() method of struct dma_async_tx_descriptor is entitled
to do sanity checks and return errors if encountered. It's not the
case for the DMA controller drivers that this client is using
(at_h/xdmac), because they currently don't do sanity checks and always
return a positive cookie at tx_submit() method. In case the controller
drivers will implement sanity checks and return errors, print a message
so that the client will be informed that something went wrong at
tx_submit() level.

Fixes: 08f738be88bb ("serial: at91: add tx dma support")
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Acked-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/tty/serial/atmel_serial.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 2c99a47a2535..376f7a9c2868 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1004,6 +1004,11 @@ static void atmel_tx_dma(struct uart_port *port)
 		desc->callback = atmel_complete_tx_dma;
 		desc->callback_param = atmel_port;
 		atmel_port->cookie_tx = dmaengine_submit(desc);
+		if (dma_submit_error(atmel_port->cookie_tx)) {
+			dev_err(port->dev, "dma_submit_error %d\n",
+				atmel_port->cookie_tx);
+			return;
+		}
 	}
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
@@ -1258,6 +1263,11 @@ static int atmel_prepare_rx_dma(struct uart_port *port)
 	desc->callback_param = port;
 	atmel_port->desc_rx = desc;
 	atmel_port->cookie_rx = dmaengine_submit(desc);
+	if (dma_submit_error(atmel_port->cookie_rx)) {
+		dev_err(port->dev, "dma_submit_error %d\n",
+			atmel_port->cookie_rx);
+		goto chan_err;
+	}
 
 	return 0;
 
-- 
2.25.1

[PATCH v2 03/13] tty: serial: atmel: Call dma_async_issue_pending()

[Tudor Ambarus]

The driver wrongly assummed that tx_submit() will start the transfer,
which is not the case, now that the at_xdmac driver is fixed. tx_submit
is supposed to push the current transaction descriptor to a pending queue,
waiting for issue_pending to be called. issue_pending must start the
transfer, not tx_submit.

Fixes: 34df42f59a60 ("serial: at91: add rx dma support")
Fixes: 08f738be88bb ("serial: at91: add tx dma support")
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/tty/serial/atmel_serial.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 376f7a9c2868..269b4500e9e7 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1009,6 +1009,8 @@ static void atmel_tx_dma(struct uart_port *port)
 				atmel_port->cookie_tx);
 			return;
 		}
+
+		dma_async_issue_pending(chan);
 	}
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
@@ -1269,6 +1271,8 @@ static int atmel_prepare_rx_dma(struct uart_port *port)
 		goto chan_err;
 	}
 
+	dma_async_issue_pending(atmel_port->chan_rx);
+
 	return 0;
 
 chan_err:
-- 
2.25.1

[PATCH v2 04/13] dmaengine: at_xdmac: Start transfer for cyclic channels in issue_pending

[Tudor Ambarus]

Cyclic channels must too call issue_pending in order to start a
transfer. This wrongly worked before, because in the past the
transfer was started at tx_submit level when only a desc in the
transfer list.

Fixes: e1f7c9eee707 ("dmaengine: at_xdmac: creation of the atmel eXtended DMA Controller driver")
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/dma/at_xdmac.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
index ccf796a3b9f3..9a5c68eda801 100644
--- a/drivers/dma/at_xdmac.c
+++ b/drivers/dma/at_xdmac.c
@@ -1778,11 +1778,9 @@ static void at_xdmac_issue_pending(struct dma_chan *chan)
 
 	dev_dbg(chan2dev(&atchan->chan), "%s\n", __func__);
 
-	if (!at_xdmac_chan_is_cyclic(atchan)) {
-		spin_lock_irqsave(&atchan->lock, flags);
-		at_xdmac_advance_work(atchan);
-		spin_unlock_irqrestore(&atchan->lock, flags);
-	}
+	spin_lock_irqsave(&atchan->lock, flags);
+	at_xdmac_advance_work(atchan);
+	spin_unlock_irqrestore(&atchan->lock, flags);
 
 	return;
 }
-- 
2.25.1

[PATCH v2 05/13] dmaengine: at_xdmac: Print debug message after realeasing the lock

[Tudor Ambarus]

It is desirable to do the prints without the lock held if possible.

Fixes: e1f7c9eee707 ("dmaengine: at_xdmac: creation of the atmel eXtended DMA Controller driver")
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/dma/at_xdmac.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
index 9a5c68eda801..b45ae4fee9db 100644
--- a/drivers/dma/at_xdmac.c
+++ b/drivers/dma/at_xdmac.c
@@ -473,10 +473,12 @@ static dma_cookie_t at_xdmac_tx_submit(struct dma_async_tx_descriptor *tx)
 	spin_lock_irqsave(&atchan->lock, irqflags);
 	cookie = dma_cookie_assign(tx);
 
-	dev_vdbg(chan2dev(tx->chan), "%s: atchan 0x%p, add desc 0x%p to xfers_list\n",
-		 __func__, atchan, desc);
 	list_add_tail(&desc->xfer_node, &atchan->xfers_list);
 	spin_unlock_irqrestore(&atchan->lock, irqflags);
+
+	dev_vdbg(chan2dev(tx->chan), "%s: atchan 0x%p, add desc 0x%p to xfers_list\n",
+		 __func__, atchan, desc);
+
 	return cookie;
 }
 
-- 
2.25.1

[PATCH v2 06/13] dmaengine: at_xdmac: Fix concurrency over chan's completed_cookie

[Tudor Ambarus]

Caller of dma_cookie_complete is expected to hold a lock to prevent
concurrency over the channel's completed cookie marker.

Fixes: e1f7c9eee707 ("dmaengine: at_xdmac: creation of the atmel eXtended DMA Controller driver")
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/dma/at_xdmac.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
index b45ae4fee9db..4d8476845c20 100644
--- a/drivers/dma/at_xdmac.c
+++ b/drivers/dma/at_xdmac.c
@@ -1703,11 +1703,10 @@ static void at_xdmac_tasklet(struct tasklet_struct *t)
 		}
 
 		txd = &desc->tx_dma_desc;
-
+		dma_cookie_complete(txd);
 		at_xdmac_remove_xfer(atchan, desc);
 		spin_unlock_irq(&atchan->lock);
 
-		dma_cookie_complete(txd);
 		if (txd->flags & DMA_PREP_INTERRUPT)
 			dmaengine_desc_get_callback_invoke(txd, NULL);
 
-- 
2.25.1

[PATCH v2 07/13] dmaengine: at_xdmac: Fix race for the tx desc callback

[Tudor Ambarus]

The transfer descriptors were wrongly moved to the free descriptors list
before calling the tx desc callback. As the DMA engine drivers drop any
locks before calling the callback function, txd could be taken again,
resulting in its callback called prematurely.

Fixes: e1f7c9eee707 ("dmaengine: at_xdmac: creation of the atmel eXtended DMA Controller driver")
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/dma/at_xdmac.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
index 4d8476845c20..2cc9af222681 100644
--- a/drivers/dma/at_xdmac.c
+++ b/drivers/dma/at_xdmac.c
@@ -1582,20 +1582,6 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 	return ret;
 }
 
-/* Call must be protected by lock. */
-static void at_xdmac_remove_xfer(struct at_xdmac_chan *atchan,
-				    struct at_xdmac_desc *desc)
-{
-	dev_dbg(chan2dev(&atchan->chan), "%s: desc 0x%p\n", __func__, desc);
-
-	/*
-	 * Remove the transfer from the transfer list then move the transfer
-	 * descriptors into the free descriptors list.
-	 */
-	list_del(&desc->xfer_node);
-	list_splice_init(&desc->descs_list, &atchan->free_descs_list);
-}
-
 static void at_xdmac_advance_work(struct at_xdmac_chan *atchan)
 {
 	struct at_xdmac_desc	*desc;
@@ -1704,7 +1690,8 @@ static void at_xdmac_tasklet(struct tasklet_struct *t)
 
 		txd = &desc->tx_dma_desc;
 		dma_cookie_complete(txd);
-		at_xdmac_remove_xfer(atchan, desc);
+		/* Remove the transfer from the transfer list. */
+		list_del(&desc->xfer_node);
 		spin_unlock_irq(&atchan->lock);
 
 		if (txd->flags & DMA_PREP_INTERRUPT)
@@ -1713,6 +1700,8 @@ static void at_xdmac_tasklet(struct tasklet_struct *t)
 		dma_run_dependencies(txd);
 
 		spin_lock_irq(&atchan->lock);
+		/* Move the xfer descriptors into the free descriptors list. */
+		list_splice_init(&desc->descs_list, &atchan->free_descs_list);
 		at_xdmac_advance_work(atchan);
 		spin_unlock_irq(&atchan->lock);
 	}
@@ -1859,8 +1848,10 @@ static int at_xdmac_device_terminate_all(struct dma_chan *chan)
 		cpu_relax();
 
 	/* Cancel all pending transfers. */
-	list_for_each_entry_safe(desc, _desc, &atchan->xfers_list, xfer_node)
-		at_xdmac_remove_xfer(atchan, desc);
+	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);
+	}
 
 	clear_bit(AT_XDMAC_CHAN_IS_PAUSED, &atchan->status);
 	clear_bit(AT_XDMAC_CHAN_IS_CYCLIC, &atchan->status);
-- 
2.25.1

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

[Tudor Ambarus]

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(-)

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

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

[Vinod Koul]

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

-- 
~Vinod

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

[Tudor.Ambarus]

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

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

[Vinod Koul]

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

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

[Vinod Koul]

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...

-- 
~Vinod

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

[Tudor.Ambarus]

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

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

[Tudor.Ambarus]

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.

[PATCH v2 09/13] dmaengine: at_xdmac: Fix concurrency over xfers_list

[Tudor Ambarus]

Since tx_submit can be called from a hard irq, xfers_list must be
protected with a lock to avoid concurency on the list's elements.

Fixes: e1f7c9eee707 ("dmaengine: at_xdmac: creation of the atmel eXtended DMA Controller driver")
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/dma/at_xdmac.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
index 8804a86a9bcc..81f6f1357dcb 100644
--- a/drivers/dma/at_xdmac.c
+++ b/drivers/dma/at_xdmac.c
@@ -1608,14 +1608,17 @@ static void at_xdmac_handle_cyclic(struct at_xdmac_chan *atchan)
 	struct at_xdmac_desc		*desc;
 	struct dma_async_tx_descriptor	*txd;
 
-	if (!list_empty(&atchan->xfers_list)) {
-		desc = list_first_entry(&atchan->xfers_list,
-					struct at_xdmac_desc, xfer_node);
-		txd = &desc->tx_dma_desc;
-
-		if (txd->flags & DMA_PREP_INTERRUPT)
-			dmaengine_desc_get_callback_invoke(txd, NULL);
+	spin_lock_irq(&atchan->lock);
+	if (list_empty(&atchan->xfers_list)) {
+		spin_unlock_irq(&atchan->lock);
+		return;
 	}
+	desc = list_first_entry(&atchan->xfers_list, struct at_xdmac_desc,
+				xfer_node);
+	spin_unlock_irq(&atchan->lock);
+	txd = &desc->tx_dma_desc;
+	if (txd->flags & DMA_PREP_INTERRUPT)
+		dmaengine_desc_get_callback_invoke(txd, NULL);
 }
 
 static void at_xdmac_handle_error(struct at_xdmac_chan *atchan)
-- 
2.25.1

[PATCH v2 10/13] dmaengine: at_xdmac: Remove a level of indentation in at_xdmac_advance_work()

[Tudor Ambarus]

It's easier to read code with fewer levels of indentation.

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

diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
index 81f6f1357dcb..89d0fc229d68 100644
--- a/drivers/dma/at_xdmac.c
+++ b/drivers/dma/at_xdmac.c
@@ -1593,14 +1593,14 @@ static void at_xdmac_advance_work(struct at_xdmac_chan *atchan)
 	 * If channel is enabled, do nothing, advance_work will be triggered
 	 * after the interruption.
 	 */
-	if (!at_xdmac_chan_is_enabled(atchan) && !list_empty(&atchan->xfers_list)) {
-		desc = list_first_entry(&atchan->xfers_list,
-					struct at_xdmac_desc,
-					xfer_node);
-		dev_vdbg(chan2dev(&atchan->chan), "%s: desc 0x%p\n", __func__, desc);
-		if (!desc->active_xfer)
-			at_xdmac_start_xfer(atchan, desc);
-	}
+	if (at_xdmac_chan_is_enabled(atchan) || list_empty(&atchan->xfers_list))
+		return;
+
+	desc = list_first_entry(&atchan->xfers_list, struct at_xdmac_desc,
+				xfer_node);
+	dev_vdbg(chan2dev(&atchan->chan), "%s: desc 0x%p\n", __func__, desc);
+	if (!desc->active_xfer)
+		at_xdmac_start_xfer(atchan, desc);
 }
 
 static void at_xdmac_handle_cyclic(struct at_xdmac_chan *atchan)
-- 
2.25.1

[PATCH v2 11/13] dmaengine: at_xdmac: Fix lld view setting

[Tudor Ambarus]

AT_XDMAC_CNDC_NDVIEW_NDV3 was set even for AT_XDMAC_MBR_UBC_NDV2,
because of the wrong bit handling. Fix it.

Fixes: ee0fe35c8dcd ("dmaengine: xdmac: Handle descriptor's view 3 registers")
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/dma/at_xdmac.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
index 89d0fc229d68..ba2fe383fa5e 100644
--- a/drivers/dma/at_xdmac.c
+++ b/drivers/dma/at_xdmac.c
@@ -99,6 +99,7 @@
 #define		AT_XDMAC_CNDC_NDE		(0x1 << 0)		/* Channel x Next Descriptor Enable */
 #define		AT_XDMAC_CNDC_NDSUP		(0x1 << 1)		/* Channel x Next Descriptor Source Update */
 #define		AT_XDMAC_CNDC_NDDUP		(0x1 << 2)		/* Channel x Next Descriptor Destination Update */
+#define		AT_XDMAC_CNDC_NDVIEW_MASK	GENMASK(28, 27)
 #define		AT_XDMAC_CNDC_NDVIEW_NDV0	(0x0 << 3)		/* Channel x Next Descriptor View 0 */
 #define		AT_XDMAC_CNDC_NDVIEW_NDV1	(0x1 << 3)		/* Channel x Next Descriptor View 1 */
 #define		AT_XDMAC_CNDC_NDVIEW_NDV2	(0x2 << 3)		/* Channel x Next Descriptor View 2 */
@@ -402,7 +403,8 @@ static void at_xdmac_start_xfer(struct at_xdmac_chan *atchan,
 	 */
 	if (at_xdmac_chan_is_cyclic(atchan))
 		reg = AT_XDMAC_CNDC_NDVIEW_NDV1;
-	else if (first->lld.mbr_ubc & AT_XDMAC_MBR_UBC_NDV3)
+	else if ((first->lld.mbr_ubc &
+		  AT_XDMAC_CNDC_NDVIEW_MASK) == AT_XDMAC_MBR_UBC_NDV3)
 		reg = AT_XDMAC_CNDC_NDVIEW_NDV3;
 	else
 		reg = AT_XDMAC_CNDC_NDVIEW_NDV2;
-- 
2.25.1

[PATCH v2 12/13] dmaengine: at_xdmac: Fix at_xdmac_lld struct definition

[Tudor Ambarus]

The hardware channel next descriptor view structure contains just
fields of 32 bits, while dma_addr_t can be of type u64 or u32
depending on CONFIG_ARCH_DMA_ADDR_T_64BIT. Force u32 to comply with
what the hardware expects.

Fixes: e1f7c9eee707 ("dmaengine: at_xdmac: creation of the atmel eXtended DMA Controller driver")
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/dma/at_xdmac.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
index ba2fe383fa5e..ccd6ddb12b83 100644
--- a/drivers/dma/at_xdmac.c
+++ b/drivers/dma/at_xdmac.c
@@ -253,15 +253,15 @@ struct at_xdmac {
 
 /* Linked List Descriptor */
 struct at_xdmac_lld {
-	dma_addr_t	mbr_nda;	/* Next Descriptor Member */
-	u32		mbr_ubc;	/* Microblock Control Member */
-	dma_addr_t	mbr_sa;		/* Source Address Member */
-	dma_addr_t	mbr_da;		/* Destination Address Member */
-	u32		mbr_cfg;	/* Configuration Register */
-	u32		mbr_bc;		/* Block Control Register */
-	u32		mbr_ds;		/* Data Stride Register */
-	u32		mbr_sus;	/* Source Microblock Stride Register */
-	u32		mbr_dus;	/* Destination Microblock Stride Register */
+	u32 mbr_nda;	/* Next Descriptor Member */
+	u32 mbr_ubc;	/* Microblock Control Member */
+	u32 mbr_sa;	/* Source Address Member */
+	u32 mbr_da;	/* Destination Address Member */
+	u32 mbr_cfg;	/* Configuration Register */
+	u32 mbr_bc;	/* Block Control Register */
+	u32 mbr_ds;	/* Data Stride Register */
+	u32 mbr_sus;	/* Source Microblock Stride Register */
+	u32 mbr_dus;	/* Destination Microblock Stride Register */
 };
 
 /* 64-bit alignment needed to update CNDA and CUBC registers in an atomic way. */
-- 
2.25.1

[PATCH v2 13/13] dmaengine: at_xdmac: Fix race over irq_status

[Tudor Ambarus]

Tasklets run with interrupts enabled, so we need to protect
atchan->irq_status with spin_lock_irq() otherwise the tasklet can be
interrupted by the IRQ that modifies irq_status. While at this, rewrite
at_xdmac_tasklet() so that we get rid of a level of indentation.

Fixes: e1f7c9eee707 ("dmaengine: at_xdmac: creation of the atmel eXtended DMA Controller driver")
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/dma/at_xdmac.c | 80 +++++++++++++++++++-----------------------
 1 file changed, 37 insertions(+), 43 deletions(-)

diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
index ccd6ddb12b83..082c18d45188 100644
--- a/drivers/dma/at_xdmac.c
+++ b/drivers/dma/at_xdmac.c
@@ -1623,6 +1623,7 @@ static void at_xdmac_handle_cyclic(struct at_xdmac_chan *atchan)
 		dmaengine_desc_get_callback_invoke(txd, NULL);
 }
 
+/* Called with atchan->lock held. */
 static void at_xdmac_handle_error(struct at_xdmac_chan *atchan)
 {
 	struct at_xdmac		*atxdmac = to_at_xdmac(atchan->chan.device);
@@ -1641,8 +1642,6 @@ static void at_xdmac_handle_error(struct at_xdmac_chan *atchan)
 	if (atchan->irq_status & AT_XDMAC_CIS_ROIS)
 		dev_err(chan2dev(&atchan->chan), "request overflow error!!!");
 
-	spin_lock_irq(&atchan->lock);
-
 	/* Channel must be disabled first as it's not done automatically */
 	at_xdmac_write(atxdmac, AT_XDMAC_GD, atchan->mask);
 	while (at_xdmac_read(atxdmac, AT_XDMAC_GS) & atchan->mask)
@@ -1652,10 +1651,8 @@ static void at_xdmac_handle_error(struct at_xdmac_chan *atchan)
 				    struct at_xdmac_desc,
 				    xfer_node);
 
-	spin_unlock_irq(&atchan->lock);
-
 	/* Print bad descriptor's details if needed */
-	dev_dbg(chan2dev(&atchan->chan),
+	dev_err(chan2dev(&atchan->chan),
 		"%s: lld: mbr_sa=%pad, mbr_da=%pad, mbr_ubc=0x%08x\n",
 		__func__, &bad_desc->lld.mbr_sa, &bad_desc->lld.mbr_da,
 		bad_desc->lld.mbr_ubc);
@@ -1665,55 +1662,52 @@ static void at_xdmac_handle_error(struct at_xdmac_chan *atchan)
 
 static void at_xdmac_tasklet(struct tasklet_struct *t)
 {
+	struct dma_async_tx_descriptor *txd;
 	struct at_xdmac_chan	*atchan = from_tasklet(atchan, t, tasklet);
 	struct at_xdmac_desc	*desc;
 	u32			error_mask;
 
+	if (at_xdmac_chan_is_cyclic(atchan))
+		return at_xdmac_handle_cyclic(atchan);
+
+	error_mask = AT_XDMAC_CIS_RBEIS | AT_XDMAC_CIS_WBEIS |
+		AT_XDMAC_CIS_ROIS;
+
+	spin_lock_irq(&atchan->lock);
 	dev_dbg(chan2dev(&atchan->chan), "%s: status=0x%08x\n",
 		__func__, atchan->irq_status);
+	if (!(atchan->irq_status & AT_XDMAC_CIS_LIS) &&
+	    !(atchan->irq_status & error_mask)) {
+		return spin_unlock_irq(&atchan->lock);
+	}
 
-	error_mask = AT_XDMAC_CIS_RBEIS
-		     | AT_XDMAC_CIS_WBEIS
-		     | AT_XDMAC_CIS_ROIS;
-
-	if (at_xdmac_chan_is_cyclic(atchan)) {
-		at_xdmac_handle_cyclic(atchan);
-	} else if ((atchan->irq_status & AT_XDMAC_CIS_LIS)
-		   || (atchan->irq_status & error_mask)) {
-		struct dma_async_tx_descriptor  *txd;
-
-		if (atchan->irq_status & error_mask)
-			at_xdmac_handle_error(atchan);
-
-		spin_lock_irq(&atchan->lock);
-		desc = list_first_entry(&atchan->xfers_list,
-					struct at_xdmac_desc,
-					xfer_node);
-		dev_vdbg(chan2dev(&atchan->chan), "%s: desc 0x%p\n", __func__, desc);
-		if (!desc->active_xfer) {
-			dev_err(chan2dev(&atchan->chan), "Xfer not active: exiting");
-			spin_unlock_irq(&atchan->lock);
-			return;
-		}
+	if (atchan->irq_status & error_mask)
+		at_xdmac_handle_error(atchan);
 
-		txd = &desc->tx_dma_desc;
-		dma_cookie_complete(txd);
-		/* Remove the transfer from the transfer list. */
-		list_del(&desc->xfer_node);
-		spin_unlock_irq(&atchan->lock);
+	desc = list_first_entry(&atchan->xfers_list, struct at_xdmac_desc,
+				xfer_node);
+	dev_vdbg(chan2dev(&atchan->chan), "%s: desc 0x%p\n", __func__, desc);
+	if (!desc->active_xfer) {
+		dev_err(chan2dev(&atchan->chan), "Xfer not active: exiting");
+		return spin_unlock_irq(&atchan->lock);
+	}
 
-		if (txd->flags & DMA_PREP_INTERRUPT)
-			dmaengine_desc_get_callback_invoke(txd, NULL);
+	txd = &desc->tx_dma_desc;
+	dma_cookie_complete(txd);
+	/* Remove the transfer from the transfer list. */
+	list_del(&desc->xfer_node);
+	spin_unlock_irq(&atchan->lock);
 
-		dma_run_dependencies(txd);
+	if (txd->flags & DMA_PREP_INTERRUPT)
+		dmaengine_desc_get_callback_invoke(txd, NULL);
 
-		spin_lock_irq(&atchan->lock);
-		/* Move the xfer descriptors into the free descriptors list. */
-		list_splice_tail_init(&desc->descs_list,
-				      &atchan->free_descs_list);
-		at_xdmac_advance_work(atchan);
-		spin_unlock_irq(&atchan->lock);
-	}
+	dma_run_dependencies(txd);
+
+	spin_lock_irq(&atchan->lock);
+	/* Move the xfer descriptors into the free descriptors list. */
+	list_splice_tail_init(&desc->descs_list, &atchan->free_descs_list);
+	at_xdmac_advance_work(atchan);
+	spin_unlock_irq(&atchan->lock);
 }
 
 static irqreturn_t at_xdmac_interrupt(int irq, void *dev_id)
-- 
2.25.1