[RFC PATCH] mmc: renesas_sdhi_internal_dmac: fall back to PIO if scatterlist doesn't match

[RFC PATCH] mmc: renesas_sdhi_internal_dmac: fall back to PIO if scatterlist doesn't match

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

If we detect an incompatible scatterlist, we should fall back to PIO,
too.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

I found this while working on the RX DMA issue. I don't see a reason why we
shouldn't fall back in this case as well. But maybe I am missing something.

Shimoda-san: what do you think?

 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index 380570a26a09..561e90755a3b 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -161,11 +161,7 @@ renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
 	enum dma_data_direction dir;
 	int ret;
 
-	/* This DMAC cannot handle if sg_len is not 1 */
-	WARN_ON(host->sg_len > 1);
-
-	/* This DMAC cannot handle if buffer is not 8-bytes alignment */
-	if (!IS_ALIGNED(sg->offset, 8))
+	if (WARN_ON(host->sg_len > 1) || !IS_ALIGNED(sg->offset, 8))
 		goto force_pio;
 
 	if (data->flags & MMC_DATA_READ) {
-- 
2.11.0

Re: [RFC PATCH] mmc: renesas_sdhi_internal_dmac: fall back to PIO if scatterlist doesn't match

[Ulf Hansson]

On 4 April 2018 at 19:00, Wolfram Sang <wsa@the-dreams.de> wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> If we detect an incompatible scatterlist, we should fall back to PIO,
> too.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> I found this while working on the RX DMA issue. I don't see a reason why we
> shouldn't fall back in this case as well. But maybe I am missing something.
>
> Shimoda-san: what do you think?
>
>  drivers/mmc/host/renesas_sdhi_internal_dmac.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> index 380570a26a09..561e90755a3b 100644
> --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> @@ -161,11 +161,7 @@ renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
>         enum dma_data_direction dir;
>         int ret;
>
> -       /* This DMAC cannot handle if sg_len is not 1 */
> -       WARN_ON(host->sg_len > 1);
> -
> -       /* This DMAC cannot handle if buffer is not 8-bytes alignment */
> -       if (!IS_ALIGNED(sg->offset, 8))
> +       if (WARN_ON(host->sg_len > 1) || !IS_ALIGNED(sg->offset, 8))

The WARN_ON becomes a bit misleading being a part of the if statement,
as it should never happen when the driver has set ->max_segs = 1.

For the alignment check, and the moving to PIO, this certainly makes
sense to me.

>                 goto force_pio;
>
>         if (data->flags & MMC_DATA_READ) {

Kind regards
Uffe

Re: [RFC PATCH] mmc: renesas_sdhi_internal_dmac: fall back to PIO if scatterlist doesn't match

[Wolfram Sang]

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


> > -       /* This DMAC cannot handle if sg_len is not 1 */
> > -       WARN_ON(host->sg_len > 1);
> > -
> > -       /* This DMAC cannot handle if buffer is not 8-bytes alignment */
> > -       if (!IS_ALIGNED(sg->offset, 8))
> > +       if (WARN_ON(host->sg_len > 1) || !IS_ALIGNED(sg->offset, 8))
> 
> The WARN_ON becomes a bit misleading being a part of the if statement,
> as it should never happen when the driver has set ->max_segs = 1.

Uh, yeah, right. So, we could simply drop the WARN_ON?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [RFC PATCH] mmc: renesas_sdhi_internal_dmac: fall back to PIO if scatterlist doesn't match

[Yoshihiro Shimoda]

Hi Wolfram-san,

Thank you for the patch!

> From: Wolfram Sang, Sent: Thursday, April 5, 2018 2:01 AM
> 
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> If we detect an incompatible scatterlist, we should fall back to PIO,
> too.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> I found this while working on the RX DMA issue. I don't see a reason why we
> shouldn't fall back in this case as well. But maybe I am missing something.
> 
> Shimoda-san: what do you think?
> 
>  drivers/mmc/host/renesas_sdhi_internal_dmac.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> index 380570a26a09..561e90755a3b 100644
> --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> @@ -161,11 +161,7 @@ renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
>  	enum dma_data_direction dir;
>  	int ret;
> 
> -	/* This DMAC cannot handle if sg_len is not 1 */

I would like to keep this comment.

> -	WARN_ON(host->sg_len > 1);
> -
> -	/* This DMAC cannot handle if buffer is not 8-bytes alignment */
> -	if (!IS_ALIGNED(sg->offset, 8))
> +	if (WARN_ON(host->sg_len > 1) || !IS_ALIGNED(sg->offset, 8))

As Ulf-san said on other thread about this WARN_ON(),
I think dropping the WARN_ON() is better.

Best regards,
Yoshihiro Shimoda