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

From: Takashi Iwai <tiwai@suse.de>
To: Cezary Rojewski <cezary.rojewski@intel.com>
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, 17 Jan 2023 15:51:10 +0100	[thread overview]
Message-ID: <87pmbd6wpt.wl-tiwai@suse.de> (raw)
In-Reply-To: <9034530a-d49c-9dd2-515e-1a865e58cb29@intel.com>

On Tue, 17 Jan 2023 15:45:17 +0100,
Cezary Rojewski wrote:
> 
> 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.

Let's focus on that bug, then.  Can we restart the thread with the
minimal change?  Otherwise it's hard to review and discuss further.

thanks,

Takashi