[PATCH] ASoC: da7219: Fix general protection fault in da7219_register_dai_clks
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
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
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
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?
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.
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!
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).
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
participants (5)
-
Adam Thomson -
Mark Brown -
Pierre-Louis Bossart -
Yong Zhi -
Zhi, Yong