[PATCH] mmc: sh_mmcif: correctly report success when obtaining DMA channels

[PATCH] mmc: sh_mmcif: correctly report success when obtaining DMA channels

[Wolfram Sang]

The debug message could still report success when getting the channels
was OK but configuring them failed. This actually caused a minor detour
when debugging DMA problems, so make sure the success is only reported
when the channels are really ready-to-use.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/sh_mmcif.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 08b4312af94e..0e7fa3e9c9fe 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -439,14 +439,15 @@ static void sh_mmcif_request_dma(struct sh_mmcif_host *host)
 		if (IS_ERR(host->chan_rx))
 			host->chan_rx = NULL;
 	}
-	dev_dbg(dev, "%s: got channel TX %p RX %p\n", __func__, host->chan_tx,
-		host->chan_rx);
 
 	if (!host->chan_tx || !host->chan_rx ||
 	    sh_mmcif_dma_slave_config(host, host->chan_tx, DMA_MEM_TO_DEV) ||
 	    sh_mmcif_dma_slave_config(host, host->chan_rx, DMA_DEV_TO_MEM))
 		goto error;
 
+	dev_dbg(dev, "%s: got channel TX %p RX %p\n", __func__, host->chan_tx,
+		host->chan_rx);
+
 	return;
 
 error:
-- 
2.45.2

Re: [PATCH] mmc: sh_mmcif: correctly report success when obtaining DMA channels

[Geert Uytterhoeven]

Hi Wolfram,

On Sat, Sep 28, 2024 at 11:45 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> The debug message could still report success when getting the channels
> was OK but configuring them failed. This actually caused a minor detour
> when debugging DMA problems, so make sure the success is only reported
> when the channels are really ready-to-use.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -439,14 +439,15 @@ static void sh_mmcif_request_dma(struct sh_mmcif_host *host)
>                 if (IS_ERR(host->chan_rx))
>                         host->chan_rx = NULL;
>         }
> -       dev_dbg(dev, "%s: got channel TX %p RX %p\n", __func__, host->chan_tx,
> -               host->chan_rx);

This was not a real success indicator, which could indeed confuse
people, but an obfuscated NULL-pointer still prints as NULL, right?

>         if (!host->chan_tx || !host->chan_rx ||
>             sh_mmcif_dma_slave_config(host, host->chan_tx, DMA_MEM_TO_DEV) ||
>             sh_mmcif_dma_slave_config(host, host->chan_rx, DMA_DEV_TO_MEM))
>                 goto error;
>
> +       dev_dbg(dev, "%s: got channel TX %p RX %p\n", __func__, host->chan_tx,
> +               host->chan_rx);

This means we no longer see a debug message in case only one DMA
channel could be requested, thus requiring manual addition of more code
to find out what was really wrong?

> +
>         return;
>
>  error:

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] mmc: sh_mmcif: correctly report success when obtaining DMA channels

[Wolfram Sang]

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


> > -       dev_dbg(dev, "%s: got channel TX %p RX %p\n", __func__, host->chan_tx,
> > -               host->chan_rx);
> 
> This was not a real success indicator, which could indeed confuse
> people, but an obfuscated NULL-pointer still prints as NULL, right?

Well...

> 
> >         if (!host->chan_tx || !host->chan_rx ||
> >             sh_mmcif_dma_slave_config(host, host->chan_tx, DMA_MEM_TO_DEV) ||
> >             sh_mmcif_dma_slave_config(host, host->chan_rx, DMA_DEV_TO_MEM))
> >                 goto error;
> >
> > +       dev_dbg(dev, "%s: got channel TX %p RX %p\n", __func__, host->chan_tx,
> > +               host->chan_rx);
> 
> This means we no longer see a debug message in case only one DMA
> channel could be requested, thus requiring manual addition of more code
> to find out what was really wrong?

... you can simply enable basic DMA debugging to find this out. I don't
see value in adding a separate debug printout in the driver.


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

Re: [PATCH] mmc: sh_mmcif: correctly report success when obtaining DMA channels

[Ulf Hansson]

On Sat, 28 Sept 2024 at 11:44, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> The debug message could still report success when getting the channels
> was OK but configuring them failed. This actually caused a minor detour
> when debugging DMA problems, so make sure the success is only reported
> when the channels are really ready-to-use.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied for next, thanks!

Kind regards
Uffe


> ---
>  drivers/mmc/host/sh_mmcif.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index 08b4312af94e..0e7fa3e9c9fe 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -439,14 +439,15 @@ static void sh_mmcif_request_dma(struct sh_mmcif_host *host)
>                 if (IS_ERR(host->chan_rx))
>                         host->chan_rx = NULL;
>         }
> -       dev_dbg(dev, "%s: got channel TX %p RX %p\n", __func__, host->chan_tx,
> -               host->chan_rx);
>
>         if (!host->chan_tx || !host->chan_rx ||
>             sh_mmcif_dma_slave_config(host, host->chan_tx, DMA_MEM_TO_DEV) ||
>             sh_mmcif_dma_slave_config(host, host->chan_rx, DMA_DEV_TO_MEM))
>                 goto error;
>
> +       dev_dbg(dev, "%s: got channel TX %p RX %p\n", __func__, host->chan_tx,
> +               host->chan_rx);
> +
>         return;
>
>  error:
> --
> 2.45.2
>