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

[Andre Przywara]

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;
> > +}
> > +