From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3705D38D40D for ; Mon, 13 Jul 2026 11:41:57 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783942919; cv=none; b=dp2ap/TZt7CyX3lgCKd3gKo83q+DgPtgcjm3qzai8oi65SkEr1/vz2DQ9MsRGZw5tMj48rBSGvb7xs5wb01POViCapacsoCi3tS/DNoxXDyjY0LoefafgbcehonJjg54g5IEt7InZYFv7Jf+6ntm8vzZRxP1bipgbTN+Y02ELDE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783942919; c=relaxed/simple; bh=bEnCwg6xpwIYz06yiiA/6o+xj0Ts2s98B2jiAJstRJQ=; h=Message-ID:Date:MIME-Version:Subject:To:References:From: In-Reply-To:Content-Type; b=pGmsiHy9V69oDJ1GHA+9nzrESja8Rndv7OEnJL58eQJQLRBfVeia3jHQTbfknbLuFnCdL957UA07neU482rNXrxgE6CNwxg93ADEJ47udz7nIdJCMvGhrLMM4v/J0sYJJCguX2VQQbLDWeENU38LSx2oWPHbZXVWbTIAJ24UI1w= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=EFm/UTt1; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="EFm/UTt1" Received: by smtp.kernel.org (Postfix) with ESMTPSA id A3BA21F000E9; Mon, 13 Jul 2026 11:41:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1783942917; bh=c8TeRE8qnP1sXZIirzkcQeAqEBSiYKdY4bpEdH1s6BI=; h=Date:Subject:To:References:From:In-Reply-To; b=EFm/UTt1QRmYTjER4xn0/K38ZEBUANjrmDLyGYCv6DPZ1MEMVueJwZLOlmn4y6r+/ 0Jrc0LsvBpde0sWGkoRP3EpjzU4OiqC8vKWOBRaw7J5fqlxmVekvhGh4ZlJdOAfFtB jwv0hSKL9oynWNt3hwQ/hkbnG+zYC4AGZ70NsDKHR7r1G/ndaVtxD8ozFvncdJrbyN geKs0joztTplz8S63/rRcRucxMcuF77zXuCAyOGppkfKXerd7mCanukGW95q4Sk4+G KbttjnToQ4R6U5eUJT+M3XbzHI7MFyfifrDhPDH/IFKV4CL769HceEL5awiCV9/aZS vJI7H06tSjAig== Message-ID: <30cbc51d-d324-4158-941b-ecbf6d2d0761@kernel.org> Date: Mon, 13 Jul 2026 06:41:56 -0500 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] firmware: stratix10-svc: add PSCI secondary CPU offline on warm reboot for agilex and stratix10 Content-Language: en-US To: Adrian Ng Ho Yin , linux-kernel@vger.kernel.org References: <1717fff5f780948a2288fa8a57821cb5d19c30f5.1783409844.git.adrian.ho.yin.ng@altera.com> From: Dinh Nguyen In-Reply-To: <1717fff5f780948a2288fa8a57821cb5d19c30f5.1783409844.git.adrian.ho.yin.ng@altera.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 > --- > 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 > #include > #include > +#include > +#include > +#include > +#include > > /** > * 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