linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] serial: samsung: use dma_ops of DMA if attached
       [not found] <CGME20210622034818epcas5p3837ce2315f5c57980576f10b8fc7efeb@epcas5p3.samsung.com>
@ 2021-06-22  3:52 ` Tamseel Shams
  2021-06-22  5:46   ` Greg KH
  2021-06-22 12:40   ` Robin Murphy
  0 siblings, 2 replies; 5+ messages in thread
From: Tamseel Shams @ 2021-06-22  3:52 UTC (permalink / raw)
  To: krzysztof.kozlowski, gregkh, jirislaby
  Cc: linux-arm-kernel, linux-samsung-soc, linux-serial, linux-kernel,
	alim.akhtar, ajaykumar.rs, Tamseel Shams

When DMA is used for TX and RX by serial driver, it should
pass the DMA device pointer to DMA API instead of UART device
pointer.

This patch is necessary to fix the SMMU page faults
which is observed when a DMA(with SMMU enabled) is attached
to UART for transfer.

Signed-off-by: Tamseel Shams <m.shams@samsung.com>
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/tty/serial/samsung_tty.c | 62 +++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 9fbc61151c2e..0c924bb6108e 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -284,8 +284,13 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port)
 	struct s3c24xx_uart_dma *dma = ourport->dma;
 	struct circ_buf *xmit = &port->state->xmit;
 	struct dma_tx_state state;
+	struct device *dma_map_ops_dev = ourport->port.dev;
 	int count;
 
+	/* Pick dma_ops of DMA device if DMA device is attached */
+	if (dma && dma->tx_chan)
+		dma_map_ops_dev = dma->tx_chan->device->dev;
+
 	if (!ourport->tx_enabled)
 		return;
 
@@ -305,7 +310,7 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port)
 		dmaengine_pause(dma->tx_chan);
 		dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
 		dmaengine_terminate_all(dma->tx_chan);
-		dma_sync_single_for_cpu(ourport->port.dev,
+		dma_sync_single_for_cpu(dma_map_ops_dev,
 			dma->tx_transfer_addr, dma->tx_size, DMA_TO_DEVICE);
 		async_tx_ack(dma->tx_desc);
 		count = dma->tx_bytes_requested - state.residue;
@@ -331,14 +336,19 @@ static void s3c24xx_serial_tx_dma_complete(void *args)
 	struct circ_buf *xmit = &port->state->xmit;
 	struct s3c24xx_uart_dma *dma = ourport->dma;
 	struct dma_tx_state state;
+	struct device *dma_map_ops_dev = ourport->port.dev;
 	unsigned long flags;
 	int count;
 
+	/* Pick dma_ops of DMA device if DMA device is attached */
+	if (dma && dma->tx_chan)
+		dma_map_ops_dev = dma->tx_chan->device->dev;
+
 	dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
 	count = dma->tx_bytes_requested - state.residue;
 	async_tx_ack(dma->tx_desc);
 
-	dma_sync_single_for_cpu(ourport->port.dev, dma->tx_transfer_addr,
+	dma_sync_single_for_cpu(dma_map_ops_dev, dma->tx_transfer_addr,
 				dma->tx_size, DMA_TO_DEVICE);
 
 	spin_lock_irqsave(&port->lock, flags);
@@ -436,6 +446,11 @@ static int s3c24xx_serial_start_tx_dma(struct s3c24xx_uart_port *ourport,
 	struct uart_port *port = &ourport->port;
 	struct circ_buf *xmit = &port->state->xmit;
 	struct s3c24xx_uart_dma *dma = ourport->dma;
+	struct device *dma_map_ops_dev = ourport->port.dev;
+
+	/* Pick dma_ops of DMA device if DMA device is attached */
+	if (dma && dma->tx_chan)
+		dma_map_ops_dev = dma->tx_chan->device->dev;
 
 	if (ourport->tx_mode != S3C24XX_TX_DMA)
 		enable_tx_dma(ourport);
@@ -443,7 +458,7 @@ static int s3c24xx_serial_start_tx_dma(struct s3c24xx_uart_port *ourport,
 	dma->tx_size = count & ~(dma_get_cache_alignment() - 1);
 	dma->tx_transfer_addr = dma->tx_addr + xmit->tail;
 
-	dma_sync_single_for_device(ourport->port.dev, dma->tx_transfer_addr,
+	dma_sync_single_for_device(dma_map_ops_dev, dma->tx_transfer_addr,
 				dma->tx_size, DMA_TO_DEVICE);
 
 	dma->tx_desc = dmaengine_prep_slave_single(dma->tx_chan,
@@ -510,12 +525,17 @@ static void s3c24xx_uart_copy_rx_to_tty(struct s3c24xx_uart_port *ourport,
 		struct tty_port *tty, int count)
 {
 	struct s3c24xx_uart_dma *dma = ourport->dma;
+	struct device *dma_map_ops_dev = ourport->port.dev;
 	int copied;
 
+	/* Pick dma_ops of DMA device if DMA device is attached */
+	if (dma && dma->rx_chan)
+		dma_map_ops_dev = dma->rx_chan->device->dev;
+
 	if (!count)
 		return;
 
-	dma_sync_single_for_cpu(ourport->port.dev, dma->rx_addr,
+	dma_sync_single_for_cpu(dma_map_ops_dev, dma->rx_addr,
 				dma->rx_size, DMA_FROM_DEVICE);
 
 	ourport->port.icount.rx += count;
@@ -635,8 +655,13 @@ static void s3c24xx_serial_rx_dma_complete(void *args)
 static void s3c64xx_start_rx_dma(struct s3c24xx_uart_port *ourport)
 {
 	struct s3c24xx_uart_dma *dma = ourport->dma;
+	struct device *dma_map_ops_dev = ourport->port.dev;
+
+	/* Pick dma_ops of DMA device if DMA device is attached */
+	if (dma && dma->rx_chan)
+		dma_map_ops_dev = dma->rx_chan->device->dev;
 
-	dma_sync_single_for_device(ourport->port.dev, dma->rx_addr,
+	dma_sync_single_for_device(dma_map_ops_dev, dma->rx_addr,
 				dma->rx_size, DMA_FROM_DEVICE);
 
 	dma->rx_desc = dmaengine_prep_slave_single(dma->rx_chan,
@@ -1045,6 +1070,7 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
 	struct s3c24xx_uart_dma	*dma = p->dma;
 	struct dma_slave_caps dma_caps;
 	const char *reason = NULL;
+	struct device *dma_map_ops_dev = p->port.dev;
 	int ret;
 
 	/* Default slave configuration parameters */
@@ -1102,18 +1128,25 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
 		goto err_release_tx;
 	}
 
-	dma->rx_addr = dma_map_single(p->port.dev, dma->rx_buf,
+	/* Pick dma_ops of DMA device if DMA device is attached */
+	if (dma && dma->rx_chan)
+		dma_map_ops_dev = dma->rx_chan->device->dev;
+
+	dma->rx_addr = dma_map_single(dma_map_ops_dev, dma->rx_buf,
 				dma->rx_size, DMA_FROM_DEVICE);
-	if (dma_mapping_error(p->port.dev, dma->rx_addr)) {
+	if (dma_mapping_error(dma_map_ops_dev, dma->rx_addr)) {
 		reason = "DMA mapping error for RX buffer";
 		ret = -EIO;
 		goto err_free_rx;
 	}
 
+	/* Pick dma_ops of DMA device if DMA device is attached */
+	if (dma && dma->tx_chan)
+		dma_map_ops_dev = dma->tx_chan->device->dev;
 	/* TX buffer */
-	dma->tx_addr = dma_map_single(p->port.dev, p->port.state->xmit.buf,
+	dma->tx_addr = dma_map_single(dma_map_ops_dev, p->port.state->xmit.buf,
 				UART_XMIT_SIZE, DMA_TO_DEVICE);
-	if (dma_mapping_error(p->port.dev, dma->tx_addr)) {
+	if (dma_mapping_error(dma_map_ops_dev, dma->tx_addr)) {
 		reason = "DMA mapping error for TX buffer";
 		ret = -EIO;
 		goto err_unmap_rx;
@@ -1122,7 +1155,9 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
 	return 0;
 
 err_unmap_rx:
-	dma_unmap_single(p->port.dev, dma->rx_addr, dma->rx_size,
+	if (dma->rx_chan)
+		dma_map_ops_dev = dma->rx_chan->device->dev;
+	dma_unmap_single(dma_map_ops_dev, dma->rx_addr, dma->rx_size,
 			 DMA_FROM_DEVICE);
 err_free_rx:
 	kfree(dma->rx_buf);
@@ -1139,10 +1174,12 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
 static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
 {
 	struct s3c24xx_uart_dma	*dma = p->dma;
+	struct device *dma_map_ops_dev = p->port.dev;
 
 	if (dma->rx_chan) {
+		dma_map_ops_dev = dma->rx_chan->device->dev;
 		dmaengine_terminate_all(dma->rx_chan);
-		dma_unmap_single(p->port.dev, dma->rx_addr,
+		dma_unmap_single(dma_map_ops_dev, dma->rx_addr,
 				dma->rx_size, DMA_FROM_DEVICE);
 		kfree(dma->rx_buf);
 		dma_release_channel(dma->rx_chan);
@@ -1150,8 +1187,9 @@ static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
 	}
 
 	if (dma->tx_chan) {
+		dma_map_ops_dev = dma->tx_chan->device->dev;
 		dmaengine_terminate_all(dma->tx_chan);
-		dma_unmap_single(p->port.dev, dma->tx_addr,
+		dma_unmap_single(dma_map_ops_dev, dma->tx_addr,
 				UART_XMIT_SIZE, DMA_TO_DEVICE);
 		dma_release_channel(dma->tx_chan);
 		dma->tx_chan = NULL;
-- 
2.17.1


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

* Re: [PATCH v2] serial: samsung: use dma_ops of DMA if attached
  2021-06-22  3:52 ` [PATCH v2] serial: samsung: use dma_ops of DMA if attached Tamseel Shams
@ 2021-06-22  5:46   ` Greg KH
  2021-06-22  6:37     ` M Tamseel Shams
  2021-06-22 12:40   ` Robin Murphy
  1 sibling, 1 reply; 5+ messages in thread
From: Greg KH @ 2021-06-22  5:46 UTC (permalink / raw)
  To: Tamseel Shams
  Cc: krzysztof.kozlowski, jirislaby, linux-arm-kernel,
	linux-samsung-soc, linux-serial, linux-kernel, alim.akhtar,
	ajaykumar.rs

On Tue, Jun 22, 2021 at 09:22:02AM +0530, Tamseel Shams wrote:
> When DMA is used for TX and RX by serial driver, it should
> pass the DMA device pointer to DMA API instead of UART device
> pointer.
> 
> This patch is necessary to fix the SMMU page faults
> which is observed when a DMA(with SMMU enabled) is attached
> to UART for transfer.
> 
> Signed-off-by: Tamseel Shams <m.shams@samsung.com>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
>  drivers/tty/serial/samsung_tty.c | 62 +++++++++++++++++++++++++-------
>  1 file changed, 50 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 9fbc61151c2e..0c924bb6108e 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -284,8 +284,13 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port)
>  	struct s3c24xx_uart_dma *dma = ourport->dma;
>  	struct circ_buf *xmit = &port->state->xmit;
>  	struct dma_tx_state state;
> +	struct device *dma_map_ops_dev = ourport->port.dev;
>  	int count;
>  
> +	/* Pick dma_ops of DMA device if DMA device is attached */
> +	if (dma && dma->tx_chan)
> +		dma_map_ops_dev = dma->tx_chan->device->dev;
> +
>  	if (!ourport->tx_enabled)
>  		return;
>  
> @@ -305,7 +310,7 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port)
>  		dmaengine_pause(dma->tx_chan);
>  		dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
>  		dmaengine_terminate_all(dma->tx_chan);
> -		dma_sync_single_for_cpu(ourport->port.dev,
> +		dma_sync_single_for_cpu(dma_map_ops_dev,
>  			dma->tx_transfer_addr, dma->tx_size, DMA_TO_DEVICE);
>  		async_tx_ack(dma->tx_desc);
>  		count = dma->tx_bytes_requested - state.residue;
> @@ -331,14 +336,19 @@ static void s3c24xx_serial_tx_dma_complete(void *args)
>  	struct circ_buf *xmit = &port->state->xmit;
>  	struct s3c24xx_uart_dma *dma = ourport->dma;
>  	struct dma_tx_state state;
> +	struct device *dma_map_ops_dev = ourport->port.dev;
>  	unsigned long flags;
>  	int count;
>  
> +	/* Pick dma_ops of DMA device if DMA device is attached */
> +	if (dma && dma->tx_chan)
> +		dma_map_ops_dev = dma->tx_chan->device->dev;
> +
>  	dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
>  	count = dma->tx_bytes_requested - state.residue;
>  	async_tx_ack(dma->tx_desc);
>  
> -	dma_sync_single_for_cpu(ourport->port.dev, dma->tx_transfer_addr,
> +	dma_sync_single_for_cpu(dma_map_ops_dev, dma->tx_transfer_addr,
>  				dma->tx_size, DMA_TO_DEVICE);
>  
>  	spin_lock_irqsave(&port->lock, flags);
> @@ -436,6 +446,11 @@ static int s3c24xx_serial_start_tx_dma(struct s3c24xx_uart_port *ourport,
>  	struct uart_port *port = &ourport->port;
>  	struct circ_buf *xmit = &port->state->xmit;
>  	struct s3c24xx_uart_dma *dma = ourport->dma;
> +	struct device *dma_map_ops_dev = ourport->port.dev;
> +
> +	/* Pick dma_ops of DMA device if DMA device is attached */
> +	if (dma && dma->tx_chan)
> +		dma_map_ops_dev = dma->tx_chan->device->dev;
>  
>  	if (ourport->tx_mode != S3C24XX_TX_DMA)
>  		enable_tx_dma(ourport);
> @@ -443,7 +458,7 @@ static int s3c24xx_serial_start_tx_dma(struct s3c24xx_uart_port *ourport,
>  	dma->tx_size = count & ~(dma_get_cache_alignment() - 1);
>  	dma->tx_transfer_addr = dma->tx_addr + xmit->tail;
>  
> -	dma_sync_single_for_device(ourport->port.dev, dma->tx_transfer_addr,
> +	dma_sync_single_for_device(dma_map_ops_dev, dma->tx_transfer_addr,
>  				dma->tx_size, DMA_TO_DEVICE);
>  
>  	dma->tx_desc = dmaengine_prep_slave_single(dma->tx_chan,
> @@ -510,12 +525,17 @@ static void s3c24xx_uart_copy_rx_to_tty(struct s3c24xx_uart_port *ourport,
>  		struct tty_port *tty, int count)
>  {
>  	struct s3c24xx_uart_dma *dma = ourport->dma;
> +	struct device *dma_map_ops_dev = ourport->port.dev;
>  	int copied;
>  
> +	/* Pick dma_ops of DMA device if DMA device is attached */
> +	if (dma && dma->rx_chan)
> +		dma_map_ops_dev = dma->rx_chan->device->dev;
> +
>  	if (!count)
>  		return;
>  
> -	dma_sync_single_for_cpu(ourport->port.dev, dma->rx_addr,
> +	dma_sync_single_for_cpu(dma_map_ops_dev, dma->rx_addr,
>  				dma->rx_size, DMA_FROM_DEVICE);
>  
>  	ourport->port.icount.rx += count;
> @@ -635,8 +655,13 @@ static void s3c24xx_serial_rx_dma_complete(void *args)
>  static void s3c64xx_start_rx_dma(struct s3c24xx_uart_port *ourport)
>  {
>  	struct s3c24xx_uart_dma *dma = ourport->dma;
> +	struct device *dma_map_ops_dev = ourport->port.dev;
> +
> +	/* Pick dma_ops of DMA device if DMA device is attached */
> +	if (dma && dma->rx_chan)
> +		dma_map_ops_dev = dma->rx_chan->device->dev;
>  
> -	dma_sync_single_for_device(ourport->port.dev, dma->rx_addr,
> +	dma_sync_single_for_device(dma_map_ops_dev, dma->rx_addr,
>  				dma->rx_size, DMA_FROM_DEVICE);
>  
>  	dma->rx_desc = dmaengine_prep_slave_single(dma->rx_chan,
> @@ -1045,6 +1070,7 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
>  	struct s3c24xx_uart_dma	*dma = p->dma;
>  	struct dma_slave_caps dma_caps;
>  	const char *reason = NULL;
> +	struct device *dma_map_ops_dev = p->port.dev;
>  	int ret;
>  
>  	/* Default slave configuration parameters */
> @@ -1102,18 +1128,25 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
>  		goto err_release_tx;
>  	}
>  
> -	dma->rx_addr = dma_map_single(p->port.dev, dma->rx_buf,
> +	/* Pick dma_ops of DMA device if DMA device is attached */
> +	if (dma && dma->rx_chan)
> +		dma_map_ops_dev = dma->rx_chan->device->dev;
> +
> +	dma->rx_addr = dma_map_single(dma_map_ops_dev, dma->rx_buf,
>  				dma->rx_size, DMA_FROM_DEVICE);
> -	if (dma_mapping_error(p->port.dev, dma->rx_addr)) {
> +	if (dma_mapping_error(dma_map_ops_dev, dma->rx_addr)) {
>  		reason = "DMA mapping error for RX buffer";
>  		ret = -EIO;
>  		goto err_free_rx;
>  	}
>  
> +	/* Pick dma_ops of DMA device if DMA device is attached */
> +	if (dma && dma->tx_chan)
> +		dma_map_ops_dev = dma->tx_chan->device->dev;
>  	/* TX buffer */
> -	dma->tx_addr = dma_map_single(p->port.dev, p->port.state->xmit.buf,
> +	dma->tx_addr = dma_map_single(dma_map_ops_dev, p->port.state->xmit.buf,
>  				UART_XMIT_SIZE, DMA_TO_DEVICE);
> -	if (dma_mapping_error(p->port.dev, dma->tx_addr)) {
> +	if (dma_mapping_error(dma_map_ops_dev, dma->tx_addr)) {
>  		reason = "DMA mapping error for TX buffer";
>  		ret = -EIO;
>  		goto err_unmap_rx;
> @@ -1122,7 +1155,9 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
>  	return 0;
>  
>  err_unmap_rx:
> -	dma_unmap_single(p->port.dev, dma->rx_addr, dma->rx_size,
> +	if (dma->rx_chan)
> +		dma_map_ops_dev = dma->rx_chan->device->dev;
> +	dma_unmap_single(dma_map_ops_dev, dma->rx_addr, dma->rx_size,
>  			 DMA_FROM_DEVICE);
>  err_free_rx:
>  	kfree(dma->rx_buf);
> @@ -1139,10 +1174,12 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
>  static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
>  {
>  	struct s3c24xx_uart_dma	*dma = p->dma;
> +	struct device *dma_map_ops_dev = p->port.dev;
>  
>  	if (dma->rx_chan) {
> +		dma_map_ops_dev = dma->rx_chan->device->dev;
>  		dmaengine_terminate_all(dma->rx_chan);
> -		dma_unmap_single(p->port.dev, dma->rx_addr,
> +		dma_unmap_single(dma_map_ops_dev, dma->rx_addr,
>  				dma->rx_size, DMA_FROM_DEVICE);
>  		kfree(dma->rx_buf);
>  		dma_release_channel(dma->rx_chan);
> @@ -1150,8 +1187,9 @@ static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
>  	}
>  
>  	if (dma->tx_chan) {
> +		dma_map_ops_dev = dma->tx_chan->device->dev;
>  		dmaengine_terminate_all(dma->tx_chan);
> -		dma_unmap_single(p->port.dev, dma->tx_addr,
> +		dma_unmap_single(dma_map_ops_dev, dma->tx_addr,
>  				UART_XMIT_SIZE, DMA_TO_DEVICE);
>  		dma_release_channel(dma->tx_chan);
>  		dma->tx_chan = NULL;
> -- 
> 2.17.1
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/SubmittingPatches for what needs to be done
  here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

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

* RE: [PATCH v2] serial: samsung: use dma_ops of DMA if attached
  2021-06-22  5:46   ` Greg KH
@ 2021-06-22  6:37     ` M Tamseel Shams
  0 siblings, 0 replies; 5+ messages in thread
From: M Tamseel Shams @ 2021-06-22  6:37 UTC (permalink / raw)
  To: 'Greg KH'
  Cc: krzysztof.kozlowski, jirislaby, linux-arm-kernel,
	linux-samsung-soc, linux-serial, linux-kernel, alim.akhtar,
	ajaykumar.rs


> > When DMA is used for TX and RX by serial driver, it should pass the
> > DMA device pointer to DMA API instead of UART device pointer.
> >
> > This patch is necessary to fix the SMMU page faults which is observed
> > when a DMA(with SMMU enabled) is attached to UART for transfer.
> >
> > Signed-off-by: Tamseel Shams <m.shams@samsung.com>
> > Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> > ---
> >  drivers/tty/serial/samsung_tty.c | 62
> > +++++++++++++++++++++++++-------
> >  1 file changed, 50 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/tty/serial/samsung_tty.c
> > b/drivers/tty/serial/samsung_tty.c
> > index 9fbc61151c2e..0c924bb6108e 100644
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> > @@ -284,8 +284,13 @@ static void s3c24xx_serial_stop_tx(struct uart_port
> *port)
> >  	struct s3c24xx_uart_dma *dma = ourport->dma;
> >  	struct circ_buf *xmit = &port->state->xmit;
> >  	struct dma_tx_state state;
> > +	struct device *dma_map_ops_dev = ourport->port.dev;
> >  	int count;
> >
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->tx_chan)
> > +		dma_map_ops_dev = dma->tx_chan->device->dev;
> > +
> >  	if (!ourport->tx_enabled)
> >  		return;
> >
> > @@ -305,7 +310,7 @@ static void s3c24xx_serial_stop_tx(struct uart_port
> *port)
> >  		dmaengine_pause(dma->tx_chan);
> >  		dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
> >  		dmaengine_terminate_all(dma->tx_chan);
> > -		dma_sync_single_for_cpu(ourport->port.dev,
> > +		dma_sync_single_for_cpu(dma_map_ops_dev,
> >  			dma->tx_transfer_addr, dma->tx_size,
> DMA_TO_DEVICE);
> >  		async_tx_ack(dma->tx_desc);
> >  		count = dma->tx_bytes_requested - state.residue; @@ -331,14
> +336,19
> > @@ static void s3c24xx_serial_tx_dma_complete(void *args)
> >  	struct circ_buf *xmit = &port->state->xmit;
> >  	struct s3c24xx_uart_dma *dma = ourport->dma;
> >  	struct dma_tx_state state;
> > +	struct device *dma_map_ops_dev = ourport->port.dev;
> >  	unsigned long flags;
> >  	int count;
> >
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->tx_chan)
> > +		dma_map_ops_dev = dma->tx_chan->device->dev;
> > +
> >  	dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
> >  	count = dma->tx_bytes_requested - state.residue;
> >  	async_tx_ack(dma->tx_desc);
> >
> > -	dma_sync_single_for_cpu(ourport->port.dev, dma->tx_transfer_addr,
> > +	dma_sync_single_for_cpu(dma_map_ops_dev, dma->tx_transfer_addr,
> >  				dma->tx_size, DMA_TO_DEVICE);
> >
> >  	spin_lock_irqsave(&port->lock, flags); @@ -436,6 +446,11 @@ static
> > int s3c24xx_serial_start_tx_dma(struct s3c24xx_uart_port *ourport,
> >  	struct uart_port *port = &ourport->port;
> >  	struct circ_buf *xmit = &port->state->xmit;
> >  	struct s3c24xx_uart_dma *dma = ourport->dma;
> > +	struct device *dma_map_ops_dev = ourport->port.dev;
> > +
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->tx_chan)
> > +		dma_map_ops_dev = dma->tx_chan->device->dev;
> >
> >  	if (ourport->tx_mode != S3C24XX_TX_DMA)
> >  		enable_tx_dma(ourport);
> > @@ -443,7 +458,7 @@ static int s3c24xx_serial_start_tx_dma(struct
> s3c24xx_uart_port *ourport,
> >  	dma->tx_size = count & ~(dma_get_cache_alignment() - 1);
> >  	dma->tx_transfer_addr = dma->tx_addr + xmit->tail;
> >
> > -	dma_sync_single_for_device(ourport->port.dev, dma-
> >tx_transfer_addr,
> > +	dma_sync_single_for_device(dma_map_ops_dev, dma-
> >tx_transfer_addr,
> >  				dma->tx_size, DMA_TO_DEVICE);
> >
> >  	dma->tx_desc = dmaengine_prep_slave_single(dma->tx_chan,
> > @@ -510,12 +525,17 @@ static void s3c24xx_uart_copy_rx_to_tty(struct
> s3c24xx_uart_port *ourport,
> >  		struct tty_port *tty, int count)
> >  {
> >  	struct s3c24xx_uart_dma *dma = ourport->dma;
> > +	struct device *dma_map_ops_dev = ourport->port.dev;
> >  	int copied;
> >
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->rx_chan)
> > +		dma_map_ops_dev = dma->rx_chan->device->dev;
> > +
> >  	if (!count)
> >  		return;
> >
> > -	dma_sync_single_for_cpu(ourport->port.dev, dma->rx_addr,
> > +	dma_sync_single_for_cpu(dma_map_ops_dev, dma->rx_addr,
> >  				dma->rx_size, DMA_FROM_DEVICE);
> >
> >  	ourport->port.icount.rx += count;
> > @@ -635,8 +655,13 @@ static void s3c24xx_serial_rx_dma_complete(void
> > *args)  static void s3c64xx_start_rx_dma(struct s3c24xx_uart_port
> > *ourport)  {
> >  	struct s3c24xx_uart_dma *dma = ourport->dma;
> > +	struct device *dma_map_ops_dev = ourport->port.dev;
> > +
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->rx_chan)
> > +		dma_map_ops_dev = dma->rx_chan->device->dev;
> >
> > -	dma_sync_single_for_device(ourport->port.dev, dma->rx_addr,
> > +	dma_sync_single_for_device(dma_map_ops_dev, dma->rx_addr,
> >  				dma->rx_size, DMA_FROM_DEVICE);
> >
> >  	dma->rx_desc = dmaengine_prep_slave_single(dma->rx_chan,
> > @@ -1045,6 +1070,7 @@ static int s3c24xx_serial_request_dma(struct
> s3c24xx_uart_port *p)
> >  	struct s3c24xx_uart_dma	*dma = p->dma;
> >  	struct dma_slave_caps dma_caps;
> >  	const char *reason = NULL;
> > +	struct device *dma_map_ops_dev = p->port.dev;
> >  	int ret;
> >
> >  	/* Default slave configuration parameters */ @@ -1102,18 +1128,25
> @@
> > static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
> >  		goto err_release_tx;
> >  	}
> >
> > -	dma->rx_addr = dma_map_single(p->port.dev, dma->rx_buf,
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->rx_chan)
> > +		dma_map_ops_dev = dma->rx_chan->device->dev;
> > +
> > +	dma->rx_addr = dma_map_single(dma_map_ops_dev, dma->rx_buf,
> >  				dma->rx_size, DMA_FROM_DEVICE);
> > -	if (dma_mapping_error(p->port.dev, dma->rx_addr)) {
> > +	if (dma_mapping_error(dma_map_ops_dev, dma->rx_addr)) {
> >  		reason = "DMA mapping error for RX buffer";
> >  		ret = -EIO;
> >  		goto err_free_rx;
> >  	}
> >
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->tx_chan)
> > +		dma_map_ops_dev = dma->tx_chan->device->dev;
> >  	/* TX buffer */
> > -	dma->tx_addr = dma_map_single(p->port.dev, p->port.state->xmit.buf,
> > +	dma->tx_addr = dma_map_single(dma_map_ops_dev,
> > +p->port.state->xmit.buf,
> >  				UART_XMIT_SIZE, DMA_TO_DEVICE);
> > -	if (dma_mapping_error(p->port.dev, dma->tx_addr)) {
> > +	if (dma_mapping_error(dma_map_ops_dev, dma->tx_addr)) {
> >  		reason = "DMA mapping error for TX buffer";
> >  		ret = -EIO;
> >  		goto err_unmap_rx;
> > @@ -1122,7 +1155,9 @@ static int s3c24xx_serial_request_dma(struct
> s3c24xx_uart_port *p)
> >  	return 0;
> >
> >  err_unmap_rx:
> > -	dma_unmap_single(p->port.dev, dma->rx_addr, dma->rx_size,
> > +	if (dma->rx_chan)
> > +		dma_map_ops_dev = dma->rx_chan->device->dev;
> > +	dma_unmap_single(dma_map_ops_dev, dma->rx_addr, dma->rx_size,
> >  			 DMA_FROM_DEVICE);
> >  err_free_rx:
> >  	kfree(dma->rx_buf);
> > @@ -1139,10 +1174,12 @@ static int s3c24xx_serial_request_dma(struct
> > s3c24xx_uart_port *p)  static void s3c24xx_serial_release_dma(struct
> > s3c24xx_uart_port *p)  {
> >  	struct s3c24xx_uart_dma	*dma = p->dma;
> > +	struct device *dma_map_ops_dev = p->port.dev;
> >
> >  	if (dma->rx_chan) {
> > +		dma_map_ops_dev = dma->rx_chan->device->dev;
> >  		dmaengine_terminate_all(dma->rx_chan);
> > -		dma_unmap_single(p->port.dev, dma->rx_addr,
> > +		dma_unmap_single(dma_map_ops_dev, dma->rx_addr,
> >  				dma->rx_size, DMA_FROM_DEVICE);
> >  		kfree(dma->rx_buf);
> >  		dma_release_channel(dma->rx_chan);
> > @@ -1150,8 +1187,9 @@ static void s3c24xx_serial_release_dma(struct
> s3c24xx_uart_port *p)
> >  	}
> >
> >  	if (dma->tx_chan) {
> > +		dma_map_ops_dev = dma->tx_chan->device->dev;
> >  		dmaengine_terminate_all(dma->tx_chan);
> > -		dma_unmap_single(p->port.dev, dma->tx_addr,
> > +		dma_unmap_single(dma_map_ops_dev, dma->tx_addr,
> >  				UART_XMIT_SIZE, DMA_TO_DEVICE);
> >  		dma_release_channel(dma->tx_chan);
> >  		dma->tx_chan = NULL;
> > --
> > 2.17.1
> >
> 
> Hi,
> 
> This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
> a patch that has triggered this response.  He used to manually respond
> to these common problems, but in order to save his sanity (he kept
> writing the same thing over and over, yet to different people), I was
> created.  Hopefully you will not take offence and will fix the problem
> in your patch and resubmit it so that it can be accepted into the Linux
> kernel tree.
> 
> You are receiving this message because of the following common error(s)
> as indicated below:
> 
> - This looks like a new version of a previously submitted patch, but you
>   did not list below the --- line any changes from the previous version.
>   Please read the section entitled "The canonical patch format" in the
>   kernel file, Documentation/SubmittingPatches for what needs to be done
>   here to properly describe this.
> 
> If you wish to discuss this problem further, or you have questions about
> how to resolve this issue, please feel free to respond to this email and
> Greg will reply once he has dug out from the pending patches received
> from other developers.
> 
> thanks,
> 
> greg k-h's patch email bot

Hi Greg,
Sorry for not mentioning the change from previous version.
I will update the patch and post the new version.

Thanks & Regards,
Tamseel Shams


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

* Re: [PATCH v2] serial: samsung: use dma_ops of DMA if attached
  2021-06-22  3:52 ` [PATCH v2] serial: samsung: use dma_ops of DMA if attached Tamseel Shams
  2021-06-22  5:46   ` Greg KH
@ 2021-06-22 12:40   ` Robin Murphy
  2021-06-23  4:19     ` M Tamseel Shams
  1 sibling, 1 reply; 5+ messages in thread
From: Robin Murphy @ 2021-06-22 12:40 UTC (permalink / raw)
  To: Tamseel Shams, krzysztof.kozlowski, gregkh, jirislaby
  Cc: linux-arm-kernel, linux-samsung-soc, linux-serial, linux-kernel,
	alim.akhtar, ajaykumar.rs

On 2021-06-22 04:52, Tamseel Shams wrote:
> When DMA is used for TX and RX by serial driver, it should
> pass the DMA device pointer to DMA API instead of UART device
> pointer.
> 
> This patch is necessary to fix the SMMU page faults
> which is observed when a DMA(with SMMU enabled) is attached
> to UART for transfer.
> 
> Signed-off-by: Tamseel Shams <m.shams@samsung.com>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
>   drivers/tty/serial/samsung_tty.c | 62 +++++++++++++++++++++++++-------
>   1 file changed, 50 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 9fbc61151c2e..0c924bb6108e 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -284,8 +284,13 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port)
>   	struct s3c24xx_uart_dma *dma = ourport->dma;
>   	struct circ_buf *xmit = &port->state->xmit;
>   	struct dma_tx_state state;
> +	struct device *dma_map_ops_dev = ourport->port.dev;
>   	int count;
>   
> +	/* Pick dma_ops of DMA device if DMA device is attached */
> +	if (dma && dma->tx_chan)
> +		dma_map_ops_dev = dma->tx_chan->device->dev;
> +
>   	if (!ourport->tx_enabled)
>   		return;
>   
> @@ -305,7 +310,7 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port)
>   		dmaengine_pause(dma->tx_chan);
>   		dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
>   		dmaengine_terminate_all(dma->tx_chan);
> -		dma_sync_single_for_cpu(ourport->port.dev,
> +		dma_sync_single_for_cpu(dma_map_ops_dev,
>   			dma->tx_transfer_addr, dma->tx_size, DMA_TO_DEVICE);
>   		async_tx_ack(dma->tx_desc);
>   		count = dma->tx_bytes_requested - state.residue;
> @@ -331,14 +336,19 @@ static void s3c24xx_serial_tx_dma_complete(void *args)
>   	struct circ_buf *xmit = &port->state->xmit;
>   	struct s3c24xx_uart_dma *dma = ourport->dma;
>   	struct dma_tx_state state;
> +	struct device *dma_map_ops_dev = ourport->port.dev;
>   	unsigned long flags;
>   	int count;
>   
> +	/* Pick dma_ops of DMA device if DMA device is attached */
> +	if (dma && dma->tx_chan)

Clearly you don't need most of these checks, if any. For example here 
you're already dereferencing dma->tx_chan unconditionally a couple of 
lines below.

AFAICS it should simply be a case of hard-coding the correct device for 
all of the DMA API calls without any of this silly conditional 
assignment. If it's even possible to reach the point of making a DMA API 
call when you don't have a valid DMA channel (and thus it would be 
pointless anyway), that implies a more general issue with the structure 
of the code which deserves fixing in its own right.

Robin.

> +		dma_map_ops_dev = dma->tx_chan->device->dev;
> +
>   	dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
>   	count = dma->tx_bytes_requested - state.residue;
>   	async_tx_ack(dma->tx_desc);
>   
> -	dma_sync_single_for_cpu(ourport->port.dev, dma->tx_transfer_addr,
> +	dma_sync_single_for_cpu(dma_map_ops_dev, dma->tx_transfer_addr,
>   				dma->tx_size, DMA_TO_DEVICE);
>   
>   	spin_lock_irqsave(&port->lock, flags);
> @@ -436,6 +446,11 @@ static int s3c24xx_serial_start_tx_dma(struct s3c24xx_uart_port *ourport,
>   	struct uart_port *port = &ourport->port;
>   	struct circ_buf *xmit = &port->state->xmit;
>   	struct s3c24xx_uart_dma *dma = ourport->dma;
> +	struct device *dma_map_ops_dev = ourport->port.dev;
> +
> +	/* Pick dma_ops of DMA device if DMA device is attached */
> +	if (dma && dma->tx_chan)
> +		dma_map_ops_dev = dma->tx_chan->device->dev;
>   
>   	if (ourport->tx_mode != S3C24XX_TX_DMA)
>   		enable_tx_dma(ourport);
> @@ -443,7 +458,7 @@ static int s3c24xx_serial_start_tx_dma(struct s3c24xx_uart_port *ourport,
>   	dma->tx_size = count & ~(dma_get_cache_alignment() - 1);
>   	dma->tx_transfer_addr = dma->tx_addr + xmit->tail;
>   
> -	dma_sync_single_for_device(ourport->port.dev, dma->tx_transfer_addr,
> +	dma_sync_single_for_device(dma_map_ops_dev, dma->tx_transfer_addr,
>   				dma->tx_size, DMA_TO_DEVICE);
>   
>   	dma->tx_desc = dmaengine_prep_slave_single(dma->tx_chan,
> @@ -510,12 +525,17 @@ static void s3c24xx_uart_copy_rx_to_tty(struct s3c24xx_uart_port *ourport,
>   		struct tty_port *tty, int count)
>   {
>   	struct s3c24xx_uart_dma *dma = ourport->dma;
> +	struct device *dma_map_ops_dev = ourport->port.dev;
>   	int copied;
>   
> +	/* Pick dma_ops of DMA device if DMA device is attached */
> +	if (dma && dma->rx_chan)
> +		dma_map_ops_dev = dma->rx_chan->device->dev;
> +
>   	if (!count)
>   		return;
>   
> -	dma_sync_single_for_cpu(ourport->port.dev, dma->rx_addr,
> +	dma_sync_single_for_cpu(dma_map_ops_dev, dma->rx_addr,
>   				dma->rx_size, DMA_FROM_DEVICE);
>   
>   	ourport->port.icount.rx += count;
> @@ -635,8 +655,13 @@ static void s3c24xx_serial_rx_dma_complete(void *args)
>   static void s3c64xx_start_rx_dma(struct s3c24xx_uart_port *ourport)
>   {
>   	struct s3c24xx_uart_dma *dma = ourport->dma;
> +	struct device *dma_map_ops_dev = ourport->port.dev;
> +
> +	/* Pick dma_ops of DMA device if DMA device is attached */
> +	if (dma && dma->rx_chan)
> +		dma_map_ops_dev = dma->rx_chan->device->dev;
>   
> -	dma_sync_single_for_device(ourport->port.dev, dma->rx_addr,
> +	dma_sync_single_for_device(dma_map_ops_dev, dma->rx_addr,
>   				dma->rx_size, DMA_FROM_DEVICE);
>   
>   	dma->rx_desc = dmaengine_prep_slave_single(dma->rx_chan,
> @@ -1045,6 +1070,7 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
>   	struct s3c24xx_uart_dma	*dma = p->dma;
>   	struct dma_slave_caps dma_caps;
>   	const char *reason = NULL;
> +	struct device *dma_map_ops_dev = p->port.dev;
>   	int ret;
>   
>   	/* Default slave configuration parameters */
> @@ -1102,18 +1128,25 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
>   		goto err_release_tx;
>   	}
>   
> -	dma->rx_addr = dma_map_single(p->port.dev, dma->rx_buf,
> +	/* Pick dma_ops of DMA device if DMA device is attached */
> +	if (dma && dma->rx_chan)
> +		dma_map_ops_dev = dma->rx_chan->device->dev;
> +
> +	dma->rx_addr = dma_map_single(dma_map_ops_dev, dma->rx_buf,
>   				dma->rx_size, DMA_FROM_DEVICE);
> -	if (dma_mapping_error(p->port.dev, dma->rx_addr)) {
> +	if (dma_mapping_error(dma_map_ops_dev, dma->rx_addr)) {
>   		reason = "DMA mapping error for RX buffer";
>   		ret = -EIO;
>   		goto err_free_rx;
>   	}
>   
> +	/* Pick dma_ops of DMA device if DMA device is attached */
> +	if (dma && dma->tx_chan)
> +		dma_map_ops_dev = dma->tx_chan->device->dev;
>   	/* TX buffer */
> -	dma->tx_addr = dma_map_single(p->port.dev, p->port.state->xmit.buf,
> +	dma->tx_addr = dma_map_single(dma_map_ops_dev, p->port.state->xmit.buf,
>   				UART_XMIT_SIZE, DMA_TO_DEVICE);
> -	if (dma_mapping_error(p->port.dev, dma->tx_addr)) {
> +	if (dma_mapping_error(dma_map_ops_dev, dma->tx_addr)) {
>   		reason = "DMA mapping error for TX buffer";
>   		ret = -EIO;
>   		goto err_unmap_rx;
> @@ -1122,7 +1155,9 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
>   	return 0;
>   
>   err_unmap_rx:
> -	dma_unmap_single(p->port.dev, dma->rx_addr, dma->rx_size,
> +	if (dma->rx_chan)
> +		dma_map_ops_dev = dma->rx_chan->device->dev;
> +	dma_unmap_single(dma_map_ops_dev, dma->rx_addr, dma->rx_size,
>   			 DMA_FROM_DEVICE);
>   err_free_rx:
>   	kfree(dma->rx_buf);
> @@ -1139,10 +1174,12 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
>   static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
>   {
>   	struct s3c24xx_uart_dma	*dma = p->dma;
> +	struct device *dma_map_ops_dev = p->port.dev;
>   
>   	if (dma->rx_chan) {
> +		dma_map_ops_dev = dma->rx_chan->device->dev;
>   		dmaengine_terminate_all(dma->rx_chan);
> -		dma_unmap_single(p->port.dev, dma->rx_addr,
> +		dma_unmap_single(dma_map_ops_dev, dma->rx_addr,
>   				dma->rx_size, DMA_FROM_DEVICE);
>   		kfree(dma->rx_buf);
>   		dma_release_channel(dma->rx_chan);
> @@ -1150,8 +1187,9 @@ static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
>   	}
>   
>   	if (dma->tx_chan) {
> +		dma_map_ops_dev = dma->tx_chan->device->dev;
>   		dmaengine_terminate_all(dma->tx_chan);
> -		dma_unmap_single(p->port.dev, dma->tx_addr,
> +		dma_unmap_single(dma_map_ops_dev, dma->tx_addr,
>   				UART_XMIT_SIZE, DMA_TO_DEVICE);
>   		dma_release_channel(dma->tx_chan);
>   		dma->tx_chan = NULL;
> 

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

* RE: [PATCH v2] serial: samsung: use dma_ops of DMA if attached
  2021-06-22 12:40   ` Robin Murphy
@ 2021-06-23  4:19     ` M Tamseel Shams
  0 siblings, 0 replies; 5+ messages in thread
From: M Tamseel Shams @ 2021-06-23  4:19 UTC (permalink / raw)
  To: 'Robin Murphy', krzysztof.kozlowski, gregkh, jirislaby
  Cc: linux-arm-kernel, linux-samsung-soc, linux-serial, linux-kernel,
	alim.akhtar, ajaykumar.rs


> On 2021-06-22 04:52, Tamseel Shams wrote:
> > When DMA is used for TX and RX by serial driver, it should pass the
> > DMA device pointer to DMA API instead of UART device pointer.
> >
> > This patch is necessary to fix the SMMU page faults which is observed
> > when a DMA(with SMMU enabled) is attached to UART for transfer.
> >
> > Signed-off-by: Tamseel Shams <m.shams@samsung.com>
> > Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> > ---
> >   drivers/tty/serial/samsung_tty.c | 62 +++++++++++++++++++++++++-------
> >   1 file changed, 50 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/tty/serial/samsung_tty.c
> > b/drivers/tty/serial/samsung_tty.c
> > index 9fbc61151c2e..0c924bb6108e 100644
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> > @@ -284,8 +284,13 @@ static void s3c24xx_serial_stop_tx(struct uart_port
> *port)
> >   	struct s3c24xx_uart_dma *dma = ourport->dma;
> >   	struct circ_buf *xmit = &port->state->xmit;
> >   	struct dma_tx_state state;
> > +	struct device *dma_map_ops_dev = ourport->port.dev;
> >   	int count;
> >
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->tx_chan)
> > +		dma_map_ops_dev = dma->tx_chan->device->dev;
> > +
> >   	if (!ourport->tx_enabled)
> >   		return;
> >
> > @@ -305,7 +310,7 @@ static void s3c24xx_serial_stop_tx(struct uart_port
> *port)
> >   		dmaengine_pause(dma->tx_chan);
> >   		dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
> >   		dmaengine_terminate_all(dma->tx_chan);
> > -		dma_sync_single_for_cpu(ourport->port.dev,
> > +		dma_sync_single_for_cpu(dma_map_ops_dev,
> >   			dma->tx_transfer_addr, dma->tx_size,
> DMA_TO_DEVICE);
> >   		async_tx_ack(dma->tx_desc);
> >   		count = dma->tx_bytes_requested - state.residue; @@ -331,14
> > +336,19 @@ static void s3c24xx_serial_tx_dma_complete(void *args)
> >   	struct circ_buf *xmit = &port->state->xmit;
> >   	struct s3c24xx_uart_dma *dma = ourport->dma;
> >   	struct dma_tx_state state;
> > +	struct device *dma_map_ops_dev = ourport->port.dev;
> >   	unsigned long flags;
> >   	int count;
> >
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->tx_chan)
> 
> Clearly you don't need most of these checks, if any. For example here you're
> already dereferencing dma->tx_chan unconditionally a couple of lines below.
> 
> AFAICS it should simply be a case of hard-coding the correct device for all of the
> DMA API calls without any of this silly conditional assignment. If it's even
> possible to reach the point of making a DMA API call when you don't have a valid
> DMA channel (and thus it would be pointless anyway), that implies a more
> general issue with the structure of the code which deserves fixing in its own
> right.
> 
> Robin.
> 
Yes, this looks like a perfect solution to remove the conditional assignments and
instead directly assign the DMA device pointer.

> > +		dma_map_ops_dev = dma->tx_chan->device->dev;
> > +
> >   	dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
> >   	count = dma->tx_bytes_requested - state.residue;
> >   	async_tx_ack(dma->tx_desc);
> >
> > -	dma_sync_single_for_cpu(ourport->port.dev, dma->tx_transfer_addr,
> > +	dma_sync_single_for_cpu(dma_map_ops_dev, dma->tx_transfer_addr,
> >   				dma->tx_size, DMA_TO_DEVICE);
> >
> >   	spin_lock_irqsave(&port->lock, flags); @@ -436,6 +446,11 @@ static
> > int s3c24xx_serial_start_tx_dma(struct s3c24xx_uart_port *ourport,
> >   	struct uart_port *port = &ourport->port;
> >   	struct circ_buf *xmit = &port->state->xmit;
> >   	struct s3c24xx_uart_dma *dma = ourport->dma;
> > +	struct device *dma_map_ops_dev = ourport->port.dev;
> > +
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->tx_chan)
> > +		dma_map_ops_dev = dma->tx_chan->device->dev;
> >
> >   	if (ourport->tx_mode != S3C24XX_TX_DMA)
> >   		enable_tx_dma(ourport);
> > @@ -443,7 +458,7 @@ static int s3c24xx_serial_start_tx_dma(struct
> s3c24xx_uart_port *ourport,
> >   	dma->tx_size = count & ~(dma_get_cache_alignment() - 1);
> >   	dma->tx_transfer_addr = dma->tx_addr + xmit->tail;
> >
> > -	dma_sync_single_for_device(ourport->port.dev, dma-
> >tx_transfer_addr,
> > +	dma_sync_single_for_device(dma_map_ops_dev, dma-
> >tx_transfer_addr,
> >   				dma->tx_size, DMA_TO_DEVICE);
> >
> >   	dma->tx_desc = dmaengine_prep_slave_single(dma->tx_chan,
> > @@ -510,12 +525,17 @@ static void s3c24xx_uart_copy_rx_to_tty(struct
> s3c24xx_uart_port *ourport,
> >   		struct tty_port *tty, int count)
> >   {
> >   	struct s3c24xx_uart_dma *dma = ourport->dma;
> > +	struct device *dma_map_ops_dev = ourport->port.dev;
> >   	int copied;
> >
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->rx_chan)
> > +		dma_map_ops_dev = dma->rx_chan->device->dev;
> > +
> >   	if (!count)
> >   		return;
> >
> > -	dma_sync_single_for_cpu(ourport->port.dev, dma->rx_addr,
> > +	dma_sync_single_for_cpu(dma_map_ops_dev, dma->rx_addr,
> >   				dma->rx_size, DMA_FROM_DEVICE);
> >
> >   	ourport->port.icount.rx += count;
> > @@ -635,8 +655,13 @@ static void s3c24xx_serial_rx_dma_complete(void
> *args)
> >   static void s3c64xx_start_rx_dma(struct s3c24xx_uart_port *ourport)
> >   {
> >   	struct s3c24xx_uart_dma *dma = ourport->dma;
> > +	struct device *dma_map_ops_dev = ourport->port.dev;
> > +
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->rx_chan)
> > +		dma_map_ops_dev = dma->rx_chan->device->dev;
> >
> > -	dma_sync_single_for_device(ourport->port.dev, dma->rx_addr,
> > +	dma_sync_single_for_device(dma_map_ops_dev, dma->rx_addr,
> >   				dma->rx_size, DMA_FROM_DEVICE);
> >
> >   	dma->rx_desc = dmaengine_prep_slave_single(dma->rx_chan,
> > @@ -1045,6 +1070,7 @@ static int s3c24xx_serial_request_dma(struct
> s3c24xx_uart_port *p)
> >   	struct s3c24xx_uart_dma	*dma = p->dma;
> >   	struct dma_slave_caps dma_caps;
> >   	const char *reason = NULL;
> > +	struct device *dma_map_ops_dev = p->port.dev;
> >   	int ret;
> >
> >   	/* Default slave configuration parameters */ @@ -1102,18 +1128,25
> > @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
> >   		goto err_release_tx;
> >   	}
> >
> > -	dma->rx_addr = dma_map_single(p->port.dev, dma->rx_buf,
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->rx_chan)
> > +		dma_map_ops_dev = dma->rx_chan->device->dev;
> > +
> > +	dma->rx_addr = dma_map_single(dma_map_ops_dev, dma->rx_buf,
> >   				dma->rx_size, DMA_FROM_DEVICE);
> > -	if (dma_mapping_error(p->port.dev, dma->rx_addr)) {
> > +	if (dma_mapping_error(dma_map_ops_dev, dma->rx_addr)) {
> >   		reason = "DMA mapping error for RX buffer";
> >   		ret = -EIO;
> >   		goto err_free_rx;
> >   	}
> >
> > +	/* Pick dma_ops of DMA device if DMA device is attached */
> > +	if (dma && dma->tx_chan)
> > +		dma_map_ops_dev = dma->tx_chan->device->dev;
> >   	/* TX buffer */
> > -	dma->tx_addr = dma_map_single(p->port.dev, p->port.state->xmit.buf,
> > +	dma->tx_addr = dma_map_single(dma_map_ops_dev,
> > +p->port.state->xmit.buf,
> >   				UART_XMIT_SIZE, DMA_TO_DEVICE);
> > -	if (dma_mapping_error(p->port.dev, dma->tx_addr)) {
> > +	if (dma_mapping_error(dma_map_ops_dev, dma->tx_addr)) {
> >   		reason = "DMA mapping error for TX buffer";
> >   		ret = -EIO;
> >   		goto err_unmap_rx;
> > @@ -1122,7 +1155,9 @@ static int s3c24xx_serial_request_dma(struct
> s3c24xx_uart_port *p)
> >   	return 0;
> >
> >   err_unmap_rx:
> > -	dma_unmap_single(p->port.dev, dma->rx_addr, dma->rx_size,
> > +	if (dma->rx_chan)
> > +		dma_map_ops_dev = dma->rx_chan->device->dev;
> > +	dma_unmap_single(dma_map_ops_dev, dma->rx_addr, dma->rx_size,
> >   			 DMA_FROM_DEVICE);
> >   err_free_rx:
> >   	kfree(dma->rx_buf);
> > @@ -1139,10 +1174,12 @@ static int s3c24xx_serial_request_dma(struct
> s3c24xx_uart_port *p)
> >   static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
> >   {
> >   	struct s3c24xx_uart_dma	*dma = p->dma;
> > +	struct device *dma_map_ops_dev = p->port.dev;
> >
> >   	if (dma->rx_chan) {
> > +		dma_map_ops_dev = dma->rx_chan->device->dev;
> >   		dmaengine_terminate_all(dma->rx_chan);
> > -		dma_unmap_single(p->port.dev, dma->rx_addr,
> > +		dma_unmap_single(dma_map_ops_dev, dma->rx_addr,
> >   				dma->rx_size, DMA_FROM_DEVICE);
> >   		kfree(dma->rx_buf);
> >   		dma_release_channel(dma->rx_chan);
> > @@ -1150,8 +1187,9 @@ static void s3c24xx_serial_release_dma(struct
> s3c24xx_uart_port *p)
> >   	}
> >
> >   	if (dma->tx_chan) {
> > +		dma_map_ops_dev = dma->tx_chan->device->dev;
> >   		dmaengine_terminate_all(dma->tx_chan);
> > -		dma_unmap_single(p->port.dev, dma->tx_addr,
> > +		dma_unmap_single(dma_map_ops_dev, dma->tx_addr,
> >   				UART_XMIT_SIZE, DMA_TO_DEVICE);
> >   		dma_release_channel(dma->tx_chan);
> >   		dma->tx_chan = NULL;
> >

Thanks & Regards,
Tamseel Shams


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

end of thread, other threads:[~2021-06-23  4:19 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20210622034818epcas5p3837ce2315f5c57980576f10b8fc7efeb@epcas5p3.samsung.com>
2021-06-22  3:52 ` [PATCH v2] serial: samsung: use dma_ops of DMA if attached Tamseel Shams
2021-06-22  5:46   ` Greg KH
2021-06-22  6:37     ` M Tamseel Shams
2021-06-22 12:40   ` Robin Murphy
2021-06-23  4:19     ` M Tamseel Shams

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).