From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f45.google.com (mail-wm1-f45.google.com [209.85.128.45]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 01F113C1094 for ; Wed, 10 Jun 2026 15:07:54 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.45 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781104076; cv=none; b=hd7aA6DpEACr6akkLvvVTxTrLKTTg9lpVn7xpKd4kHcwYx+YHTIXR3FM4e8vy6Apozc9TJc6t/+uY3DDna6HL7HUVOycF7/fM32XektSBchTgBZ7EMVayB3zjIK+vd/LDIXZW4u8UCnfn1f4dx06cxtWc6EO+DAn8Hh+cOaWOA8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781104076; c=relaxed/simple; bh=58f25bTT4YoEtaY+ECpkwpOWUnLt7jdCAYoUReU5Q4E=; h=Mime-Version:Content-Type:Date:Message-Id:Cc:Subject:From:To: References:In-Reply-To; b=mR9jBkfaK9TpNOFhlQoT/JV2/q+TCqKiXq2+SJZGtTFVXAZ08DwhA09K8knYbVV4GLwzMD8scCwCJQ+yi91pbBIzXSgiuxGUXTS4dw6301nk2OzcwQXewIzYIC87H0YazrgTxMtESxqldaNrzPfut04l6ZuGO6YJHM+tYHFLFJo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=luwAhd02; arc=none smtp.client-ip=209.85.128.45 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="luwAhd02" Received: by mail-wm1-f45.google.com with SMTP id 5b1f17b1804b1-490afc47455so36865655e9.2 for ; Wed, 10 Jun 2026 08:07:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1781104073; x=1781708873; darn=vger.kernel.org; h=in-reply-to:references:to:from:subject:cc:message-id:date :content-transfer-encoding:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=FelCh2VDHUplvnJ8mnvXATyOKPI2L/eG8fwTa4ZA6GU=; b=luwAhd02DIShXYU+eJ4F+iPcw4Nr28LtCSo/v4r0mIftbgTZcz0CsXyNHBlb1j5X7v nSEfzwlAKKwZueWlmjRExEgGObsl6N9s2zzN3FynQZohJSYi2yS0ExoApMOSLFVwb8rB B4AKIPRcZ7T4B8hWrJzqhuEiEoI1LE/zwFf79ME5jhrSFCmFjnaEt7tYP1Lid3gXqSJ5 8XiAPgDVLXJomHbWduh+WkWpQKMwqM8pzU/YfV9Sd5JFytF/OMB0z2dmCb64T2QMujb2 MyHt4dMwaKA41MNLijyqyv9Gjw8leRNZy3sR50AWeI+hqLzh4a66FWSNaFIZ9GZDy93+ L2oQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781104073; x=1781708873; h=in-reply-to:references:to:from:subject:cc:message-id:date :content-transfer-encoding:mime-version:x-gm-gg:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=FelCh2VDHUplvnJ8mnvXATyOKPI2L/eG8fwTa4ZA6GU=; b=lxEW9tlq23cUTTVexGVEFaD658Mc3D7WtZiVsftEtBp8R9llJTHjls0kJ5f3apAq5h E/9hhI2l8hwbqKhhhmtAQuPX5YewRBdeA3nVun+fVjwRlk5LHywxEh020cNkepIvdzdf IO1Ya1/zCFLMTuBIOyDLrP/O0UVlZ7X2i7y725U63+cLkgieP1fvwTUuS/3uUKu5tdgH fmYvFCqowcpGdrBVKdeiAaLAKxaNmc0iE3dtcPr5tvQGL5/6NrvlLQU1UPSgml9jCUlS aOn2Y9atfmiivGD8Zdeb1T4QtC01hHS+FqL7tdYSGeCHCmcMq06itcyDej/nKA8mAm9h jGvw== X-Forwarded-Encrypted: i=1; AFNElJ+S5nTAf3umocPrCkjDcxEb8/Cw8jXcEo1+fF9wXp1NR6g5Xkhs9TtPI2bH7iU4Asjn2ZA8TZR88I3t2A0=@vger.kernel.org X-Gm-Message-State: AOJu0Yy5O5Gea1f0BvbP0lkm8XqHljsLvb9f0jlXrJlgjy5wKq88KDQK fgeFjOa4benMIaPZzEHIv3TpF1T+UVSsSkGsJufybebEQypkOdOQSJnQAG2zg6/2i/Y= X-Gm-Gg: Acq92OE20lru8fhcv2ExFX8DJntcT3jhYXxvFMdRasmKq2Q+rFH36ut5vKFV/CkIw0y NvvU4ASqk371j77YDBMEo6Ev7RYUaksgviUfu6UQ7mjT2pYKntRaus/0LRwHyD6T41WAts9xg7K Xk6ApHcbwOr7iNGjq3ii+EXaPT+POC8xalpmJ8vaUSVbmx1vlpLNv2E1+DmKZEbOWa8UNp7RIgx wf2goI5bI/L6H9/4K4jn4/FaOyMifc/VHJC+86Z8Y7/pRnQVbwCPc+u0NlmdCRtnDr+YX+NDA6m ccEl/2JMnMXQdeSP3mszkzRZG8bPwQnDc3e6DfW4CFey0tvcGrgbeGDejH3/y2ncQdl+Ty65ypy K9w+oYEEeBzRC000HQAedlDmWy89cUsBwaHku5cnYsyCni955TWPhZ2VBANpmSpZhWAsh9h+NvJ nIb5HtsS0qJur7wwSevjvXcHUQ12HyEs+LjW17eOXS+W5Z5b7+KjCCD14F6ETeE5QM1jIxbWidg X2iPIMfhG9OxPeFSdZaxl94 X-Received: by 2002:a05:600c:8b0c:b0:490:44eb:c1ea with SMTP id 5b1f17b1804b1-490d722a0e9mr93925085e9.24.1781104073271; Wed, 10 Jun 2026 08:07:53 -0700 (PDT) Received: from localhost ([94.4.195.193]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-490be1f69bcsm610037105e9.8.2026.06.10.08.07.52 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 10 Jun 2026 08:07:52 -0700 (PDT) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 10 Jun 2026 16:07:51 +0100 Message-Id: Cc: "Krzysztof Kozlowski" , "Alim Akhtar" , "Sam Protsenko" , , , , , "Sashiko" Subject: Re: [PATCH 3/3] soc: samsung: exynos-pmu: fix error paths in cpuhotplug/idle states setup From: "Alexey Klimov" To: "Peter Griffin" , "Alexey Klimov" X-Mailer: aerc 0.21.0 References: <20260605-exynos-pmu-cpuhp-idle-fixes-v1-0-0cd05c81a82d@linaro.org> <20260605-exynos-pmu-cpuhp-idle-fixes-v1-3-0cd05c81a82d@linaro.org> In-Reply-To: On Wed Jun 10, 2026 at 2:34 PM BST, Peter Griffin wrote: > Hi Alexey, Hi Peter, > Thanks for your patch! > > On Fri, 5 Jun 2026 at 21:19, Alexey Klimov wro= te: >> >> The setup_cpuhp_and_cpuidle() initialisation sequence currently ignores >> the return values of cpuhp_setup_state(), cpu_pm_register_notifier(), an= d >> register_reboot_notifier(). If any of these registrations fail during >> probe() routine, the driver returns 0, leaving the driver partially >> configured. > > I originally made the failure non-fatal because the system still boots > without the notifiers registered (and all other Arm64 Exynos SoCs > upstream don't register notifiers and AFAICT have broken cpu hotplug > and cpu idle). > > In hindsight, that seems like a mistake. I think your patch to fully > unwind everything in case of failure makes more sense. See small > comment below about destroy_cpuhp_and_cpuidle() Wait, setup_cpuhp_and_cpuidle() should be non-fatal and shouldn't return any errors? Why do we need to have notifiers (say cpu_pm_register_notifier()) registered if, for instance, cpuhp_setup_state() fails? The other thing I didn't get is that this doesn't deal with handling errors/return values of cpuhp_setup_state() in probe() and there are still a lot of errors returned from setup_cpuhp_and_cpuidle(). >> Furthermore, if anything after setup_cpuhp_and_cpuidle() fails in probe(= ) >> routine, for instance devm_mfd_add_devices(), the probe() lacks an error >> path and leaves notifiers and cpu hotplug states registered. >> >> Introduce variables for the cpu hotplug state IDs in exynos_pmu_context >> struct, that should be initialised to CPUHP_INVALID by default. Check al= l >> return codes in setup_cpuhp_and_cpuidle(), and add an error path to remo= ve >> registered states on failure. Finally, add destroy_cpuhp_and_cpuidle() >> helper to safely tear down notifiers and cpu hotplug states. >> >> Reported-by: Sashiko >> Closes: https://sashiko.dev/#/patchset/20260513-exynos850-cpuhotplug-v4-= 0-54fec5f65362@linaro.org?part=3D3 >> Fixes: 78b72897a5c8 ("soc: samsung: exynos-pmu: Enable CPU Idle for gs10= 1") >> Cc: stable@vger.kernel.org >> Signed-off-by: Alexey Klimov >> --- >> drivers/soc/samsung/exynos-pmu.c | 57 +++++++++++++++++++++++++++++++++= +------ >> 1 file changed, 49 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exyn= os-pmu.c >> index 9636287f6794..846313a28e9a 100644 >> --- a/drivers/soc/samsung/exynos-pmu.c >> +++ b/drivers/soc/samsung/exynos-pmu.c >> @@ -38,6 +38,8 @@ struct exynos_pmu_context { >> unsigned long *in_cpuhp; >> bool sys_insuspend; >> bool sys_inreboot; >> + int cpuhp_prepare_state; >> + int cpuhp_online_state; >> }; >> >> void __iomem *pmu_base_addr; >> @@ -404,6 +406,17 @@ static struct notifier_block exynos_cpupm_reboot_nb= =3D { >> .notifier_call =3D exynos_cpupm_reboot_notifier, >> }; >> >> +static void destroy_cpuhp_and_cpuidle(void) >> +{ >> + cpu_pm_unregister_notifier(&gs101_cpu_pm_notifier); >> + unregister_reboot_notifier(&exynos_cpupm_reboot_nb); >> + >> + if (pmu_context->cpuhp_prepare_state !=3D CPUHP_INVALID) >> + cpuhp_remove_state(pmu_context->cpuhp_prepare_state); >> + if (pmu_context->cpuhp_online_state !=3D CPUHP_INVALID) >> + cpuhp_remove_state(pmu_context->cpuhp_online_state); >> +} >> + >> static int setup_cpuhp_and_cpuidle(struct device *dev) >> { >> struct device_node *intr_gen_node; >> @@ -465,16 +478,42 @@ static int setup_cpuhp_and_cpuidle(struct device *= dev) >> gs101_cpuhp_pmu_online(cpu); >> >> /* register CPU hotplug callbacks */ >> - cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "soc/exynos-pmu:prepare"= , >> - gs101_cpuhp_pmu_online, NULL); >> + pmu_context->cpuhp_prepare_state =3D CPUHP_INVALID; >> + pmu_context->cpuhp_online_state =3D CPUHP_INVALID; >> >> - cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "soc/exynos-pmu:online", >> - NULL, gs101_cpuhp_pmu_offline); >> + ret =3D cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "soc/exynos-pmu:= prepare", >> + gs101_cpuhp_pmu_online, NULL); >> + if (ret < 0) >> + return ret; >> + >> + pmu_context->cpuhp_prepare_state =3D ret; >> + >> + ret =3D cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "soc/exynos-pmu:o= nline", >> + NULL, gs101_cpuhp_pmu_offline); >> + if (ret < 0) >> + goto clean_cpuhp_states; >> + >> + pmu_context->cpuhp_online_state =3D ret; >> >> /* register CPU PM notifiers for cpuidle */ >> - cpu_pm_register_notifier(&gs101_cpu_pm_notifier); >> - register_reboot_notifier(&exynos_cpupm_reboot_nb); >> - return 0; >> + ret =3D cpu_pm_register_notifier(&gs101_cpu_pm_notifier); >> + if (ret) >> + goto clean_cpuhp_states; >> + >> + ret =3D register_reboot_notifier(&exynos_cpupm_reboot_nb); >> + if (!ret) >> + /* Success */ >> + return ret; >> + >> + cpu_pm_unregister_notifier(&gs101_cpu_pm_notifier); >> + >> +clean_cpuhp_states: >> + if (pmu_context->cpuhp_prepare_state !=3D CPUHP_INVALID) >> + cpuhp_remove_state(pmu_context->cpuhp_prepare_state); >> + if (pmu_context->cpuhp_online_state !=3D CPUHP_INVALID) >> + cpuhp_remove_state(pmu_context->cpuhp_online_state); >> + >> + return ret; >> } >> >> static int exynos_pmu_probe(struct platform_device *pdev) >> @@ -548,8 +587,10 @@ static int exynos_pmu_probe(struct platform_device = *pdev) >> >> ret =3D devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, exynos_pm= u_devs, >> ARRAY_SIZE(exynos_pmu_devs), NULL, 0,= NULL); >> - if (ret) >> + if (ret) { >> + destroy_cpuhp_and_cpuidle(); > > You only want to do this if pmu_cpuhp =3D=3D true, as currently only gs10= 1 > registers the notifiers. Thanks! That's good catch. Best regards, Alexey