Re: [PATCH v4 4/6] soc: samsung: exynos-pmu: add Exynos850 CPU hotplug support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] exynos850_cpu_pmu_online() and exynos850_cpu_pmu_offline() incorrectly configure PMU registers for the executing control CPU instead of the target CPU.
- [High] Lock nesting violation (sleeping in atomic context) when accessing the Exynos850 syscon regmap.
- [Low] Missing parentheses around the macro argument `cl` in `EXYNOS850_CLUSTER_CPU_OFFSET`.
--

commit 1fa894e1840a285a12214486e682639554622ecb
Author: Alexey Klimov <alexey.klimov@linaro.org>

soc: samsung: exynos-pmu: add Exynos850 CPU hotplug support

This commit adds CPU hotplug support for Exynos850 platforms by providing
specific PMU and interrupt generation register updates for sleep states
and CPU online/offline transitions.

> diff --git a/drivers/soc/samsung/exynos850-pmu.c b/drivers/soc/samsung/exynos850-pmu.c
> new file mode 100644
> index 0000000000000..40df158819239
> --- /dev/null
> +++ b/drivers/soc/samsung/exynos850-pmu.c

[ ... ]

> +static int exynos850_cpu_pmu_offline(struct exynos_pmu_context *pmu_context, unsigned int cpu)
> +	__must_hold(&pmu_context->cpupm_lock)
> +{
> +	u32 this_cluster = MPIDR_AFFINITY_LEVEL(read_cpuid_mpidr(), 2);
> +	u32 cluster_cpu = MPIDR_AFFINITY_LEVEL(read_cpuid_mpidr(), 1);
> +	unsigned int cpuhint = smp_processor_id();

Since exynos850_cpu_pmu_offline() is invoked during the CPUHP_BP_PREPARE_DYN
hotplug state, won't this code execute on the control CPU (Boot Processor)
instead of the target CPU being taken offline?

By using read_cpuid_mpidr() and smp_processor_id() here, it looks like this
reads the physical ID and hardware MPIDR of the executing control CPU rather
than deriving them from the target cpu argument.

> +	u32 reg, mask;
> +
> +	/* set cpu inform hint */
> +	regmap_write(pmu_context->pmureg, EXYNOS850_CPU_INFORM(cpuhint),
> +		     CPU_INFORM_C2);

Could calling regmap_write() here cause a lock nesting regression?

Because cpupm_lock is a raw_spinlock_t, and the pmureg for Exynos850 is
created via syscon_node_to_regmap(), pmureg is protected by a standard
spinlock_t instead of a raw spinlock. Acquiring a spinlock_t while holding
a raw_spinlock_t can lead to a sleeping in atomic context bug on PREEMPT_RT.

> +
> +	mask = BIT(cpu);

Does this need to convert the logical cpu parameter to a physical core ID
using cpu_logical_map(cpu) before using it as a hardware bitmask?

Using the logical CPU index directly might configure the wrong hardware
registers if the logical to physical mapping is not 1:1.

[ ... ]

> diff --git a/include/linux/soc/samsung/exynos-regs-pmu.h b/include/linux/soc/samsung/exynos-regs-pmu.h
> index 9c4d3da41dbf2..93c4d724c8eac 100644
> --- a/include/linux/soc/samsung/exynos-regs-pmu.h
> +++ b/include/linux/soc/samsung/exynos-regs-pmu.h

[ ... ]

> @@ -1015,6 +1015,11 @@
>  #define EXYNOS_GRP2_INTR_BID_UPEND				(0x0208)
>  #define EXYNOS_GRP2_INTR_BID_CLEAR				(0x020c)
>  
> +/* Exynos850 PMU Alive */
> +#define EXYNOS850_CPU_INFORM(cpu)		(0x0860 + ((cpu) & 7) * 4)
> +#define EXYNOS850_CLUSTER_CPU_OFFSET(cl, cpu)	(0x1000 + ((cl * 0x400) + ((cpu) * 0x80)))

Is it safer to wrap the cl argument in parentheses?

If an arithmetic expression is passed for cl in the future, operator
precedence could cause it to evaluate incorrectly and result in out-of-bounds
register offsets.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-exynos850-cpuhotplug-v4-0-54fec5f65362@linaro.org?part=4