Re: [PATCH v5 RESEND 6/6] firmware: smccc: lfa: add sysfs ABI documentation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] firmware: smccc: lfa: premature teardown on success in lfa_smccc_probe()
- [High] firmware: smccc: lfa: infinite loop in prime_fw_component() on retry
- [Medium] firmware: smccc: lfa: unconditional devm_free_irq() in lfa_smccc_remove()
- [Low] firmware: smccc: lfa: make file-local smc_lock static
--

commit 93a930701ee9d0e354315cd9d9c704901ce6e5c2
Author: Andre Przywara <andre.przywara@arm.com>

firmware: smccc: lfa: add sysfs ABI documentation

This series adds support for the Arm Live Firmware Activation (LFA)
specification over SMCCC, including sysfs attributes, ACPI notifications,
devicetree interrupts, and ABI documentation.

> diff --git a/drivers/firmware/smccc/lfa_fw.c b/drivers/firmware/smccc/lfa_fw.c
> new file mode 100644
> index 0000000000000..3ff368dca9497
> --- /dev/null
> +++ b/drivers/firmware/smccc/lfa_fw.c

[ ... ]

> +static struct kset *lfa_kset;
> +static struct workqueue_struct *fw_comps_update_wq;
> +static struct work_struct fw_comps_update_work;
> +static struct attribute *component_default_attrs[LFA_ATTR_NR_IMAGES + 1];
> +static unsigned int lfa_irq;
> +
> +/*
> + * A successful component activation might change the number of available
> + * components, leading to a re-order and thus re-assignment of the sequence IDs.
> + * The lock protects the connection between a firmware component (through its
> + * user visible UUID) and the sequence IDs. Anyone doing an SMC call with
> + * a sequence ID needs to take the readers lock. Doing an activation requires
> + * the writer lock, as that process might change the assocications.
> + */
> +struct rw_semaphore smc_lock;

[Severity: Low]
This isn't a bug, but since smc_lock is only used within
drivers/firmware/smccc/lfa_fw.c, should it be declared static like the
other file-scope variables above?

[ ... ]

> +static int prime_fw_component(struct fw_component *comp)
> +{
> +	ktime_t end = ktime_add_ms(ktime_get(), LFA_PRIME_BUDGET_MS);
> +	bool retry = false;
> +	int ret = 0;
> +
> +	if (comp->may_reset_cpu) {
> +		pr_err("CPU reset not supported by kernel driver\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * LFA_PRIME will return 1 in regs.a1 if the firmware priming
> +	 * is still in progress. In that case LFA_PRIME will need to
> +	 * be called again.
> +	 * regs.a1 will become 0 once the prime process completes.
> +	 */
> +	do {
> +		struct arm_smccc_1_2_regs regs = {
> +			.a0 = ARM_SMCCC_LFA_PRIME,
> +		};
> +
> +		touch_nmi_watchdog();
> +		down_read(&smc_lock);
> +		regs.a1 = comp->fw_seq_id;
> +		arm_smccc_1_2_invoke(&regs, &regs);
> +		up_read(&smc_lock);
> +
> +		if ((s64)regs.a0 < 0) {
> +			pr_err("LFA_PRIME for component %s failed: %s (%ld)\n",
> +			       kobject_name(&comp->kobj),
> +			       lfa_error_string((s64)regs.a0), regs.a0);
> +			return lfa_to_linux_errno((s64)regs.a0);
> +		}
> +
> +		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);

[Severity: High]
Can prime_fw_component() enter an infinite loop when firmware priming
takes more than one call?

In prime_fw_component(), retry is initialized to false before the do-while
loop rather than being reset on each iteration. If the first
ARM_SMCCC_LFA_PRIME call returns LFA_SUCCESS with LFA_PRIME_CALL_AGAIN set
in regs.a1, retry is set to true.

When a subsequent ARM_SMCCC_LFA_PRIME call completes priming and returns
LFA_SUCCESS with LFA_PRIME_CALL_AGAIN cleared, the if block checking
LFA_PRIME_CALL_AGAIN is skipped while retry remains true. Because the
timeout check and msleep_interruptible() are inside that conditional block,
will the loop spin indefinitely calling ARM_SMCCC_LFA_PRIME?

[ ... ]

> +static int lfa_smccc_probe(struct arm_smccc_device *sdev)
> +{

[ ... ]

> +	err = update_fw_components_tree();
> +	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");
> +	} else {
> +		err = lfa_register_dt(&sdev->dev);
> +		if (!err)
> +			pr_info("registered LFA DT notification interrupt\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;
> +}

[Severity: High]
When lfa_register_acpi() or lfa_register_dt() succeeds in
lfa_smccc_probe(), err is 0. Since 0 != -ENODEV evaluates to true, doesn't
this error path tear down lfa_kset and fw_comps_update_wq on a successful
registration while still returning 0 from probe, leaving child component
kobjects and the notification handler or interrupt active with freed
resources?

Also, if notification setup fails with a real error after
update_fw_components_tree() has populated component kobjects, should
clean_fw_components_tree() and flush_workqueue() be called before
destroying fw_comps_update_wq and unregistering lfa_kset?

> +
> +static void lfa_smccc_remove(struct arm_smccc_device *sdev)
> +{
> +	if (!acpi_disabled)
> +		lfa_remove_acpi(&sdev->dev);
> +	devm_free_irq(&sdev->dev, lfa_irq, NULL);
> +	clean_fw_components_tree();
> +	flush_workqueue(fw_comps_update_wq);
> +	destroy_workqueue(fw_comps_update_wq);
> +	kset_unregister(lfa_kset);
> +}

[Severity: Medium]
In lfa_smccc_remove(), devm_free_irq() is called unconditionally even when
running with ACPI enabled or when no arm,lfa DT interrupt was registered
during lfa_smccc_probe(). Will this trigger the WARN_ON in devm_free_irq()
when unbinding the driver or unloading the module on systems where no DT
interrupt was requested?

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