Re: [PATCH v5 RESEND 3/6] firmware: smccc: lfa: Register ACPI notification

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] firmware: smccc: lfa: inverted error check in lfa_smccc_probe() tears down on 0
- [High] firmware: smccc: lfa: missing component cleanup on error in lfa_smccc_probe()
- [High] firmware: smccc: lfa: stale fw_seq_id used in activate_pending_components()
- [High] firmware: smccc: lfa: infinite loop in prime_fw_component() when retry is set
- [High] firmware: smccc: lfa: race with remove_invalid_fw_components() on activation
- [Medium] firmware: smccc: lfa: dangling ACPI companion pointer in lfa_remove_acpi()
- [Low] firmware: smccc: lfa: unused DRIVER_NAME macro and stale commit description
--

commit 9b467f592a89b6ca0bddc96f9df536713b9c357c
Author: Vedashree Vidwans <vvidwans@nvidia.com>

firmware: smccc: lfa: Register ACPI notification

This commit adds ACPI notification support for Arm Live Firmware Activation
(LFA) so the platform can notify the kernel when pending firmware component
updates are ready to be primed and activated.

>     firmware: smccc: lfa: Register ACPI notification
>     
>     The Arm LFA spec describes an ACPI notification mechanism, where the
>     platform (firmware) can notify an LFA client about newly available
>     firmware imag updates ("pending images" in LFA terms).
>     
>     Add a faux device after discovering the existence of an LFA agent via
>     the SMCCC discovery mechnism, and use that device to check for the ACPI
>     notification description. Register this when one is provided.
>     
>     The notification just conveys the fact that at least one firmware image
>     has now a pending update, it doesn't say which, also there could be more
>     than one pending. Loop through all images to find every which needs to
>     be activated, and trigger the activation. We need to do this is a loop,
>     since an activation might change the number and the status of available
>     images.
>     
>     Signed-off-by: Vedashree Vidwans <vvidwans@nvidia.com>
>     [Andre: convert from platform driver to smccc bus]
>     Signed-off-by: Andre Przywara <andre.przywar@arm.com>
> 
> diff --git a/drivers/firmware/smccc/lfa_fw.c b/drivers/firmware/smccc/lfa_fw.c
> index db2a7fe344d5d..4b4daf79611bb 100644
> --- a/drivers/firmware/smccc/lfa_fw.c
> +++ b/drivers/firmware/smccc/lfa_fw.c
[ ... ]
> @@ -49,11 +51,13 @@
>  #include <linux/stop_machine.h>
>  #include <linux/string.h>
>  #include <linux/sysfs.h>
> +#include <linux/types.h>
>  #include <linux/uuid.h>
>  #include <linux/workqueue.h>
>  
>  #include <uapi/linux/psci.h>
>  
> +#define DRIVER_NAME	"ARM_LFA"

[Severity: Low]
This isn't a bug, but DRIVER_NAME does not appear to be used anywhere in
drivers/firmware/smccc/lfa_fw.c.

Also, the commit message still describes adding a faux device and
re-enumerating images in a loop on activation, whereas the patch uses the
existing arm_smccc_device on the SMCCC bus and snapshots the candidate array
once in activate_pending_components(). Could the unused macro be dropped and
the commit description updated to match the implementation?

>  #undef pr_fmt
>  #define pr_fmt(fmt) "Arm LFA: " fmt
>  
> @@ -753,6 +757,135 @@ static void clean_fw_components_tree(void)
>  	}
>  }
>  
> +/*
> + * Iterate over all FW components and trigger activation of all activatible and
> + * pending components.
> + * Since we would need to re-enumerate the list of all components after a
> + * successful activation, and this could trigger infinite loops if a firmware
> + * component still appears as pending, we create an array of update candidates
> + * first, then activate each one of them.
> + */
> +static void activate_pending_components(void)
> +{
> +	struct fw_component **all_comps;
> +	struct fw_component *comp;
> +	struct kobject *kobj;
> +	int nr_comps = 0;
> +	int ret, i;
> +
> +	down_read(&smc_lock);
> +	spin_lock(&lfa_kset->list_lock);
> +	all_comps = kmalloc_array(list_count_nodes(&lfa_kset->list),
> +				  sizeof(*all_comps), GFP_NOWAIT);
> +	if (!all_comps) {
> +		spin_unlock(&lfa_kset->list_lock);
> +		up_read(&smc_lock);
> +		pr_warn("LFA: automatic activation failed: -ENOMEM\n");
> +		return;
> +	}
> +
> +	list_for_each_entry(kobj, &lfa_kset->list, entry) {
> +		comp = kobj_to_fw_component(kobj);
> +
> +		if (comp->fw_seq_id == -1)
> +			continue; /* Invalid FW component */
> +
> +		_update_fw_component_pending(comp);
> +		if (comp->activation_capable && comp->activation_pending) {
> +			kobject_get(&comp->kobj);
> +			all_comps[nr_comps++] = comp;
> +		}
> +	}
> +	spin_unlock(&lfa_kset->list_lock);
> +	up_read(&smc_lock);
> +
> +	for (i = 0; i < nr_comps; i++) {
> +		comp = all_comps[i];
> +
> +		ret = prime_fw_component(comp);

[Severity: High]
As reported by Sashiko on v2 [1], can activate_pending_components() end up
calling prime_fw_component() and activate_fw_component() with an invalidated
comp->fw_seq_id of -1 when multiple components are in all_comps[] or when a
concurrent activation runs?

In activate_pending_components(), candidate pointers are collected into
all_comps[] under smc_lock and lfa_kset->list_lock, and then both locks are
released before the activation loop. When all_comps[0] is activated,
activate_fw_component() calls update_fw_components_tree() under
down_write(&smc_lock):

drivers/firmware/smccc/lfa_fw.c:update_fw_components_tree() {
	...
	spin_lock(&lfa_kset->list_lock);
	list_for_each_entry(kobj, &lfa_kset->list, entry) {
		struct fw_component *comp = kobj_to_fw_component(kobj);

		set_component_flags(comp, -1, 0b1000, 0, 0);
	}
	spin_unlock(&lfa_kset->list_lock);
	...
}

If a subsequent entry all_comps[i] is no longer present in the firmware
inventory after the earlier activation, had its activation_pending or
activation_capable bit cleared, or if update_fw_components_tree() failed
partway through, its comp->fw_seq_id remains -1.

Because prime_fw_component(), activate_fw_component(), and lfa_cancel() read
comp->fw_seq_id under smc_lock without checking whether comp->fw_seq_id == -1
or whether the component is still pending and activation-capable:

drivers/firmware/smccc/lfa_fw.c:prime_fw_component() {
	...
		touch_nmi_watchdog();
		down_read(&smc_lock);
		regs.a1 = comp->fw_seq_id;
		arm_smccc_1_2_invoke(&regs, &regs);
		up_read(&smc_lock);
	...
}

won't the driver invoke ARM_SMCCC_LFA_PRIME, ARM_SMCCC_LFA_ACTIVATE, and
ARM_SMCCC_LFA_CANCEL with regs.a1 = -1? Also, during the initial scan,
_update_fw_component_pending() only refreshes activation_pending without
updating activation_capable, may_reset_cpu, or cpu_rendezvous.

[Severity: High]
What happens in prime_fw_component() when the first ARM_SMCCC_LFA_PRIME call
returns LFA_PRIME_CALL_AGAIN in regs.a1 and a subsequent call completes with
regs.a1 == 0?

Looking at prime_fw_component() in drivers/firmware/smccc/lfa_fw.c:

drivers/firmware/smccc/lfa_fw.c:prime_fw_component() {
	...
	bool retry = false;
	...
	do {
		...
		if ((regs.a1 & LFA_PRIME_CALL_AGAIN)) {
			if (ktime_before(ktime_get(), end)) {
				if (msleep_interruptible(LFA_PRIME_DELAY_MS)) {
					ret = -EINTR;
					break;
				}
			} else {
				pr_err("LFA_PRIME for component %s timed out",
				       kobject_name(&comp->kobj));
				ret = -ETIMEDOUT;
				break;
			}
			retry = true;
		}
	} while (retry);
	...
}

Once retry is set to true on an iteration where LFA_PRIME_CALL_AGAIN is set,
nothing resets retry to false when a later call succeeds with
LFA_PRIME_CALL_AGAIN cleared. Because the sleep and timeout checks are both
inside the if ((regs.a1 & LFA_PRIME_CALL_AGAIN)) block and
touch_nmi_watchdog() is called on each pass, wouldn't this spin in an
unthrottled infinite loop issuing ARM_SMCCC_LFA_PRIME calls?

> +		if (ret) {
> +			pr_warn("LFA: LFA_PRIME failed for component %s\n",
> +				kobject_name(&comp->kobj));
> +			kobject_put(&comp->kobj);
> +			continue;
> +		}
> +
> +		ret = activate_fw_component(comp);

[Severity: High]
Could activating multiple components in activate_pending_components() race
with the asynchronous remove_invalid_fw_components() work item?

When the first component in all_comps[] is activated,
activate_fw_component() -> update_fw_components_tree() queues
fw_comps_update_work (remove_invalid_fw_components()) onto the unbound
workqueue fw_comps_update_wq and then drops smc_lock.

When the next component in all_comps[] is activated,
update_fw_components_tree() acquires lfa_kset->list_lock, resets
comp->fw_seq_id = -1 on every component in lfa_kset->list, and drops
lfa_kset->list_lock while querying ARM_SMCCC_LFA_GET_INVENTORY over SMC:

drivers/firmware/smccc/lfa_fw.c:update_fw_components_tree() {
	...
	spin_lock(&lfa_kset->list_lock);
	list_for_each_entry(kobj, &lfa_kset->list, entry) {
		struct fw_component *comp = kobj_to_fw_component(kobj);

		set_component_flags(comp, -1, 0b1000, 0, 0);
	}
	spin_unlock(&lfa_kset->list_lock);

	regs.a0 = ARM_SMCCC_LFA_GET_INVENTORY;
	for (int i = 0; i < num_of_components; i++) {
		...
}

Meanwhile, remove_invalid_fw_components() only takes lfa_kset->list_lock and
does not take smc_lock:

drivers/firmware/smccc/lfa_fw.c:remove_invalid_fw_components() {
	...
	spin_lock(&lfa_kset->list_lock);
	list_for_each_entry_safe(kobj, tmp, &lfa_kset->list, entry) {
		struct fw_component *comp = kobj_to_fw_component(kobj);

		if (comp->fw_seq_id == -1)
			list_move_tail(&kobj->entry, &comps_to_delete);
	}
	spin_unlock(&lfa_kset->list_lock);
	...
}

If remove_invalid_fw_components() runs during that window, won't it see
comp->fw_seq_id == -1 on valid components that have not yet been refreshed by
the inventory loop, remove them from lfa_kset->list, and delete their sysfs
kobjects?

[ ... ]
> +static void lfa_remove_acpi(struct device *dev)
> +{
> +	struct acpi_device *acpi_dev = ACPI_COMPANION(dev);
> +	acpi_handle handle = acpi_device_handle(acpi_dev);
> +
> +	if (handle)
> +		acpi_remove_notify_handler(handle,
> +					   ACPI_DEVICE_NOTIFY,
> +					   lfa_acpi_notify_handler);
> +	acpi_dev_put(acpi_dev);
> +}

[Severity: Medium]
In lfa_register_acpi(), ACPI_COMPANION_SET(dev, acpi_dev) attaches acpi_dev
as the fwnode companion on sdev->dev. When lfa_remove_acpi() later drops the
reference with acpi_dev_put(acpi_dev) on driver unbind or module unload,
should it also clear the companion pointer via ACPI_COMPANION_SET(dev, NULL)
so the persistent arm_smccc_device is not left with a dangling unreferenced
fwnode pointer?

[ ... ]
> @@ -813,13 +946,32 @@ static int lfa_smccc_probe(struct arm_smccc_device *sdev)
>  	if (err != 0) {
>  		kset_unregister(lfa_kset);
>  		destroy_workqueue(fw_comps_update_wq);
> +		return err;
> +	}
> +
> +	if (!acpi_disabled) {
> +		err = lfa_register_acpi(&sdev->dev);
> +		if (!err)
> +			pr_info("registered LFA ACPI notification\n");
> +	}
> +
> +	/*
> +	 * An ACPI notification or an interrupt are optional, but we must
> +	 * handle errors that occurred during their setup.
> +	 */
> +	if (err != -ENODEV) {
> +		kset_unregister(lfa_kset);
> +		destroy_workqueue(fw_comps_update_wq);
> +		return err;
>  	}

[Severity: Critical]
As reported by Sudeep Holla on v4 [2], when lfa_register_acpi() succeeds (or
when acpi_disabled is true after update_fw_components_tree() succeeded), err
is 0.

Because 0 != -ENODEV is true, won't lfa_smccc_probe() enter this error block,
unregister lfa_kset, destroy fw_comps_update_wq, and then return 0 (success)
with the ACPI notify handler still registered? Any subsequent 0x80 ACPI
notification or driver removal in lfa_smccc_remove() will then access the
destroyed lfa_kset and fw_comps_update_wq. Should this check be:

	if (err && err != -ENODEV)

instead?

[Severity: High]
On the same error path in lfa_smccc_probe(), if lfa_register_acpi() returns a
real error (such as -EIO) after update_fw_components_tree() has already
populated lfa_kset->list and queued fw_comps_update_work onto
fw_comps_update_wq, doesn't this unwind path leak all child struct
fw_component kobjects and race with the queued work item?

In lfa_smccc_remove(), teardown cleans the component tree and flushes/destroys
the workqueue before unregistering lfa_kset:

drivers/firmware/smccc/lfa_fw.c:lfa_smccc_remove() {
	if (!acpi_disabled)
		lfa_remove_acpi(&sdev->dev);
	clean_fw_components_tree();
	flush_workqueue(fw_comps_update_wq);
	destroy_workqueue(fw_comps_update_wq);
	kset_unregister(lfa_kset);
}

Should the probe failure path after update_fw_components_tree() follow the
same cleanup sequence?

[1] https://lore.kernel.org/r/20260317103336.1273582-9-andre.przywara@arm.com
[2] https://lore.kernel.org/r/20260921-smiling-rare-salamander-6c442d@sudeepholla

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102165421.3651202-1-andre.przywara@arm.com?part=3