Message-ID: <5dad5a2f-1282-43a4-928f-a7b8da25fb82@arm.com>
Date: Fri, 30 Jan 2026 21:31:13 +0100
From: Andre Przywara <andre.przywara@....com>
To: Vedashree Vidwans <vvidwans@...dia.com>, Salman Nabi
 <salman.nabi@....com>, sudeep.holla@....com, mark.rutland@....com,
 lpieralisi@...nel.org
Cc: ardb@...nel.org, chao.gao@...el.com,
 linux-arm-kernel@...ts.infradead.org, linux-coco@...ts.linux.dev,
 linux-kernel@...r.kernel.org, sdonthineni@...dia.com, vsethi@...dia.com,
 vwadekar@...dia.com
Subject: Re: [PATCH 1/1] firmware: smccc: add support for Live Firmware
 Activation (LFA)

Hi,

On 1/30/26 20:29, Vedashree Vidwans wrote:
> Hi Andre,
> 
> On 1/29/26 09:26, Andre Przywara wrote:
>> Hi Vedashree,
>>
>> many thanks for having a look!
>>
>> On 1/28/26 00:01, Vedashree Vidwans wrote:
>>> Hello,
>>>
>>> On 1/19/26 04: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
>>

[ .... ]
>>>> +
>>>> +    for (int i = 0; i < ARRAY_SIZE(fw_images_uuids); i++) {
>>>> +        if (!strcmp(fw_images_uuids[i].uuid, fw_uuid))
>>>> +            image_name = fw_images_uuids[i].name;
>>>> +    }
>>> I would recommend using fw_uuid as the image_name when UUID is not
>>> found in fw_images_uuids[], currently the driver assigns 'unknown'
>>> in such case.
>>> There is a valid possibility that platform-specific FW images, not
>>> listed in fw_images_uuids[], are used by LFA agent for live FW
>>> activation. In such scenarios, falling back to 'unknown' would lose
>>> important information especially when errors surface in
>>> call_lfa_activate(). Using UUID directly would indicate which
>>> image failed or behaved unexpectedly.
>>
>> Well, I think if you want to identify an image clearly, you always 
>> have to use the UUID, as shown by the directory name. The "name" sysfs 
>> file is there just for convenience, to make this easier for *users* 
>> when dealing with well-known firmware image. As you rightly said, we 
>> can never guarantee that the kernel knows a certain UUID, and it 
>> wouldn't be necessary for proper operation at all.
>> So I was expecting this name to be only used by reporting scripts or 
>> such. But indeed some "unknown" string is a bit fragile as a 
>> placeholder name, I was wondering if we should just provide an empty 
>> string in this case? This would allow scripts to detect this special 
>> case reliably and provide their own rendering then.
>> Does this make sense?
> Thank you for teh clarification, this helps. I agree that UUID is the 
> canonical identifier and that 'name' sysfs file is mainly for user 
> convenience.
> One concern I have is around error reporting. For example, if 
> call_lfa_activate() fails, the kernel prints:
> ACTIVATE for image <name_string> failed: LFA_BUSY
> 
> If the 'name_string' is "unknown" or an empty string, the error message 
> doesn't indicate which firmware component failed activation, especially 
> on system with multiple LFA-capable FW images.

Yes, that's a good point.

> In such cases, having meaningful fallback identifier is helpful for 
> debugging. Using UUID in the error log would help identify which FW 
> image encountered the failure. This avoids ambiguity and makes error 
> triage much easier, especially when platform-specific firmware images 
> are not listed in fw_images_uuids[] but are still valid LFA components.
> So eitehr of the following would solve the issue:
> 1. Use fw_uuid as the fallback image_name
> 2. Update error print in call_lfa_activate() and prime_fw_image() to use 
> UUID instead of image_name.

I'd say number 2 is better. And if we leave the name char* as NULL or an 
empty string, then this is also easy to detect in the code, and we can 
print the UUID instead. If we do this more than once, then this could be 
nicely hidden in a function.

Thanks,
Andre


> Best,
> Veda
>>
>> Cheers,
>> Andre
>>
>>> Thank you,
>>> Veda
>>>> +
>>>> +    attrs->image_dir = kobject_create_and_add(fw_uuid, lfa_dir);
>>>> +    if (!attrs->image_dir)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    INIT_LIST_HEAD(&attrs->image_node);
>>>> +    attrs->image_name = image_name;
>>>> +    attrs->cpu_rendezvous_forced = 1;
>>>> +    set_image_flags(attrs, seq_id, image_flags, reg_current_ver,
>>>> +            reg_pending_ver);
>>>> +
>>>> +    /*
>>>> +     * The attributes for each sysfs file are constant (handler 
>>>> functions,
>>>> +     * name and permissions are the same within each directory), 
>>>> but we
>>>> +     * need a per-directory copy regardless, to get a unique handle
>>>> +     * for each directory, so that container_of can do its magic.
>>>> +     * Also this requires an explicit sysfs_attr_init(), since it's 
>>>> a new
>>>> +     * copy, to make LOCKDEP happy.
>>>> +     */
>>>> +    memcpy(attrs->image_attrs, image_attrs_group,
>>>> +           sizeof(attrs->image_attrs));
>>>> +    for (int i = 0; i < LFA_ATTR_NR_IMAGES; i++) {
>>>> +        struct attribute *attr = &attrs->image_attrs[i].attr;
>>>> +
>>>> +        sysfs_attr_init(attr);
>>>> +        ret = sysfs_create_file(attrs->image_dir, attr);
>>>> +        if (ret) {
>>>> +            pr_err("creating sysfs file for uuid %s: %d\n",
>>>> +                   fw_uuid, ret);
>>>> +            clean_fw_images_tree();
>>>> +
>>>> +            return ret;
>>>> +        }
>>>> +    }
>>>> +    list_add(&attrs->image_node, &lfa_fw_images);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int update_fw_images_tree(void)
>>>> +{
>>>> +    struct arm_smccc_1_2_regs reg = { 0 };
>>>> +    struct uuid_regs image_uuid;
>>>> +    char image_id_str[40];
>>>> +    int ret, num_of_components;
>>>> +
>>>> +    num_of_components = get_nr_lfa_components();
>>>> +    if (num_of_components <= 0) {
>>>> +        pr_err("Error getting number of LFA components\n");
>>>> +        return -ENODEV;
>>>> +    }
>>>> +
>>>> +    for (int i = 0; i < num_of_components; i++) {
>>>> +        reg.a0 = LFA_1_0_FN_GET_INVENTORY;
>>>> +        reg.a1 = i; /* fw_seq_id under consideration */
>>>> +        arm_smccc_1_2_invoke(&reg, &reg);
>>>> +        if (reg.a0 == LFA_SUCCESS) {
>>>> +            image_uuid.uuid_lo = reg.a1;
>>>> +            image_uuid.uuid_hi = reg.a2;
>>>> +
>>>> +            snprintf(image_id_str, sizeof(image_id_str), "%pUb",
>>>> +                 &image_uuid);
>>>> +            ret = update_fw_image_node(image_id_str, i,
>>>> +                            reg.a3, reg.a4, reg.a5);
>>>> +            if (ret)
>>>> +                return ret;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int __init lfa_init(void)
>>>> +{
>>>> +    struct arm_smccc_1_2_regs reg = { 0 };
>>>> +    int err;
>>>> +
>>>> +    reg.a0 = LFA_1_0_FN_GET_VERSION;
>>>> +    arm_smccc_1_2_invoke(&reg, &reg);
>>>> +    if (reg.a0 == -LFA_NOT_SUPPORTED) {
>>>> +        pr_info("Live Firmware activation: no firmware agent 
>>>> found\n");
>>>> +        return -ENODEV;
>>>> +    }
>>>> +
>>>> +    fw_images_update_wq = alloc_workqueue("fw_images_update_wq",
>>>> +                         WQ_UNBOUND | WQ_MEM_RECLAIM, 1);
>>>> +    if (!fw_images_update_wq) {
>>>> +        pr_err("Live Firmware Activation: Failed to allocate 
>>>> workqueue.\n");
>>>> +
>>>> +        return -ENOMEM;
>>>> +    }
>>>> +
>>>> +    pr_info("Live Firmware Activation: detected v%ld.%ld\n",
>>>> +        reg.a0 >> 16, reg.a0 & 0xffff);
>>>> +
>>>> +    lfa_dir = kobject_create_and_add("lfa", firmware_kobj);
>>>> +    if (!lfa_dir)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    mutex_lock(&lfa_lock);
>>>> +    err = update_fw_images_tree();
>>>> +    if (err != 0)
>>>> +        kobject_put(lfa_dir);
>>>> +
>>>> +    mutex_unlock(&lfa_lock);
>>>> +    return err;
>>>> +}
>>>> +module_init(lfa_init);
>>>> +
>>>> +static void __exit lfa_exit(void)
>>>> +{
>>>> +    flush_workqueue(fw_images_update_wq);
>>>> +    destroy_workqueue(fw_images_update_wq);
>>>> +
>>>> +    mutex_lock(&lfa_lock);
>>>> +    clean_fw_images_tree();
>>>> +    mutex_unlock(&lfa_lock);
>>>> +
>>>> +    kobject_put(lfa_dir);
>>>> +}
>>>> +module_exit(lfa_exit);
>>>> +
>>>> +MODULE_DESCRIPTION("ARM Live Firmware Activation (LFA)");
>>>> +MODULE_LICENSE("GPL");
>>>
>>
> 

Powered by blists - more mailing lists