[PATCH] ASoC: amd: added error checks in dma driver

[PATCH] ASoC: amd: added error checks in dma driver

[Vijendar Mukunda]

added error checks in acp dma driver

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
Signed-off-by: Akshu Agrawal <Akshu.Agrawal@amd.com>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
---
 sound/soc/amd/acp-pcm-dma.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 17d76fa..804e659 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -848,6 +848,9 @@ static snd_pcm_uframes_t acp_dma_pointer(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct audio_substream_data *rtd = runtime->private_data;
 
+	if (!rtd)
+		return -EINVAL;
+
 	buffersize = frames_to_bytes(runtime, runtime->buffer_size);
 	bytescount = acp_get_byte_count(rtd->acp_mmio, substream->stream);
 
@@ -873,6 +876,8 @@ static int acp_dma_prepare(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct audio_substream_data *rtd = runtime->private_data;
 
+	if (!rtd)
+		return -EINVAL;
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		config_acp_dma_channel(rtd->acp_mmio, SYSRAM_TO_ACP_CH_NUM,
 					PLAYBACK_START_DMA_DESCR_CH12,
@@ -1066,6 +1071,10 @@ static int acp_audio_probe(struct platform_device *pdev)
 	struct resource *res;
 	const u32 *pdata = pdev->dev.platform_data;
 
+	if (!pdata) {
+		dev_err(&pdev->dev, "Missing platform data\n");
+		return -ENODEV;
+	}
 	audio_drv_data = devm_kzalloc(&pdev->dev, sizeof(struct audio_drv_data),
 					GFP_KERNEL);
 	if (audio_drv_data == NULL)
@@ -1074,6 +1083,8 @@ static int acp_audio_probe(struct platform_device *pdev)
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	audio_drv_data->acp_mmio = devm_ioremap_resource(&pdev->dev, res);
 
+	if (IS_ERR(audio_drv_data->acp_mmio))
+		return PTR_ERR(audio_drv_data->acp_mmio);
 	/* The following members gets populated in device 'open'
 	 * function. Till then interrupts are disabled in 'acp_init'
 	 * and device doesn't generate any interrupts.
@@ -1099,7 +1110,11 @@ static int acp_audio_probe(struct platform_device *pdev)
 	dev_set_drvdata(&pdev->dev, audio_drv_data);
 
 	/* Initialize the ACP */
-	acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
+	status = acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
+	if (status) {
+		dev_err(&pdev->dev, "ACP Init failed\n");
+		return status;
+	}
 
 	status = snd_soc_register_platform(&pdev->dev, &acp_asoc_platform);
 	if (status != 0) {
@@ -1116,9 +1131,14 @@ static int acp_audio_probe(struct platform_device *pdev)
 
 static int acp_audio_remove(struct platform_device *pdev)
 {
+	int status;
 	struct audio_drv_data *adata = dev_get_drvdata(&pdev->dev);
 
-	acp_deinit(adata->acp_mmio);
+	status = acp_deinit(adata->acp_mmio);
+	if (status) {
+		dev_err(&pdev->dev, "ACP Deinit failed\n");
+		return status;
+	}
 	snd_soc_unregister_platform(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
@@ -1128,9 +1148,14 @@ static int acp_audio_remove(struct platform_device *pdev)
 static int acp_pcm_resume(struct device *dev)
 {
 	u16 bank;
+	int status;
 	struct audio_drv_data *adata = dev_get_drvdata(dev);
 
-	acp_init(adata->acp_mmio, adata->asic_type);
+	status = acp_init(adata->acp_mmio, adata->asic_type);
+	if (status) {
+		dev_err(dev, "ACP Init failed\n");
+		return status;
+	}
 
 	if (adata->play_stream && adata->play_stream->runtime) {
 		/* For Stoney, Memory gating is disabled,i.e SRAM Banks
@@ -1162,18 +1187,28 @@ static int acp_pcm_resume(struct device *dev)
 
 static int acp_pcm_runtime_suspend(struct device *dev)
 {
+	int status;
 	struct audio_drv_data *adata = dev_get_drvdata(dev);
 
-	acp_deinit(adata->acp_mmio);
+	status = acp_deinit(adata->acp_mmio);
+	if (status) {
+		dev_err(dev, "ACP Deinit failed\n");
+		return status;
+	}
 	acp_reg_write(0, adata->acp_mmio, mmACP_EXTERNAL_INTR_ENB);
 	return 0;
 }
 
 static int acp_pcm_runtime_resume(struct device *dev)
 {
+	int status;
 	struct audio_drv_data *adata = dev_get_drvdata(dev);
 
-	acp_init(adata->acp_mmio, adata->asic_type);
+	status = acp_init(adata->acp_mmio, adata->asic_type);
+	if (status) {
+		dev_err(dev, "ACP Init failed\n");
+		return status;
+	}
 	acp_reg_write(1, adata->acp_mmio, mmACP_EXTERNAL_INTR_ENB);
 	return 0;
 }
-- 
2.7.4

Re: [PATCH] ASoC: amd: added error checks in dma driver

[Guenter Roeck]

On Thu, Nov 23, 2017 at 8:30 AM, Vijendar Mukunda
<Vijendar.Mukunda@amd.com> wrote:
> added error checks in acp dma driver
>
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> Signed-off-by: Akshu Agrawal <Akshu.Agrawal@amd.com>
> Signed-off-by: Guenter Roeck <groeck@chromium.org>

This is inappropriate.

Guenter

> ---
>  sound/soc/amd/acp-pcm-dma.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
> index 17d76fa..804e659 100644
> --- a/sound/soc/amd/acp-pcm-dma.c
> +++ b/sound/soc/amd/acp-pcm-dma.c
> @@ -848,6 +848,9 @@ static snd_pcm_uframes_t acp_dma_pointer(struct snd_pcm_substream *substream)
>         struct snd_pcm_runtime *runtime = substream->runtime;
>         struct audio_substream_data *rtd = runtime->private_data;
>
> +       if (!rtd)
> +               return -EINVAL;
> +
>         buffersize = frames_to_bytes(runtime, runtime->buffer_size);
>         bytescount = acp_get_byte_count(rtd->acp_mmio, substream->stream);
>
> @@ -873,6 +876,8 @@ static int acp_dma_prepare(struct snd_pcm_substream *substream)
>         struct snd_pcm_runtime *runtime = substream->runtime;
>         struct audio_substream_data *rtd = runtime->private_data;
>
> +       if (!rtd)
> +               return -EINVAL;
>         if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>                 config_acp_dma_channel(rtd->acp_mmio, SYSRAM_TO_ACP_CH_NUM,
>                                         PLAYBACK_START_DMA_DESCR_CH12,
> @@ -1066,6 +1071,10 @@ static int acp_audio_probe(struct platform_device *pdev)
>         struct resource *res;
>         const u32 *pdata = pdev->dev.platform_data;
>
> +       if (!pdata) {
> +               dev_err(&pdev->dev, "Missing platform data\n");
> +               return -ENODEV;
> +       }
>         audio_drv_data = devm_kzalloc(&pdev->dev, sizeof(struct audio_drv_data),
>                                         GFP_KERNEL);
>         if (audio_drv_data == NULL)
> @@ -1074,6 +1083,8 @@ static int acp_audio_probe(struct platform_device *pdev)
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>         audio_drv_data->acp_mmio = devm_ioremap_resource(&pdev->dev, res);
>
> +       if (IS_ERR(audio_drv_data->acp_mmio))
> +               return PTR_ERR(audio_drv_data->acp_mmio);
>         /* The following members gets populated in device 'open'
>          * function. Till then interrupts are disabled in 'acp_init'
>          * and device doesn't generate any interrupts.
> @@ -1099,7 +1110,11 @@ static int acp_audio_probe(struct platform_device *pdev)
>         dev_set_drvdata(&pdev->dev, audio_drv_data);
>
>         /* Initialize the ACP */
> -       acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
> +       status = acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
> +       if (status) {
> +               dev_err(&pdev->dev, "ACP Init failed\n");
> +               return status;
> +       }
>
>         status = snd_soc_register_platform(&pdev->dev, &acp_asoc_platform);
>         if (status != 0) {
> @@ -1116,9 +1131,14 @@ static int acp_audio_probe(struct platform_device *pdev)
>
>  static int acp_audio_remove(struct platform_device *pdev)
>  {
> +       int status;
>         struct audio_drv_data *adata = dev_get_drvdata(&pdev->dev);
>
> -       acp_deinit(adata->acp_mmio);
> +       status = acp_deinit(adata->acp_mmio);
> +       if (status) {
> +               dev_err(&pdev->dev, "ACP Deinit failed\n");
> +               return status;
> +       }
>         snd_soc_unregister_platform(&pdev->dev);
>         pm_runtime_disable(&pdev->dev);
>
> @@ -1128,9 +1148,14 @@ static int acp_audio_remove(struct platform_device *pdev)
>  static int acp_pcm_resume(struct device *dev)
>  {
>         u16 bank;
> +       int status;
>         struct audio_drv_data *adata = dev_get_drvdata(dev);
>
> -       acp_init(adata->acp_mmio, adata->asic_type);
> +       status = acp_init(adata->acp_mmio, adata->asic_type);
> +       if (status) {
> +               dev_err(dev, "ACP Init failed\n");
> +               return status;
> +       }
>
>         if (adata->play_stream && adata->play_stream->runtime) {
>                 /* For Stoney, Memory gating is disabled,i.e SRAM Banks
> @@ -1162,18 +1187,28 @@ static int acp_pcm_resume(struct device *dev)
>
>  static int acp_pcm_runtime_suspend(struct device *dev)
>  {
> +       int status;
>         struct audio_drv_data *adata = dev_get_drvdata(dev);
>
> -       acp_deinit(adata->acp_mmio);
> +       status = acp_deinit(adata->acp_mmio);
> +       if (status) {
> +               dev_err(dev, "ACP Deinit failed\n");
> +               return status;
> +       }
>         acp_reg_write(0, adata->acp_mmio, mmACP_EXTERNAL_INTR_ENB);
>         return 0;
>  }
>
>  static int acp_pcm_runtime_resume(struct device *dev)
>  {
> +       int status;
>         struct audio_drv_data *adata = dev_get_drvdata(dev);
>
> -       acp_init(adata->acp_mmio, adata->asic_type);
> +       status = acp_init(adata->acp_mmio, adata->asic_type);
> +       if (status) {
> +               dev_err(dev, "ACP Init failed\n");
> +               return status;
> +       }
>         acp_reg_write(1, adata->acp_mmio, mmACP_EXTERNAL_INTR_ENB);
>         return 0;
>  }
> --
> 2.7.4
>

Re: [PATCH] ASoC: amd: added error checks in dma driver

[Mark Brown]

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

On Thu, Nov 23, 2017 at 08:59:43AM -0800, Guenter Roeck wrote:
> On Thu, Nov 23, 2017 at 8:30 AM, Vijendar Mukunda
> <Vijendar.Mukunda@amd.com> wrote:
> > added error checks in acp dma driver

> > Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> > Signed-off-by: Akshu Agrawal <Akshu.Agrawal@amd.com>
> > Signed-off-by: Guenter Roeck <groeck@chromium.org>

> This is inappropriate.

Specifically: if Guenter wasn't involved in writing or forwarding the
patch he shouldn't have a signoff in there, and if you're the one
sending the mail you should be the last person in the chain of signoffs.
Please see SubmittingPatches for details of what a signoff means and why
they're important.

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

Re: [PATCH] ASoC: amd: added error checks in dma driver

[Mukunda,Vijendar]

On Thursday 23 November 2017 10:59 PM, Mark Brown wrote:
> On Thu, Nov 23, 2017 at 08:59:43AM -0800, Guenter Roeck wrote:
>> On Thu, Nov 23, 2017 at 8:30 AM, Vijendar Mukunda
>> <Vijendar.Mukunda@amd.com> wrote:
>>> added error checks in acp dma driver
>>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>>> Signed-off-by: Akshu Agrawal <Akshu.Agrawal@amd.com>
>>> Signed-off-by: Guenter Roeck <groeck@chromium.org>
>> This is inappropriate.
> Specifically: if Guenter wasn't involved in writing or forwarding the
> patch he shouldn't have a signoff in there, and if you're the one
> sending the mail you should be the last person in the chain of signoffs.
> Please see SubmittingPatches for details of what a signoff means and why
> they're important.

   This patch was implemented on top of changes implemented by Guenter.
   There is a separate thread - RE: [PATCH] ASoC: amd: Add error checking
   to probe function in which Guenter posted changes.
     
   Got it, apologies will post changes as v2 version.

Re: [PATCH] ASoC: amd: added error checks in dma driver

[Guenter Roeck]

On Fri, Nov 24, 2017 at 3:07 AM, Mukunda,Vijendar
<vijendar.mukunda@amd.com> wrote:
>
>
>
> On Thursday 23 November 2017 10:59 PM, Mark Brown wrote:
>>
>> On Thu, Nov 23, 2017 at 08:59:43AM -0800, Guenter Roeck wrote:
>>>
>>> On Thu, Nov 23, 2017 at 8:30 AM, Vijendar Mukunda
>>> <Vijendar.Mukunda@amd.com> wrote:
>>>>
>>>> added error checks in acp dma driver
>>>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>>>> Signed-off-by: Akshu Agrawal <Akshu.Agrawal@amd.com>
>>>> Signed-off-by: Guenter Roeck <groeck@chromium.org>
>>>
>>> This is inappropriate.
>>
>> Specifically: if Guenter wasn't involved in writing or forwarding the
>> patch he shouldn't have a signoff in there, and if you're the one
>> sending the mail you should be the last person in the chain of signoffs.
>> Please see SubmittingPatches for details of what a signoff means and why
>> they're important.
>
>
>   This patch was implemented on top of changes implemented by Guenter.
>   There is a separate thread - RE: [PATCH] ASoC: amd: Add error checking
>   to probe function in which Guenter posted changes.

That was my patch. This is yours.

Guenter

>       Got it, apologies will post changes as v2 version.
>

Re: [PATCH] ASoC: amd: added error checks in dma driver

[Mukunda,Vijendar]

On Friday 24 November 2017 01:41 PM, Guenter Roeck wrote:
> On Fri, Nov 24, 2017 at 3:07 AM, Mukunda,Vijendar
> <vijendar.mukunda@amd.com> wrote:
>>
>>
>> On Thursday 23 November 2017 10:59 PM, Mark Brown wrote:
>>> On Thu, Nov 23, 2017 at 08:59:43AM -0800, Guenter Roeck wrote:
>>>> On Thu, Nov 23, 2017 at 8:30 AM, Vijendar Mukunda
>>>> <Vijendar.Mukunda@amd.com> wrote:
>>>>> added error checks in acp dma driver
>>>>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>>>>> Signed-off-by: Akshu Agrawal <Akshu.Agrawal@amd.com>
>>>>> Signed-off-by: Guenter Roeck <groeck@chromium.org>
>>>> This is inappropriate.
>>> Specifically: if Guenter wasn't involved in writing or forwarding the
>>> patch he shouldn't have a signoff in there, and if you're the one
>>> sending the mail you should be the last person in the chain of signoffs.
>>> Please see SubmittingPatches for details of what a signoff means and why
>>> they're important.
>>
>>    This patch was implemented on top of changes implemented by Guenter.
>>    There is a separate thread - RE: [PATCH] ASoC: amd: Add error checking
>>    to probe function in which Guenter posted changes.
> That was my patch. This is yours.
>
> Guenter
     Got it , Let your patch go as it is. Will submit a fresh patch for additional
     error checks in acp dma driver.
>
>>        Got it, apologies will post changes as v2 version.
>>

[PATCH] ASoC: amd: added error checks in dma driver

[Vijendar Mukunda]

added additional error checks in acp dma driver

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 sound/soc/amd/acp-pcm-dma.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index fb09578..71ab5b5 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -848,6 +848,9 @@ static snd_pcm_uframes_t acp_dma_pointer(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct audio_substream_data *rtd = runtime->private_data;
 
+	if (!rtd)
+		return -EINVAL;
+
 	buffersize = frames_to_bytes(runtime, runtime->buffer_size);
 	bytescount = acp_get_byte_count(rtd->acp_mmio, substream->stream);
 
@@ -873,6 +876,8 @@ static int acp_dma_prepare(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct audio_substream_data *rtd = runtime->private_data;
 
+	if (!rtd)
+		return -EINVAL;
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		config_acp_dma_channel(rtd->acp_mmio, SYSRAM_TO_ACP_CH_NUM,
 					PLAYBACK_START_DMA_DESCR_CH12,
@@ -1082,7 +1087,11 @@ static int acp_audio_probe(struct platform_device *pdev)
 	dev_set_drvdata(&pdev->dev, audio_drv_data);
 
 	/* Initialize the ACP */
-	acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
+	status = acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
+	if (status) {
+		dev_err(&pdev->dev, "ACP Init failed\n");
+		return status;
+	}
 
 	status = snd_soc_register_platform(&pdev->dev, &acp_asoc_platform);
 	if (status != 0) {
@@ -1099,9 +1108,14 @@ static int acp_audio_probe(struct platform_device *pdev)
 
 static int acp_audio_remove(struct platform_device *pdev)
 {
+	int status;
 	struct audio_drv_data *adata = dev_get_drvdata(&pdev->dev);
 
-	acp_deinit(adata->acp_mmio);
+	status = acp_deinit(adata->acp_mmio);
+	if (status) {
+		dev_err(&pdev->dev, "ACP Deinit failed\n");
+		return status;
+	}
 	snd_soc_unregister_platform(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
@@ -1111,9 +1125,14 @@ static int acp_audio_remove(struct platform_device *pdev)
 static int acp_pcm_resume(struct device *dev)
 {
 	u16 bank;
+	int status;
 	struct audio_drv_data *adata = dev_get_drvdata(dev);
 
-	acp_init(adata->acp_mmio, adata->asic_type);
+	status = acp_init(adata->acp_mmio, adata->asic_type);
+	if (status) {
+		dev_err(dev, "ACP Init failed\n");
+		return status;
+	}
 
 	if (adata->play_stream && adata->play_stream->runtime) {
 		/* For Stoney, Memory gating is disabled,i.e SRAM Banks
@@ -1145,18 +1164,28 @@ static int acp_pcm_resume(struct device *dev)
 
 static int acp_pcm_runtime_suspend(struct device *dev)
 {
+	int status;
 	struct audio_drv_data *adata = dev_get_drvdata(dev);
 
-	acp_deinit(adata->acp_mmio);
+	status = acp_deinit(adata->acp_mmio);
+	if (status) {
+		dev_err(dev, "ACP Deinit failed\n");
+		return status;
+	}
 	acp_reg_write(0, adata->acp_mmio, mmACP_EXTERNAL_INTR_ENB);
 	return 0;
 }
 
 static int acp_pcm_runtime_resume(struct device *dev)
 {
+	int status;
 	struct audio_drv_data *adata = dev_get_drvdata(dev);
 
-	acp_init(adata->acp_mmio, adata->asic_type);
+	status = acp_init(adata->acp_mmio, adata->asic_type);
+	if (status) {
+		dev_err(dev, "ACP Init failed\n");
+		return status;
+	}
 	acp_reg_write(1, adata->acp_mmio, mmACP_EXTERNAL_INTR_ENB);
 	return 0;
 }
-- 
2.7.4

Re: [PATCH] ASoC: amd: added error checks in dma driver

[Mark Brown]

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

On Tue, Nov 28, 2017 at 10:13:44AM +0530, Vijendar Mukunda wrote:

> -	acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
> +	status = acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
> +	if (status) {
> +		dev_err(&pdev->dev, "ACP Init failed\n");
> +		return status;
> +	}
>  

Better to print the error code to help people see what went wrong.

>  static int acp_audio_remove(struct platform_device *pdev)
>  {
> +	int status;
>  	struct audio_drv_data *adata = dev_get_drvdata(&pdev->dev);
>  
> -	acp_deinit(adata->acp_mmio);
> +	status = acp_deinit(adata->acp_mmio);
> +	if (status) {
> +		dev_err(&pdev->dev, "ACP Deinit failed\n");
> +		return status;
> +	}
>  	snd_soc_unregister_platform(&pdev->dev);

Remove operations can't meaningfully fail, better to just log the error
and carry on.

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

Re: [PATCH] ASoC: amd: added error checks in dma driver

[Mukunda,Vijendar]

On Tuesday 28 November 2017 05:22 PM, Mark Brown wrote:
> On Tue, Nov 28, 2017 at 10:13:44AM +0530, Vijendar Mukunda wrote:
>
>> -	acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
>> +	status = acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
>> +	if (status) {
>> +		dev_err(&pdev->dev, "ACP Init failed\n");
>> +		return status;
>> +	}
>>   
> Better to print the error code to help people see what went wrong.
>
>>   static int acp_audio_remove(struct platform_device *pdev)
>>   {
>> +	int status;
>>   	struct audio_drv_data *adata = dev_get_drvdata(&pdev->dev);
>>   
>> -	acp_deinit(adata->acp_mmio);
>> +	status = acp_deinit(adata->acp_mmio);
>> +	if (status) {
>> +		dev_err(&pdev->dev, "ACP Deinit failed\n");
>> +		return status;
>> +	}
>>   	snd_soc_unregister_platform(&pdev->dev);
> Remove operations can't meaningfully fail, better to just log the error
> and carry on.
    Will prepare a patch based on your review comments and post it as V2 version.

[PATCH] ASoC: amd: added error checks in dma driver

[Vijendar Mukunda]

added additional error checks in acp dma driver
v2: printed error codes for acp init & acp deinit
failure cases.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 sound/soc/amd/acp-pcm-dma.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index fb09578..29be517 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -848,6 +848,9 @@ static snd_pcm_uframes_t acp_dma_pointer(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct audio_substream_data *rtd = runtime->private_data;
 
+	if (!rtd)
+		return -EINVAL;
+
 	buffersize = frames_to_bytes(runtime, runtime->buffer_size);
 	bytescount = acp_get_byte_count(rtd->acp_mmio, substream->stream);
 
@@ -873,6 +876,8 @@ static int acp_dma_prepare(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct audio_substream_data *rtd = runtime->private_data;
 
+	if (!rtd)
+		return -EINVAL;
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		config_acp_dma_channel(rtd->acp_mmio, SYSRAM_TO_ACP_CH_NUM,
 					PLAYBACK_START_DMA_DESCR_CH12,
@@ -1082,7 +1087,11 @@ static int acp_audio_probe(struct platform_device *pdev)
 	dev_set_drvdata(&pdev->dev, audio_drv_data);
 
 	/* Initialize the ACP */
-	acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
+	status = acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
+	if (status) {
+		dev_err(&pdev->dev, "ACP Init failed status:%d\n", status);
+		return status;
+	}
 
 	status = snd_soc_register_platform(&pdev->dev, &acp_asoc_platform);
 	if (status != 0) {
@@ -1099,9 +1108,12 @@ static int acp_audio_probe(struct platform_device *pdev)
 
 static int acp_audio_remove(struct platform_device *pdev)
 {
+	int status;
 	struct audio_drv_data *adata = dev_get_drvdata(&pdev->dev);
 
-	acp_deinit(adata->acp_mmio);
+	status = acp_deinit(adata->acp_mmio);
+	if (status)
+		dev_err(&pdev->dev, "ACP Deinit failed status:%d\n", status);
 	snd_soc_unregister_platform(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
@@ -1111,9 +1123,14 @@ static int acp_audio_remove(struct platform_device *pdev)
 static int acp_pcm_resume(struct device *dev)
 {
 	u16 bank;
+	int status;
 	struct audio_drv_data *adata = dev_get_drvdata(dev);
 
-	acp_init(adata->acp_mmio, adata->asic_type);
+	status = acp_init(adata->acp_mmio, adata->asic_type);
+	if (status) {
+		dev_err(dev, "ACP Init failed status:%d\n", status);
+		return status;
+	}
 
 	if (adata->play_stream && adata->play_stream->runtime) {
 		/* For Stoney, Memory gating is disabled,i.e SRAM Banks
@@ -1145,18 +1162,26 @@ static int acp_pcm_resume(struct device *dev)
 
 static int acp_pcm_runtime_suspend(struct device *dev)
 {
+	int status;
 	struct audio_drv_data *adata = dev_get_drvdata(dev);
 
-	acp_deinit(adata->acp_mmio);
+	status = acp_deinit(adata->acp_mmio);
+	if (status)
+		dev_err(dev, "ACP Deinit failed status:%d\n", status);
 	acp_reg_write(0, adata->acp_mmio, mmACP_EXTERNAL_INTR_ENB);
 	return 0;
 }
 
 static int acp_pcm_runtime_resume(struct device *dev)
 {
+	int status;
 	struct audio_drv_data *adata = dev_get_drvdata(dev);
 
-	acp_init(adata->acp_mmio, adata->asic_type);
+	status = acp_init(adata->acp_mmio, adata->asic_type);
+	if (status) {
+		dev_err(dev, "ACP Init failed status:%d\n", status);
+		return status;
+	}
 	acp_reg_write(1, adata->acp_mmio, mmACP_EXTERNAL_INTR_ENB);
 	return 0;
 }
-- 
2.7.4

Re: [PATCH] ASoC: amd: added error checks in dma driver

[Mark Brown]

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

On Mon, Dec 04, 2017 at 08:46:24PM +0530, Vijendar Mukunda wrote:
> added additional error checks in acp dma driver
> v2: printed error codes for acp init & acp deinit
> failure cases.

Don't include noise like inter-version changelogs in commit messages,
add them after the --- if they're important as covered in
SubmittingPatches.

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