| Message ID | 1511238476-10314-1-git-send-email-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fdaa451107ce543d345a339b4d5e20e8e4bac396 |
| Headers | show |
> -----Original Message----- > From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter > Roeck > Sent: Monday, November 20, 2017 11:28 PM > To: Liam Girdwood > Cc: Mark Brown; Jaroslav Kysela; Takashi Iwai; alsa-devel@alsa-project.org; > linux-kernel@vger.kernel.org; Guenter Roeck; Deucher, Alexander; Dominik > Behr; Daniel Kurtz > Subject: [PATCH] ASoC: amd: Add error checking to probe function > > The acp_audio_dma does not perform sufficient error checking in its probe > function. This can result in crashes if a critical error path is > encountered. > > Fixes: 7c31335a03b6a ("ASoC: AMD: add AMD ASoC ACP 2.x DMA driver") > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Dominik Behr <dbehr@chromium.org> > Cc: Daniel Kurtz <djkurtz@chromium.org> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > I didn't add an error check to acp_init() since I was not sure if > its return value is ignored on purpose. Vijendar, Akshu can you comment? The patch looks good to me. Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > > sound/soc/amd/acp-pcm-dma.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm- > dma.c > index 9f521a55d610..b5e41df6bb3a 100644 > --- a/sound/soc/amd/acp-pcm-dma.c > +++ b/sound/soc/amd/acp-pcm-dma.c > @@ -1051,6 +1051,11 @@ 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) > @@ -1058,6 +1063,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' > -- > 2.7.4
On 11/21/2017 10:17 AM, Deucher, Alexander wrote: >> -----Original Message----- >> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter >> Roeck >> Sent: Monday, November 20, 2017 11:28 PM >> To: Liam Girdwood >> Cc: Mark Brown; Jaroslav Kysela; Takashi Iwai; alsa-devel@alsa-project.org; >> linux-kernel@vger.kernel.org; Guenter Roeck; Deucher, Alexander; Dominik >> Behr; Daniel Kurtz >> Subject: [PATCH] ASoC: amd: Add error checking to probe function >> >> The acp_audio_dma does not perform sufficient error checking in its probe >> function. This can result in crashes if a critical error path is >> encountered. >> >> Fixes: 7c31335a03b6a ("ASoC: AMD: add AMD ASoC ACP 2.x DMA driver") >> Cc: Alex Deucher <alexander.deucher@amd.com> >> Cc: Dominik Behr <dbehr@chromium.org> >> Cc: Daniel Kurtz <djkurtz@chromium.org> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> I didn't add an error check to acp_init() since I was not sure if >> its return value is ignored on purpose. > > Vijendar, Akshu can you comment? This is also the case of missing error check. acp_init will return error if either sw reset did not happen or clock did not get enabled. In both cases we should error out in probe. > > The patch looks good to me. > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > >> >> sound/soc/amd/acp-pcm-dma.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm- >> dma.c >> index 9f521a55d610..b5e41df6bb3a 100644 >> --- a/sound/soc/amd/acp-pcm-dma.c >> +++ b/sound/soc/amd/acp-pcm-dma.c >> @@ -1051,6 +1051,11 @@ 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) >> @@ -1058,6 +1063,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' >> -- >> 2.7.4 >
> -----Original Message----- > From: Agrawal, Akshu > Sent: Tuesday, November 21, 2017 1:15 AM > To: Deucher, Alexander; 'Guenter Roeck'; Liam Girdwood; Mukunda, > Vijendar > Cc: Mark Brown; Jaroslav Kysela; Takashi Iwai; alsa-devel@alsa-project.org; > linux-kernel@vger.kernel.org; Dominik Behr; Daniel Kurtz > Subject: Re: [PATCH] ASoC: amd: Add error checking to probe function > > > > On 11/21/2017 10:17 AM, Deucher, Alexander wrote: > >> -----Original Message----- > >> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter > >> Roeck > >> Sent: Monday, November 20, 2017 11:28 PM > >> To: Liam Girdwood > >> Cc: Mark Brown; Jaroslav Kysela; Takashi Iwai; alsa-devel@alsa- > project.org; > >> linux-kernel@vger.kernel.org; Guenter Roeck; Deucher, Alexander; > Dominik > >> Behr; Daniel Kurtz > >> Subject: [PATCH] ASoC: amd: Add error checking to probe function > >> > >> The acp_audio_dma does not perform sufficient error checking in its > probe > >> function. This can result in crashes if a critical error path is > >> encountered. > >> > >> Fixes: 7c31335a03b6a ("ASoC: AMD: add AMD ASoC ACP 2.x DMA driver") > >> Cc: Alex Deucher <alexander.deucher@amd.com> > >> Cc: Dominik Behr <dbehr@chromium.org> > >> Cc: Daniel Kurtz <djkurtz@chromium.org> > >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> > >> --- > >> I didn't add an error check to acp_init() since I was not sure if > >> its return value is ignored on purpose. > > > > Vijendar, Akshu can you comment? > > This is also the case of missing error check. > acp_init will return error if either sw reset did not happen or clock > did not get enabled. In both cases we should error out in probe. > Can you send out a patch to enable that error checking? Thanks, Alex > > > > The patch looks good to me. > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > > > >> > >> sound/soc/amd/acp-pcm-dma.c | 7 +++++++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm- > >> dma.c > >> index 9f521a55d610..b5e41df6bb3a 100644 > >> --- a/sound/soc/amd/acp-pcm-dma.c > >> +++ b/sound/soc/amd/acp-pcm-dma.c > >> @@ -1051,6 +1051,11 @@ 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) > >> @@ -1058,6 +1063,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' > >> -- > >> 2.7.4 > >
On Tuesday 21 November 2017 08:38 PM, Deucher, Alexander wrote: >> -----Original Message----- >> From: Agrawal, Akshu >> Sent: Tuesday, November 21, 2017 1:15 AM >> To: Deucher, Alexander; 'Guenter Roeck'; Liam Girdwood; Mukunda, >> Vijendar >> Cc: Mark Brown; Jaroslav Kysela; Takashi Iwai; alsa-devel@alsa-project.org; >> linux-kernel@vger.kernel.org; Dominik Behr; Daniel Kurtz >> Subject: Re: [PATCH] ASoC: amd: Add error checking to probe function >> >> >> >> On 11/21/2017 10:17 AM, Deucher, Alexander wrote: >>>> -----Original Message----- >>>> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter >>>> Roeck >>>> Sent: Monday, November 20, 2017 11:28 PM >>>> To: Liam Girdwood >>>> Cc: Mark Brown; Jaroslav Kysela; Takashi Iwai; alsa-devel@alsa- >> project.org; >>>> linux-kernel@vger.kernel.org; Guenter Roeck; Deucher, Alexander; >> Dominik >>>> Behr; Daniel Kurtz >>>> Subject: [PATCH] ASoC: amd: Add error checking to probe function >>>> >>>> The acp_audio_dma does not perform sufficient error checking in its >> probe >>>> function. This can result in crashes if a critical error path is >>>> encountered. >>>> >>>> Fixes: 7c31335a03b6a ("ASoC: AMD: add AMD ASoC ACP 2.x DMA driver") >>>> Cc: Alex Deucher <alexander.deucher@amd.com> >>>> Cc: Dominik Behr <dbehr@chromium.org> >>>> Cc: Daniel Kurtz <djkurtz@chromium.org> >>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>>> --- >>>> I didn't add an error check to acp_init() since I was not sure if >>>> its return value is ignored on purpose. >>> Vijendar, Akshu can you comment? >> This is also the case of missing error check. >> acp_init will return error if either sw reset did not happen or clock >> did not get enabled. In both cases we should error out in probe. >> > Can you send out a patch to enable that error checking? > > Thanks, > > Alex > on top of this patch will add more error checks and submit a new patch >>> The patch looks good to me. >>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> >>> >>>> sound/soc/amd/acp-pcm-dma.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm- >>>> dma.c >>>> index 9f521a55d610..b5e41df6bb3a 100644 >>>> --- a/sound/soc/amd/acp-pcm-dma.c >>>> +++ b/sound/soc/amd/acp-pcm-dma.c >>>> @@ -1051,6 +1051,11 @@ 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) >>>> @@ -1058,6 +1063,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' >>>> -- >>>> 2.7.4
diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c index 9f521a55d610..b5e41df6bb3a 100644 --- a/sound/soc/amd/acp-pcm-dma.c +++ b/sound/soc/amd/acp-pcm-dma.c @@ -1051,6 +1051,11 @@ 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) @@ -1058,6 +1063,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'
The acp_audio_dma does not perform sufficient error checking in its probe function. This can result in crashes if a critical error path is encountered. Fixes: 7c31335a03b6a ("ASoC: AMD: add AMD ASoC ACP 2.x DMA driver") Cc: Alex Deucher <alexander.deucher@amd.com> Cc: Dominik Behr <dbehr@chromium.org> Cc: Daniel Kurtz <djkurtz@chromium.org> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- I didn't add an error check to acp_init() since I was not sure if its return value is ignored on purpose. sound/soc/amd/acp-pcm-dma.c | 7 +++++++ 1 file changed, 7 insertions(+)