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

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2022-07-15 4:55 PM, Takashi Iwai wrote:
> On Tue, 12 Jul 2022 12:58:09 +0200, Cezary Rojewski wrote:

>> This is how ASoC HDAudio codec component was behaving for years, see
>> sound/soc/codecs/hdac_hda.c. If the intention is _not_ do call
>> snd_hda_codec_cleanup_for_unbind() then the teardown procedure will
>> probably need a little update.
> 
> Do we see a similar crash with the hdac-hda stuff, too?
> 
> And, after avs_hdaudio_driver_exit() is called, why the codec object
> still remains bound with the HD-audio (Realtek or whatever) codec
> driver?


Hello Takashi,

Your reply was somehow missed by me and shows as a review for patch 5/9 
in my email-client. Sorry for the delay.

In regard to the hdac_hda.c question, we did test reloading for the 
skylake-driver and there are several places where the driver can cause 
panics, that is, it may not even get to hdac_hda failing - some other 
panic will pop up faster.

But yes, the exact same problem exists there as both implementations 
handle hdev_attach/detach() and component's probe/remove() is similar 
fashion.

>> Actually.. I'm afraid the init one
>> would need an update to. Given how the implementation of HDAudio codec
>> component's probe() and remove() looks like, there is no dropping the
>> cleanup function without changing the upward path accordingly.
>>
>> Well, to be honest the init/free procedures of HDAudio codec are a
>> little hairy, perhaps it's time to address this.
> 
> Admittedly, the plumbing work for ASoC HD-audio was somewhat messy,
> and it's fine if we can clean things up.
> 
> snd_hda_codec_cleanup_for_unbind() is certainly written for unbinding
> the codec driver, and if a part of that function code is needed for
> different purposes, it should be factored out properly, at least.

On ASoC side, component->probe() and component->remove() basically mimic 
the behavior of hda_codec_driver_probe/remove() found in 
sound/pci/hda/hda_bind.c. As ASoC sound card may be unbound without 
codec device being actually removed from the system, relying solely on 
driver's (not component's) probe/remove() may not be an option.

So, the discussion does not circle around just 
snd_hda_codec_cleanup_for_unbind() but basically any function that takes 
part in driver's probe() and remove() routines.

Right now, we are in a situation where user can generate a panic with a 
single rmmod. Also, our tests show no regression with modprobe/rmmod on 
snd_hda_intel side with this patch applied.


Regards,
Czarek