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

["Zhi, Yong" <yong.zhi@intel.com>]

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