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

Message ID 1490270519-4046-2-git-send-email-m.szyprowski@samsung.com
State New
Headers show
Series
  • [v2,1/2] serial: samsung: Use right device for DMA-mapping calls
Related show

Commit Message

Marek Szyprowski March 23, 2017, 12:01 p.m.
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(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Krzysztof Kozlowski March 23, 2017, 8:04 p.m. | #1
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

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

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)