[PATCH v2 0/2] firmware_loader/ALSA: Fix TAS2781 async firmware teardown

[PATCH v2 0/2] firmware_loader/ALSA: Fix TAS2781 async firmware teardown

[Cássio Gabriel]

TAS2781 HDA I2C and SPI queue RCA firmware loading with
request_firmware_nowait() during component bind. The firmware loader
keeps the callback module pinned and holds a device reference, but it
does not provide a way for drivers to cancel or synchronize the queued
callback before tearing down driver-private state.

Add a small firmware-loader helper to cancel or synchronize async firmware
requests, then use it from TAS2781 HDA unbind before controls and DSP state
are removed.

No hardware runtime test was available.

---
Changes in v2:
- Add request_firmware_nowait_cancel() in the firmware loader instead of
  tracking the callback lifetime locally in the TAS2781 HDA driver.
- Keep the TAS2781 change to a cancel/sync call in I2C and SPI unbind.
- Drop the unrelated cached kcontrol pointer cleanup from the previous
  local-driver version.
- Link to v1: https://patch.msgid.link/20260430-alsa-hda-tas2781-fw-callback-teardown-v1-1-874367d6b41b@gmail.com

To: Luis Chamberlain <mcgrof@kernel.org>
To: Russ Weight <russ.weight@linux.dev>
To: Danilo Krummrich <dakr@kernel.org>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: "Rafael J. Wysocki" <rafael@kernel.org>
To: Takashi Iwai <tiwai@suse.com>
To: Shenghao Ding <shenghao-ding@ti.com>
To: Kevin Lu <kevin-lu@ti.com>
To: Baojun Xu <baojun.xu@ti.com>
To: Jaroslav Kysela <perex@perex.cz>
Cc: driver-core@lists.linux.dev
Cc: linux-kernel@vger.kernel.org
Cc: linux-sound@vger.kernel.org
Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Cássio Gabriel <cassiogabrielcontato@gmail.com>

---
Cássio Gabriel (2):
      firmware_loader: Add cancel helper for async requests
      ALSA: hda/tas2781: Cancel async firmware request at unbind

 drivers/base/firmware_loader/main.c            | 82 +++++++++++++++++++++++---
 include/linux/firmware.h                       |  9 +++
 sound/hda/codecs/side-codecs/tas2781_hda_i2c.c |  2 +
 sound/hda/codecs/side-codecs/tas2781_hda_spi.c |  2 +
 4 files changed, 87 insertions(+), 8 deletions(-)
---
base-commit: 1bc46462f4c09f8d429ae8ec17f92886d604659f
change-id: 20260421-alsa-hda-tas2781-fw-callback-teardown-3b76404bb928

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

[PATCH v2 1/2] firmware_loader: Add cancel helper for async requests

[Cássio Gabriel]

request_firmware_nowait() keeps the callback module pinned and holds
a device reference until the firmware work completes.

Callers still have no way to cancel or synchronize the queued callback
before tearing down their driver-private state.

Track async firmware work with devres and add
request_firmware_nowait_cancel(). The helper cancels work that has not
started yet and waits for an already-running callback to return. If the
request has already completed, it is a no-op.

The devres release path uses the same synchronization so device teardown
cannot free the firmware work state while the queued work can still run.

Signed-off-by: Cássio Gabriel <cassiogabrielcontato@gmail.com>
---
 drivers/base/firmware_loader/main.c | 82 +++++++++++++++++++++++++++++++++----
 include/linux/firmware.h            |  9 ++++
 2 files changed, 83 insertions(+), 8 deletions(-)

diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index a11b30dda23b..d7d80949ecd7 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -1140,6 +1140,30 @@ struct firmware_work {
 	u32 opt_flags;
 };
 
+static void firmware_work_free(struct firmware_work *fw_work)
+{
+	put_device(fw_work->device); /* taken in request_firmware_nowait() */
+	module_put(fw_work->module);
+	kfree_const(fw_work->name);
+}
+
+static int firmware_work_match(struct device *dev, void *res, void *data)
+{
+	return res == data;
+}
+
+static void firmware_work_release(struct device *dev, void *res)
+{
+	struct firmware_work *fw_work = res;
+
+	/*
+	 * Devres release can run before the async work has completed, so
+	 * synchronize it before dropping the references used by the worker.
+	 */
+	cancel_work_sync(&fw_work->work);
+	firmware_work_free(fw_work);
+}
+
 static void request_firmware_work_func(struct work_struct *work)
 {
 	struct firmware_work *fw_work;
@@ -1150,11 +1174,16 @@ 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);
+	/*
+	 * If teardown already removed the devres entry, it owns the final
+	 * cleanup after cancel_work_sync() waits for this worker.
+	 */
+	if (devres_remove(fw_work->device, firmware_work_release,
+			  firmware_work_match, fw_work)) {
+		firmware_work_free(fw_work);
+		devres_free(fw_work);
+	}
 }
 
 
@@ -1165,14 +1194,14 @@ static int _request_firmware_nowait(
 {
 	struct firmware_work *fw_work;
 
-	fw_work = kzalloc_obj(struct firmware_work, gfp);
+	fw_work = devres_alloc(firmware_work_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 +1213,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 +1289,42 @@ int firmware_request_nowait_nowarn(
 }
 EXPORT_SYMBOL_GPL(firmware_request_nowait_nowarn);
 
+static int firmware_work_cont_match(struct device *dev, void *res, void *data)
+{
+	struct firmware_work *fw_work = res;
+
+	return fw_work->cont == data;
+}
+
+/**
+ * request_firmware_nowait_cancel() - cancel an async firmware request
+ * @device: device for which the firmware is being loaded
+ * @cont: callback passed to request_firmware_nowait()
+ *
+ * Cancel a pending request_firmware_nowait() request for @device and @cont.
+ * If the associated work has already started, this function waits until the
+ * callback has returned. If the callback has already completed, this function
+ * does nothing.
+ *
+ * This function may sleep.
+ */
+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_work_release,
+				firmware_work_cont_match, cont);
+	if (!fw_work)
+		return;
+
+	cancel_work_sync(&fw_work->work);
+	firmware_work_free(fw_work);
+	devres_free(fw_work);
+}
+EXPORT_SYMBOL_GPL(request_firmware_nowait_cancel);
+
 #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..ce55b4987726 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)
 {
 }

-- 
2.54.0

Re: [PATCH v2 1/2] firmware_loader: Add cancel helper for async requests

[Danilo Krummrich]

On Thu Apr 30, 2026 at 11:15 PM CEST, Cássio Gabriel wrote:
> request_firmware_nowait() keeps the callback module pinned and holds
> a device reference until the firmware work completes.
>
> Callers still have no way to cancel or synchronize the queued callback
> before tearing down their driver-private state.
>
> Track async firmware work with devres and add
> request_firmware_nowait_cancel().

I assume this is needed for manual ordering in case the callback accesses
resources that are not managed by devres, but are released in remove().

So, I think this is mixing two patterns, managed by the caller and devres
managed.

For the former we just want the ability to cancel things, this should be
compatible with the existing request_firmware_nowait().

Note that some (hopefully most) drivers already open code synchronization
patterns, e.g. a completion that is waited for in remove().

However, some drivers also seem to just check a pointer, which is obviously
vulnerable to TOCTOU races.

For a devres managed version, I think it should be a new API,
devm_request_firmware_nowait(), which should be possible to simply layer on top
of the existing API with devm_add_action(), so you can just call cancel() from
the release callback.

> The helper cancels work that has not
> started yet and waits for an already-running callback to return. If the
> request has already completed, it is a no-op.
>
> The devres release path uses the same synchronization so device teardown
> cannot free the firmware work state while the queued work can still run.
>
> Signed-off-by: Cássio Gabriel <cassiogabrielcontato@gmail.com>

<snip>

> @@ -1184,18 +1213,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);

This puts the requirement on request_firmware_nowait() that it must be called
from a context where the device is guaranteed to not be unbound concurrently,
otherwise this may race with firmware_work_release(), which could free fw_work
before this functions attempts to schedule it below.

As mentioned above, I think this patch should just add a cancel() function, so
we can layer devres managed functionality on top of that as needed.

Re: [PATCH v2 1/2] firmware_loader: Add cancel helper for async requests

[Cássio Gabriel Monteiro Pires]

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

Hi!

On 4/30/26 19:44, Danilo Krummrich wrote:
> On Thu Apr 30, 2026 at 11:15 PM CEST, Cássio Gabriel wrote:
>> request_firmware_nowait() keeps the callback module pinned and holds
>> a device reference until the firmware work completes.
>>
>> Callers still have no way to cancel or synchronize the queued callback
>> before tearing down their driver-private state.
>>
>> Track async firmware work with devres and add
>> request_firmware_nowait_cancel().
> 
> I assume this is needed for manual ordering in case the callback accesses
> resources that are not managed by devres, but are released in remove().
> 
> So, I think this is mixing two patterns, managed by the caller and devres
> managed.
> 
> For the former we just want the ability to cancel things, this should be
> compatible with the existing request_firmware_nowait().
> 
> Note that some (hopefully most) drivers already open code synchronization
> patterns, e.g. a completion that is waited for in remove().
> 
> However, some drivers also seem to just check a pointer, which is obviously
> vulnerable to TOCTOU races.
> 
> For a devres managed version, I think it should be a new API,
> devm_request_firmware_nowait(), which should be possible to simply layer on top
> of the existing API with devm_add_action(), so you can just call cancel() from
> the release callback.
> 
>> The helper cancels work that has not
>> started yet and waits for an already-running callback to return. If the
>> request has already completed, it is a no-op.
>>
>> The devres release path uses the same synchronization so device teardown
>> cannot free the firmware work state while the queued work can still run.
>>
>> Signed-off-by: Cássio Gabriel <cassiogabrielcontato@gmail.com>
> 
> <snip>
> 
>> @@ -1184,18 +1213,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);
> 
> This puts the requirement on request_firmware_nowait() that it must be called
> from a context where the device is guaranteed to not be unbound concurrently,
> otherwise this may race with firmware_work_release(), which could free fw_work
> before this functions attempts to schedule it below.
> 
> As mentioned above, I think this patch should just add a cancel() function, so
> we can layer devres managed functionality on top of that as needed.

Thanks for pointing this out.

I’ll split the two lifetime models. For v3 I’ll keep the existing
request_firmware_nowait() semantics and add only an explicit cancel/sync path
for callers that manage teardown manually.

That should cover the TAS2781 case without making the existing API implicitly
devres-managed. If a managed variant is useful, it can be added separately as
devm_request_firmware_nowait() on top of the explicit cancel path.

I’ll also avoid the devres_add() / schedule_work() ordering issue in the
current version.

-- 
Thanks,
Cássio


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

[PATCH v2 2/2] ALSA: hda/tas2781: Cancel async firmware request at unbind

[Cássio Gabriel]

TAS2781 HDA I2C and SPI queue RCA firmware loading from component
bind with request_firmware_nowait(). The firmware loader keeps the
callback module pinned and holds a device reference, but the callback
still uses driver-private HDA state.

Component unbind removes controls and DSP state immediately. Later
device removal tears down the TAS2781 private data, including
codec_lock. If the async firmware callback runs after unbind has
started, it can operate on state that is being torn down.

Cancel or synchronize the async firmware request before removing
controls and DSP state. A queued callback is cancelled, and an
already-running callback is allowed to finish before unbind continues.

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_i2c.c | 2 ++
 sound/hda/codecs/side-codecs/tas2781_hda_spi.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c b/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c
index 67240ce184e1..34c6940f3521 100644
--- a/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c
+++ b/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c
@@ -588,6 +588,8 @@ static void tas2781_hda_unbind(struct device *dev,
 		comp->playback_hook = NULL;
 	}
 
+	request_firmware_nowait_cancel(tas_hda->priv->dev, tasdev_fw_ready);
+
 	tas2781_hda_remove_controls(tas_hda);
 
 	tasdevice_config_info_remove(tas_hda->priv);
diff --git a/sound/hda/codecs/side-codecs/tas2781_hda_spi.c b/sound/hda/codecs/side-codecs/tas2781_hda_spi.c
index 0e4f3553f273..6b0d6c764009 100644
--- a/sound/hda/codecs/side-codecs/tas2781_hda_spi.c
+++ b/sound/hda/codecs/side-codecs/tas2781_hda_spi.c
@@ -750,6 +750,8 @@ static void tas2781_hda_unbind(struct device *dev, struct device *master,
 		comp->playback_hook = NULL;
 	}
 
+	request_firmware_nowait_cancel(tas_priv->dev, tasdev_fw_ready);
+
 	tas2781_hda_remove_controls(tas_hda);
 
 	tasdevice_config_info_remove(tas_priv);

-- 
2.54.0