[PATCH RESEND] ALSA: hda/cs35l41: Fix firmware load work teardown

[PATCH RESEND] ALSA: hda/cs35l41: Fix firmware load work teardown

[Cássio Gabriel]

cs35l41_hda creates ALSA controls whose private data points at the
cs35l41_hda object. The firmware load control can also queue
fw_load_work.

Those controls are not removed on component unbind, and device remove
only cancels fw_load_work through cs35l41_remove_dsp(). That helper is
skipped when halo_initialized is false. With firmware_autostart
disabled, a firmware load can be requested before the DSP has been
initialized. If the component or device is removed before the queued
work runs, the worker can run after teardown and dereference driver
state that is no longer valid.

Track the created controls and remove them on unbind so no new control
callback can reach the driver data or queue more work. Then cancel
fw_load_work to drain any request that was already queued. Also cancel
the work unconditionally during device remove before runtime PM teardown.

Fixes: 47ceabd99a28 ("ALSA: hda: cs35l41: Support Firmware switching and reloading")
Fixes: 4c870513fbb0 ("ALSA: hda: cs35l41: Add read-only ALSA control for forced mute")
Cc: stable@vger.kernel.org
Signed-off-by: Cássio Gabriel <cassiogabrielcontato@gmail.com>
---
 sound/hda/codecs/side-codecs/cs35l41_hda.c | 77 +++++++++++++++++++++---------
 sound/hda/codecs/side-codecs/cs35l41_hda.h |  5 ++
 2 files changed, 60 insertions(+), 22 deletions(-)

diff --git a/sound/hda/codecs/side-codecs/cs35l41_hda.c b/sound/hda/codecs/side-codecs/cs35l41_hda.c
index b64890006bb7..7f18be0ccf5a 100644
--- a/sound/hda/codecs/side-codecs/cs35l41_hda.c
+++ b/sound/hda/codecs/side-codecs/cs35l41_hda.c
@@ -1325,6 +1325,43 @@ static int cs35l41_fw_type_ctl_info(struct snd_kcontrol *kcontrol, struct snd_ct
 	return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(cs35l41_hda_fw_ids), cs35l41_hda_fw_ids);
 }
 
+static void cs35l41_remove_controls(struct cs35l41_hda *cs35l41)
+{
+	if (!cs35l41->codec)
+		return;
+
+	snd_ctl_remove(cs35l41->codec->card, cs35l41->mute_override_ctl);
+	cs35l41->mute_override_ctl = NULL;
+
+	snd_ctl_remove(cs35l41->codec->card, cs35l41->fw_load_ctl);
+	cs35l41->fw_load_ctl = NULL;
+
+	snd_ctl_remove(cs35l41->codec->card, cs35l41->fw_type_ctl);
+	cs35l41->fw_type_ctl = NULL;
+}
+
+static int cs35l41_add_control(struct cs35l41_hda *cs35l41,
+			       struct snd_kcontrol_new *ctl,
+			       struct snd_kcontrol **kctl)
+{
+	int ret;
+
+	*kctl = snd_ctl_new1(ctl, cs35l41);
+	if (!*kctl)
+		return -ENOMEM;
+
+	ret = snd_ctl_add(cs35l41->codec->card, *kctl);
+	if (ret) {
+		dev_err(cs35l41->dev, "Failed to add KControl %s = %d\n", ctl->name, ret);
+		*kctl = NULL;
+		return ret;
+	}
+
+	dev_dbg(cs35l41->dev, "Added Control %s\n", ctl->name);
+
+	return 0;
+}
+
 static int cs35l41_create_controls(struct cs35l41_hda *cs35l41)
 {
 	char fw_type_ctl_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
@@ -1360,32 +1397,23 @@ static int cs35l41_create_controls(struct cs35l41_hda *cs35l41)
 	scnprintf(mute_override_ctl_name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, "%s Forced Mute Status",
 		  cs35l41->amp_name);
 
-	ret = snd_ctl_add(cs35l41->codec->card, snd_ctl_new1(&fw_type_ctl, cs35l41));
-	if (ret) {
-		dev_err(cs35l41->dev, "Failed to add KControl %s = %d\n", fw_type_ctl.name, ret);
-		return ret;
-	}
-
-	dev_dbg(cs35l41->dev, "Added Control %s\n", fw_type_ctl.name);
-
-	ret = snd_ctl_add(cs35l41->codec->card, snd_ctl_new1(&fw_load_ctl, cs35l41));
-	if (ret) {
-		dev_err(cs35l41->dev, "Failed to add KControl %s = %d\n", fw_load_ctl.name, ret);
-		return ret;
-	}
-
-	dev_dbg(cs35l41->dev, "Added Control %s\n", fw_load_ctl.name);
+	ret = cs35l41_add_control(cs35l41, &fw_type_ctl, &cs35l41->fw_type_ctl);
+	if (ret)
+		goto err;
 
-	ret = snd_ctl_add(cs35l41->codec->card, snd_ctl_new1(&mute_override_ctl, cs35l41));
-	if (ret) {
-		dev_err(cs35l41->dev, "Failed to add KControl %s = %d\n", mute_override_ctl.name,
-			ret);
-		return ret;
-	}
+	ret = cs35l41_add_control(cs35l41, &fw_load_ctl, &cs35l41->fw_load_ctl);
+	if (ret)
+		goto err;
 
-	dev_dbg(cs35l41->dev, "Added Control %s\n", mute_override_ctl.name);
+	ret = cs35l41_add_control(cs35l41, &mute_override_ctl, &cs35l41->mute_override_ctl);
+	if (ret)
+		goto err;
 
 	return 0;
+
+err:
+	cs35l41_remove_controls(cs35l41);
+	return ret;
 }
 
 static bool cs35l41_dsm_supported(acpi_handle handle, unsigned int commands)
@@ -1522,6 +1550,10 @@ static void cs35l41_hda_unbind(struct device *dev, struct device *master, void *
 		device_link_remove(&cs35l41->codec->core.dev, cs35l41->dev);
 		unlock_system_sleep(sleep_flags);
 		memset(comp, 0, sizeof(*comp));
+
+		cs35l41_remove_controls(cs35l41);
+		cancel_work_sync(&cs35l41->fw_load_work);
+		cs35l41->codec = NULL;
 	}
 }
 
@@ -2058,6 +2090,7 @@ void cs35l41_hda_remove(struct device *dev)
 	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
 
 	component_del(cs35l41->dev, &cs35l41_hda_comp_ops);
+	cancel_work_sync(&cs35l41->fw_load_work);
 
 	pm_runtime_get_sync(cs35l41->dev);
 	pm_runtime_dont_use_autosuspend(cs35l41->dev);
diff --git a/sound/hda/codecs/side-codecs/cs35l41_hda.h b/sound/hda/codecs/side-codecs/cs35l41_hda.h
index 7d003c598e93..56ec07c0bb74 100644
--- a/sound/hda/codecs/side-codecs/cs35l41_hda.h
+++ b/sound/hda/codecs/side-codecs/cs35l41_hda.h
@@ -57,6 +57,8 @@ enum control_bus {
 	SPI
 };
 
+struct snd_kcontrol;
+
 struct cs35l41_hda {
 	struct device *dev;
 	struct regmap *regmap;
@@ -75,6 +77,9 @@ struct cs35l41_hda {
 	int speaker_id;
 	struct mutex fw_mutex;
 	struct work_struct fw_load_work;
+	struct snd_kcontrol *fw_type_ctl;
+	struct snd_kcontrol *fw_load_ctl;
+	struct snd_kcontrol *mute_override_ctl;
 
 	struct regmap_irq_chip_data *irq_data;
 	bool firmware_running;

---
base-commit: 1bc46462f4c09f8d429ae8ec17f92886d604659f
change-id: 20260421-alsa-hda-cs35l41-fw-work-teardown-48cdba14a9cd

Best regards,
--
Cássio Gabriel <cassiogabrielcontato@gmail.com>
-- 
Cássio Gabriel <cassiogabrielcontato@gmail.com>

Re: [PATCH RESEND] ALSA: hda/cs35l41: Fix firmware load work teardown

[Takashi Iwai]

On Mon, 11 May 2026 06:29:34 +0200,
Cássio Gabriel wrote:
> 
> cs35l41_hda creates ALSA controls whose private data points at the
> cs35l41_hda object. The firmware load control can also queue
> fw_load_work.
> 
> Those controls are not removed on component unbind, and device remove
> only cancels fw_load_work through cs35l41_remove_dsp(). That helper is
> skipped when halo_initialized is false. With firmware_autostart
> disabled, a firmware load can be requested before the DSP has been
> initialized. If the component or device is removed before the queued
> work runs, the worker can run after teardown and dereference driver
> state that is no longer valid.
> 
> Track the created controls and remove them on unbind so no new control
> callback can reach the driver data or queue more work. Then cancel
> fw_load_work to drain any request that was already queued. Also cancel
> the work unconditionally during device remove before runtime PM teardown.
> 
> Fixes: 47ceabd99a28 ("ALSA: hda: cs35l41: Support Firmware switching and reloading")
> Fixes: 4c870513fbb0 ("ALSA: hda: cs35l41: Add read-only ALSA control for forced mute")
> Cc: stable@vger.kernel.org
> Signed-off-by: Cássio Gabriel <cassiogabrielcontato@gmail.com>

Cirrus people, could you take a look?  I'd like to let this better
reviewed since it's non-trivial changes.


thanks,

Takashi

> ---
>  sound/hda/codecs/side-codecs/cs35l41_hda.c | 77 +++++++++++++++++++++---------
>  sound/hda/codecs/side-codecs/cs35l41_hda.h |  5 ++
>  2 files changed, 60 insertions(+), 22 deletions(-)
> 
> diff --git a/sound/hda/codecs/side-codecs/cs35l41_hda.c b/sound/hda/codecs/side-codecs/cs35l41_hda.c
> index b64890006bb7..7f18be0ccf5a 100644
> --- a/sound/hda/codecs/side-codecs/cs35l41_hda.c
> +++ b/sound/hda/codecs/side-codecs/cs35l41_hda.c
> @@ -1325,6 +1325,43 @@ static int cs35l41_fw_type_ctl_info(struct snd_kcontrol *kcontrol, struct snd_ct
>  	return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(cs35l41_hda_fw_ids), cs35l41_hda_fw_ids);
>  }
>  
> +static void cs35l41_remove_controls(struct cs35l41_hda *cs35l41)
> +{
> +	if (!cs35l41->codec)
> +		return;
> +
> +	snd_ctl_remove(cs35l41->codec->card, cs35l41->mute_override_ctl);
> +	cs35l41->mute_override_ctl = NULL;
> +
> +	snd_ctl_remove(cs35l41->codec->card, cs35l41->fw_load_ctl);
> +	cs35l41->fw_load_ctl = NULL;
> +
> +	snd_ctl_remove(cs35l41->codec->card, cs35l41->fw_type_ctl);
> +	cs35l41->fw_type_ctl = NULL;
> +}
> +
> +static int cs35l41_add_control(struct cs35l41_hda *cs35l41,
> +			       struct snd_kcontrol_new *ctl,
> +			       struct snd_kcontrol **kctl)
> +{
> +	int ret;
> +
> +	*kctl = snd_ctl_new1(ctl, cs35l41);
> +	if (!*kctl)
> +		return -ENOMEM;
> +
> +	ret = snd_ctl_add(cs35l41->codec->card, *kctl);
> +	if (ret) {
> +		dev_err(cs35l41->dev, "Failed to add KControl %s = %d\n", ctl->name, ret);
> +		*kctl = NULL;
> +		return ret;
> +	}
> +
> +	dev_dbg(cs35l41->dev, "Added Control %s\n", ctl->name);
> +
> +	return 0;
> +}
> +
>  static int cs35l41_create_controls(struct cs35l41_hda *cs35l41)
>  {
>  	char fw_type_ctl_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
> @@ -1360,32 +1397,23 @@ static int cs35l41_create_controls(struct cs35l41_hda *cs35l41)
>  	scnprintf(mute_override_ctl_name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, "%s Forced Mute Status",
>  		  cs35l41->amp_name);
>  
> -	ret = snd_ctl_add(cs35l41->codec->card, snd_ctl_new1(&fw_type_ctl, cs35l41));
> -	if (ret) {
> -		dev_err(cs35l41->dev, "Failed to add KControl %s = %d\n", fw_type_ctl.name, ret);
> -		return ret;
> -	}
> -
> -	dev_dbg(cs35l41->dev, "Added Control %s\n", fw_type_ctl.name);
> -
> -	ret = snd_ctl_add(cs35l41->codec->card, snd_ctl_new1(&fw_load_ctl, cs35l41));
> -	if (ret) {
> -		dev_err(cs35l41->dev, "Failed to add KControl %s = %d\n", fw_load_ctl.name, ret);
> -		return ret;
> -	}
> -
> -	dev_dbg(cs35l41->dev, "Added Control %s\n", fw_load_ctl.name);
> +	ret = cs35l41_add_control(cs35l41, &fw_type_ctl, &cs35l41->fw_type_ctl);
> +	if (ret)
> +		goto err;
>  
> -	ret = snd_ctl_add(cs35l41->codec->card, snd_ctl_new1(&mute_override_ctl, cs35l41));
> -	if (ret) {
> -		dev_err(cs35l41->dev, "Failed to add KControl %s = %d\n", mute_override_ctl.name,
> -			ret);
> -		return ret;
> -	}
> +	ret = cs35l41_add_control(cs35l41, &fw_load_ctl, &cs35l41->fw_load_ctl);
> +	if (ret)
> +		goto err;
>  
> -	dev_dbg(cs35l41->dev, "Added Control %s\n", mute_override_ctl.name);
> +	ret = cs35l41_add_control(cs35l41, &mute_override_ctl, &cs35l41->mute_override_ctl);
> +	if (ret)
> +		goto err;
>  
>  	return 0;
> +
> +err:
> +	cs35l41_remove_controls(cs35l41);
> +	return ret;
>  }
>  
>  static bool cs35l41_dsm_supported(acpi_handle handle, unsigned int commands)
> @@ -1522,6 +1550,10 @@ static void cs35l41_hda_unbind(struct device *dev, struct device *master, void *
>  		device_link_remove(&cs35l41->codec->core.dev, cs35l41->dev);
>  		unlock_system_sleep(sleep_flags);
>  		memset(comp, 0, sizeof(*comp));
> +
> +		cs35l41_remove_controls(cs35l41);
> +		cancel_work_sync(&cs35l41->fw_load_work);
> +		cs35l41->codec = NULL;
>  	}
>  }
>  
> @@ -2058,6 +2090,7 @@ void cs35l41_hda_remove(struct device *dev)
>  	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
>  
>  	component_del(cs35l41->dev, &cs35l41_hda_comp_ops);
> +	cancel_work_sync(&cs35l41->fw_load_work);
>  
>  	pm_runtime_get_sync(cs35l41->dev);
>  	pm_runtime_dont_use_autosuspend(cs35l41->dev);
> diff --git a/sound/hda/codecs/side-codecs/cs35l41_hda.h b/sound/hda/codecs/side-codecs/cs35l41_hda.h
> index 7d003c598e93..56ec07c0bb74 100644
> --- a/sound/hda/codecs/side-codecs/cs35l41_hda.h
> +++ b/sound/hda/codecs/side-codecs/cs35l41_hda.h
> @@ -57,6 +57,8 @@ enum control_bus {
>  	SPI
>  };
>  
> +struct snd_kcontrol;
> +
>  struct cs35l41_hda {
>  	struct device *dev;
>  	struct regmap *regmap;
> @@ -75,6 +77,9 @@ struct cs35l41_hda {
>  	int speaker_id;
>  	struct mutex fw_mutex;
>  	struct work_struct fw_load_work;
> +	struct snd_kcontrol *fw_type_ctl;
> +	struct snd_kcontrol *fw_load_ctl;
> +	struct snd_kcontrol *mute_override_ctl;
>  
>  	struct regmap_irq_chip_data *irq_data;
>  	bool firmware_running;
> 
> ---
> base-commit: 1bc46462f4c09f8d429ae8ec17f92886d604659f
> change-id: 20260421-alsa-hda-cs35l41-fw-work-teardown-48cdba14a9cd
> 
> Best regards,
> --
> Cássio Gabriel <cassiogabrielcontato@gmail.com>
> -- 
> Cássio Gabriel <cassiogabrielcontato@gmail.com>
> 

Re: [PATCH RESEND] ALSA: hda/cs35l41: Fix firmware load work teardown

[Charles Keepax]

On Mon, May 11, 2026 at 01:29:34AM -0300, Cássio Gabriel wrote:
> cs35l41_hda creates ALSA controls whose private data points at the
> cs35l41_hda object. The firmware load control can also queue
> fw_load_work.
> 
> Those controls are not removed on component unbind, and device remove
> only cancels fw_load_work through cs35l41_remove_dsp(). That helper is
> skipped when halo_initialized is false. With firmware_autostart
> disabled, a firmware load can be requested before the DSP has been
> initialized. If the component or device is removed before the queued
> work runs, the worker can run after teardown and dereference driver
> state that is no longer valid.
> 
> Track the created controls and remove them on unbind so no new control
> callback can reach the driver data or queue more work. Then cancel
> fw_load_work to drain any request that was already queued. Also cancel
> the work unconditionally during device remove before runtime PM teardown.
> 
> Fixes: 47ceabd99a28 ("ALSA: hda: cs35l41: Support Firmware switching and reloading")
> Fixes: 4c870513fbb0 ("ALSA: hda: cs35l41: Add read-only ALSA control for forced mute")
> Cc: stable@vger.kernel.org
> Signed-off-by: Cássio Gabriel <cassiogabrielcontato@gmail.com>
> ---
>  static bool cs35l41_dsm_supported(acpi_handle handle, unsigned int commands)
> @@ -1522,6 +1550,10 @@ static void cs35l41_hda_unbind(struct device *dev, struct device *master, void *
>  		device_link_remove(&cs35l41->codec->core.dev, cs35l41->dev);
>  		unlock_system_sleep(sleep_flags);
>  		memset(comp, 0, sizeof(*comp));
> +
> +		cs35l41_remove_controls(cs35l41);
> +		cancel_work_sync(&cs35l41->fw_load_work);
> +		cs35l41->codec = NULL;

Hmm... are we sure the controls are actually still accessible
from user-space here? Feels like generally it would make more
sense to make all the cards controls inaccessible before we start
tearing the card down as a core feature.

Adding the cancel works looks very sensible.

@Stefan, could you also please have a look.

Thanks,
Charles

RE: [PATCH RESEND] ALSA: hda/cs35l41: Fix firmware load work teardown

[Stefan Binding (Opensource)]

Hi,

> -----Original Message-----
> From: Charles Keepax <ckeepax@opensource.cirrus.com>
> Sent: Friday, May 15, 2026 11:12 AM
> To: Cássio Gabriel <cassiogabrielcontato@gmail.com>
> Cc: David Rhodes <david.rhodes@cirrus.com>; Richard Fitzgerald
> <rf@opensource.cirrus.com>; Takashi Iwai <tiwai@suse.com>; Stefan Binding
> <sbinding@opensource.cirrus.com>; Vitaly Rodionov
> <vitalyr@opensource.cirrus.com>; Jaroslav Kysela <perex@perex.cz>; linux-
> sound@vger.kernel.org; patches@opensource.cirrus.com; linux-
> kernel@vger.kernel.org; stable@vger.kernel.org
> Subject: Re: [PATCH RESEND] ALSA: hda/cs35l41: Fix firmware load work
> teardown
> 
> On Mon, May 11, 2026 at 01:29:34AM -0300, Cássio Gabriel wrote:
> > cs35l41_hda creates ALSA controls whose private data points at the
> > cs35l41_hda object. The firmware load control can also queue
> > fw_load_work.
> >
> > Those controls are not removed on component unbind, and device remove
> > only cancels fw_load_work through cs35l41_remove_dsp(). That helper is
> > skipped when halo_initialized is false. With firmware_autostart
> > disabled, a firmware load can be requested before the DSP has been
> > initialized. If the component or device is removed before the queued
> > work runs, the worker can run after teardown and dereference driver
> > state that is no longer valid.
> >
> > Track the created controls and remove them on unbind so no new control
> > callback can reach the driver data or queue more work. Then cancel
> > fw_load_work to drain any request that was already queued. Also cancel
> > the work unconditionally during device remove before runtime PM
teardown.
> >
> > Fixes: 47ceabd99a28 ("ALSA: hda: cs35l41: Support Firmware switching
> > and reloading")
> > Fixes: 4c870513fbb0 ("ALSA: hda: cs35l41: Add read-only ALSA control
> > for forced mute")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Cássio Gabriel <cassiogabrielcontato@gmail.com>
> > ---
> >  static bool cs35l41_dsm_supported(acpi_handle handle, unsigned int
> > commands) @@ -1522,6 +1550,10 @@ static void cs35l41_hda_unbind(struct
> device *dev, struct device *master, void *
> >  		device_link_remove(&cs35l41->codec->core.dev, cs35l41->dev);
> >  		unlock_system_sleep(sleep_flags);
> >  		memset(comp, 0, sizeof(*comp));
> > +
> > +		cs35l41_remove_controls(cs35l41);
> > +		cancel_work_sync(&cs35l41->fw_load_work);
> > +		cs35l41->codec = NULL;
> 
> Hmm... are we sure the controls are actually still accessible from
user-space
> here? Feels like generally it would make more sense to make all the cards
> controls inaccessible before we start tearing the card down as a core
feature.
> 
> Adding the cancel works looks very sensible.
> 
> @Stefan, could you also please have a look.

I think this is fine to do, and I did some tests to make sure it doesn’t
break anything.
Reviewed-by: Stefan Binding <sbinding@opensource.cirrus.com>

> 
> Thanks,
> Charles