[PATCH 1/2] [RFC] spi: rspi: Handle dmaengine_prep_slave_sg() failures gracefully

[PATCH 1/2] [RFC] spi: rspi: Handle dmaengine_prep_slave_sg() failures gracefully

[Geert Uytterhoeven]

As typically a shmobile SoC has less DMA channels than devices that can use
DMA, we may want to prioritize access to the DMA channels in the future.
This means that dmaengine_prep_slave_sg() may start failing arbitrarily.

Handle dmaengine_prep_slave_sg() failures gracefully by falling back to
PIO.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/spi/spi-rspi.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
index 38fd938d6360..c850dfdfa9e3 100644
--- a/drivers/spi/spi-rspi.c
+++ b/drivers/spi/spi-rspi.c
@@ -477,7 +477,7 @@ static int rspi_dma_transfer(struct rspi_data *rspi, struct sg_table *tx,
 					tx->sgl, tx->nents, DMA_TO_DEVICE,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 		if (!desc_tx)
-			return -EIO;
+			goto no_dma;
 
 		irq_mask |= SPCR_SPTIE;
 	}
@@ -486,7 +486,7 @@ static int rspi_dma_transfer(struct rspi_data *rspi, struct sg_table *tx,
 					rx->sgl, rx->nents, DMA_FROM_DEVICE,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 		if (!desc_rx)
-			return -EIO;
+			goto no_dma;
 
 		irq_mask |= SPCR_SPRIE;
 	}
@@ -540,6 +540,12 @@ static int rspi_dma_transfer(struct rspi_data *rspi, struct sg_table *tx,
 		enable_irq(rspi->rx_irq);
 
 	return ret;
+
+no_dma:
+	pr_warn_once("%s %s: DMA not available, falling back to PIO\n",
+		     dev_driver_string(&rspi->master->dev),
+		     dev_name(&rspi->master->dev));
+	return -EAGAIN;
 }
 
 static void rspi_receive_init(const struct rspi_data *rspi)
@@ -593,8 +599,10 @@ static int rspi_common_transfer(struct rspi_data *rspi,
 
 	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
 		/* rx_buf can be NULL on RSPI on SH in TX-only Mode */
-		return rspi_dma_transfer(rspi, &xfer->tx_sg,
-					 xfer->rx_buf ? &xfer->rx_sg : NULL);
+		ret = rspi_dma_transfer(rspi, &xfer->tx_sg,
+					xfer->rx_buf ? &xfer->rx_sg : NULL);
+		if (ret != -EAGAIN)
+			return ret;
 	}
 
 	ret = rspi_pio_transfer(rspi, xfer->tx_buf, xfer->rx_buf, xfer->len);
@@ -648,8 +656,11 @@ static int qspi_transfer_out(struct rspi_data *rspi, struct spi_transfer *xfer)
 {
 	int ret;
 
-	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer))
-		return rspi_dma_transfer(rspi, &xfer->tx_sg, NULL);
+	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
+		ret = rspi_dma_transfer(rspi, &xfer->tx_sg, NULL);
+		if (ret != -EAGAIN)
+			return ret;
+	}
 
 	ret = rspi_pio_transfer(rspi, xfer->tx_buf, NULL, xfer->len);
 	if (ret < 0)
@@ -663,8 +674,11 @@ static int qspi_transfer_out(struct rspi_data *rspi, struct spi_transfer *xfer)
 
 static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer *xfer)
 {
-	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer))
-		return rspi_dma_transfer(rspi, NULL, &xfer->rx_sg);
+	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
+		int ret = rspi_dma_transfer(rspi, NULL, &xfer->rx_sg);
+		if (ret != -EAGAIN)
+			return ret;
+	}
 
 	return rspi_pio_transfer(rspi, NULL, xfer->rx_buf, xfer->len);
 }
-- 
1.9.1

[PATCH 2/2] [RFC] spi: sh-msiof: Handle dmaengine_prep_slave_single() failures gracefully

[Geert Uytterhoeven]

As typically a shmobile SoC has less DMA channels than devices that can use
DMA, we may want to prioritize access to the DMA channels in the future.
This means that dmaengine_prep_slave_single() may start failing
arbitrarily.

Handle dmaengine_prep_slave_single() failures gracefully by falling back to
PIO. This requires moving DMA-specific configuration of the MSIOF device
after the call(s) to dmaengine_prep_slave_single().

Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
---
 drivers/spi/spi-sh-msiof.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index 4f0f1cbc92ef..2d7c45bbb6f3 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -636,12 +636,6 @@ static int sh_msiof_dma_once(struct sh_msiof_spi_priv *p, const void *tx,
 	dma_cookie_t cookie;
 	int ret;
 
-	/* 1 stage FIFO watermarks for DMA */
-	sh_msiof_write(p, FCTR, FCTR_TFWM_1 | FCTR_RFWM_1);
-
-	/* setup msiof transfer mode registers (32-bit words) */
-	sh_msiof_spi_set_mode_regs(p, tx, rx, 32, len / 4);
-
 	if (tx) {
 		ier_bits |= IER_TDREQE | IER_TDMAE;
 		dma_sync_single_for_device(&p->pdev->dev, p->tx_dma_addr, len,
@@ -650,7 +644,7 @@ static int sh_msiof_dma_once(struct sh_msiof_spi_priv *p, const void *tx,
 					p->tx_dma_addr, len, DMA_TO_DEVICE,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 		if (!desc_tx)
-			return -EIO;
+			return -EAGAIN;
 	}
 
 	if (rx) {
@@ -659,8 +653,15 @@ static int sh_msiof_dma_once(struct sh_msiof_spi_priv *p, const void *tx,
 					p->rx_dma_addr, len, DMA_FROM_DEVICE,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 		if (!desc_rx)
-			return -EIO;
+			return -EAGAIN;
 	}
+
+	/* 1 stage FIFO watermarks for DMA */
+	sh_msiof_write(p, FCTR, FCTR_TFWM_1 | FCTR_RFWM_1);
+
+	/* setup msiof transfer mode registers (32-bit words) */
+	sh_msiof_spi_set_mode_regs(p, tx, rx, 32, len / 4);
+
 	sh_msiof_write(p, IER, ier_bits);
 
 	reinit_completion(&p->done);
@@ -822,6 +823,12 @@ static int sh_msiof_transfer_one(struct spi_master *master,
 			copy32(p->tx_dma_page, tx_buf, l / 4);
 
 		ret = sh_msiof_dma_once(p, tx_buf, rx_buf, l);
+		if (ret == -EAGAIN) {
+			pr_warn_once("%s %s: DMA not available, falling back to PIO\n",
+				     dev_driver_string(&p->pdev->dev),
+				     dev_name(&p->pdev->dev));
+			break;
+		}
 		if (ret)
 			return ret;
 
-- 
1.9.1

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

Re: [PATCH 1/2] [RFC] spi: rspi: Handle dmaengine_prep_slave_sg() failures gracefully

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Wednesday 09 July 2014 12:26:22 Geert Uytterhoeven wrote:
> As typically a shmobile SoC has less DMA channels than devices that can use
> DMA, we may want to prioritize access to the DMA channels in the future.
> This means that dmaengine_prep_slave_sg() may start failing arbitrarily.
> 
> Handle dmaengine_prep_slave_sg() failures gracefully by falling back to
> PIO.

Just a random thought, do you think there would be performance-sensitive cases 
where failing the transfer completely would be better than using PIO ?

> Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>

-EAGAIN might not be the best error code to return in this case, as it would 
imply that the caller should just call rspi_dma_transfer() again. On the other 
hand I don't see another error code that would be a perfect match, so

Acked-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

> ---
>  drivers/spi/spi-rspi.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
> index 38fd938d6360..c850dfdfa9e3 100644
> --- a/drivers/spi/spi-rspi.c
> +++ b/drivers/spi/spi-rspi.c
> @@ -477,7 +477,7 @@ static int rspi_dma_transfer(struct rspi_data *rspi,
> struct sg_table *tx, tx->sgl, tx->nents, DMA_TO_DEVICE,
>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>  		if (!desc_tx)
> -			return -EIO;
> +			goto no_dma;
> 
>  		irq_mask |= SPCR_SPTIE;
>  	}
> @@ -486,7 +486,7 @@ static int rspi_dma_transfer(struct rspi_data *rspi,
> struct sg_table *tx, rx->sgl, rx->nents, DMA_FROM_DEVICE,
>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>  		if (!desc_rx)
> -			return -EIO;
> +			goto no_dma;
> 
>  		irq_mask |= SPCR_SPRIE;
>  	}
> @@ -540,6 +540,12 @@ static int rspi_dma_transfer(struct rspi_data *rspi,
> struct sg_table *tx, enable_irq(rspi->rx_irq);
> 
>  	return ret;
> +
> +no_dma:
> +	pr_warn_once("%s %s: DMA not available, falling back to PIO\n",
> +		     dev_driver_string(&rspi->master->dev),
> +		     dev_name(&rspi->master->dev));
> +	return -EAGAIN;
>  }
> 
>  static void rspi_receive_init(const struct rspi_data *rspi)
> @@ -593,8 +599,10 @@ static int rspi_common_transfer(struct rspi_data *rspi,
> 
>  	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
>  		/* rx_buf can be NULL on RSPI on SH in TX-only Mode */
> -		return rspi_dma_transfer(rspi, &xfer->tx_sg,
> -					 xfer->rx_buf ? &xfer->rx_sg : NULL);
> +		ret = rspi_dma_transfer(rspi, &xfer->tx_sg,
> +					xfer->rx_buf ? &xfer->rx_sg : NULL);
> +		if (ret != -EAGAIN)
> +			return ret;
>  	}
> 
>  	ret = rspi_pio_transfer(rspi, xfer->tx_buf, xfer->rx_buf, xfer->len);
> @@ -648,8 +656,11 @@ static int qspi_transfer_out(struct rspi_data *rspi,
> struct spi_transfer *xfer) {
>  	int ret;
> 
> -	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer))
> -		return rspi_dma_transfer(rspi, &xfer->tx_sg, NULL);
> +	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
> +		ret = rspi_dma_transfer(rspi, &xfer->tx_sg, NULL);
> +		if (ret != -EAGAIN)
> +			return ret;
> +	}
> 
>  	ret = rspi_pio_transfer(rspi, xfer->tx_buf, NULL, xfer->len);
>  	if (ret < 0)
> @@ -663,8 +674,11 @@ static int qspi_transfer_out(struct rspi_data *rspi,
> struct spi_transfer *xfer)
> 
>  static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer
> *xfer) {
> -	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer))
> -		return rspi_dma_transfer(rspi, NULL, &xfer->rx_sg);
> +	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
> +		int ret = rspi_dma_transfer(rspi, NULL, &xfer->rx_sg);
> +		if (ret != -EAGAIN)
> +			return ret;
> +	}
> 
>  	return rspi_pio_transfer(rspi, NULL, xfer->rx_buf, xfer->len);
>  }

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 1/2] [RFC] spi: rspi: Handle dmaengine_prep_slave_sg() failures gracefully

[Laurent Pinchart]

Hi Geert,

On Wednesday 09 July 2014 12:26:22 Geert Uytterhoeven wrote:
> As typically a shmobile SoC has less DMA channels than devices that can use
> DMA, we may want to prioritize access to the DMA channels in the future.
> This means that dmaengine_prep_slave_sg() may start failing arbitrarily.
> 
> Handle dmaengine_prep_slave_sg() failures gracefully by falling back to
> PIO.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/spi/spi-rspi.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
> index 38fd938d6360..c850dfdfa9e3 100644
> --- a/drivers/spi/spi-rspi.c
> +++ b/drivers/spi/spi-rspi.c
> @@ -477,7 +477,7 @@ static int rspi_dma_transfer(struct rspi_data *rspi,
> struct sg_table *tx, tx->sgl, tx->nents, DMA_TO_DEVICE,
>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>  		if (!desc_tx)
> -			return -EIO;
> +			goto no_dma;
> 
>  		irq_mask |= SPCR_SPTIE;
>  	}
> @@ -486,7 +486,7 @@ static int rspi_dma_transfer(struct rspi_data *rspi,
> struct sg_table *tx, rx->sgl, rx->nents, DMA_FROM_DEVICE,
>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>  		if (!desc_rx)
> -			return -EIO;
> +			goto no_dma;

This is not a new issue introduced by this patch, but aren't you leaking 
desc_tx here ?

>  		irq_mask |= SPCR_SPRIE;
>  	}
> @@ -540,6 +540,12 @@ static int rspi_dma_transfer(struct rspi_data *rspi,
> struct sg_table *tx, enable_irq(rspi->rx_irq);
> 
>  	return ret;
> +
> +no_dma:
> +	pr_warn_once("%s %s: DMA not available, falling back to PIO\n",
> +		     dev_driver_string(&rspi->master->dev),
> +		     dev_name(&rspi->master->dev));
> +	return -EAGAIN;
>  }
> 
>  static void rspi_receive_init(const struct rspi_data *rspi)
> @@ -593,8 +599,10 @@ static int rspi_common_transfer(struct rspi_data *rspi,
> 
>  	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
>  		/* rx_buf can be NULL on RSPI on SH in TX-only Mode */
> -		return rspi_dma_transfer(rspi, &xfer->tx_sg,
> -					 xfer->rx_buf ? &xfer->rx_sg : NULL);
> +		ret = rspi_dma_transfer(rspi, &xfer->tx_sg,
> +					xfer->rx_buf ? &xfer->rx_sg : NULL);
> +		if (ret != -EAGAIN)
> +			return ret;
>  	}
> 
>  	ret = rspi_pio_transfer(rspi, xfer->tx_buf, xfer->rx_buf, xfer->len);
> @@ -648,8 +656,11 @@ static int qspi_transfer_out(struct rspi_data *rspi,
> struct spi_transfer *xfer) {
>  	int ret;
> 
> -	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer))
> -		return rspi_dma_transfer(rspi, &xfer->tx_sg, NULL);
> +	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
> +		ret = rspi_dma_transfer(rspi, &xfer->tx_sg, NULL);
> +		if (ret != -EAGAIN)
> +			return ret;
> +	}
> 
>  	ret = rspi_pio_transfer(rspi, xfer->tx_buf, NULL, xfer->len);
>  	if (ret < 0)
> @@ -663,8 +674,11 @@ static int qspi_transfer_out(struct rspi_data *rspi,
> struct spi_transfer *xfer)
> 
>  static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer
> *xfer) {
> -	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer))
> -		return rspi_dma_transfer(rspi, NULL, &xfer->rx_sg);
> +	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
> +		int ret = rspi_dma_transfer(rspi, NULL, &xfer->rx_sg);
> +		if (ret != -EAGAIN)
> +			return ret;
> +	}
> 
>  	return rspi_pio_transfer(rspi, NULL, xfer->rx_buf, xfer->len);
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] [RFC] spi: sh-msiof: Handle dmaengine_prep_slave_single() failures gracefully

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Wednesday 09 July 2014 12:26:23 Geert Uytterhoeven wrote:
> As typically a shmobile SoC has less DMA channels than devices that can use
> DMA, we may want to prioritize access to the DMA channels in the future.
> This means that dmaengine_prep_slave_single() may start failing
> arbitrarily.
> 
> Handle dmaengine_prep_slave_single() failures gracefully by falling back to
> PIO. This requires moving DMA-specific configuration of the MSIOF device
> after the call(s) to dmaengine_prep_slave_single().
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>

Acked-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

Please see below for a small comment.

> ---
>  drivers/spi/spi-sh-msiof.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
> index 4f0f1cbc92ef..2d7c45bbb6f3 100644
> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
> @@ -636,12 +636,6 @@ static int sh_msiof_dma_once(struct sh_msiof_spi_priv
> *p, const void *tx, dma_cookie_t cookie;
>  	int ret;
> 
> -	/* 1 stage FIFO watermarks for DMA */
> -	sh_msiof_write(p, FCTR, FCTR_TFWM_1 | FCTR_RFWM_1);
> -
> -	/* setup msiof transfer mode registers (32-bit words) */
> -	sh_msiof_spi_set_mode_regs(p, tx, rx, 32, len / 4);
> -
>  	if (tx) {
>  		ier_bits |= IER_TDREQE | IER_TDMAE;
>  		dma_sync_single_for_device(&p->pdev->dev, p->tx_dma_addr, len,
> @@ -650,7 +644,7 @@ static int sh_msiof_dma_once(struct sh_msiof_spi_priv
> *p, const void *tx, p->tx_dma_addr, len, DMA_TO_DEVICE,
>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>  		if (!desc_tx)
> -			return -EIO;
> +			return -EAGAIN;
>  	}
> 
>  	if (rx) {
> @@ -659,8 +653,15 @@ static int sh_msiof_dma_once(struct sh_msiof_spi_priv
> *p, const void *tx, p->rx_dma_addr, len, DMA_FROM_DEVICE,
>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>  		if (!desc_rx)
> -			return -EIO;
> +			return -EAGAIN;

This isn't a new issue introduced by this patch, but aren't you leaking 
desc_tx here ?

>  	}
> +
> +	/* 1 stage FIFO watermarks for DMA */
> +	sh_msiof_write(p, FCTR, FCTR_TFWM_1 | FCTR_RFWM_1);
> +
> +	/* setup msiof transfer mode registers (32-bit words) */
> +	sh_msiof_spi_set_mode_regs(p, tx, rx, 32, len / 4);
> +
>  	sh_msiof_write(p, IER, ier_bits);
> 
>  	reinit_completion(&p->done);
> @@ -822,6 +823,12 @@ static int sh_msiof_transfer_one(struct spi_master
> *master, copy32(p->tx_dma_page, tx_buf, l / 4);
> 
>  		ret = sh_msiof_dma_once(p, tx_buf, rx_buf, l);
> +		if (ret == -EAGAIN) {
> +			pr_warn_once("%s %s: DMA not available, falling back to PIO\n",
> +				     dev_driver_string(&p->pdev->dev),
> +				     dev_name(&p->pdev->dev));
> +			break;
> +		}
>  		if (ret)
>  			return ret;

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 1/2] [RFC] spi: rspi: Handle dmaengine_prep_slave_sg() failures gracefully

[Geert Uytterhoeven]

Hi Laurent,

On Thu, Jul 10, 2014 at 1:05 PM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:
> On Wednesday 09 July 2014 12:26:22 Geert Uytterhoeven wrote:
>> As typically a shmobile SoC has less DMA channels than devices that can use
>> DMA, we may want to prioritize access to the DMA channels in the future.
>> This means that dmaengine_prep_slave_sg() may start failing arbitrarily.
>>
>> Handle dmaengine_prep_slave_sg() failures gracefully by falling back to
>> PIO.
>
> Just a random thought, do you think there would be performance-sensitive cases
> where failing the transfer completely would be better than using PIO ?

QSPI Flash reads gain most from DMA.

However, failing transfers (and thus the whole SPI message, which typically
consists of multiple transfers) means we'll have to retry the message later.
This could be done by the SPI driver, the SPI subsystem, or the upper layer.
E.g. m25p80 and mtd do not retry SPI FLASH reads and writes.

>> Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
>
> -EAGAIN might not be the best error code to return in this case, as it would
> imply that the caller should just call rspi_dma_transfer() again. On the other
> hand I don't see another error code that would be a perfect match, so

I followed the exact same reasoning to arrive at -EAGAIN...

BTW, calling rspi_dma_transfer() again is an option ;-)

> Acked-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] [RFC] spi: rspi: Handle dmaengine_prep_slave_sg() failures gracefully

[Geert Uytterhoeven]

Hi Laurent,

(cc dmaengine)

On Thu, Jul 10, 2014 at 1:08 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> --- a/drivers/spi/spi-rspi.c
>> +++ b/drivers/spi/spi-rspi.c
>> @@ -477,7 +477,7 @@ static int rspi_dma_transfer(struct rspi_data *rspi,
>> struct sg_table *tx, tx->sgl, tx->nents, DMA_TO_DEVICE,
>>                                       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>>               if (!desc_tx)
>> -                     return -EIO;
>> +                     goto no_dma;
>>
>>               irq_mask |= SPCR_SPTIE;
>>       }
>> @@ -486,7 +486,7 @@ static int rspi_dma_transfer(struct rspi_data *rspi,
>> struct sg_table *tx, rx->sgl, rx->nents, DMA_FROM_DEVICE,
>>                                       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>>               if (!desc_rx)
>> -                     return -EIO;
>> +                     goto no_dma;
>
> This is not a new issue introduced by this patch, but aren't you leaking
> desc_tx here ?

AFAIK, descriptors are cleaned up automatically after use, and the only
function that takes a dma_async_tx_descriptor is dmaengine_submit().

But indeed, if you don't use them, that doesn't happen.
So calling dmaengine_terminate_all() seems appropriate to fix this.

But, Documentation/dmaengine.txt says:

        desc = dmaengine_prep_slave_sg(chan, sgl, nr_sg, direction, flags);

   Once a descriptor has been obtained, the callback information can be
   added and the descriptor must then be submitted.  Some DMA engine
   drivers may hold a spinlock between a successful preparation and
   submission so it is important that these two operations are closely
   paired.

W.r.t. the spinlock, is it safe to call dmaengine_terminate_all() for a prepared
but not submitted transfer?
Is there another/better way?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] [RFC] spi: rspi: Handle dmaengine_prep_slave_sg() failures gracefully

[Laurent Pinchart]

Hi Geert,

On Thursday 10 July 2014 13:55:43 Geert Uytterhoeven wrote:
> On Thu, Jul 10, 2014 at 1:08 PM, Laurent Pinchart wrote:
> >> --- a/drivers/spi/spi-rspi.c
> >> +++ b/drivers/spi/spi-rspi.c
> >> @@ -477,7 +477,7 @@ static int rspi_dma_transfer(struct rspi_data *rspi,
> >> struct sg_table *tx, tx->sgl, tx->nents, DMA_TO_DEVICE,
> >>                                       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> >>               if (!desc_tx)
> >> -                     return -EIO;
> >> +                     goto no_dma;
> >>               irq_mask |= SPCR_SPTIE;
> >>       }
> >> 
> >> @@ -486,7 +486,7 @@ static int rspi_dma_transfer(struct rspi_data *rspi,
> >> struct sg_table *tx, rx->sgl, rx->nents, DMA_FROM_DEVICE,
> >>                                       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> >>               if (!desc_rx)
> >> -                     return -EIO;
> >> +                     goto no_dma;
> > 
> > This is not a new issue introduced by this patch, but aren't you leaking
> > desc_tx here ?
> 
> AFAIK, descriptors are cleaned up automatically after use, and the only
> function that takes a dma_async_tx_descriptor is dmaengine_submit().
> 
> But indeed, if you don't use them, that doesn't happen.
> So calling dmaengine_terminate_all() seems appropriate to fix this.
> 
> But, Documentation/dmaengine.txt says:
> 
>         desc = dmaengine_prep_slave_sg(chan, sgl, nr_sg, direction, flags);
> 
>    Once a descriptor has been obtained, the callback information can be
>    added and the descriptor must then be submitted.  Some DMA engine
>    drivers may hold a spinlock between a successful preparation and
>    submission so it is important that these two operations are closely
>    paired.
> 
> W.r.t. the spinlock, is it safe to call dmaengine_terminate_all() for a
> prepared but not submitted transfer?
> Is there another/better way?

Basically, I have no idea. I'm pretty sure some drivers will support it, 
others won't. Reading the code won't help much, as there's no available 
information regarding what the expected behaviour is. Welcome to the wonderful 
world of the undocumented DMA engine API :-)

The best way to move forward would be to decide on a behaviour and document 
it. If nobody objects, drivers that don't implement the correct behaviour 
could be considered as broken, and should be fixed. If someone objects, then a 
discussion should spring up, and hopefully an agreement will be achieved on 
what the correct behaviour is.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 1/2] [RFC] spi: rspi: Handle dmaengine_prep_slave_sg() failures gracefully

[Geert Uytterhoeven]

Hi Laurent,

On Fri, Jul 11, 2014 at 1:47 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Thursday 10 July 2014 13:55:43 Geert Uytterhoeven wrote:
>> On Thu, Jul 10, 2014 at 1:08 PM, Laurent Pinchart wrote:
>> >> --- a/drivers/spi/spi-rspi.c
>> >> +++ b/drivers/spi/spi-rspi.c
>> >> @@ -477,7 +477,7 @@ static int rspi_dma_transfer(struct rspi_data *rspi,
>> >> struct sg_table *tx, tx->sgl, tx->nents, DMA_TO_DEVICE,
>> >>                                       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>> >>               if (!desc_tx)
>> >> -                     return -EIO;
>> >> +                     goto no_dma;
>> >>               irq_mask |= SPCR_SPTIE;
>> >>       }
>> >>
>> >> @@ -486,7 +486,7 @@ static int rspi_dma_transfer(struct rspi_data *rspi,
>> >> struct sg_table *tx, rx->sgl, rx->nents, DMA_FROM_DEVICE,
>> >>                                       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>> >>               if (!desc_rx)
>> >> -                     return -EIO;
>> >> +                     goto no_dma;
>> >
>> > This is not a new issue introduced by this patch, but aren't you leaking
>> > desc_tx here ?
>>
>> AFAIK, descriptors are cleaned up automatically after use, and the only
>> function that takes a dma_async_tx_descriptor is dmaengine_submit().
>>
>> But indeed, if you don't use them, that doesn't happen.
>> So calling dmaengine_terminate_all() seems appropriate to fix this.
>>
>> But, Documentation/dmaengine.txt says:
>>
>>         desc = dmaengine_prep_slave_sg(chan, sgl, nr_sg, direction, flags);
>>
>>    Once a descriptor has been obtained, the callback information can be
>>    added and the descriptor must then be submitted.  Some DMA engine
>>    drivers may hold a spinlock between a successful preparation and
>>    submission so it is important that these two operations are closely
>>    paired.
>>
>> W.r.t. the spinlock, is it safe to call dmaengine_terminate_all() for a
>> prepared but not submitted transfer?
>> Is there another/better way?
>
> Basically, I have no idea. I'm pretty sure some drivers will support it,
> others won't. Reading the code won't help much, as there's no available
> information regarding what the expected behaviour is. Welcome to the wonderful
> world of the undocumented DMA engine API :-)

I did dive a bit into the code...

1.  The spinlock comment seems to apply to INTEL_IOATDMA only.
    This driver doesn't implement dma_device.device_control(), so
    dmaengine_terminate_all() is a no-op there, and there doesn't seem to be
    a way to release a descriptor without submitting it first.

2.  While I thought dmaengine_terminate_all() would release all descriptors
    on shdma, as it calls shdma_chan_ld_cleanup(), it only releases the
    descriptors that are at least in submitted state.
    Hence after a while, you get

        sh-dma-engine e6700020.dma-controller: No free link descriptor available

    Interestingly, this contradicts with the comment in
    shdma_free_chan_resources():

        /* Prepared and not submitted descriptors can still be on the queue */
        if (!list_empty(&schan->ld_queue))
                shdma_chan_ld_cleanup(schan, true);

As dmaengine_submit() will not start the DMA operation, but merely add it
to the pending queue (starting needs a call to dma_async_issue_pending()),
it seems the right solution is to continue submitting the request for which
preparation succeeded, and then aborting everything using
dmaengine_terminate_all().

Note that this also means that if submitting the RX request failed, you should
still submit the TX request, as it has been prepared.

Alternatively, you can interleave prep and submit for both channels, which
makes the error recovery code less convoluted.

> The best way to move forward would be to decide on a behaviour and document
> it. If nobody objects, drivers that don't implement the correct behaviour
> could be considered as broken, and should be fixed. If someone objects, then a
> discussion should spring up, and hopefully an agreement will be achieved on
> what the correct behaviour is.

Right...

The document already says "the descriptor must then be submitted",
which matches with the above.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] [RFC] spi: rspi: Handle dmaengine_prep_slave_sg() failures gracefully

[Laurent Pinchart]

Hi Geert,

On Friday 11 July 2014 15:22:14 Geert Uytterhoeven wrote:
> On Fri, Jul 11, 2014 at 1:47 AM, Laurent Pinchart wrote:
> > On Thursday 10 July 2014 13:55:43 Geert Uytterhoeven wrote:
> >> On Thu, Jul 10, 2014 at 1:08 PM, Laurent Pinchart wrote:
> >> >> --- a/drivers/spi/spi-rspi.c
> >> >> +++ b/drivers/spi/spi-rspi.c
> >> >> @@ -477,7 +477,7 @@ static int rspi_dma_transfer(struct rspi_data
> >> >> *rspi,
> >> >> struct sg_table *tx, tx->sgl, tx->nents, DMA_TO_DEVICE,
> >> >>                                       DMA_PREP_INTERRUPT |
> >> >>                                       DMA_CTRL_ACK);
> >> >>               if (!desc_tx)
> >> >> -                     return -EIO;
> >> >> +                     goto no_dma;
> >> >> 
> >> >>               irq_mask |= SPCR_SPTIE;
> >> >>       }
> >> >> @@ -486,7 +486,7 @@ static int rspi_dma_transfer(struct rspi_data
> >> >> *rspi,
> >> >> struct sg_table *tx, rx->sgl, rx->nents, DMA_FROM_DEVICE,
> >> >>                                       DMA_PREP_INTERRUPT |
> >> >>                                       DMA_CTRL_ACK);
> >> >>               if (!desc_rx)
> >> >> -                     return -EIO;
> >> >> +                     goto no_dma;
> >> > 
> >> > This is not a new issue introduced by this patch, but aren't you
> >> > leaking desc_tx here ?
> >> 
> >> AFAIK, descriptors are cleaned up automatically after use, and the only
> >> function that takes a dma_async_tx_descriptor is dmaengine_submit().
> >> 
> >> But indeed, if you don't use them, that doesn't happen.
> >> So calling dmaengine_terminate_all() seems appropriate to fix this.
> >> 
> >> But, Documentation/dmaengine.txt says:
> >>         desc = dmaengine_prep_slave_sg(chan, sgl, nr_sg, direction,
> >>         flags);
> >>    
> >>    Once a descriptor has been obtained, the callback information can be
> >>    added and the descriptor must then be submitted.  Some DMA engine
> >>    drivers may hold a spinlock between a successful preparation and
> >>    submission so it is important that these two operations are closely
> >>    paired.
> >> 
> >> W.r.t. the spinlock, is it safe to call dmaengine_terminate_all() for a
> >> prepared but not submitted transfer?
> >> Is there another/better way?
> > 
> > Basically, I have no idea. I'm pretty sure some drivers will support it,
> > others won't. Reading the code won't help much, as there's no available
> > information regarding what the expected behaviour is. Welcome to the
> > wonderful world of the undocumented DMA engine API :-)
> 
> I did dive a bit into the code...
> 
> 1.  The spinlock comment seems to apply to INTEL_IOATDMA only.
>     This driver doesn't implement dma_device.device_control(), so
>     dmaengine_terminate_all() is a no-op there, and there doesn't seem to be
>     a way to release a descriptor without submitting it first.
> 
> 2.  While I thought dmaengine_terminate_all() would release all descriptors
>     on shdma, as it calls shdma_chan_ld_cleanup(), it only releases the
>     descriptors that are at least in submitted state.
>     Hence after a while, you get
> 
>         sh-dma-engine e6700020.dma-controller: No free link descriptor
>         available
> 
>     Interestingly, this contradicts with the comment in
>     shdma_free_chan_resources():
> 
>         /* Prepared and not submitted descriptors can still be on the queue
> */
>         if (!list_empty(&schan->ld_queue))
>                 shdma_chan_ld_cleanup(schan, true);
> 
> As dmaengine_submit() will not start the DMA operation, but merely add it
> to the pending queue (starting needs a call to dma_async_issue_pending()),
> it seems the right solution is to continue submitting the request for which
> preparation succeeded, and then aborting everything using
> dmaengine_terminate_all().
> 
> Note that this also means that if submitting the RX request failed, you
> should still submit the TX request, as it has been prepared.
> 
> Alternatively, you can interleave prep and submit for both channels, which
> makes the error recovery code less convoluted.

How about fixing the DMA API to allow cleaning up a prepared request without 
submitting it ?

> > The best way to move forward would be to decide on a behaviour and
> > document it. If nobody objects, drivers that don't implement the correct
> > behaviour could be considered as broken, and should be fixed. If someone
> > objects, then a discussion should spring up, and hopefully an agreement
> > will be achieved on what the correct behaviour is.
> 
> Right...
> 
> The document already says "the descriptor must then be submitted",
> which matches with the above.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] [RFC] spi: rspi: Handle dmaengine_prep_slave_sg() failures gracefully

[Geert Uytterhoeven]

Hi Laurent,

On Fri, Jul 11, 2014 at 3:27 PM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:
>> >> AFAIK, descriptors are cleaned up automatically after use, and the only
>> >> function that takes a dma_async_tx_descriptor is dmaengine_submit().
>> >> But indeed, if you don't use them, that doesn't happen.
>> >> So calling dmaengine_terminate_all() seems appropriate to fix this.
>> >>
>> >> But, Documentation/dmaengine.txt says:
>> >>         desc = dmaengine_prep_slave_sg(chan, sgl, nr_sg, direction,
>> >>         flags);
>> >>
>> >>    Once a descriptor has been obtained, the callback information can be
>> >>    added and the descriptor must then be submitted.  Some DMA engine
>> >>    drivers may hold a spinlock between a successful preparation and
>> >>    submission so it is important that these two operations are closely
>> >>    paired.
>> >>
>> >> W.r.t. the spinlock, is it safe to call dmaengine_terminate_all() for a
>> >> prepared but not submitted transfer?
>> >> Is there another/better way?
>> >
>> > Basically, I have no idea. I'm pretty sure some drivers will support it,
>> > others won't. Reading the code won't help much, as there's no available
>> > information regarding what the expected behaviour is. Welcome to the
>> > wonderful world of the undocumented DMA engine API :-)
>>
>> I did dive a bit into the code...
>>
>> 1.  The spinlock comment seems to apply to INTEL_IOATDMA only.
>>     This driver doesn't implement dma_device.device_control(), so
>>     dmaengine_terminate_all() is a no-op there, and there doesn't seem to be
>>     a way to release a descriptor without submitting it first.
>>
>> 2.  While I thought dmaengine_terminate_all() would release all descriptors
>>     on shdma, as it calls shdma_chan_ld_cleanup(), it only releases the
>>     descriptors that are at least in submitted state.
>>     Hence after a while, you get
>>
>>         sh-dma-engine e6700020.dma-controller: No free link descriptor
>>         available
>>
>>     Interestingly, this contradicts with the comment in
>>     shdma_free_chan_resources():
>>
>>         /* Prepared and not submitted descriptors can still be on the queue
>> */
>>         if (!list_empty(&schan->ld_queue))
>>                 shdma_chan_ld_cleanup(schan, true);
>>
>> As dmaengine_submit() will not start the DMA operation, but merely add it
>> to the pending queue (starting needs a call to dma_async_issue_pending()),
>> it seems the right solution is to continue submitting the request for which
>> preparation succeeded, and then aborting everything using
>> dmaengine_terminate_all().
>>
>> Note that this also means that if submitting the RX request failed, you
>> should still submit the TX request, as it has been prepared.
>>
>> Alternatively, you can interleave prep and submit for both channels, which
>> makes the error recovery code less convoluted.
>
> How about fixing the DMA API to allow cleaning up a prepared request without
> submitting it ?

That's another option. But that would require updating all drivers.

Note that only ioat, iop-adma, mv_xor, and ppc4xx do not implement
.device_control() and/or DMA_TERMINATE_ALL.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] [RFC] spi: rspi: Handle dmaengine_prep_slave_sg() failures gracefully

[Laurent Pinchart]

Hi Geert,

On Friday 11 July 2014 15:58:07 Geert Uytterhoeven wrote:
> On Fri, Jul 11, 2014 at 3:27 PM, Laurent Pinchart wrote:
> >> >> AFAIK, descriptors are cleaned up automatically after use, and the
> >> >> only function that takes a dma_async_tx_descriptor is
> >> >> dmaengine_submit(). But indeed, if you don't use them, that doesn't
> >> >> happen. So calling dmaengine_terminate_all() seems appropriate to fix
> >> >> this.
> >> >> 
> >> >> But, Documentation/dmaengine.txt says:
> >> >>         desc = dmaengine_prep_slave_sg(chan, sgl, nr_sg, direction,
> >> >>         flags);
> >> >>    
> >> >>    Once a descriptor has been obtained, the callback information can
> >> >>    be added and the descriptor must then be submitted.  Some DMA
> >> >>    engine drivers may hold a spinlock between a successful preparation
> >> >>    and submission so it is important that these two operations are
> >> >>    closely paired.
> >> >> 
> >> >> W.r.t. the spinlock, is it safe to call dmaengine_terminate_all() for
> >> >> a prepared but not submitted transfer?
> >> >> Is there another/better way?
> >> > 
> >> > Basically, I have no idea. I'm pretty sure some drivers will support
> >> > it, others won't. Reading the code won't help much, as there's no
> >> > available information regarding what the expected behaviour is. Welcome
> >> > to the wonderful world of the undocumented DMA engine API :-)
> >> 
> >> I did dive a bit into the code...
> >> 
> >> 1.  The spinlock comment seems to apply to INTEL_IOATDMA only.
> >> 
> >>     This driver doesn't implement dma_device.device_control(), so
> >>     dmaengine_terminate_all() is a no-op there, and there doesn't seem to
> >>     be a way to release a descriptor without submitting it first.
> >> 
> >> 2.  While I thought dmaengine_terminate_all() would release all
> >>     descriptors on shdma, as it calls shdma_chan_ld_cleanup(), it only
> >>     releases the descriptors that are at least in submitted state.
> >>     Hence after a while, you get
> >>     
> >>         sh-dma-engine e6700020.dma-controller: No free link descriptor
> >>         available
> >>     
> >>     Interestingly, this contradicts with the comment in
> >>     shdma_free_chan_resources():
> >>     
> >>         /* Prepared and not submitted descriptors can still be on the
> >>         queue */
> >>         if (!list_empty(&schan->ld_queue))
> >>                 shdma_chan_ld_cleanup(schan, true);
> >> 
> >> As dmaengine_submit() will not start the DMA operation, but merely add it
> >> to the pending queue (starting needs a call to
> >> dma_async_issue_pending()),it seems the right solution is to continue
> >> submitting the request for which preparation succeeded, and then aborting
> >> everything using dmaengine_terminate_all().
> >> 
> >> Note that this also means that if submitting the RX request failed, you
> >> should still submit the TX request, as it has been prepared.
> >> 
> >> Alternatively, you can interleave prep and submit for both channels,
> >> which
> >> makes the error recovery code less convoluted.
> > 
> > How about fixing the DMA API to allow cleaning up a prepared request
> > without submitting it ?
> 
> That's another option. But that would require updating all drivers.

Isn't it worth it if the API can be made simpler and more robust ? Even though 
the number of drivers to change isn't small, the update could be rolled out 
gradually.

I wonder what the rationale for the DMA engine cookie system was, and if it 
still applies today. Wouldn't it be easier if slave drivers stored pointers to 
the async_tx descriptors instead of storing cookies, and used them in place of 
cookies through the whole API ? Slaves would then need to release the 
descriptors explicitly, which could occur at any point of time if they were 
reference counted.

> Note that only ioat, iop-adma, mv_xor, and ppc4xx do not implement
> .device_control() and/or DMA_TERMINATE_ALL.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 1/2] [RFC] spi: rspi: Handle dmaengine_prep_slave_sg() failures gracefully

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 319 bytes --]

On Wed, Jul 09, 2014 at 12:26:22PM +0200, Geert Uytterhoeven wrote:
> As typically a shmobile SoC has less DMA channels than devices that can use
> DMA, we may want to prioritize access to the DMA channels in the future.
> This means that dmaengine_prep_slave_sg() may start failing arbitrarily.

Applied both, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]