From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ed1-f50.google.com (mail-ed1-f50.google.com [209.85.208.50]) (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 0930540BCB2 for ; Wed, 10 Jun 2026 13:34:34 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=pass smtp.client-ip=209.85.208.50 ARC-Seal:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781098481; cv=pass; b=bun9sWIlDntBfiuhFAPuuGg2rCeyelRtNtOjPGSMbESvrnOouDKSWqJry49eMmu2BoVeuSpchL4jcL9zqHaagPHfy1Eu0XwBZNHiM97l3D8qa+JVN0M2P2206Y0XOY1GnX2Kd40IAJODFW/OvmgD8IDlnCMjtVSuPnMWCM1B7QE= ARC-Message-Signature:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781098481; c=relaxed/simple; bh=JAR4sei5juO8181/3mmsMW50Yqk1AXNa41CYDKywtHU=; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject: To:Cc:Content-Type; b=jxsr1xCGNgRjCY+5f88m9P1MHc5LNDnnRTxGz2RFZQ66xbl29TuqBrBzPoLivyk11ibpwKPboPTJNXb5hK+ik3R93+UMI5DZTDv4m4qc5lcnO8AGuP0IP45ug+P563NRo3NYZqQ7lxdxJ60M9WSWb0FU8xRX7TAdnd9xGc6PsJU= ARC-Authentication-Results:i=2; 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=r65yP2o4; arc=pass smtp.client-ip=209.85.208.50 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="r65yP2o4" Received: by mail-ed1-f50.google.com with SMTP id 4fb4d7f45d1cf-68c5eecdd56so10145749a12.3 for ; Wed, 10 Jun 2026 06:34:34 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1781098473; cv=none; d=google.com; s=arc-20240605; b=Vx8SrSeqarfcJwA1pqTkbdRUGLtCvIqiI+7Qdx2H27Mlaum3eSmRwlyAS8P78d7M+A HIAxPbGWzW8aMfHd5I39zl2HmnJDpLPbezdrFFenI1RqxUcnVTeqiVJdlfNHWJQ/vBgj dJx4JZa9ulWD6Sl1QzxZeko+ikIzg1KsXu820LZ6Y6JVYmzzJPYSCpXdMYAa2gAubzRt lOfWN2As1r5Hg+kTQmfYPHmZ3es8hS7kflr1r5D4QESQGKWebYeWKVjR/0pxCjDm/MUL ARz7KNxZk75NIKkanUP9tXZIy7z0wqhX0vL4VXgW6NWP9E/T1H3fSGCeJgngDJJoT48W XwrQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:dkim-signature; bh=wusSRoPz5XHuYf/hgRAbVTth3meUxSSzw6AAFjCcAZ8=; fh=U/GDF1wRqkz1LPeahlnRxY8eToGbC7Ds22XEaAkU7z4=; b=B5H3x05Z1erXgznKHFiE2GNgU+/SHtT0SiocV/MsKYFZpmcQ3A5kuvZBxB7Ds3tJGO K8pS8m8bn+iL4+10Fxqa/Jfop1Epep4Ikvs4blC6S2oLK+mojQ6R+xSVrTt3Ak9YO2bW uUAjlWkU5jxQdpDV6EFM4sU7ZJFEj32PH7NNpNrcySG7IS7FwqXbtLjuQyb1FOfgBwyw UME/V6mJWhSHPVvnEWhJYtapULTWFbLE9xuZS8RodIqXI2I90S5j6AHvdjxzyrXKUIGc ANPiFOGaMXFKNjT3O9qkIB4AYGAuEXamwO17VWCsksVnCp7tZ0iNrBTRI8fMFdkAqNYb S0Yw==; darn=vger.kernel.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1781098473; x=1781703273; darn=vger.kernel.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=wusSRoPz5XHuYf/hgRAbVTth3meUxSSzw6AAFjCcAZ8=; b=r65yP2o4m6EbvApoz6B9olIJyccqqM+nsr9La+vvKTORiM9dhjL+w8m31GYAsnJr+m DpZRmHfMImMwwa5BsihxuKsmVYxTcP25BVyyMB9Q9k/wertyodkhP5KFNb6umUoG+XgW 5Bnnx/FkW/oii78eVtgvjthXKAy6Qo1GjtU78Ypix94oqFc8srX5iI11fZsAWTJk5XTz mV1d3D2GJPHp16g74gjP/ZxSS6p9wt2E0LqBHCGkwTmoBrIp3ez0mY5WGnzor+0QH0B9 dOFFuS7gty6/1yUkWPgxbOuvFThtifc0bA/154DG/Y0PIuDIK1kPK6eUIwUCsrIcHas6 lM1w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781098473; x=1781703273; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=wusSRoPz5XHuYf/hgRAbVTth3meUxSSzw6AAFjCcAZ8=; b=gpd/SI/ZcUoJg2Of0pOlESF1RbJ1oNPyAI78NiyPHd744bKFvKtxELHRgTZcMUvPyU 9QGmQSRONOOH/FkfS/XmhNEvdLSvQdloF8jye3nJHheDW2TtpCPmPnGot2LgSbUgaNqB RkWTB/FX38lFsgmHb8ZpGhMhLYRefJe5kjVUKOaj6DcfA24nIOMY4D73o97qYHaOvW1v LAWm8Itoy2PJaAQj+KaxbSjLwa9nz3d11qRw79eHPdcfl165vfTBmb0iWlnrIL1RfPSN W1lW4TJWa89ZNdrN5rNL+BdaEg+ziyyM1fagKY5JQu5uhPOu2e4K4mJBVyTdiOOaYIED kuGQ== X-Forwarded-Encrypted: i=1; AFNElJ9V1f6uyhvhc0otOdeo6HcXGmiEp15oCGVxT5cHlA+7iDxnbEj4cK74N2ZBW8D64uHCPHAqDcnjq/UBVoA=@vger.kernel.org X-Gm-Message-State: AOJu0YyU+mnMsEIqPlrpS2JPaiAqwTruPL6loNEaxftzCwqzyuCSIqjw aD4i3UCwrfQAYe6sRJEEtpflIfC3BUm3GPqFJdvICjpH2oPfAho+wp73BsTS2iMwo2ywtJM77WI tUzPxwzDXBlmwT9y5GfakMlVlsZV9MSLwloZvERw9xg== X-Gm-Gg: Acq92OFX74NAuvHw1VcOjtLgYMV5LbYcvpBEGHHW4fLOclfcvHxD6Ofq7GEAblRedUn qkxxWKJ69QDOxQa5JD2O0HoPZfsDlFMIvI4Tz12RwQTaHE0hyYgy/TU8fli0WGewhA2S6XmB8oY OPcYYCMdtk9WGzy1keU4mfyrShOjiZ7fiHuDhMzpqgjIk3nXZfxCRJ89oCXXwBrr/3hWoL72jG4 SuSUlIjv3jueqVDjjvvxUh1qIKlwk00yDakRcWL5x7m0pJv2YyIhv5qDe36aiccDTDHxMcM/yPK JHWUA1CUBCYdepoEDxN2elxKy4UGxHCVxu+LUjuzmp8lrVy5yAmK X-Received: by 2002:a05:6402:3905:b0:68b:f026:f381 with SMTP id 4fb4d7f45d1cf-68fa4e3140bmr11010010a12.8.1781098473183; Wed, 10 Jun 2026 06:34:33 -0700 (PDT) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.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: <20260605-exynos-pmu-cpuhp-idle-fixes-v1-3-0cd05c81a82d@linaro.org> From: Peter Griffin Date: Wed, 10 Jun 2026 14:34:21 +0100 X-Gm-Features: AVVi8CdudhNuk1JulHq85Six9g2TTnXsPBjPVrWOLueCdeIJGpUeMcdSZ2qVZRE Message-ID: Subject: Re: [PATCH 3/3] soc: samsung: exynos-pmu: fix error paths in cpuhotplug/idle states setup To: Alexey Klimov Cc: Krzysztof Kozlowski , Alim Akhtar , Sam Protsenko , linux-samsung-soc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org, Sashiko Content-Type: text/plain; charset="UTF-8" Hi Alexey, Thanks for your patch! On Fri, 5 Jun 2026 at 21:19, Alexey Klimov wrote: > > The setup_cpuhp_and_cpuidle() initialisation sequence currently ignores > the return values of cpuhp_setup_state(), cpu_pm_register_notifier(), and > 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() > > 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 all > return codes in setup_cpuhp_and_cpuidle(), and add an error path to remove > 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=3 > Fixes: 78b72897a5c8 ("soc: samsung: exynos-pmu: Enable CPU Idle for gs101") > 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/exynos-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 = { > .notifier_call = 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 != CPUHP_INVALID) > + cpuhp_remove_state(pmu_context->cpuhp_prepare_state); > + if (pmu_context->cpuhp_online_state != 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 = CPUHP_INVALID; > + pmu_context->cpuhp_online_state = CPUHP_INVALID; > > - cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "soc/exynos-pmu:online", > - NULL, gs101_cpuhp_pmu_offline); > + ret = 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 = ret; > + > + ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "soc/exynos-pmu:online", > + NULL, gs101_cpuhp_pmu_offline); > + if (ret < 0) > + goto clean_cpuhp_states; > + > + pmu_context->cpuhp_online_state = 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 = cpu_pm_register_notifier(&gs101_cpu_pm_notifier); > + if (ret) > + goto clean_cpuhp_states; > + > + ret = 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 != CPUHP_INVALID) > + cpuhp_remove_state(pmu_context->cpuhp_prepare_state); > + if (pmu_context->cpuhp_online_state != 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 = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, exynos_pmu_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 == true, as currently only gs101 registers the notifiers. Thanks, Peter