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

["Cássio Gabriel Monteiro Pires" <cassiogabrielcontato@gmail.com>]

[-- 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 --]