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

[Takashi Iwai <tiwai@suse.de>]

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)
 {
 }