Re: [PATCH 1/9] ALSA: hda: Do not unset preset when cleaning up codec

From: Cezary Rojewski <cezary.rojewski@intel.com>
To: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org,
	pierre-louis.bossart@linux.intel.com, tiwai@suse.com,
	hdegoede@redhat.com, broonie@kernel.org,
	amadeuszx.slawinski@linux.intel.com
Subject: Re: [PATCH 1/9] ALSA: hda: Do not unset preset when cleaning up codec
Date: Tue, 12 Jul 2022 11:42:56 +0200	[thread overview]
Message-ID: <8bdd56fc-de6b-381e-24f5-5d2c28e337f7@intel.com> (raw)
In-Reply-To: <877d4jsppp.wl-tiwai@suse.de>

On 2022-07-11 4:12 PM, Takashi Iwai wrote:
> On Mon, 11 Jul 2022 10:25:17 +0200,
> Cezary Rojewski wrote:

...

>> avs-driver i.e. the bus driver takes responsibility for the codec
>> device only. There is no real probe(), just the device creation and
>> initialization of its fields. The rest is handled by the component
>> driver (sound/soc/hda.c). If this field is cleared and the test is
>> limited to reloading HDAudio codec module alone, we get a
>> panic. Something similar to the stack found below my message.
>>
>> In regard to the other question - are presets freed at all? It seems
>> all of them are part of the static device-driver matching list. If so,
>> the pointer is always valid.
> 
> When the codec driver is unbound and the module is unloaded, the whole
> objects and symbols are gone.

hda_codec_driver_remove() won't get even called when soc-card is being 
unbound so everything is still here.

>> [  136.827856] RIP: 0010:hda_codec_probe+0x16c/0x560 [snd_soc_hda_codec]

>> [  136.828568] Call Trace:
>> [  136.828593]  <TASK>
>> [  136.828628]  snd_soc_component_probe+0x3a/0x60 [snd_soc_core]
>> [  136.828981]  soc_probe_component+0x276/0x4a0 [snd_soc_core]
>> [  136.829274]  snd_soc_bind_card+0x819/0x13d0 [snd_soc_core]
>> [  136.829560]  ? __kasan_slab_alloc+0x32/0x90
>> [  136.829614]  snd_soc_register_card+0x24e/0x260 [snd_soc_core]
>> [  136.829900]  devm_snd_soc_register_card+0x48/0x90 [snd_soc_core]
>> [  136.830204]  avs_hdaudio_probe+0x298/0x2c0 [snd_soc_avs_hdaudio]

>> (...)
> 
> Hmm, in the Oops above, at which moment,
> snd_hda_codec_cleanup_for_unbind() is called via which function?
> Is it the unload of HD-audio codec driver during the probe of AVS
> HD-audio?
> 
> The preset is assigned to the given HD-audio device object for the
> attached codec driver.  Once after the codec driver gets unbound, you
> must not access to this codec driver's methods any longer, hence we
> clear the preset field.
> 
> So I wonder how the access to the codec->preset happens after the
> codec unbind.

Test scenario:
- enumerate avs-driver stack on machine with HDAudio codec present
- rmmod snd_soc_avs_hdaudio // just the machine board driver i.e. 
soc-card driver
- modprobe snd_soc_avs_hdaudio
 >>> panic <<<

snd_hda_codec_cleanup_for_unbind() is called in more places than just 
HDAudio codec driver's probe() and remove(). It's also called whenever 
HDAudio codec soc-component is being removed. Relevant part of the stack 
showing when does the cleanup function get called during rmmod:

[  220.549349]  snd_hda_codec_cleanup_for_unbind+0x25/0x451 [snd_hda_codec]
[  220.549536]  ? dump_stack_lvl+0x45/0x49
[  220.549568]  hda_codec_remove.cold+0x14/0x138 [snd_soc_hda_codec]
[  220.549609]  snd_soc_component_remove+0x34/0x40 [snd_soc_core]
[  220.549942]  soc_remove_component+0x113/0x120 [snd_soc_core]
[  220.550249]  soc_cleanup_card_resources+0x1a7/0x4a0 [snd_soc_core]
[  220.550561]  snd_soc_unbind_card+0x9e/0x190 [snd_soc_core]
[  220.550885]  snd_soc_unregister_card+0x28/0x80 [snd_soc_core]
[  220.551193]  devm_card_release+0x1d/0x20 [snd_soc_core]
[  220.551527]  release_nodes+0x73/0x170
[  220.551549]  ? preempt_count_sub+0x18/0xc0
[  220.551576]  devres_release_all+0x10a/0x150
[  220.551600]  ? devres_remove_group+0x260/0x260
[  220.551630]  device_unbind_cleanup+0x14/0xd0
[  220.551656]  device_release_driver_internal+0x146/0x1d0
[  220.551688]  driver_detach+0x81/0xf0
[  220.551716]  bus_remove_driver+0xae/0x170
[  220.551743]  driver_unregister+0x4d/0x70
[  220.551770]  platform_driver_unregister+0x12/0x20
[  220.551799]  avs_hdaudio_driver_exit+0x10/0x12 [snd_soc_avs_hdaudio]

Regards,
Czarek