On Wed, 18 Jan 2023 13:04:05 +0100, Cezary Rojewski wrote: > > On 2023-01-17 5:01 PM, Takashi Iwai wrote: > > ... > > >> This is a continuation of a discussion that begun in the middle of 2022 > >> [1] and was part of a larger series addressing several HDAudio topics. > >> > >> Single rmmod on ASoC's codec driver module is enough to cause a panic. > >> Given our results, no regression shows up with modprobe/rmmod on > >> snd_hda_intel side with this patch applied. > > > > I think one possible regression by this change would be the case you > > reload another codec driver. With keeping codec->preset, it's still > > thought as if already matched, and a wrong one could be used. > > > > And, this would be nothing but a leak of the possibly freed address. > > After hda_codec_driver_remove(), card->preset may point to an already > > freed address. > > > > So, just removing isn't right. It has to be cleared somewhere > > instead, e.g. in hda_bind.c. > > > > But, one thing I'm still concerned is that your comment about the call > > without the card binding. Do you mean that the > > snd_hda_codec_cleanup_for_unbind() may be called even if codec->card > > isn't set? > > One proposition would be to add line: > codec->preset = NULL; > > after both invocations of snd_hda_codec_cleanup_for_unbind() in > hda_codec_driver_probe/remove() in sound/pci/hda/hda_bind.c. Yes, that's what I meant, too. > In regard to your last question - no, cleanup is only called either in > component->probe()'s error path or in component->remove() once > snd_hda_codec_device_new() succeeds and thus, codec->card points to a > valid sound card. > > What I meant by my comment, is that removal of any components of an > ASoC sound card will cause all other components to have their > ->remove() op called too. Let's say I unload snd_soc_avs_hdaudio > module without unloading snd_soc_avs. As bus (snd_soc_avs) owns the > codecs, its platform component->remove() gets called right after codec > component->remove() but the actual devices are still present in the > system even with the sound card module (snd_soc_avs_hdaudio) unloaded. OK, then the snd_hda_codec object itself is kept bound on the bus, hence the call of snd_hda_codec_cleanup_for_unbind() is somehow inappropriate. The function could be renamed for avoid misunderstanding. thanks, Takashi
