added additional error checks in acp dma driver
v2: printed error codes for acp init & acp deinit
failure cases.
Signed-off-by: Vijendar Mukunda <[email protected]>
---
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
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.
The patch
ASoC: amd: added error checks in dma driver
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
>From 7afa535eb107d587b22ffbbbaaeb4a0b87b94496 Mon Sep 17 00:00:00 2001
From: "Mukunda, Vijendar" <[email protected]>
Date: Mon, 4 Dec 2017 20:46:24 +0530
Subject: [PATCH] ASoC: amd: added error checks in dma driver
added additional error checks in acp dma driver
v2: printed error codes for acp init & acp deinit
failure cases.
Signed-off-by: Vijendar Mukunda <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
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 b5e41df6bb3a..c33a512283a4 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -850,6 +850,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);
@@ -875,6 +878,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,
@@ -1091,7 +1096,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) {
@@ -1108,9 +1117,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);
@@ -1120,9 +1132,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
@@ -1154,18 +1171,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.15.0
On Friday 24 November 2017 01:41 PM, Guenter Roeck wrote:
> On Fri, Nov 24, 2017 at 3:07 AM, Mukunda,Vijendar
> <[email protected]> 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
>>>> <[email protected]> wrote:
>>>>> added error checks in acp dma driver
>>>>> Signed-off-by: Vijendar Mukunda <[email protected]>
>>>>> Signed-off-by: Akshu Agrawal <[email protected]>
>>>>> Signed-off-by: Guenter Roeck <[email protected]>
>>>> 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.
>>
From 1584934324703374518@xxx Fri Nov 24 08:12:38 +0000 2017
X-GM-THRID: 1584853998505310579
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Fri, Nov 24, 2017 at 3:07 AM, Mukunda,Vijendar
<[email protected]> 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
>>> <[email protected]> wrote:
>>>>
>>>> added error checks in acp dma driver
>>>> Signed-off-by: Vijendar Mukunda <[email protected]>
>>>> Signed-off-by: Akshu Agrawal <[email protected]>
>>>> Signed-off-by: Guenter Roeck <[email protected]>
>>>
>>> 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.
>
From 1584924389702036975@xxx Fri Nov 24 05:34:43 +0000 2017
X-GM-THRID: 1584853998505310579
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
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
>> <[email protected]> wrote:
>>> added error checks in acp dma driver
>>> Signed-off-by: Vijendar Mukunda <[email protected]>
>>> Signed-off-by: Akshu Agrawal <[email protected]>
>>> Signed-off-by: Guenter Roeck <[email protected]>
>> 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.
From 1584878842647406692@xxx Thu Nov 23 17:30:46 +0000 2017
X-GM-THRID: 1584853998505310579
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Thu, Nov 23, 2017 at 08:59:43AM -0800, Guenter Roeck wrote:
> On Thu, Nov 23, 2017 at 8:30 AM, Vijendar Mukunda
> <[email protected]> wrote:
> > added error checks in acp dma driver
> > Signed-off-by: Vijendar Mukunda <[email protected]>
> > Signed-off-by: Akshu Agrawal <[email protected]>
> > Signed-off-by: Guenter Roeck <[email protected]>
> 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.
On Thu, Nov 23, 2017 at 8:30 AM, Vijendar Mukunda
<[email protected]> wrote:
> added error checks in acp dma driver
>
> Signed-off-by: Vijendar Mukunda <[email protected]>
> Signed-off-by: Akshu Agrawal <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
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
>
From 1584853998505310579@xxx Thu Nov 23 10:55:53 +0000 2017
X-GM-THRID: 1584853998505310579
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
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.
From 1585310660988146132@xxx Tue Nov 28 11:54:20 +0000 2017
X-GM-THRID: 1585283607714561760
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
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.