Re: [PATCH v2 2/2] soc: samsung: exynos-pmu: fix error paths in cpuhotplug/idle states setup

[Krzysztof Kozlowski]

On 28/08/2026 07:36, Alexey Klimov wrote:
>  void __iomem *pmu_base_addr;
> @@ -407,6 +409,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);
> +}

cleanup follows the setup usually, so this function should be after
setup_cpuhp_and_cpuidle().

> +
>  static int setup_cpuhp_and_cpuidle(struct device *dev)
>  {
>  	struct device_node *intr_gen_node __free(device_node) =
> @@ -458,16 +471,46 @@ 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;
> +
> +	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;

You have only one state to clean here, no? Error paths must be specific
- clean only what's needed, not a catch-all with if-checks.

>  
> -	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "soc/exynos-pmu:online",
> -			  NULL, gs101_cpuhp_pmu_offline);
> +	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);
> +		pmu_context->cpuhp_prepare_state = CPUHP_INVALID;
> +	}
> +	if (pmu_context->cpuhp_online_state != CPUHP_INVALID) {
> +		cpuhp_remove_state(pmu_context->cpuhp_online_state);
> +		pmu_context->cpuhp_online_state = CPUHP_INVALID;
> +	}
> +
> +	return ret;
>  }
>  
>  static int exynos_pmu_probe(struct platform_device *pdev)
> @@ -541,8 +584,12 @@ 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) {
> +		if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_cpuhp)
> +			destroy_cpuhp_and_cpuidle();
> +
>  		return ret;
> +	}
>  
>  	if (devm_of_platform_populate(dev))
>  		dev_err(dev, "Error populating children, reboot and poweroff might not work properly\n");
> 


Best regards,
Krzysztof