Message-ID: <6c6d0e84-f371-42fd-b51b-dc5dcbea2a45@arm.com>
Date: Mon, 19 Jan 2026 19:50:26 +0000
From: Salman Nabi <salman.nabi@....com>
To: Vedashree Vidwans <vvidwans@...dia.com>, sudeep.holla@....com,
 andre.przywara@....com, lpieralisi@...nel.org, mark.rutland@....com
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: [RFC PATCH 5/5] firmware: smccc: lfa: refresh fw details

Hi Veda,

First of all sorry for the delay in responding back to you. You may have already seen my patch submission, following the RFC, here: https://lore.kernel.org/all/20260119122729.287522-2-salman.nabi@arm.com/

I have incorporated all the bug fixes and code refactor/cleanup suggestions from the RFC. It also includes changes to the LFA spec, mainly current and pending firmware version retrieval. I did not include your work and am hoping you would submit follow-up patches to include the updated ACPI table, IRQ handling, and platform driver registration etc.

I would also like to point a few issues here, this would also act as additional justification to some of the code changes I have introduced, for example, a work_queue() for updating firmware images from the activate_store handler.

On 12/8/25 22:13, Vedashree Vidwans wrote:
> FW image details are querried with a SMC call. Currently, these FW
> details are added as nodes in a linked list. This patch updates the
> FW node creation and update functions. Now the linked list is updated
> based on the current value of num_lfa_components.
> As per spec [1], FW inventory is updated after each activation.
>
> [1] https://developer.arm.com/documentation/den0147/latest/
>
> Signed-off-by: Vedashree Vidwans <vvidwans@...dia.com>
> ---
>  drivers/firmware/smccc/lfa_fw.c | 148 +++++++++++++++++++++-----------
>  1 file changed, 100 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/firmware/smccc/lfa_fw.c b/drivers/firmware/smccc/lfa_fw.c
> index 24916fc53420..334090708405 100644
> --- a/drivers/firmware/smccc/lfa_fw.c
> +++ b/drivers/firmware/smccc/lfa_fw.c
> @@ -133,6 +133,7 @@ static const struct fw_image_uuid {
>  	},
>  };
>  
> +static int update_fw_images_tree(void);
>  static struct kobject *lfa_dir;
>  static DEFINE_MUTEX(lfa_lock);
>  
> @@ -282,17 +283,6 @@ static int activate_fw_image(struct image_props *attrs)
>  	return ret;
>  }
>  
> -static void set_image_flags(struct image_props *attrs, int seq_id,
> -			    u32 image_flags)
> -{
> -	attrs->fw_seq_id = seq_id;
> -	attrs->activation_capable = !!(image_flags & BIT(0));
> -	attrs->activation_pending = !!(image_flags & BIT(1));
> -	attrs->may_reset_cpu = !!(image_flags & BIT(2));
> -	/* cpu_rendezvous_optional bit has inverse logic in the spec */
> -	attrs->cpu_rendezvous = !(image_flags & BIT(3));
> -}
> -
>  static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,
>  			 char *buf)
>  {
> @@ -395,7 +385,9 @@ static ssize_t activate_store(struct kobject *kobj, struct kobj_attribute *attr,
>  
>  	pr_info("Firmware activation succeeded\n");
>  
> -	/* TODO: refresh image flags here*/
> +	ret = update_fw_images_tree();

Part of the firmware images update process, there is a potential of a firmware component and its attributes being deleted following activation. The to-be-removed component could very well be the one where the activate call has been triggered from. This results in a deadlock as removing a sysfs attribute from within its _store handler results in kernfs to wait for the in-flight store() operation to finish before it can remove itself.

To cater for this scenario in my patch I am differing the firmware images update process, triggered from the _store handler, to a workqueue().

> +	if (ret)
> +		pr_err("Failed to update FW images tree");
>  	mutex_unlock(&lfa_lock);
>  	return count;
>  }
> @@ -425,20 +417,41 @@ static struct kobj_attribute image_attrs_group[LFA_ATTR_NR_IMAGES] = {
>  	[LFA_ATTR_CANCEL]		= __ATTR_WO(cancel)
>  };
>  
> +static void delete_fw_image_node(struct image_props *attrs)
> +{
> +	int i;
> +
> +	for (i = 0; i < LFA_ATTR_NR_IMAGES; i++)
> +		sysfs_remove_file(attrs->image_dir, &attrs->image_attrs[i].attr);
> +
> +	kobject_put(attrs->image_dir);
> +	attrs->image_dir = NULL;
> +	list_del(&attrs->image_node);
> +	kfree(attrs);
> +}
> +
>  static void clean_fw_images_tree(void)
>  {
>  	struct image_props *attrs, *tmp;
>  
> -	list_for_each_entry_safe(attrs, tmp, &lfa_fw_images, image_node) {
> -		kobject_put(attrs->image_dir);
> -		list_del(&attrs->image_node);
> -		kfree(attrs);
> -	}
> +	list_for_each_entry_safe(attrs, tmp, &lfa_fw_images, image_node)
> +		delete_fw_image_node(attrs);
> +}
> +
> +static void update_fw_image_node(struct image_props *attrs, int seq_id,
> +			    char *fw_uuid, u32 image_flags)
> +{
> +	attrs->fw_seq_id = seq_id;
> +	attrs->image_name = fw_uuid;
We introduced a C struct "fw_image_uuid" to provide names for each firmware component in the LFA driver rather than using the UUID for a name which is non-intuitive. If UUIDs are desired for comparison then "attrs->image_dir->name" could be used.
> +	attrs->activation_capable = !!(image_flags & BIT(0));
> +	attrs->activation_pending = !!(image_flags & BIT(1));
> +	attrs->may_reset_cpu = !!(image_flags & BIT(2));
> +	/* cpu_rendezvous_optional bit has inverse logic in the spec */
> +	attrs->cpu_rendezvous = !(image_flags & BIT(3));
>  }
>  
> -static int create_fw_inventory(char *fw_uuid, int seq_id, u32 image_flags)
> +static int create_fw_image_node(int seq_id, char *fw_uuid, u32 image_flags)
>  {
> -	const char *image_name = "(unknown)";
>  	struct image_props *attrs;
>  	int ret;
>  
> @@ -446,21 +459,12 @@ static int create_fw_inventory(char *fw_uuid, int seq_id, u32 image_flags)
>  	if (!attrs)
>  		return -ENOMEM;
>  
> -	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;
> -		else
> -			image_name = fw_uuid;
> -	}
> -
>  	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);
> +	/* Update FW attributes */
> +	update_fw_image_node(attrs, seq_id, fw_uuid, image_flags);
>  
>  	/*
>  	 * The attributes for each sysfs file are constant (handler functions,
> @@ -485,17 +489,19 @@ static int create_fw_inventory(char *fw_uuid, int seq_id, u32 image_flags)
>  			return ret;
>  		}
>  	}
> -	list_add(&attrs->image_node, &lfa_fw_images);
> +	list_add_tail(&attrs->image_node, &lfa_fw_images);
>  
>  	return ret;
>  }
>  
> -static int create_fw_images_tree(void)
> +static int update_fw_images_tree(void)
>  {
>  	struct arm_smccc_1_2_regs reg = { 0 };
>  	struct uuid_regs image_uuid;
> +	struct image_props *attrs, *tmp;
>  	char image_id_str[40];
>  	int ret, num_of_components;
> +	int node_idx = 0;
>  
>  	num_of_components = get_nr_lfa_components();
>  	if (num_of_components <= 0) {
> @@ -503,22 +509,67 @@ static int create_fw_images_tree(void)
>  		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) {
> +	/*
> +	 * Pass 1:
> +	 *    For nodes < num_of_components, update fw_image_node
> +	 *    For nodes >= num_of_components, delete
> +	 */
> +	list_for_each_entry_safe(attrs, tmp, &lfa_fw_images, image_node) {
> +		if (attrs->fw_seq_id < num_of_components) {
> +			/* Update this FW image node */
> +
> +			/* Get FW details */
> +			reg.a0 = LFA_1_0_FN_GET_INVENTORY;
> +			reg.a1 = attrs->fw_seq_id;
> +			arm_smccc_1_2_invoke(&reg, &reg);
> +
> +			if (reg.a0 != LFA_SUCCESS)
> +				return -EINVAL;
> +
> +			/* Build image name with UUID */
>  			image_uuid.uuid_lo = reg.a1;
>  			image_uuid.uuid_hi = reg.a2;
> +			snprintf(image_id_str, sizeof(image_id_str), "%pUb", &image_uuid);
>  
> -			snprintf(image_id_str, sizeof(image_id_str), "%pUb",
> -				 &image_uuid);
> -			ret = create_fw_inventory(image_id_str, i, reg.a3);
> -			if (ret)
> -				return ret;
> +			if (strcmp(attrs->image_name, image_id_str)) {
attrs->image_name is supposed to be the name of the firmware image e.g. "TF-RMM" and not the UUID. If we need to compare the UUIDs I think we should use the specified firmware's directory name instead.
> +				/* UUID doesn't match previous UUID for given FW, not expected */

This is an expected behavior per the LFA spec. For example, firmware seq_ids get scrambled following an activation, the firmware image with seq_id 0 before activation has now a seq_id 1 following the activation in the LFA agent. When you attempt to request information for the firmware image using its old seq_id stored in the driver, the UUIDs would not match and thus the driver would bail out. We can't bail out for an expected behavior.

Just to confirm, my recent submission includes work on update of firmware images list following an activation.

Many thanks,
Salman

> +				pr_err("FW seq id %u: Previous UUID %s doesn't match current %s",
> +					attrs->fw_seq_id, attrs->image_name, image_id_str);
> +				return -EINVAL;
> +			}
> +
> +			/* Update FW attributes */
> +			update_fw_image_node(attrs, attrs->fw_seq_id, image_id_str, reg.a3);
> +			node_idx++;
> +		} else {
> +			/* This node is beyond valid FW components list */
> +			delete_fw_image_node(attrs);
>  		}
>  	}
>  
> +	/*
> +	 * Pass 2:
> +	 *    If current FW components number is more than previous list, add new component nodes.
> +	 */
> +	for (node_idx; node_idx < num_of_components; node_idx++) {
> +		/* Get FW details */
> +		reg.a0 = LFA_1_0_FN_GET_INVENTORY;
> +		reg.a1 = node_idx;
> +		arm_smccc_1_2_invoke(&reg, &reg);
> +
> +		if (reg.a0 != LFA_SUCCESS)
> +			return -EINVAL;
> +
> +		/* Build image name with UUID */
> +		image_uuid.uuid_lo = reg.a1;
> +		image_uuid.uuid_hi = reg.a2;
> +		snprintf(image_id_str, sizeof(image_id_str), "%pUb", &image_uuid);
> +
> +		ret = create_fw_image_node(node_idx, image_id_str, reg.a3);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -538,8 +589,9 @@ static irqreturn_t lfa_irq_thread(int irq, void *data)
>  	 */
>  
>  	do {
> -		/* TODO: refresh image flags here */
> -		/* If refresh fails goto exit_unlock */
> +		ret = update_fw_images_tree();
> +		if (ret)
> +			goto exit_unlock;
>  
>  		/* Initialize counters to track list traversal  */
>  		num_of_components = get_nr_lfa_components();
> @@ -561,13 +613,13 @@ static irqreturn_t lfa_irq_thread(int irq, void *data)
>  			}
>  
>  			pr_info("Firmware %s activation succeeded", attrs->image_name);
> -			/* Refresh FW component details */
>  			break;
>  		}
>  	} while (curr_component < num_of_components);
>  
> -	/* TODO: refresh image flags here */
> -	/* If refresh fails goto exit_unlock */
> +	ret = update_fw_images_tree();
> +	if (ret)
> +		goto exit_unlock;
>  
>  exit_unlock:
>  	mutex_unlock(&lfa_lock);
> @@ -657,7 +709,7 @@ static int __init lfa_init(void)
>  		return -ENOMEM;
>  
>  	mutex_lock(&lfa_lock);
> -	err = create_fw_images_tree();
> +	err = update_fw_images_tree();
>  	if (err != 0)
>  		kobject_put(lfa_dir);
>  

Powered by blists - more mailing lists