Re: [PATCH v5 RESEND 5/6] firmware: smccc: lfa: Register DT interrupt

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] firmware: smccc: lfa: probe success treated as error in lfa_smccc_probe()
- [High] firmware: smccc: lfa: invalid IRQF_COND_ONESHOT in lfa_register_dt()
- [High] firmware: smccc: lfa: infinite retry loop in prime_fw_component()
- [Medium] firmware: smccc: lfa: unconditional devm_free_irq() in lfa_smccc_remove()
- [Medium] firmware: smccc: lfa: missing kobject cleanup on lfa_smccc_probe() error
- [Low] firmware: smccc: lfa: missing linux/interrupt.h include in lfa_fw.c
--

commit 336251e18192897c278c68cc88128e6b0243d876
Author: Andre Przywara <andre.przywara@arm.com>

firmware: smccc: lfa: Register DT interrupt

This commit adds support for finding an arm,lfa devicetree node and
registering its threaded interrupt handler to trigger pending Live
Firmware Activation updates when ACPI is disabled.

> diff --git a/drivers/firmware/smccc/lfa_fw.c b/drivers/firmware/smccc/lfa_fw.c
> index 65f28ffc9d108..3ff368dca9497 100644
> --- a/drivers/firmware/smccc/lfa_fw.c
> +++ b/drivers/firmware/smccc/lfa_fw.c
> @@ -46,6 +46,8 @@
>  #include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/nmi.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
>  #include <linux/psci.h>
>  #include <linux/rwsem.h>
>  #include <linux/stop_machine.h>

[Severity: Low]
As reported by kernel test robot on v2 [1], does drivers/firmware/smccc/lfa_fw.c
fail to build when CONFIG_ACPI is disabled?

The new code uses irqreturn_t, IRQ_HANDLED, IRQF_COND_ONESHOT,
devm_request_threaded_irq(), and devm_free_irq(), which are declared in
linux/interrupt.h. When CONFIG_ACPI=y, linux/acpi.h pulls in
linux/interrupt.h transitively, but with CONFIG_ACPI=n neither linux/of.h
nor linux/of_irq.h includes it, resulting in implicit function declaration
and undeclared identifier errors. Should linux/interrupt.h be included
explicitly here?

[ ... ]

> @@ -913,6 +916,31 @@ static void lfa_remove_acpi(struct device *dev)
>  }
>  #endif
>  
> +static irqreturn_t lfa_irq_handler_thread(int irq, void *dev_id)
> +{
> +	activate_pending_components();
> +
> +	return IRQ_HANDLED;
> +}

[Severity: High]
When lfa_irq_handler_thread() invokes activate_pending_components() ->
prime_fw_component(), can prime_fw_component() get stuck in an infinite
loop if priming requires at least one retry?

In 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 LFA_PRIME_CALL_AGAIN is set on an initial ARM_SMCCC_LFA_PRIME call,
retry is set to true. On a subsequent iteration where firmware finishes
priming and returns regs.a0 == 0 with LFA_PRIME_CALL_AGAIN cleared in
regs.a1, the if ((regs.a1 & LFA_PRIME_CALL_AGAIN)) branch is not entered
and retry is never reset to false, causing while (retry) to spin
indefinitely issuing SMC calls without sleeping or checking the timeout.

> +
> +static int lfa_register_dt(struct device *dev)
> +{
> +	struct device_node *np;
> +
> +	np = of_find_compatible_node(NULL, NULL, "arm,lfa");
> +	if (!np)
> +		return -ENODEV;
> +
> +	lfa_irq = irq_of_parse_and_map(np, 0);
> +	of_node_put(np);
> +	if (!lfa_irq)
> +		return -ENODEV;
> +
> +	return devm_request_threaded_irq(dev, lfa_irq, NULL,
> +					 lfa_irq_handler_thread, IRQF_COND_ONESHOT,
> +					 NULL, NULL);
> +}

[Severity: High]
As reported by Sashiko on v2 [2], will devm_request_threaded_irq() in
lfa_register_dt() always fail with -EINVAL on standard Arm GIC interrupt
controllers?

Passing NULL as the primary handler causes request_threaded_irq() to assign
irq_default_primary_handler. In __setup_irq(), IRQF_COND_ONESHOT only sets
IRQF_ONESHOT when joining an already-shared interrupt line whose existing
action has IRQF_ONESHOT set. Because this interrupt is not shared,
IRQF_ONESHOT remains unset, and __setup_irq() rejects the request:

	} else if (new->handler == irq_default_primary_handler &&
		   !(desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)) {
		pr_err("Threaded irq requested with handler=NULL and !ONESHOT for %s (irq %d)\n",
		       new->name, irq);
		ret = -EINVAL;
		goto out_unlock;
	}

Since standard Arm GIC irqchips do not set IRQCHIP_ONESHOT_SAFE, this
returns -EINVAL and aborts lfa_smccc_probe(). Should this pass IRQF_ONESHOT
instead of IRQF_COND_ONESHOT?

> +
>  static bool lfa_smccc_has_all_functions(void)
>  {
>  	struct arm_smccc_1_2_regs regs = { 0 }, res;
> @@ -980,6 +1008,10 @@ static int lfa_smccc_probe(struct arm_smccc_device *sdev)
>  		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");
>  	}
>  
>  	/*

[Severity: Critical]
When lfa_register_dt() or lfa_register_acpi() succeeds and returns 0 in
lfa_smccc_probe(), doesn't the immediately following error check tear down
lfa_kset and fw_comps_update_wq while still returning 0 from probe?

In drivers/firmware/smccc/lfa_fw.c:lfa_smccc_probe():

	/*
	 * 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;
	}

When err is 0, (err != -ENODEV) is true, so lfa_smccc_probe() unregisters
lfa_kset, destroys fw_comps_update_wq, and returns 0. Because probe returns
0, the device remains bound with the DT interrupt or ACPI notify handler
registered, leading to a use-after-free on lfa_kset and fw_comps_update_wq
when a notification fires or when lfa_smccc_remove() runs. Should the
condition be if (err && err != -ENODEV)?

[Severity: Medium]
As reported by Sudeep Holla on v4 [3], when lfa_smccc_probe() fails after
update_fw_components_tree() (for example when lfa_register_dt() or
lfa_register_acpi() returns a real error, or if update_fw_components_tree()
fails after adding components), does the error path leak the allocated
fw_component kobjects and race with fw_comps_update_work?

Unlike lfa_smccc_remove(), the error cleanup in lfa_smccc_probe():

	if (err != -ENODEV) {
		kset_unregister(lfa_kset);
		destroy_workqueue(fw_comps_update_wq);
		return err;
	}

does not call clean_fw_components_tree() to drop the child fw_component
kobjects added under lfa_kset, and it calls kset_unregister(lfa_kset)
before flushing and destroying fw_comps_update_wq while
remove_invalid_fw_components() may still be accessing lfa_kset.

> @@ -999,6 +1031,7 @@ 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);

[Severity: Medium]
In lfa_smccc_remove(), won't calling devm_free_irq() unconditionally
trigger a WARN_ON() when ACPI is used or when the optional DT interrupt
was not registered?

On ACPI systems (!acpi_disabled) or DT systems where the arm,lfa node or
its interrupt is absent, devm_request_threaded_irq() is never called and
lfa_irq remains 0. Calling devm_free_irq(&sdev->dev, lfa_irq, NULL) in
lfa_smccc_remove() causes devres_release() in kernel/irq/devres.c to fail
to find a matching devres entry and trigger WARN_ON(). Should
devm_free_irq() only be called when lfa_irq is non-zero (or omitted since
the IRQ is managed via devm on &sdev->dev)?

[1] https://lore.kernel.org/r/202603181913.NFRwBsN4-lkp@intel.com
[2] https://lore.kernel.org/r/20260317103336.1273582-8-andre.przywara@arm.com
[3] https://lore.kernel.org/r/20260921-aloof-sympathetic-python-54aa7d@sudeepholla

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