On 3/25/22 18:10, Rolf Eike Beer wrote:
> Am Freitag, 25. März 2022, 15:38:33 CET schrieb Helge Deller:
>> Add relevant code to __cpu_die() and __cpu_disable() to finally enable
>> the CPU hotplugging features. Reset the irq count values in smp_callin()
>> to zero before bringing up the CPU.
>>
>> Use "chcpu -d 1" to bring CPU1 down, and "chcpu -e 1" to bring it up.
>>
>> Signed-off-by: Helge Deller <deller@xxxxxx>
>> ---
>> arch/parisc/Kconfig | 1 +
>> arch/parisc/include/asm/smp.h | 9 +---
>> arch/parisc/kernel/smp.c | 80 +++++++++++++++++++++++++++++++++--
>> 3 files changed, 79 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/parisc/kernel/smp.c b/arch/parisc/kernel/smp.c
>> index a32a882a2d58..60cc33fd345c 100644
>> --- a/arch/parisc/kernel/smp.c
>> +++ b/arch/parisc/kernel/smp.c
>> @@ -430,10 +444,68 @@ void smp_cpus_done(unsigned int cpu_max)
>>
>> int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>> {
>> - if (cpu != 0 && cpu < parisc_max_cpus && smp_boot_one_cpu(cpu,
> tidle))
>> - return -ENOSYS;
>> + if (cpu_online(cpu))
>> + return 0;
>> +
>> + if (num_online_cpus() < parisc_max_cpus && smp_boot_one_cpu(cpu,
> tidle))
>> + return -EIO;
>
> I had to look up parisc_max_cpus, and found this:
>
>> static int parisc_max_cpus = 1;
>
> Hm, signed?
>
>> parisc_max_cpus = max_cpus;
>> if (!max_cpus)
>> printk(KERN_INFO "SMP mode deactivated.\n");
>
> So parisc_max_cpus is now 0, which seems wrong. Shouldn't the check be before
> the assignment? This would have avoided the "cpu != 0" in the old code
> completely.
No, the old
cpu != 0 && ....smp_boot_one_cpu(cpu, tidle))
was to avoid that smp_boot_one_cpu() gets called for the monarch cpu.
Anyway, you are right, the code could need cleanups.
But in my patch series I wanted to keep the changes minimal in the first place.
>
>> +
>> + return cpu_online(cpu) ? 0 : -EIO;
>> +}
>> +
>> +/*
>> + * __cpu_disable runs on the processor to be shutdown.
>> + */
>> +int __cpu_disable(void)
>> +{
>> +#ifdef CONFIG_HOTPLUG_CPU
>> + unsigned int cpu = smp_processor_id();
>> +
>> + remove_cpu_topology(cpu);
>> +
>> + /*
>> + * Take this CPU offline. Once we clear this, we can't return,
>> + * and we must not schedule until we're ready to give up the cpu.
>> + */
>> + set_cpu_online(cpu, false);
>> +
>> + /*
>> + * disable IPI interrupt
>> + */
>> + disable_percpu_irq(IPI_IRQ);
>> +
>> + /*
>> + * migrate IRQs away from this CPU
>> + */
>> + irq_migrate_all_off_this_cpu();
>
> While I really enjoy good code comments the last 2 seem a t bit wasteful,
> given that the code is basically exactly the same as the text.
Yep.
Helge
