Hi, All, > -----Original Message----- > From: Adam Thomson <Adam.Thomson.Opensource@xxxxxxxxxxx> > Sent: Thursday, July 30, 2020 2:16 PM > To: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>; Mark Brown > <broonie@xxxxxxxxxx>; Adam Thomson > <Adam.Thomson.Opensource@xxxxxxxxxxx> > Cc: Zhi, Yong <yong.zhi@xxxxxxxxx>; alsa-devel@xxxxxxxxxxxxxxxx > 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
