[PATCH] ASoC: da7219: Fix general protection fault in da7219_register_dai_clks

[PATCH] ASoC: da7219: Fix general protection fault in da7219_register_dai_clks

[Yong Zhi]

clkdev_drop(cl) does not null the removed cl, if da7219_register_dai_clks()
entered again after card removal, devm_clk_register() will return -EEXIST,
the goto err to clkdev_drop() will trigger board reboot.

Test commands:
   modprobe -r snd_sof_pci
   modprobe snd_sof_pci

The oops looks like:

da7219 i2c-DLGS7219:00: Using default DAI clk names: da7219-dai-wclk, da7219-dai-bclk
da7219 i2c-DLGS7219:00: Failed to register da7219-dai-wclk: -17
general protection fault: 0000 [#1] PREEMPT SMP NOPTI
RIP: 0010:clkdev_drop+0x20/0x52
Call Trace:
 da7219_probe+0x52e/0x6dc [snd_soc_da7219]
 soc_probe_component+0x206/0x3a1
 snd_soc_bind_card+0x4ee/0x9a6
 devm_snd_soc_register_card+0x48/0x7b
 audio_probe+0x1f0/0x221 [snd_soc_sof_da7219_max98373]
 platform_drv_probe+0x89/0xa2
 really_probe+0x129/0x30d
 driver_probe_device+0x59/0xec
 ? driver_sysfs_remove+0x55/0x55
 bus_for_each_drv+0xa1/0xdc
 __device_attach+0xc2/0x146
 bus_probe_device+0x32/0x97
 device_add+0x311/0x3b4
 platform_device_add+0x184/0x1eb

Fix by marking (nullifying) the da7219->dai_clks_lookup[i]
after clkdev_drop().

Signed-off-by: Yong Zhi <yong.zhi@intel.com>
---
 sound/soc/codecs/da7219.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 153ea30b5a8f..54da7cfbb5f4 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -2369,8 +2369,10 @@ static void da7219_remove(struct snd_soc_component *component)
 
 #ifdef CONFIG_COMMON_CLK
 	for (i = DA7219_DAI_NUM_CLKS - 1; i >= 0; --i) {
-		if (da7219->dai_clks_lookup[i])
+		if (da7219->dai_clks_lookup[i]) {
 			clkdev_drop(da7219->dai_clks_lookup[i]);
+			da7219->dai_clks_lookup[i] = NULL;
+		}
 	}
 #endif
 
-- 
2.7.4

RE: [PATCH] ASoC: da7219: Fix general protection fault in da7219_register_dai_clks

[Adam Thomson]

On 30 July 2020 15:04, Yong Zhi wrote:

> clkdev_drop(cl) does not null the removed cl, if da7219_register_dai_clks()
> entered again after card removal, devm_clk_register() will return -EEXIST,
> the goto err to clkdev_drop() will trigger board reboot.
> 
> Test commands:
>    modprobe -r snd_sof_pci
>    modprobe snd_sof_pci
> 
> The oops looks like:
> 
> da7219 i2c-DLGS7219:00: Using default DAI clk names: da7219-dai-wclk, da7219-
> dai-bclk
> da7219 i2c-DLGS7219:00: Failed to register da7219-dai-wclk: -17
> general protection fault: 0000 [#1] PREEMPT SMP NOPTI
> RIP: 0010:clkdev_drop+0x20/0x52
> Call Trace:
>  da7219_probe+0x52e/0x6dc [snd_soc_da7219]
>  soc_probe_component+0x206/0x3a1
>  snd_soc_bind_card+0x4ee/0x9a6
>  devm_snd_soc_register_card+0x48/0x7b
>  audio_probe+0x1f0/0x221 [snd_soc_sof_da7219_max98373]
>  platform_drv_probe+0x89/0xa2
>  really_probe+0x129/0x30d
>  driver_probe_device+0x59/0xec
>  ? driver_sysfs_remove+0x55/0x55
>  bus_for_each_drv+0xa1/0xdc
>  __device_attach+0xc2/0x146
>  bus_probe_device+0x32/0x97
>  device_add+0x311/0x3b4
>  platform_device_add+0x184/0x1eb
> 
> Fix by marking (nullifying) the da7219->dai_clks_lookup[i]
> after clkdev_drop().
> 
> Signed-off-by: Yong Zhi <yong.zhi@intel.com>
> ---
>  sound/soc/codecs/da7219.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
> index 153ea30b5a8f..54da7cfbb5f4 100644
> --- a/sound/soc/codecs/da7219.c
> +++ b/sound/soc/codecs/da7219.c
> @@ -2369,8 +2369,10 @@ static void da7219_remove(struct
> snd_soc_component *component)
> 
>  #ifdef CONFIG_COMMON_CLK
>  	for (i = DA7219_DAI_NUM_CLKS - 1; i >= 0; --i) {
> -		if (da7219->dai_clks_lookup[i])
> +		if (da7219->dai_clks_lookup[i]) {
>  			clkdev_drop(da7219->dai_clks_lookup[i]);
> +			da7219->dai_clks_lookup[i] = NULL;
> +		}

It seems to me that devm_* functions should have freed up everything when the
codec module was removed. I can only assume the codec isn't being removed in
your test hence devm is never freeing the clock resource and is why you're
getting -EEXIST. Is this the case and is your use-case expected behaviour? It's
not something that has been reported previously so am keen to understand exactly
what's happening here.

>  	}
>  #endif
> 
> --
> 2.7.4

RE: [PATCH] ASoC: da7219: Fix general protection fault in da7219_register_dai_clks

[Zhi, Yong]

Hi, Adam,

> -----Original Message-----
> From: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> Sent: Thursday, July 30, 2020 10:54 AM
> To: Zhi, Yong <yong.zhi@intel.com>; alsa-devel@alsa-project.org;
> broonie@kernel.org
> Cc: pierre-louis.bossart@linux.intel.com; Adam Thomson
> <Adam.Thomson.Opensource@diasemi.com>
> Subject: RE: [PATCH] ASoC: da7219: Fix general protection fault in
> da7219_register_dai_clks
> 
> On 30 July 2020 15:04, Yong Zhi wrote:
> 
> > clkdev_drop(cl) does not null the removed cl, if
> > da7219_register_dai_clks() entered again after card removal,
> > devm_clk_register() will return -EEXIST, the goto err to clkdev_drop() will
> trigger board reboot.
> >
> > Test commands:
> >    modprobe -r snd_sof_pci
> >    modprobe snd_sof_pci
> >
> > The oops looks like:
> >
> > da7219 i2c-DLGS7219:00: Using default DAI clk names: da7219-dai-wclk,
> > da7219- dai-bclk
> > da7219 i2c-DLGS7219:00: Failed to register da7219-dai-wclk: -17
> > general protection fault: 0000 [#1] PREEMPT SMP NOPTI
> > RIP: 0010:clkdev_drop+0x20/0x52
> > Call Trace:
> >  da7219_probe+0x52e/0x6dc [snd_soc_da7219]
> >  soc_probe_component+0x206/0x3a1
> >  snd_soc_bind_card+0x4ee/0x9a6
> >  devm_snd_soc_register_card+0x48/0x7b
> >  audio_probe+0x1f0/0x221 [snd_soc_sof_da7219_max98373]
> >  platform_drv_probe+0x89/0xa2
> >  really_probe+0x129/0x30d
> >  driver_probe_device+0x59/0xec
> >  ? driver_sysfs_remove+0x55/0x55
> >  bus_for_each_drv+0xa1/0xdc
> >  __device_attach+0xc2/0x146
> >  bus_probe_device+0x32/0x97
> >  device_add+0x311/0x3b4
> >  platform_device_add+0x184/0x1eb
> >
> > Fix by marking (nullifying) the da7219->dai_clks_lookup[i] after
> > clkdev_drop().
> >
> > Signed-off-by: Yong Zhi <yong.zhi@intel.com>
> > ---
> >  sound/soc/codecs/da7219.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
> > index 153ea30b5a8f..54da7cfbb5f4 100644
> > --- a/sound/soc/codecs/da7219.c
> > +++ b/sound/soc/codecs/da7219.c
> > @@ -2369,8 +2369,10 @@ static void da7219_remove(struct
> > snd_soc_component *component)
> >
> >  #ifdef CONFIG_COMMON_CLK
> >  	for (i = DA7219_DAI_NUM_CLKS - 1; i >= 0; --i) {
> > -		if (da7219->dai_clks_lookup[i])
> > +		if (da7219->dai_clks_lookup[i]) {
> >  			clkdev_drop(da7219->dai_clks_lookup[i]);
> > +			da7219->dai_clks_lookup[i] = NULL;
> > +		}
> 
> It seems to me that devm_* functions should have freed up everything when the
> codec module was removed. I can only assume the codec isn't being removed in
> your test hence devm is never freeing the clock resource and is why you're
> getting -EEXIST. Is this the case and is your use-case expected behaviour? It's
> not something that has been reported previously so am keen to understand
> exactly what's happening here.
> 
> >  	}
> >  #endif
> >

When the card was uninstalled with modprobe -r, the da7219 codec was not removed, only component da7219_remove() is invoked, do you suggest the component driver probe and remove has to happen with da7219_i2_driver probe and remove together? Thanks for the code review.  

> > --
> > 2.7.4

RE: [PATCH] ASoC: da7219: Fix general protection fault in da7219_register_dai_clks

[Adam Thomson]

On 30 July 2020 17:06, Yong Zhi wrote:
> >
> > > clkdev_drop(cl) does not null the removed cl, if
> > > da7219_register_dai_clks() entered again after card removal,
> > > devm_clk_register() will return -EEXIST, the goto err to clkdev_drop() will
> > trigger board reboot.
> > >
> > > Test commands:
> > >    modprobe -r snd_sof_pci
> > >    modprobe snd_sof_pci
> > >
> > > The oops looks like:
> > >
> > > da7219 i2c-DLGS7219:00: Using default DAI clk names: da7219-dai-wclk,
> > > da7219- dai-bclk
> > > da7219 i2c-DLGS7219:00: Failed to register da7219-dai-wclk: -17
> > > general protection fault: 0000 [#1] PREEMPT SMP NOPTI
> > > RIP: 0010:clkdev_drop+0x20/0x52
> > > Call Trace:
> > >  da7219_probe+0x52e/0x6dc [snd_soc_da7219]
> > >  soc_probe_component+0x206/0x3a1
> > >  snd_soc_bind_card+0x4ee/0x9a6
> > >  devm_snd_soc_register_card+0x48/0x7b
> > >  audio_probe+0x1f0/0x221 [snd_soc_sof_da7219_max98373]
> > >  platform_drv_probe+0x89/0xa2
> > >  really_probe+0x129/0x30d
> > >  driver_probe_device+0x59/0xec
> > >  ? driver_sysfs_remove+0x55/0x55
> > >  bus_for_each_drv+0xa1/0xdc
> > >  __device_attach+0xc2/0x146
> > >  bus_probe_device+0x32/0x97
> > >  device_add+0x311/0x3b4
> > >  platform_device_add+0x184/0x1eb
> > >
> > > Fix by marking (nullifying) the da7219->dai_clks_lookup[i] after
> > > clkdev_drop().
> > >
> > > Signed-off-by: Yong Zhi <yong.zhi@intel.com>
> > > ---
> > >  sound/soc/codecs/da7219.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
> > > index 153ea30b5a8f..54da7cfbb5f4 100644
> > > --- a/sound/soc/codecs/da7219.c
> > > +++ b/sound/soc/codecs/da7219.c
> > > @@ -2369,8 +2369,10 @@ static void da7219_remove(struct
> > > snd_soc_component *component)
> > >
> > >  #ifdef CONFIG_COMMON_CLK
> > >  	for (i = DA7219_DAI_NUM_CLKS - 1; i >= 0; --i) {
> > > -		if (da7219->dai_clks_lookup[i])
> > > +		if (da7219->dai_clks_lookup[i]) {
> > >  			clkdev_drop(da7219->dai_clks_lookup[i]);
> > > +			da7219->dai_clks_lookup[i] = NULL;
> > > +		}
> >
> > It seems to me that devm_* functions should have freed up everything when
> the
> > codec module was removed. I can only assume the codec isn't being removed
> in
> > your test hence devm is never freeing the clock resource and is why you're
> > getting -EEXIST. Is this the case and is your use-case expected behaviour? It's
> > not something that has been reported previously so am keen to understand
> > exactly what's happening here.
> >
> > >  	}
> > >  #endif
> > >
> 
> When the card was uninstalled with modprobe -r, the da7219 codec was not
> removed, only component da7219_remove() is invoked, do you suggest the
> component driver probe and remove has to happen with da7219_i2_driver probe
> and remove together? Thanks for the code review.

Well as far as I understand it the the devm_* allocated resources are tied to
the i2c dev. If I'm correct then unless that's removed then those resources
won't be freed. If this is a valid scenario then we would probably have to look
at avoiding all devm_ calls in the da7219_probe() code as they wouldn't be
released when doing what you are here.

Mark, what's your take on this? Am I missing something obvious?

Re: [PATCH] ASoC: da7219: Fix general protection fault in da7219_register_dai_clks

[Mark Brown]

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

On Thu, Jul 30, 2020 at 04:19:01PM +0000, Adam Thomson wrote:
> On 30 July 2020 17:06, Yong Zhi wrote:

> > When the card was uninstalled with modprobe -r, the da7219 codec was not
> > removed, only component da7219_remove() is invoked, do you suggest the
> > component driver probe and remove has to happen with da7219_i2_driver probe
> > and remove together? Thanks for the code review.

> Well as far as I understand it the the devm_* allocated resources are tied to
> the i2c dev. If I'm correct then unless that's removed then those resources
> won't be freed. If this is a valid scenario then we would probably have to look
> at avoiding all devm_ calls in the da7219_probe() code as they wouldn't be
> released when doing what you are here.

> Mark, what's your take on this? Am I missing something obvious?

You're not missing anything, you shouldn't be doing devm allocations at
the CODEC level only at the device model level.  I'm somewhat confused
why you would be registering clocks at the device model level TBH.

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

Re: [PATCH] ASoC: da7219: Fix general protection fault in da7219_register_dai_clks

[Pierre-Louis Bossart]

>>> When the card was uninstalled with modprobe -r, the da7219 codec was not
>>> removed, only component da7219_remove() is invoked, do you suggest the
>>> component driver probe and remove has to happen with da7219_i2_driver probe
>>> and remove together? Thanks for the code review.
> 
>> Well as far as I understand it the the devm_* allocated resources are tied to
>> the i2c dev. If I'm correct then unless that's removed then those resources
>> won't be freed. If this is a valid scenario then we would probably have to look
>> at avoiding all devm_ calls in the da7219_probe() code as they wouldn't be
>> released when doing what you are here.
> 
>> Mark, what's your take on this? Am I missing something obvious?
> 
> You're not missing anything, you shouldn't be doing devm allocations at
> the CODEC level only at the device model level.  I'm somewhat confused
> why you would be registering clocks at the device model level TBH.

I am afraid we have quite a few codec drivers used in x86/ACPI platforms 
using devm_ clk functions at the component probe level...see e.g.:

da7213, da7218, da7219, es8316, es8328, max98090, max98095, rt5514, 
rt5616, rt5640, rt5682, tlk320aic32x4

some do even worse: nau8825, tlv320aic32x4 call devm_ functions in 
set_sysclk.

The module load/unload tests used for SOF remove all the drivers, so as 
Adam was saying this should not happen if you remove the codec driver.

It already took us quite a bit of effort to make sure all resources are 
properly allocated/released. We still have known issues when removing 
the platform driver only, mainly with HDaudio. I wasn't aware of this 
case for I2C codecs, I guess this just made the list of TODO cleanups 
even longer. D'oh!

RE: [PATCH] ASoC: da7219: Fix general protection fault in da7219_register_dai_clks

[Adam Thomson]

On 30 July 2020 17:58, Pierre-Louis Bossart wrote: 

> >>> When the card was uninstalled with modprobe -r, the da7219 codec was not
> >>> removed, only component da7219_remove() is invoked, do you suggest the
> >>> component driver probe and remove has to happen with da7219_i2_driver
> probe
> >>> and remove together? Thanks for the code review.
> >
> >> Well as far as I understand it the the devm_* allocated resources are tied to
> >> the i2c dev. If I'm correct then unless that's removed then those resources
> >> won't be freed. If this is a valid scenario then we would probably have to look
> >> at avoiding all devm_ calls in the da7219_probe() code as they wouldn't be
> >> released when doing what you are here.
> >
> >> Mark, what's your take on this? Am I missing something obvious?
> >
> > You're not missing anything, you shouldn't be doing devm allocations at
> > the CODEC level only at the device model level.  I'm somewhat confused
> > why you would be registering clocks at the device model level TBH.

Ignorance on my part when I wrote the code I'd say.

> 
> I am afraid we have quite a few codec drivers used in x86/ACPI platforms
> using devm_ clk functions at the component probe level...see e.g.:
> 
> da7213, da7218, da7219, es8316, es8328, max98090, max98095, rt5514,
> rt5616, rt5640, rt5682, tlk320aic32x4
> 
> some do even worse: nau8825, tlv320aic32x4 call devm_ functions in
> set_sysclk.
> 
> The module load/unload tests used for SOF remove all the drivers, so as
> Adam was saying this should not happen if you remove the codec driver.
> 
> It already took us quite a bit of effort to make sure all resources are
> properly allocated/released. We still have known issues when removing
> the platform driver only, mainly with HDaudio. I wasn't aware of this
> case for I2C codecs, I guess this just made the list of TODO cleanups
> even longer. D'oh!

Well, I'll make sure I at least sort the da721x drivers to rectify this
discrepancy and provide the correct solution. On the back of this I have also
spotted something else that looks stupid in the cold light day, so will deal
with that as well (relates to these updates).

RE: [PATCH] ASoC: da7219: Fix general protection fault in da7219_register_dai_clks

[Zhi, Yong]

Hi, All,

> -----Original Message-----
> From: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> Sent: Thursday, July 30, 2020 2:16 PM
> To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; Mark Brown
> <broonie@kernel.org>; Adam Thomson
> <Adam.Thomson.Opensource@diasemi.com>
> Cc: Zhi, Yong <yong.zhi@intel.com>; alsa-devel@alsa-project.org
> Subject: RE: [PATCH] ASoC: da7219: Fix general protection fault in
> da7219_register_dai_clks
> 
> On 30 July 2020 17:58, Pierre-Louis Bossart wrote:
> 
> > >>> When the card was uninstalled with modprobe -r, the da7219 codec
> > >>> was not removed, only component da7219_remove() is invoked, do you
> > >>> suggest the component driver probe and remove has to happen with
> > >>> da7219_i2_driver
> > probe
> > >>> and remove together? Thanks for the code review.
> > >
> > >> Well as far as I understand it the the devm_* allocated resources
> > >> are tied to the i2c dev. If I'm correct then unless that's removed
> > >> then those resources won't be freed. If this is a valid scenario
> > >> then we would probably have to look at avoiding all devm_ calls in
> > >> the da7219_probe() code as they wouldn't be released when doing what
> you are here.
> > >
> > >> Mark, what's your take on this? Am I missing something obvious?
> > >
> > > You're not missing anything, you shouldn't be doing devm allocations
> > > at the CODEC level only at the device model level.  I'm somewhat
> > > confused why you would be registering clocks at the device model level TBH.
> 
> Ignorance on my part when I wrote the code I'd say.
> 
> >
> > I am afraid we have quite a few codec drivers used in x86/ACPI
> > platforms using devm_ clk functions at the component probe level...see e.g.:
> >
> > da7213, da7218, da7219, es8316, es8328, max98090, max98095, rt5514,
> > rt5616, rt5640, rt5682, tlk320aic32x4
> >
> > some do even worse: nau8825, tlv320aic32x4 call devm_ functions in
> > set_sysclk.
> >
> > The module load/unload tests used for SOF remove all the drivers, so
> > as Adam was saying this should not happen if you remove the codec driver.
> >
> > It already took us quite a bit of effort to make sure all resources
> > are properly allocated/released. We still have known issues when
> > removing the platform driver only, mainly with HDaudio. I wasn't aware
> > of this case for I2C codecs, I guess this just made the list of TODO
> > cleanups even longer. D'oh!
> 
> Well, I'll make sure I at least sort the da721x drivers to rectify this discrepancy
> and provide the correct solution. On the back of this I have also spotted
> something else that looks stupid in the cold light day, so will deal with that as
> well (relates to these updates).

Sounds great, thanks everyone for your attention, please ignore the current patch since Adam's fix will come soon. -Yong