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