diff mbox series

[2/6] ASoC: sunxi: sun4i-codec: Drop redundant error messages

Message ID 20260715095525.40668-3-phucduc.bui@gmail.com (mailing list archive)
State New
Headers
Series ASoC: sunxi: Simplify error handling |

Commit Message

Bui Duc Phuc July 15, 2026, 9:55 a.m. UTC 
From: bui duc phuc <phucduc.bui@gmail.com>

The called functions already log failures where appropriate. Return the
original error directly and avoid duplicate error messages.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/sunxi/sun4i-codec.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

Comments

Chen-Yu Tsai July 15, 2026, 3:02 p.m. UTC | #1 
On Wed, Jul 15, 2026 at 5:56 PM <phucduc.bui@gmail.com> wrote:
>
> From: bui duc phuc <phucduc.bui@gmail.com>
>
> The called functions already log failures where appropriate. Return the
> original error directly and avoid duplicate error messages.
>
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>  sound/soc/sunxi/sun4i-codec.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c
> index 05308df3ae5b..71f7a1fedd88 100644
> --- a/sound/soc/sunxi/sun4i-codec.c
> +++ b/sound/soc/sunxi/sun4i-codec.c
> @@ -2380,31 +2380,22 @@ static int sun4i_codec_probe(struct platform_device *pdev)
>
>         ret = devm_snd_soc_register_component(&pdev->dev, quirks->codec,
>                                      &sun4i_codec_dai, 1);
> -       if (ret) {
> -               dev_err(&pdev->dev, "Failed to register our codec\n");
> +       if (ret)
>                 return ret;
> -       }
>
>         ret = devm_snd_soc_register_component(&pdev->dev,
>                                               &sun4i_codec_component,
>                                               &dummy_cpu_dai, 1);
> -       if (ret) {
> -               dev_err(&pdev->dev, "Failed to register our DAI\n");
> +       if (ret)
>                 return ret;
> -       }
>
>         ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);

snd_dmaengine_pcm_register() doesn't seem to print an error when
dma_request_chan() fails.

> -       if (ret) {
> -               dev_err(&pdev->dev, "Failed to register against DMAEngine\n");
> +       if (ret)
>                 return ret;
> -       }
>
>         card = quirks->create_card(&pdev->dev);
> -       if (IS_ERR(card)) {
> -               ret = PTR_ERR(card);
> -               dev_err(&pdev->dev, "Failed to create our card\n");
> -               return ret;
> -       }
> +       if (IS_ERR(card))
> +               return PTR_ERR(card);
>
>         snd_soc_card_set_drvdata(card, scodec);
>
> --
> 2.43.0
>
Bui Duc Phuc July 16, 2026, 6:19 a.m. UTC | #2 
Hi Chen-Yu Tsai,

Thank you for your review.


> >         ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
>
> snd_dmaengine_pcm_register() doesn't seem to print an error when
> dma_request_chan() fails.
>
> > -       if (ret) {
> > -               dev_err(&pdev->dev, "Failed to register against DMAEngine\n");
> > +       if (ret)
> >                 return ret;
> > -       }
> >

You are right that dma_request_chan() itself has several code paths where
it doesn't print any error logs when it fails.
However, if we look at how the caller handles the return value of
dma_request_chan():

---------------------------------
chan = dma_request_chan(dev, name);
if (IS_ERR(chan)) {
                /*
                * Only report probe deferral errors, channels
                * might not be present for devices that
                * support only TX or only RX.
                */
                if (PTR_ERR(chan) == -EPROBE_DEFER)
                        return -EPROBE_DEFER;
                pcm->chan[i] = NULL;
} else {
                pcm->chan[i] = chan;
}
---------------------------------------

As shown here, the code only propagates a single error code, -EPROBE_DEFER
regardless of whatever other errors might have occurred. The reasoning behind
this is already well-documented in the comment block above.

Therefore, keeping the following error log is not particularly useful:

----------------------
ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
if (ret) {
        dev_err(&pdev->dev, "Failed to register against DMAEngine\n");
        return ret;
}
-----------------------

This logging statement does not make much sense here because if the returned
error is -EPROBE_DEFER , we should definitely avoid printing error messages to
the kernel log.

Let me know if this makes sense to you, or if you'd still prefer to keep
the log with a check to avoid printing on -EPROBE_DEFER.

Best regards,
Phuc
Bui Duc Phuc July 17, 2026, 2:43 a.m. UTC | #3 
Hi all,

Actually, following our discussion about dma_request_chan() not always
printing error logs internally, I did some further investigation into
the code paths and have submitted two patches to address the underlying
issues more comprehensively:

1. A patch for dma_request_chan() itself to prevent potential NULL pointer
   dereferences on invalid arguments:
   https://lore.kernel.org/all/20260716052758.23465-1-phucduc.bui@gmail.com/

2. A patch to improve the error handling flow inside
   dmaengine_pcm_request_chan_of() when calling dma_request_chan():
   https://lore.kernel.org/all/20260716080428.57979-1-phucduc.bui@gmail.com/

I wanted to share this here to provide more context on the proposed changes to
these DMA engine error-handling flows.

Best regards,
Phuc
diff mbox series

Patch

diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c
index 05308df3ae5b..71f7a1fedd88 100644
--- a/sound/soc/sunxi/sun4i-codec.c
+++ b/sound/soc/sunxi/sun4i-codec.c
@@ -2380,31 +2380,22 @@  static int sun4i_codec_probe(struct platform_device *pdev)
 
 	ret = devm_snd_soc_register_component(&pdev->dev, quirks->codec,
 				     &sun4i_codec_dai, 1);
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to register our codec\n");
+	if (ret)
 		return ret;
-	}
 
 	ret = devm_snd_soc_register_component(&pdev->dev,
 					      &sun4i_codec_component,
 					      &dummy_cpu_dai, 1);
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to register our DAI\n");
+	if (ret)
 		return ret;
-	}
 
 	ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to register against DMAEngine\n");
+	if (ret)
 		return ret;
-	}
 
 	card = quirks->create_card(&pdev->dev);
-	if (IS_ERR(card)) {
-		ret = PTR_ERR(card);
-		dev_err(&pdev->dev, "Failed to create our card\n");
-		return ret;
-	}
+	if (IS_ERR(card))
+		return PTR_ERR(card);
 
 	snd_soc_card_set_drvdata(card, scodec);