[PATCH v2 1/2] serial: samsung: Use right device for DMA-mapping calls

[PATCH v2 1/2] serial: samsung: Use right device for DMA-mapping calls

[Marek Szyprowski]

Driver should provide its own struct device for all DMA-mapping calls instead
of extracting device pointer from DMA engine channel.

Reported-by: Seung-Woo Kim <sw0312.kim@samsung.com>
Fixes: 62c37eedb74c8 ("serial: samsung: add dma reqest/release functions")
CC: stable@vger.kernel.org # v4.0+
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
v2:
- fixed commit id in 'fixes' tag, added 'reviewed-by' tag
---
 drivers/tty/serial/samsung.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
index 7a17aedbf902..9f3759bdb44f 100644
--- a/drivers/tty/serial/samsung.c
+++ b/drivers/tty/serial/samsung.c
@@ -901,14 +901,13 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
 		return -ENOMEM;
 	}
 
-	dma->rx_addr = dma_map_single(dma->rx_chan->device->dev, dma->rx_buf,
+	dma->rx_addr = dma_map_single(p->port.dev, dma->rx_buf,
 				dma->rx_size, DMA_FROM_DEVICE);
 
 	spin_lock_irqsave(&p->port.lock, flags);
 
 	/* TX buffer */
-	dma->tx_addr = dma_map_single(dma->tx_chan->device->dev,
-				p->port.state->xmit.buf,
+	dma->tx_addr = dma_map_single(p->port.dev, p->port.state->xmit.buf,
 				UART_XMIT_SIZE, DMA_TO_DEVICE);
 
 	spin_unlock_irqrestore(&p->port.lock, flags);
@@ -922,7 +921,7 @@ static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
 
 	if (dma->rx_chan) {
 		dmaengine_terminate_all(dma->rx_chan);
-		dma_unmap_single(dma->rx_chan->device->dev, dma->rx_addr,
+		dma_unmap_single(p->port.dev, dma->rx_addr,
 				dma->rx_size, DMA_FROM_DEVICE);
 		kfree(dma->rx_buf);
 		dma_release_channel(dma->rx_chan);
@@ -931,7 +930,7 @@ static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
 
 	if (dma->tx_chan) {
 		dmaengine_terminate_all(dma->tx_chan);
-		dma_unmap_single(dma->tx_chan->device->dev, dma->tx_addr,
+		dma_unmap_single(p->port.dev, dma->tx_addr,
 				UART_XMIT_SIZE, DMA_TO_DEVICE);
 		dma_release_channel(dma->tx_chan);
 		dma->tx_chan = NULL;
-- 
1.9.1

[PATCH v2 2/2] serial: samsung: Add missing checks for dma_map_single failure

[Marek Szyprowski]

This patch adds missing checks for dma_map_single() failure and proper error
reporting. While touching this part of the code, it also removes unnecessary
spinlock calls around dma_map_single() for TX buffer. This finally solves all
the issues reported by DMA API debug framework.

Reported-by: Seung-Woo Kim <sw0312.kim@samsung.com>
Fixes: 62c37eedb74c8 ("serial: samsung: add dma reqest/release functions")
CC: stable@vger.kernel.org # v4.10+
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
This issue was there since adding DMA support, but this patch applies cleanly
only to v4.10+ kernels due to other changes in the surrounding code.

v2:
- fixed commit id in 'fixes' tag, added 'reviewed-by' tag
---
 drivers/tty/serial/samsung.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
index 9f3759bdb44f..8aca18c4cdea 100644
--- a/drivers/tty/serial/samsung.c
+++ b/drivers/tty/serial/samsung.c
@@ -859,7 +859,7 @@ static void s3c24xx_serial_break_ctl(struct uart_port *port, int break_state)
 static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
 {
 	struct s3c24xx_uart_dma	*dma = p->dma;
-	unsigned long flags;
+	int ret;
 
 	/* Default slave configuration parameters */
 	dma->rx_conf.direction		= DMA_DEV_TO_MEM;
@@ -884,8 +884,8 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
 
 	dma->tx_chan = dma_request_chan(p->port.dev, "tx");
 	if (IS_ERR(dma->tx_chan)) {
-		dma_release_channel(dma->rx_chan);
-		return PTR_ERR(dma->tx_chan);
+		ret = PTR_ERR(dma->tx_chan);
+		goto err_release_rx;
 	}
 
 	dmaengine_slave_config(dma->tx_chan, &dma->tx_conf);
@@ -894,25 +894,38 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
 	dma->rx_size = PAGE_SIZE;
 
 	dma->rx_buf = kmalloc(dma->rx_size, GFP_KERNEL);
-
 	if (!dma->rx_buf) {
-		dma_release_channel(dma->rx_chan);
-		dma_release_channel(dma->tx_chan);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_release_tx;
 	}
 
 	dma->rx_addr = dma_map_single(p->port.dev, dma->rx_buf,
 				dma->rx_size, DMA_FROM_DEVICE);
-
-	spin_lock_irqsave(&p->port.lock, flags);
+	if (dma_mapping_error(p->port.dev, dma->rx_addr)) {
+		ret = -EIO;
+		goto err_free_rx;
+	}
 
 	/* TX buffer */
 	dma->tx_addr = dma_map_single(p->port.dev, p->port.state->xmit.buf,
 				UART_XMIT_SIZE, DMA_TO_DEVICE);
-
-	spin_unlock_irqrestore(&p->port.lock, flags);
+	if (dma_mapping_error(p->port.dev, dma->tx_addr)) {
+		ret = -EIO;
+		goto err_unmap_rx;
+	}
 
 	return 0;
+
+err_unmap_rx:
+	dma_unmap_single(p->port.dev, dma->rx_addr, dma->rx_size,
+			 DMA_FROM_DEVICE);
+err_free_rx:
+	kfree(dma->rx_buf);
+err_release_tx:
+	dma_release_channel(dma->tx_chan);
+err_release_rx:
+	dma_release_channel(dma->rx_chan);
+	return ret;
 }
 
 static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
-- 
1.9.1

Re: [PATCH v2 1/2] serial: samsung: Use right device for DMA-mapping calls

[Krzysztof Kozlowski]

On Thu, Mar 23, 2017 at 01:01:52PM +0100, Marek Szyprowski wrote:
> Driver should provide its own struct device for all DMA-mapping calls instead
> of extracting device pointer from DMA engine channel.
 
For the purpose of cc-stable it might be useful to answer here what
visible error is fixed (or why driver should not use DMA's device) but
on the other hand this is actually obvious... Anyway:

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof


> Reported-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> Fixes: 62c37eedb74c8 ("serial: samsung: add dma reqest/release functions")
> CC: stable@vger.kernel.org # v4.0+
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
> v2:
> - fixed commit id in 'fixes' tag, added 'reviewed-by' tag
> ---
>  drivers/tty/serial/samsung.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)

Re: [PATCH v2 2/2] serial: samsung: Add missing checks for dma_map_single failure

[Krzysztof Kozlowski]

On Thu, Mar 23, 2017 at 01:01:53PM +0100, Marek Szyprowski wrote:
> This patch adds missing checks for dma_map_single() failure and proper error
> reporting. While touching this part of the code, it also removes unnecessary
> spinlock calls around dma_map_single() for TX buffer. This finally solves all
> the issues reported by DMA API debug framework.
> 
> Reported-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> Fixes: 62c37eedb74c8 ("serial: samsung: add dma reqest/release functions")
> CC: stable@vger.kernel.org # v4.10+
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
> This issue was there since adding DMA support, but this patch applies cleanly
> only to v4.10+ kernels due to other changes in the surrounding code.
> 
> v2:
> - fixed commit id in 'fixes' tag, added 'reviewed-by' tag
> ---
>  drivers/tty/serial/samsung.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> index 9f3759bdb44f..8aca18c4cdea 100644
> --- a/drivers/tty/serial/samsung.c
> +++ b/drivers/tty/serial/samsung.c
> @@ -859,7 +859,7 @@ static void s3c24xx_serial_break_ctl(struct uart_port *port, int break_state)
>  static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
>  {
>  	struct s3c24xx_uart_dma	*dma = p->dma;
> -	unsigned long flags;
> +	int ret;
>  
>  	/* Default slave configuration parameters */
>  	dma->rx_conf.direction		= DMA_DEV_TO_MEM;
> @@ -884,8 +884,8 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
>  
>  	dma->tx_chan = dma_request_chan(p->port.dev, "tx");
>  	if (IS_ERR(dma->tx_chan)) {
> -		dma_release_channel(dma->rx_chan);
> -		return PTR_ERR(dma->tx_chan);
> +		ret = PTR_ERR(dma->tx_chan);
> +		goto err_release_rx;
>  	}
>  
>  	dmaengine_slave_config(dma->tx_chan, &dma->tx_conf);
> @@ -894,25 +894,38 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
>  	dma->rx_size = PAGE_SIZE;
>  
>  	dma->rx_buf = kmalloc(dma->rx_size, GFP_KERNEL);
> -
>  	if (!dma->rx_buf) {
> -		dma_release_channel(dma->rx_chan);
> -		dma_release_channel(dma->tx_chan);
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto err_release_tx;
>  	}
>  
>  	dma->rx_addr = dma_map_single(p->port.dev, dma->rx_buf,
>  				dma->rx_size, DMA_FROM_DEVICE);
> -
> -	spin_lock_irqsave(&p->port.lock, flags);

Error paths look fine but how about splitting spinlock removal out of
this patch?  Logically they are two different changes and unrelated code
should not be backported.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] serial: samsung: Use right device for DMA-mapping calls

[Greg Kroah-Hartman]

On Thu, Mar 23, 2017 at 09:51:55PM +0200, Krzysztof Kozlowski wrote:
> On Thu, Mar 23, 2017 at 01:01:52PM +0100, Marek Szyprowski wrote:
> > Driver should provide its own struct device for all DMA-mapping calls instead
> > of extracting device pointer from DMA engine channel.
>  
> For the purpose of cc-stable it might be useful to answer here what
> visible error is fixed (or why driver should not use DMA's device) but
> on the other hand this is actually obvious... Anyway:

No, please make it obvious on the next version of this series.

thanks,

greg k-h