Re: [PATCH] firmware: stratix10-svc: add PSCI secondary CPU offline on warm reboot for agilex and stratix10

[Dinh Nguyen <dinguyen@kernel.org>]

On 7/7/26 02:39, Adrian Ng Ho Yin wrote:
> On Agilex and Stratix10 SoCs, secondary CPUs must be offlined before a
> warm reboot to ensure a clean PSCI state. Add a reboot notifier that
> queues work items to offline each secondary CPU via remove_cpu() when a
> SYS_RESTART is triggered in REBOOT_WARM mode.
> 
> The workqueue, completion, pending CPU counter and reboot notifier block
> are stored in stratix10_svc_controller to maintain per-device ownership.
> The notifier callback recovers the controller pointer via container_of().
> 
> CPUs are pre-counted before any work is queued to avoid a race where a
> fast-completing worker could signal the completion before all CPUs are
> enqueued. The mechanism is guarded by of_device_is_compatible() and is
> not activated on platforms such as Agilex5 that do not require it.
> 
> Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
> ---
>   drivers/firmware/stratix10-svc.c | 134 ++++++++++++++++++++++++++++++-
>   1 file changed, 133 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index de938ab2db0b..0924027b41d7 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
> @@ -24,6 +24,10 @@
>   #include <linux/firmware/intel/stratix10-smc.h>
>   #include <linux/firmware/intel/stratix10-svc-client.h>
>   #include <linux/types.h>
> +#include <linux/psci.h>
> +#include <linux/of.h>
> +#include <linux/cpuhplock.h>
> +#include <linux/reboot.h>
>   
>   /**
>    * SVC_NUM_DATA_IN_FIFO - number of struct stratix10_svc_data in the FIFO
> @@ -96,6 +100,12 @@
>   #define STRATIX10_GET_SDM_STATUS_CODE(status) \
>   	(FIELD_GET(STRATIX10_SDM_STATUS_MASK, status))
>   
> +struct psci_cpu_off_work {
> +	struct work_struct work;
> +	struct stratix10_svc_controller *ctrl;
> +	int cpu;
> +};
> +
>   typedef void (svc_invoke_fn)(unsigned long, unsigned long, unsigned long,
>   			     unsigned long, unsigned long, unsigned long,
>   			     unsigned long, unsigned long,
> @@ -280,6 +290,10 @@ struct stratix10_svc_chan {
>    * @svc: manages the list of client svc drivers
>    * @sdm_lock: only allows a single command single response to SDM
>    * @actrl: async control structure
> + * @psci_reboot_nb: reboot notifier for PSCI secondary CPU offlining
> + * @psci_cpu_off_wq: workqueue used to offline secondary CPUs on warm reboot
> + * @psci_offline_done: completion signalled when all secondary CPUs are offline
> + * @psci_pending_cpus: number of secondary CPUs not yet offlined
>    * @chans: array of service channels
>    *
>    * This struct is used to create communication channels for service clients, to
> @@ -296,6 +310,10 @@ struct stratix10_svc_controller {
>   	struct stratix10_svc *svc;
>   	struct mutex sdm_lock;
>   	struct stratix10_async_ctrl actrl;
> +	struct notifier_block psci_reboot_nb;
> +	struct workqueue_struct *psci_cpu_off_wq;
> +	struct completion psci_offline_done;
> +	atomic_t psci_pending_cpus;
>   	struct stratix10_svc_chan chans[] __counted_by(num_chans);
>   };
>   
> @@ -1981,6 +1999,92 @@ void stratix10_svc_free_memory(struct stratix10_svc_chan *chan, void *kaddr)
>   }
>   EXPORT_SYMBOL_GPL(stratix10_svc_free_memory);
>   
> +static void psci_cpu_off_worker(struct work_struct *work)
> +{
> +	struct psci_cpu_off_work *cw =
> +		container_of(work, struct psci_cpu_off_work, work);
> +	struct stratix10_svc_controller *ctrl = cw->ctrl;
> +	int ret;
> +
> +	ret = remove_cpu(cw->cpu);

Could cw->cpu be NULL here?

> +	if (ret)
> +		pr_err("psci_cpu_off: failed to offline CPU%d: %d\n", cw->cpu, ret);
> +
> +	if (atomic_dec_and_test(&ctrl->psci_pending_cpus))
> +		complete(&ctrl->psci_offline_done);
If remove_cpu() failed, but you continue to decrement the pending count 
and potentially trigger the completion.

The parent thread waiting on psci_offline_done will wake up assuming the 
system is cleanly parked, even if a CPU refused to offline.

If this is intended as a "best-effort" shutdown before a forced reboot, 
this is fine.

If the caller requires all CPUs to be successfully offlined before 
proceeding (to avoid hardware deadlocks), you need to pass a failure 
state back to the ctrl structure so the caller knows the operation was 
compromised.

> +
> +	kfree(cw);
> +}
> +
> +static void psci_offline_secondary_cpus(struct stratix10_svc_controller *ctrl)
> +{
> +	int cpu, me;
> +	struct psci_cpu_off_work *cw;
> +
> +	/* safely get current CPU id (disables preemption briefly) */
> +	me = get_cpu();
> +	put_cpu();
> +
> +	/* Pre-count before queuing to avoid a worker calling dec_and_test
> +	 * and completing before all work items have been queued.
> +	 */
> +	atomic_set(&ctrl->psci_pending_cpus, 0);
> +	for_each_online_cpu(cpu)
> +		if (cpu != me)
> +			atomic_inc(&ctrl->psci_pending_cpus);
> +
> +	if (!atomic_read(&ctrl->psci_pending_cpus))
> +		return;
> +
> +	reinit_completion(&ctrl->psci_offline_done);
> +
> +	for_each_online_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		cw = kzalloc_obj(*cw, GFP_KERNEL);
> +		if (!cw) {
> +			if (atomic_dec_and_test(&ctrl->psci_pending_cpus))
> +				complete(&ctrl->psci_offline_done);
> +			continue;
> +		}
> +
> +		cw->ctrl = ctrl;
> +		INIT_WORK(&cw->work, psci_cpu_off_worker);

I dug deeper in the sashiko warning about this[0]. From Gemini 3.1 Pro:

"Spawning multiple threads here adds unnecessary scheduler overhead, 
memory allocations (kfree(cw)), and code complexity for zero performance 
gain. You should replace this fan-in worker pattern with a single work 
item that simply loops through the target CPUs and offlines them 
sequentially."

> +		cw->cpu = cpu;
> +		queue_work(ctrl->psci_cpu_off_wq, &cw->work);
> +	}
> +
> +	if (!wait_for_completion_timeout(&ctrl->psci_offline_done,
> +					 msecs_to_jiffies(1000)))
> +		pr_warn("psci_cpu_off: timed out waiting for secondary CPUs to offline\n");
> +}
> +
> +static int psci_cpu_off_reboot_notifier(struct notifier_block *nb,
> +					unsigned long action, void *data)
> +{
> +	struct stratix10_svc_controller *ctrl =
> +		container_of(nb, struct stratix10_svc_controller, psci_reboot_nb);
> +
> +	if (reboot_mode != REBOOT_WARM)
> +		return NOTIFY_DONE;
> +
> +	if (action == SYS_RESTART)
> +		psci_offline_secondary_cpus(ctrl);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static void psci_cpu_off_teardown(struct stratix10_svc_controller *ctrl)
> +{
> +	unregister_reboot_notifier(&ctrl->psci_reboot_nb);
> +	if (ctrl->psci_cpu_off_wq) {
> +		flush_workqueue(ctrl->psci_cpu_off_wq);
> +		destroy_workqueue(ctrl->psci_cpu_off_wq);
> +		ctrl->psci_cpu_off_wq = NULL;
> +	}
> +}
> +
>   static const struct of_device_id stratix10_svc_drv_match[] = {
>   	{.compatible = "intel,stratix10-svc"},
>   	{.compatible = "intel,agilex-svc"},
> @@ -2001,6 +2105,7 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
>   	struct gen_pool *genpool;
>   	struct stratix10_svc_sh_memory *sh_memory;
>   	struct stratix10_svc *svc = NULL;
> +	struct device_node *node = pdev->dev.of_node;
>   
>   	svc_invoke_fn *invoke_fn;
>   	size_t fifo_size;
> @@ -2040,13 +2145,36 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
>   	INIT_LIST_HEAD(&controller->node);
>   	init_completion(&controller->complete_status);
>   
> +	if (of_device_is_compatible(node, "intel,agilex-svc") ||
> +	    of_device_is_compatible(node, "intel,stratix10-svc")) {

It'd be better if you avoid using these funtions and instead have a data 
structure associated with these compatibles.


Dinh

[0] 
https://sashiko.dev/#/patchset/1717fff5f780948a2288fa8a57821cb5d19c30f5.1783409844.git.adrian.ho.yin.ng%40altera.com