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

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

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