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@xxxxxxxxx>

<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.