Re: [PATCH v3 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: 2329 bytes --]

On 5/4/26 12:25, Takashi Iwai wrote:
> On Fri, 01 May 2026 12:22:12 +0200,
> Cássio Gabriel wrote:
>> +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 void request_firmware_work_func(struct work_struct *work)
>>  {
>>  	struct firmware_work *fw_work;
>>  	const struct firmware *fw;
>> +	unsigned long flags;
>>  
>>  	fw_work = container_of(work, struct firmware_work, 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);
>> +	spin_lock_irqsave(&firmware_work_lock, flags);
> 
> This is a sleepable context, so you can use spin_lock_irq().

Thanks, I'll fold those in v4.

> 
>> +	if (!list_empty(&fw_work->list)) {
>> +		list_del_init(&fw_work->list);
>> +		spin_unlock_irqrestore(&firmware_work_lock, flags);
>> +		firmware_work_free(fw_work);
>> +		kfree(fw_work);
> 
> Can kfree(fw_work) be in firmware_work_free(), too?  All callers free
> it in anyway.

The worker and cancel paths are both sleepable contexts, so we can switch those
two sites from spin_lock_irqsave() to spin_lock_irq().

I think we need to keep irqsave in _request_firmware_nowait(), since that path
still supports GFP_ATOMIC callers and should not assume IRQs are enabled.

> 
> (snip)
>> +void request_firmware_nowait_cancel(struct device *device, void *context,
>> +				    void (*cont)(const struct firmware *fw,
>> +						 void *context))
>> +{
>> +	struct firmware_work *fw_work = NULL;
>> +	struct firmware_work *tmp;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&firmware_work_lock, flags);
> 
> Here you can use spin_lock_irq() as well.

Thanks, I'll change it in v4.

We can also move kfree(fw_work) into firmware_work_free(), since the successful
completion and cancel paths both free the object after releasing the device,
module and firmware-name references.
 
> thanks,
> 
> Takashi

Thanks,
Cássio

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