Re: [PATCH v5 RESEND 4/6] firmware: smccc: lfa: Add auto_activate sysfs file

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] firmware: smccc: lfa: teardown on success in lfa_smccc_probe()
- [High] firmware: smccc: lfa: infinite loop in prime_fw_component() on retry
- [Medium] firmware: smccc: lfa: kobject leak in update_fw_components_tree()
--

commit 9e007983d149d16e1c362d811e6caba071d543eb
Author: Andre Przywara <andre.przywara@arm.com>

firmware: smccc: lfa: Add auto_activate sysfs file

This commit adds a per-component auto_activate sysfs attribute to control
whether platform-initiated notifications automatically trigger firmware
activation in activate_pending_components().

> diff --git a/drivers/firmware/smccc/lfa_fw.c b/drivers/firmware/smccc/lfa_fw.c
> index 4b4daf79611bb..65f28ffc9d108 100644
> --- a/drivers/firmware/smccc/lfa_fw.c
> +++ b/drivers/firmware/smccc/lfa_fw.c

[ ... ]

> @@ -306,6 +308,7 @@ static int update_fw_components_node(char *fw_uuid, int seq_id,
>  
>  	comp->kobj.kset = lfa_kset;
>  	comp->cpu_rendezvous_forced = true;
> +	comp->auto_activate = false;
>  	set_component_flags(comp, seq_id, comp_flags, reg_current_ver,
>  			    reg_pending_ver);
>  	if (kobject_init_and_add(&comp->kobj, &component_ktype, NULL,

[Severity: Medium]
As reported by Sashiko on v2 [1], if update_fw_components_node() fails after
earlier iterations of the loop in update_fw_components_tree() have already
registered fw_component kobjects:

drivers/firmware/smccc/lfa_fw.c:update_fw_components_tree() {
	...
	for (int i = 0; i < num_of_components; i++) {
		...
		if (res.a0 == LFA_SUCCESS) {
			...
			ret = update_fw_components_node(comp_id_str, i, res.a3,
							res.a4, res.a5);
			if (ret)
				return ret;
		}
	}
	...
}

and lfa_smccc_probe() then unwinds on that error:

drivers/firmware/smccc/lfa_fw.c:lfa_smccc_probe() {
	...
	err = update_fw_components_tree();
	if (err != 0) {
		kset_unregister(lfa_kset);
		destroy_workqueue(fw_comps_update_wq);
		return err;
	}
	...
}

does this leak the fw_component kobjects (and their references on lfa_kset)
created in prior iterations because clean_fw_components_tree() is never
called before kset_unregister(lfa_kset)? Also, if this fails during runtime
activation, returning before queue_work() skips removing components whose
fw_seq_id was invalidated to -1 at the start of update_fw_components_tree().

[ ... ]

> @@ -791,7 +817,8 @@ static void activate_pending_components(void)
>  			continue; /* Invalid FW component */
>  
>  		_update_fw_component_pending(comp);
> -		if (comp->activation_capable && comp->activation_pending) {
> +		if (comp->activation_capable && comp->activation_pending &&
> +		    comp->auto_activate) {
>  			kobject_get(&comp->kobj);
>  			all_comps[nr_comps++] = comp;
>  		}

[Severity: High]
When an ACPI notification triggers lfa_acpi_notify() ->
activate_pending_components(), can lfa_kset already be unregistered and
freed after a successful probe?

In lfa_smccc_probe(), after update_fw_components_tree() succeeds with
err == 0, either acpi_disabled is true (leaving err == 0) or
lfa_register_acpi(&sdev->dev) succeeds and returns err == 0:

drivers/firmware/smccc/lfa_fw.c:lfa_smccc_probe() {
	...
	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;
	}

	return 0;
}

Because 0 != -ENODEV is true, doesn't lfa_smccc_probe() tear down lfa_kset
and fw_comps_update_wq whenever probe succeeds, while returning 0 with the
driver still bound and the ACPI notify handler still installed (as well as
leaking the child fw_component kobjects without calling
clean_fw_components_tree())? Note that this remains present at the end of the
series when IRQ setup is added in commit 93a930701ee9.

[Severity: High]
When activate_pending_components() (or activate_store()) calls
prime_fw_component(comp), what happens if ARM_SMCCC_LFA_PRIME returns
LFA_PRIME_CALL_AGAIN on the first call and then succeeds with regs.a1 == 0 on
a subsequent iteration?

drivers/firmware/smccc/lfa_fw.c:prime_fw_component() {
	ktime_t end = ktime_add_ms(ktime_get(), LFA_PRIME_BUDGET_MS);
	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);
	...
}

Since retry is initialized before the do-while loop and never reset to false
when LFA_PRIME_CALL_AGAIN is cleared, won't the loop spin indefinitely
calling ARM_SMCCC_LFA_PRIME without sleeping once priming finishes?

[1] https://lore.kernel.org/r/20260317103336.1273582-7-andre.przywara@arm.com

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