Re: [PATCH 1/1] firmware: smccc: add support for Live Firmware Activation (LFA)

[Andre Przywara <andre.przywara@arm.com>]

Hi Nirmoy,

On 3/13/26 10:46, Nirmoy Das wrote:
> Hi Salman and Andre,
> 
> 
> We found an bug while testing LFA. See below:
> 
> On 19.01.26 14:27, Salman Nabi wrote:
>> The Arm Live Firmware Activation (LFA) is a specification [1] to describe
>> activating firmware components without a reboot. Those components
>> (like TF-A's BL31, EDK-II, TF-RMM, secure paylods) would be updated the
>> usual way: via fwupd, FF-A or other secure storage methods, or via some
>> IMPDEF Out-Of-Bound method. The user can then activate this new firmware,
>> at system runtime, without requiring a reboot.
>> The specification covers the SMCCC interface to list and query available
>> components and eventually trigger the activation.

[ .... ]

>> +
>> +    update_fw_images_tree();
>> +
>> +    /*
>> +     * Removing non-valid image directories at the end of an activation.
>> +     * We can't remove the sysfs attributes while in the respective
>> +     * _store() handler, so have to postpone the list removal to a
>> +     * workqueue.
>> +     */
>> +    INIT_WORK(&fw_images_update_work, remove_invalid_fw_images);
> 
> 
> This can get invoke multiple times so re-initializing a work item that 
> may already be queued or running
> 
> is unsafe. This should be moved to lfa_init() so it is only called once. 
> I suggest:

Ah, good point, thanks for spotting and reporting. Will fold this into 
the next post!

Cheers,
Andre

> 
> diff --git a/drivers/firmware/smccc/lfa_fw.c b/drivers/firmware/smccc/ 
> lfa_fw.c
> index 90727a66e49a5..135358113104c 100644
> --- a/drivers/firmware/smccc/lfa_fw.c
> +++ b/drivers/firmware/smccc/lfa_fw.c
> @@ -653,7 +653,6 @@ static int update_fw_images_tree(void)
>           * _store() handler, so have to postpone the list removal to a
>           * workqueue.
>           */
> -       INIT_WORK(&fw_images_update_work, remove_invalid_fw_images);
>          queue_work(fw_images_update_wq, &fw_images_update_work);
> 
>          return 0;
> @@ -680,7 +679,7 @@ static void lfa_notify_handler(acpi_handle handle, 
> u32 event, void *data)
>           * of all activable and pending images.
>           */
>          do {
> -               /* Reset activable image flag */
> +               flush_workqueue(fw_images_update_wq);
>                  found_activable_image = false;
>                  list_for_each_entry(attrs, &lfa_fw_images, image_node) {
>                          if (attrs->fw_seq_id == -1)
> @@ -782,6 +781,8 @@ static int __init lfa_init(void)
>                  return -ENOMEM;
>          }
> 
> +       INIT_WORK(&fw_images_update_work, remove_invalid_fw_images);
> +
>          pr_info("Live Firmware Activation: detected v%ld.%ld\n",
>                  reg.a0 >> 16, reg.a0 & 0xffff);
> 
> 
> Regards,
> 
> Nirmoy
> 
>> +    queue_work(fw_images_update_wq, &fw_images_update_work);
>> +    mutex_unlock(&lfa_lock);
>> +
>> +    return ret;
>> +}
>> +