[PATCH] ALSA: hda/tas2781: Wait for async firmware callback at unbind

[PATCH] ALSA: hda/tas2781: Wait for async firmware callback at unbind

[Cássio Gabriel]

The TAS2781 HDA I2C and SPI side-codec drivers queue the RCA
firmware load with request_firmware_nowait() from component bind. The
firmware loader keeps a device reference and pins the callback module,
but it does not protect the driver's HDA private state from component
unbind.

The callback dereferences tas_hda/tas_priv, takes codec_lock,
creates ALSA controls, updates RCA/DSP state, runs runtime PM, and may
load DSP and calibration data. Component unbind currently removes
controls and DSP state immediately, and the later device remove destroys
codec_lock through tasdevice_remove(). A delayed callback can therefore
run after the HDA component state has been torn down.

Track the pending HDA RCA request with a completion. Mark it cancelled
at unbind, let a callback that observes cancellation exit before parsing
firmware or creating controls, and wait for any already-running callback
before tearing down HDA controls and DSP state.

Clear cached kcontrol pointers as controls are removed, and when
snd_ctl_add() rejects them, so a later cancelled or failed bind cannot
remove stale controls from an earlier bind.

Fixes: 5be27f1e3ec9 ("ALSA: hda/tas2781: Add tas2781 HDA driver")
Fixes: bb5f86ea50ff ("ALSA: hda/tas2781: Add tas2781 hda SPI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Cássio Gabriel <cassiogabrielcontato@gmail.com>
---
 sound/hda/codecs/side-codecs/tas2781_hda.c     | 39 ++++++++++++++++++++++++++
 sound/hda/codecs/side-codecs/tas2781_hda.h     |  8 ++++++
 sound/hda/codecs/side-codecs/tas2781_hda_i2c.c | 18 +++++++++++-
 sound/hda/codecs/side-codecs/tas2781_hda_spi.c | 26 +++++++++++++++--
 4 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/sound/hda/codecs/side-codecs/tas2781_hda.c b/sound/hda/codecs/side-codecs/tas2781_hda.c
index b22f93424c62..52674f675461 100644
--- a/sound/hda/codecs/side-codecs/tas2781_hda.c
+++ b/sound/hda/codecs/side-codecs/tas2781_hda.c
@@ -241,6 +241,45 @@ int tas2781_save_calibration(struct tas2781_hda *hda)
 }
 EXPORT_SYMBOL_NS_GPL(tas2781_save_calibration, "SND_HDA_SCODEC_TAS2781");
 
+void tas2781_hda_fw_request_init(struct tas2781_hda *tas_hda)
+{
+	WRITE_ONCE(tas_hda->fw_cancel, false);
+	init_completion(&tas_hda->fw_done);
+	complete_all(&tas_hda->fw_done);
+}
+EXPORT_SYMBOL_NS_GPL(tas2781_hda_fw_request_init,
+		     "SND_HDA_SCODEC_TAS2781");
+
+void tas2781_hda_fw_request_start(struct tas2781_hda *tas_hda)
+{
+	WRITE_ONCE(tas_hda->fw_cancel, false);
+	reinit_completion(&tas_hda->fw_done);
+}
+EXPORT_SYMBOL_NS_GPL(tas2781_hda_fw_request_start,
+		     "SND_HDA_SCODEC_TAS2781");
+
+void tas2781_hda_fw_request_done(struct tas2781_hda *tas_hda)
+{
+	complete_all(&tas_hda->fw_done);
+}
+EXPORT_SYMBOL_NS_GPL(tas2781_hda_fw_request_done,
+		     "SND_HDA_SCODEC_TAS2781");
+
+void tas2781_hda_fw_request_cancel(struct tas2781_hda *tas_hda)
+{
+	WRITE_ONCE(tas_hda->fw_cancel, true);
+	wait_for_completion(&tas_hda->fw_done);
+}
+EXPORT_SYMBOL_NS_GPL(tas2781_hda_fw_request_cancel,
+		     "SND_HDA_SCODEC_TAS2781");
+
+bool tas2781_hda_fw_request_cancelled(struct tas2781_hda *tas_hda)
+{
+	return READ_ONCE(tas_hda->fw_cancel);
+}
+EXPORT_SYMBOL_NS_GPL(tas2781_hda_fw_request_cancelled,
+		     "SND_HDA_SCODEC_TAS2781");
+
 void tas2781_hda_remove(struct device *dev,
 	const struct component_ops *ops)
 {
diff --git a/sound/hda/codecs/side-codecs/tas2781_hda.h b/sound/hda/codecs/side-codecs/tas2781_hda.h
index 66188909a0bb..d536108e4559 100644
--- a/sound/hda/codecs/side-codecs/tas2781_hda.h
+++ b/sound/hda/codecs/side-codecs/tas2781_hda.h
@@ -7,6 +7,7 @@
 #ifndef __TAS2781_HDA_H__
 #define __TAS2781_HDA_H__
 
+#include <linux/completion.h>
 #include <sound/asound.h>
 
 /* Flag of calibration registers address. */
@@ -59,13 +60,20 @@ struct tas2781_hda {
 	struct snd_kcontrol *dsp_prog_ctl;
 	struct snd_kcontrol *dsp_conf_ctl;
 	struct snd_kcontrol *prof_ctl;
+	struct completion fw_done;
 	enum device_catlog_id catlog_id;
 	void *hda_priv;
+	bool fw_cancel;
 };
 
 extern const efi_guid_t tasdev_fct_efi_guid[];
 
 int tas2781_save_calibration(struct tas2781_hda *p);
+void tas2781_hda_fw_request_init(struct tas2781_hda *tas_hda);
+void tas2781_hda_fw_request_start(struct tas2781_hda *tas_hda);
+void tas2781_hda_fw_request_done(struct tas2781_hda *tas_hda);
+void tas2781_hda_fw_request_cancel(struct tas2781_hda *tas_hda);
+bool tas2781_hda_fw_request_cancelled(struct tas2781_hda *tas_hda);
 void tas2781_hda_remove(struct device *dev,
 	const struct component_ops *ops);
 int tasdevice_info_profile(struct snd_kcontrol *kctl,
diff --git a/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c b/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c
index 67240ce184e1..6693f4aa29fa 100644
--- a/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c
+++ b/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c
@@ -401,12 +401,17 @@ static void tas2781_hda_remove_controls(struct tas2781_hda *tas_hda)
 	struct hda_codec *codec = tas_hda->priv->codec;
 
 	snd_ctl_remove(codec->card, tas_hda->dsp_prog_ctl);
+	tas_hda->dsp_prog_ctl = NULL;
 	snd_ctl_remove(codec->card, tas_hda->dsp_conf_ctl);
+	tas_hda->dsp_conf_ctl = NULL;
 
-	for (int i = ARRAY_SIZE(hda_priv->snd_ctls) - 1; i >= 0; i--)
+	for (int i = ARRAY_SIZE(hda_priv->snd_ctls) - 1; i >= 0; i--) {
 		snd_ctl_remove(codec->card, hda_priv->snd_ctls[i]);
+		hda_priv->snd_ctls[i] = NULL;
+	}
 
 	snd_ctl_remove(codec->card, tas_hda->prof_ctl);
+	tas_hda->prof_ctl = NULL;
 }
 
 static void tasdev_add_kcontrols(struct tasdevice_priv *tas_priv,
@@ -423,6 +428,7 @@ static void tasdev_add_kcontrols(struct tasdevice_priv *tas_priv,
 			dev_err(tas_priv->dev,
 				"Failed to add KControl %s = %d\n",
 				tas_snd_ctrls[i].name, ret);
+			ctls[i] = NULL;
 			break;
 		}
 	}
@@ -492,6 +498,9 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context)
 	guard(pm_runtime_active_auto)(tas_priv->dev);
 	guard(mutex)(&tas_priv->codec_lock);
 
+	if (tas2781_hda_fw_request_cancelled(tas_hda))
+		goto out;
+
 	ret = tasdevice_rca_parser(tas_priv, fmw);
 	if (ret)
 		goto out;
@@ -527,6 +536,7 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context)
 
 out:
 	release_firmware(fmw);
+	tas2781_hda_fw_request_done(tas_hda);
 }
 
 static int tas2781_hda_bind(struct device *dev, struct device *master,
@@ -567,9 +577,12 @@ static int tas2781_hda_bind(struct device *dev, struct device *master,
 
 	strscpy(comp->name, dev_name(dev), sizeof(comp->name));
 
+	tas2781_hda_fw_request_start(tas_hda);
 	ret = tascodec_init(tas_hda->priv, codec, THIS_MODULE, tasdev_fw_ready);
 	if (!ret)
 		comp->playback_hook = tas2781_hda_playback_hook;
+	else
+		tas2781_hda_fw_request_done(tas_hda);
 
 	return ret;
 }
@@ -588,6 +601,8 @@ static void tas2781_hda_unbind(struct device *dev,
 		comp->playback_hook = NULL;
 	}
 
+	tas2781_hda_fw_request_cancel(tas_hda);
+
 	tas2781_hda_remove_controls(tas_hda);
 
 	tasdevice_config_info_remove(tas_hda->priv);
@@ -617,6 +632,7 @@ static int tas2781_hda_i2c_probe(struct i2c_client *clt)
 		return -ENOMEM;
 
 	tas_hda->hda_priv = hda_priv;
+	tas2781_hda_fw_request_init(tas_hda);
 
 	dev_set_drvdata(&clt->dev, tas_hda);
 	tas_hda->dev = &clt->dev;
diff --git a/sound/hda/codecs/side-codecs/tas2781_hda_spi.c b/sound/hda/codecs/side-codecs/tas2781_hda_spi.c
index 0e4f3553f273..a716a4fc644a 100644
--- a/sound/hda/codecs/side-codecs/tas2781_hda_spi.c
+++ b/sound/hda/codecs/side-codecs/tas2781_hda_spi.c
@@ -538,13 +538,18 @@ static void tas2781_hda_remove_controls(struct tas2781_hda *tas_hda)
 	struct tas2781_hda_spi_priv *h_priv = tas_hda->hda_priv;
 
 	snd_ctl_remove(codec->card, tas_hda->dsp_prog_ctl);
+	tas_hda->dsp_prog_ctl = NULL;
 
 	snd_ctl_remove(codec->card, tas_hda->dsp_conf_ctl);
+	tas_hda->dsp_conf_ctl = NULL;
 
-	for (int i = ARRAY_SIZE(h_priv->snd_ctls) - 1; i >= 0; i--)
+	for (int i = ARRAY_SIZE(h_priv->snd_ctls) - 1; i >= 0; i--) {
 		snd_ctl_remove(codec->card, h_priv->snd_ctls[i]);
+		h_priv->snd_ctls[i] = NULL;
+	}
 
 	snd_ctl_remove(codec->card, tas_hda->prof_ctl);
+	tas_hda->prof_ctl = NULL;
 }
 
 static int tas2781_hda_spi_prf_ctl(struct tas2781_hda *h)
@@ -558,9 +563,11 @@ static int tas2781_hda_spi_prf_ctl(struct tas2781_hda *h)
 	tas2781_prof_ctl.name = name;
 	h->prof_ctl = snd_ctl_new1(&tas2781_prof_ctl, p);
 	rc = snd_ctl_add(c->card, h->prof_ctl);
-	if (rc)
+	if (rc) {
 		dev_err(p->dev, "Failed to add KControl: %s, rc = %d\n",
 			tas2781_prof_ctl.name, rc);
+		h->prof_ctl = NULL;
+	}
 	return rc;
 }
 
@@ -580,6 +587,7 @@ static int tas2781_hda_spi_snd_ctls(struct tas2781_hda *h)
 	if (rc) {
 		dev_err(p->dev, "Failed to add KControl: %s, rc = %d\n",
 			tas2781_snd_ctls[i].name, rc);
+		h_priv->snd_ctls[i] = NULL;
 		return rc;
 	}
 	i++;
@@ -590,6 +598,7 @@ static int tas2781_hda_spi_snd_ctls(struct tas2781_hda *h)
 	if (rc) {
 		dev_err(p->dev, "Failed to add KControl: %s, rc = %d\n",
 			tas2781_snd_ctls[i].name, rc);
+		h_priv->snd_ctls[i] = NULL;
 		return rc;
 	}
 	i++;
@@ -600,6 +609,7 @@ static int tas2781_hda_spi_snd_ctls(struct tas2781_hda *h)
 	if (rc) {
 		dev_err(p->dev, "Failed to add KControl: %s, rc = %d\n",
 			tas2781_snd_ctls[i].name, rc);
+		h_priv->snd_ctls[i] = NULL;
 	}
 	return rc;
 }
@@ -619,6 +629,7 @@ static int tas2781_hda_spi_dsp_ctls(struct tas2781_hda *h)
 	if (rc) {
 		dev_err(p->dev, "Failed to add KControl: %s, rc = %d\n",
 			tas2781_dsp_ctls[i].name, rc);
+		h->dsp_prog_ctl = NULL;
 		return rc;
 	}
 	i++;
@@ -629,6 +640,7 @@ static int tas2781_hda_spi_dsp_ctls(struct tas2781_hda *h)
 	if (rc) {
 		dev_err(p->dev, "Failed to add KControl: %s, rc = %d\n",
 			tas2781_dsp_ctls[i].name, rc);
+		h->dsp_conf_ctl = NULL;
 	}
 
 	return rc;
@@ -644,6 +656,9 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context)
 	guard(pm_runtime_active_auto)(tas_priv->dev);
 	guard(mutex)(&tas_priv->codec_lock);
 
+	if (tas2781_hda_fw_request_cancelled(tas_hda))
+		goto out;
+
 	ret = tasdevice_rca_parser(tas_priv, fmw);
 	if (ret)
 		goto out;
@@ -698,6 +713,7 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context)
 	tas2781_save_calibration(tas_hda);
 out:
 	release_firmware(fmw);
+	tas2781_hda_fw_request_done(tas_hda);
 }
 
 static int tas2781_hda_bind(struct device *dev, struct device *master,
@@ -724,10 +740,13 @@ static int tas2781_hda_bind(struct device *dev, struct device *master,
 
 	strscpy(comp->name, dev_name(dev), sizeof(comp->name));
 
+	tas2781_hda_fw_request_start(tas_hda);
 	ret = tascodec_spi_init(tas_hda->priv, codec, THIS_MODULE,
 		tasdev_fw_ready);
 	if (!ret)
 		comp->playback_hook = tas2781_hda_playback_hook;
+	else
+		tas2781_hda_fw_request_done(tas_hda);
 
 	/* Only HP Laptop support SPI-based TAS2781 */
 	tas_hda->catlog_id = HP;
@@ -750,6 +769,8 @@ static void tas2781_hda_unbind(struct device *dev, struct device *master,
 		comp->playback_hook = NULL;
 	}
 
+	tas2781_hda_fw_request_cancel(tas_hda);
+
 	tas2781_hda_remove_controls(tas_hda);
 
 	tasdevice_config_info_remove(tas_priv);
@@ -780,6 +801,7 @@ static int tas2781_hda_spi_probe(struct spi_device *spi)
 		return -ENOMEM;
 
 	tas_hda->hda_priv = hda_priv;
+	tas2781_hda_fw_request_init(tas_hda);
 	spi->max_speed_hz = TAS2781_SPI_MAX_FREQ;
 
 	tas_priv = devm_kzalloc(&spi->dev, sizeof(*tas_priv), GFP_KERNEL);

---
base-commit: 1bc46462f4c09f8d429ae8ec17f92886d604659f
change-id: 20260421-alsa-hda-tas2781-fw-callback-teardown-3b76404bb928

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

Re: [PATCH] ALSA: hda/tas2781: Wait for async firmware callback at unbind

[Takashi Iwai]

On Thu, 30 Apr 2026 06:02:02 +0200,
Cássio Gabriel wrote:
> 
> The TAS2781 HDA I2C and SPI side-codec drivers queue the RCA
> firmware load with request_firmware_nowait() from component bind. The
> firmware loader keeps a device reference and pins the callback module,
> but it does not protect the driver's HDA private state from component
> unbind.
> 
> The callback dereferences tas_hda/tas_priv, takes codec_lock,
> creates ALSA controls, updates RCA/DSP state, runs runtime PM, and may
> load DSP and calibration data. Component unbind currently removes
> controls and DSP state immediately, and the later device remove destroys
> codec_lock through tasdevice_remove(). A delayed callback can therefore
> run after the HDA component state has been torn down.
> 
> Track the pending HDA RCA request with a completion. Mark it cancelled
> at unbind, let a callback that observes cancellation exit before parsing
> firmware or creating controls, and wait for any already-running callback
> before tearing down HDA controls and DSP state.
> 
> Clear cached kcontrol pointers as controls are removed, and when
> snd_ctl_add() rejects them, so a later cancelled or failed bind cannot
> remove stale controls from an earlier bind.
> 
> Fixes: 5be27f1e3ec9 ("ALSA: hda/tas2781: Add tas2781 HDA driver")
> Fixes: bb5f86ea50ff ("ALSA: hda/tas2781: Add tas2781 hda SPI driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Cássio Gabriel <cassiogabrielcontato@gmail.com>

Hmm, this looks too complex than needed.  Basically what we want is a
simple cancel or sync for async firmware loading work.  Once when such
a helper is provided, the rest in the HD-audio side will be just a
call of it at the remove or unbind.  And, I guess we can implement the
helper in the f/w loader with a help of devres or such.


thanks,

Takashi

Re: [PATCH] ALSA: hda/tas2781: Wait for async firmware callback at unbind

[Takashi Iwai]

On Thu, 30 Apr 2026 15:55:33 +0200,
Takashi Iwai wrote:
> 
> On Thu, 30 Apr 2026 06:02:02 +0200,
> Cássio Gabriel wrote:
> > 
> > The TAS2781 HDA I2C and SPI side-codec drivers queue the RCA
> > firmware load with request_firmware_nowait() from component bind. The
> > firmware loader keeps a device reference and pins the callback module,
> > but it does not protect the driver's HDA private state from component
> > unbind.
> > 
> > The callback dereferences tas_hda/tas_priv, takes codec_lock,
> > creates ALSA controls, updates RCA/DSP state, runs runtime PM, and may
> > load DSP and calibration data. Component unbind currently removes
> > controls and DSP state immediately, and the later device remove destroys
> > codec_lock through tasdevice_remove(). A delayed callback can therefore
> > run after the HDA component state has been torn down.
> > 
> > Track the pending HDA RCA request with a completion. Mark it cancelled
> > at unbind, let a callback that observes cancellation exit before parsing
> > firmware or creating controls, and wait for any already-running callback
> > before tearing down HDA controls and DSP state.
> > 
> > Clear cached kcontrol pointers as controls are removed, and when
> > snd_ctl_add() rejects them, so a later cancelled or failed bind cannot
> > remove stale controls from an earlier bind.
> > 
> > Fixes: 5be27f1e3ec9 ("ALSA: hda/tas2781: Add tas2781 HDA driver")
> > Fixes: bb5f86ea50ff ("ALSA: hda/tas2781: Add tas2781 hda SPI driver")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Cássio Gabriel <cassiogabrielcontato@gmail.com>
> 
> Hmm, this looks too complex than needed.  Basically what we want is a
> simple cancel or sync for async firmware loading work.  Once when such
> a helper is provided, the rest in the HD-audio side will be just a
> call of it at the remove or unbind.  And, I guess we can implement the
> helper in the f/w loader with a help of devres or such.

I meant something like below (caution: totally untested)


Takashi

-- 8< --

diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index a11b30dda23b..bd99c5417be8 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -1140,6 +1140,20 @@ struct firmware_work {
 	u32 opt_flags;
 };
 
+static void firmware_devres_release(struct device *dev, void *res)
+{
+	struct firmware_work *fw_work = res;
+
+	module_put(fw_work->module);
+	kfree_const(fw_work->name);
+	put_device(fw_work->device); /* taken in request_firmware_nowait() */
+}
+
+static int firmware_devres_match(struct device *dev, void *res, void *data)
+{
+	return res == data;
+}
+
 static void request_firmware_work_func(struct work_struct *work)
 {
 	struct firmware_work *fw_work;
@@ -1150,14 +1164,10 @@ static void request_firmware_work_func(struct work_struct *work)
 	_request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0, 0,
 			  fw_work->opt_flags);
 	fw_work->cont(fw, fw_work->context);
-	put_device(fw_work->device); /* taken in request_firmware_nowait() */
-
-	module_put(fw_work->module);
-	kfree_const(fw_work->name);
-	kfree(fw_work);
+	devres_release(fw_work->device, firmware_devres_release,
+		       firmware_devres_match, fw_work);
 }
 
-
 static int _request_firmware_nowait(
 	struct module *module, bool uevent,
 	const char *name, struct device *device, gfp_t gfp, void *context,
@@ -1165,14 +1175,14 @@ static int _request_firmware_nowait(
 {
 	struct firmware_work *fw_work;
 
-	fw_work = kzalloc_obj(struct firmware_work, gfp);
+	fw_work = devres_alloc(firmware_devres_release, sizeof(*fw_work), gfp);
 	if (!fw_work)
 		return -ENOMEM;
 
 	fw_work->module = module;
 	fw_work->name = kstrdup_const(name, gfp);
 	if (!fw_work->name) {
-		kfree(fw_work);
+		devres_free(fw_work);
 		return -ENOMEM;
 	}
 	fw_work->device = device;
@@ -1184,18 +1194,19 @@ static int _request_firmware_nowait(
 
 	if (!uevent && fw_cache_is_setup(device, name)) {
 		kfree_const(fw_work->name);
-		kfree(fw_work);
+		devres_free(fw_work);
 		return -EOPNOTSUPP;
 	}
 
 	if (!try_module_get(module)) {
 		kfree_const(fw_work->name);
-		kfree(fw_work);
+		devres_free(fw_work);
 		return -EFAULT;
 	}
 
 	get_device(fw_work->device);
 	INIT_WORK(&fw_work->work, request_firmware_work_func);
+	devres_add(device, fw_work);
 	schedule_work(&fw_work->work);
 	return 0;
 }
@@ -1259,6 +1270,28 @@ int firmware_request_nowait_nowarn(
 }
 EXPORT_SYMBOL_GPL(firmware_request_nowait_nowarn);
 
+static int firmware_devres_cont_match(struct device *dev, void *res, void *data)
+{
+	struct firmware_work *fw_work = res;
+
+	return fw_work->cont == data;
+}
+
+void request_firmware_nowait_cancel(
+	struct device *device,
+	void (*cont)(const struct firmware *fw, void *context))
+{
+	struct firmware_work *fw_work;
+
+	fw_work = devres_remove(device, firmware_devres_release,
+				firmware_devres_cont_match, cont);
+	if (!fw_work)
+		return;
+	cancel_work_sync(&fw_work->work);
+	firmware_devres_release(fw_work->device, fw_work);
+	devres_free(fw_work);
+}
+
 #ifdef CONFIG_FW_CACHE
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
 
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index aae1b85ffc10..f7a80ed9c825 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -110,6 +110,9 @@ int request_firmware_nowait(
 	struct module *module, bool uevent,
 	const char *name, struct device *device, gfp_t gfp, void *context,
 	void (*cont)(const struct firmware *fw, void *context));
+void request_firmware_nowait_cancel(
+	struct device *device,
+	void (*cont)(const struct firmware *fw, void *context));
 int request_firmware_direct(const struct firmware **fw, const char *name,
 			    struct device *device);
 int request_firmware_into_buf(const struct firmware **firmware_p,
@@ -157,6 +160,12 @@ static inline int request_firmware_nowait(
 	return -EINVAL;
 }
 
+static inline void request_firmware_nowait_cancel(
+	struct device *device,
+	void (*cont)(const struct firmware *fw, void *context))
+{
+}
+
 static inline void release_firmware(const struct firmware *fw)
 {
 }

Re: [PATCH] ALSA: hda/tas2781: Wait for async firmware callback at unbind

[Cássio Gabriel Monteiro Pires]

[-- Attachment #1.1: Type: text/plain, Size: 7312 bytes --]

On 4/30/26 11:29, Takashi Iwai wrote:
> On Thu, 30 Apr 2026 15:55:33 +0200,
> Takashi Iwai wrote:
>>
>> On Thu, 30 Apr 2026 06:02:02 +0200,
>> Cássio Gabriel wrote:
>>>
>>> The TAS2781 HDA I2C and SPI side-codec drivers queue the RCA
>>> firmware load with request_firmware_nowait() from component bind. The
>>> firmware loader keeps a device reference and pins the callback module,
>>> but it does not protect the driver's HDA private state from component
>>> unbind.
>>>
>>> The callback dereferences tas_hda/tas_priv, takes codec_lock,
>>> creates ALSA controls, updates RCA/DSP state, runs runtime PM, and may
>>> load DSP and calibration data. Component unbind currently removes
>>> controls and DSP state immediately, and the later device remove destroys
>>> codec_lock through tasdevice_remove(). A delayed callback can therefore
>>> run after the HDA component state has been torn down.
>>>
>>> Track the pending HDA RCA request with a completion. Mark it cancelled
>>> at unbind, let a callback that observes cancellation exit before parsing
>>> firmware or creating controls, and wait for any already-running callback
>>> before tearing down HDA controls and DSP state.
>>>
>>> Clear cached kcontrol pointers as controls are removed, and when
>>> snd_ctl_add() rejects them, so a later cancelled or failed bind cannot
>>> remove stale controls from an earlier bind.
>>>
>>> Fixes: 5be27f1e3ec9 ("ALSA: hda/tas2781: Add tas2781 HDA driver")
>>> Fixes: bb5f86ea50ff ("ALSA: hda/tas2781: Add tas2781 hda SPI driver")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Cássio Gabriel <cassiogabrielcontato@gmail.com>
>>
>> Hmm, this looks too complex than needed.  Basically what we want is a
>> simple cancel or sync for async firmware loading work.  Once when such
>> a helper is provided, the rest in the HD-audio side will be just a
>> call of it at the remove or unbind.  And, I guess we can implement the
>> helper in the f/w loader with a help of devres or such.
> 
> I meant something like below (caution: totally untested)
> 
> 
> Takashi
> 
> -- 8< --
> 
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index a11b30dda23b..bd99c5417be8 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -1140,6 +1140,20 @@ struct firmware_work {
>  	u32 opt_flags;
>  };
>  
> +static void firmware_devres_release(struct device *dev, void *res)
> +{
> +	struct firmware_work *fw_work = res;
> +
> +	module_put(fw_work->module);
> +	kfree_const(fw_work->name);
> +	put_device(fw_work->device); /* taken in request_firmware_nowait() */
> +}
> +
> +static int firmware_devres_match(struct device *dev, void *res, void *data)
> +{
> +	return res == data;
> +}
> +
>  static void request_firmware_work_func(struct work_struct *work)
>  {
>  	struct firmware_work *fw_work;
> @@ -1150,14 +1164,10 @@ static void request_firmware_work_func(struct work_struct *work)
>  	_request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0, 0,
>  			  fw_work->opt_flags);
>  	fw_work->cont(fw, fw_work->context);
> -	put_device(fw_work->device); /* taken in request_firmware_nowait() */
> -
> -	module_put(fw_work->module);
> -	kfree_const(fw_work->name);
> -	kfree(fw_work);
> +	devres_release(fw_work->device, firmware_devres_release,
> +		       firmware_devres_match, fw_work);
>  }
>  
> -
>  static int _request_firmware_nowait(
>  	struct module *module, bool uevent,
>  	const char *name, struct device *device, gfp_t gfp, void *context,
> @@ -1165,14 +1175,14 @@ static int _request_firmware_nowait(
>  {
>  	struct firmware_work *fw_work;
>  
> -	fw_work = kzalloc_obj(struct firmware_work, gfp);
> +	fw_work = devres_alloc(firmware_devres_release, sizeof(*fw_work), gfp);
>  	if (!fw_work)
>  		return -ENOMEM;
>  
>  	fw_work->module = module;
>  	fw_work->name = kstrdup_const(name, gfp);
>  	if (!fw_work->name) {
> -		kfree(fw_work);
> +		devres_free(fw_work);
>  		return -ENOMEM;
>  	}
>  	fw_work->device = device;
> @@ -1184,18 +1194,19 @@ static int _request_firmware_nowait(
>  
>  	if (!uevent && fw_cache_is_setup(device, name)) {
>  		kfree_const(fw_work->name);
> -		kfree(fw_work);
> +		devres_free(fw_work);
>  		return -EOPNOTSUPP;
>  	}
>  
>  	if (!try_module_get(module)) {
>  		kfree_const(fw_work->name);
> -		kfree(fw_work);
> +		devres_free(fw_work);
>  		return -EFAULT;
>  	}
>  
>  	get_device(fw_work->device);
>  	INIT_WORK(&fw_work->work, request_firmware_work_func);
> +	devres_add(device, fw_work);
>  	schedule_work(&fw_work->work);
>  	return 0;
>  }
> @@ -1259,6 +1270,28 @@ int firmware_request_nowait_nowarn(
>  }
>  EXPORT_SYMBOL_GPL(firmware_request_nowait_nowarn);
>  
> +static int firmware_devres_cont_match(struct device *dev, void *res, void *data)
> +{
> +	struct firmware_work *fw_work = res;
> +
> +	return fw_work->cont == data;
> +}
> +
> +void request_firmware_nowait_cancel(
> +	struct device *device,
> +	void (*cont)(const struct firmware *fw, void *context))
> +{
> +	struct firmware_work *fw_work;
> +
> +	fw_work = devres_remove(device, firmware_devres_release,
> +				firmware_devres_cont_match, cont);
> +	if (!fw_work)
> +		return;
> +	cancel_work_sync(&fw_work->work);
> +	firmware_devres_release(fw_work->device, fw_work);
> +	devres_free(fw_work);
> +}
> +
>  #ifdef CONFIG_FW_CACHE
>  static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
>  
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index aae1b85ffc10..f7a80ed9c825 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -110,6 +110,9 @@ int request_firmware_nowait(
>  	struct module *module, bool uevent,
>  	const char *name, struct device *device, gfp_t gfp, void *context,
>  	void (*cont)(const struct firmware *fw, void *context));
> +void request_firmware_nowait_cancel(
> +	struct device *device,
> +	void (*cont)(const struct firmware *fw, void *context));
>  int request_firmware_direct(const struct firmware **fw, const char *name,
>  			    struct device *device);
>  int request_firmware_into_buf(const struct firmware **firmware_p,
> @@ -157,6 +160,12 @@ static inline int request_firmware_nowait(
>  	return -EINVAL;
>  }
>  
> +static inline void request_firmware_nowait_cancel(
> +	struct device *device,
> +	void (*cont)(const struct firmware *fw, void *context))
> +{
> +}
> +
>  static inline void release_firmware(const struct firmware *fw)
>  {
>  }

Hmm, I see. Thanks for the ideas.

Ok, so that's what I thought:
 
1. add a firmware-loader cancel/sync helper for request_firmware_nowait(),
then use it from the TAS2781 HDA I2C/SPI unbind paths before controls/DSP teardown.

2. make the core helper handle the devres teardown case carefully,
so automatic devres release cannot free the firmware_work while the queued
work can still run, and document that the helper cancels a not-yet-running
callback or waits for an already-running callback to return.

I'll send a v2 two-patch series, because of the a generic firmware-loader API/contract
change in drivers/base/firmware_loader/ and include/linux/firmware.h

-- 
Thanks,
Cássio


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]