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

[Nirmoy Das <nirmoyd@nvidia.com>]

Hi Andre

On 02.11.26 17:54, Andre Przywara wrote:
> From: Vedashree Vidwans <vvidwans@nvidia.com>
>
> 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>
> ---
>   drivers/firmware/smccc/lfa_fw.c | 154 +++++++++++++++++++++++++++++++-
>   1 file changed, 153 insertions(+), 1 deletion(-)
>
> 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
> @@ -33,12 +33,14 @@
>    * skipped by setting the force_cpu_rendezvous sysfs file to false.
>    */
>   
> +#include <linux/acpi.h>
>   #include <linux/arm-smccc.h>
>   #include <linux/arm-smccc-bus.h>
>   #include <linux/array_size.h>
>   #include <linux/delay.h>
>   #include <linux/fs.h>
>   #include <linux/init.h>
> +#include <linux/kernel.h>
>   #include <linux/kobject.h>
>   #include <linux/ktime.h>
>   #include <linux/list.h>
> @@ -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"
>   #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);
> +		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);
> +		if (ret) {
> +			pr_warn("LFA: LFA_ACTIVATE failed for component %s\n",
> +				kobject_name(&comp->kobj));
> +			kobject_put(&comp->kobj);
> +			continue;
> +		}
> +
> +		pr_info("%s: automatic activation succeeded\n",
> +			kobject_name(&comp->kobj));
> +		kobject_put(&comp->kobj);
> +	}
> +
> +	kfree(all_comps);
> +}
> +
> +#ifdef CONFIG_ACPI
> +static void lfa_acpi_notify_handler(acpi_handle handle, u32 event, void *data)
> +{
> +	/* The spec explicitly mentions 0x80 only for the update event. */
> +	if (event != 0x80)
> +		return;
> +
> +	activate_pending_components();
> +}
> +
> +static int lfa_register_acpi(struct device *dev)
> +{
> +	struct acpi_device *acpi_dev;
> +	acpi_handle handle;
> +	acpi_status status;
> +
> +	acpi_dev = acpi_dev_get_first_match_dev("ARML0003", NULL, -1);
> +	if (!acpi_dev)
> +		return -ENODEV;
> +	handle = acpi_device_handle(acpi_dev);
> +	if (!handle) {
> +		acpi_dev_put(acpi_dev);
> +		return -ENODEV;
> +	}
> +
> +	/* Register notify handler that indicates LFA updates are available */
> +	status = acpi_install_notify_handler(handle, ACPI_DEVICE_NOTIFY,
> +					     lfa_acpi_notify_handler, NULL);
> +	if (ACPI_FAILURE(status)) {
> +		acpi_dev_put(acpi_dev);
> +		return -EIO;
> +	}
> +
> +	ACPI_COMPANION_SET(dev, acpi_dev);
> +
> +	return 0;
> +}
> +
> +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);
> +}
> +#else	/* !CONFIG_ACPI */
> +static int lfa_register_acpi(struct device *dev)
> +{
> +	return -ENODEV;
> +}
> +
> +static void lfa_remove_acpi(struct device *dev)
> +{
> +}
> +#endif
> +
>   static bool lfa_smccc_has_all_functions(void)
>   {
>   	struct arm_smccc_1_2_regs regs = { 0 }, res;
> @@ -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) {

On successful registration, err is zero, so this condition still 
unregisters the inventory and destroys the workqueue before returning 
success. Should it be:

if (err && err != -ENODEV) {


Regards,

Nirmoy

> +		kset_unregister(lfa_kset);
> +		destroy_workqueue(fw_comps_update_wq);
> +		return err;
>   	}
>   
> -	return err;
> +	return 0;
>   }
>   
>   static void lfa_smccc_remove(struct arm_smccc_device *sdev)
>   {
> +	if (!acpi_disabled)
> +		lfa_remove_acpi(&sdev->dev);
>   	clean_fw_components_tree();
>   	flush_workqueue(fw_comps_update_wq);
>   	destroy_workqueue(fw_comps_update_wq);