Re: [PATCH v2 4/4] powerpc/pseries/cpuhp: remove obsolete comment from pseries_cpu_die

[Nathan Lynch <nathanl@linux.ibm.com>]

Michael Ellerman <mpe@ellerman.id.au> writes:
> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
>> This is enough to say that we can't easily see the history behind this comment.
>> I also believe that we're better of without it since it doesn't make sense
>> with the current codebase.
>
> It was added by the original CPU hotplug commit for ppc64::
>
> https://github.com/mpe/linux-fullhistory/commit/0e9fd9441cd2113b67b14e739267c9e69761489b
>
>
> The code was fairly similar:
>
> void __cpu_die(unsigned int cpu)
> {
> 	int tries;
> 	int cpu_status;
> 	unsigned int pcpu = get_hard_smp_processor_id(cpu);
>
> 	for (tries = 0; tries < 5; tries++) {
> 		cpu_status = query_cpu_stopped(pcpu);
>
> 		if (cpu_status == 0)
> 			break;
> 		set_current_state(TASK_UNINTERRUPTIBLE);
> 		schedule_timeout(HZ);
> 	}
> 	if (cpu_status != 0) {
> 		printk("Querying DEAD? cpu %i (%i) shows %i\n",
> 		       cpu, pcpu, cpu_status);
> 	}
>
> 	/* Isolation and deallocation are definatly done by
> 	 * drslot_chrp_cpu.  If they were not they would be
> 	 * done here.  Change isolate state to Isolate and
> 	 * change allocation-state to Unusable.
> 	 */
> 	paca[cpu].xProcStart = 0;
>
> 	/* So we can recognize if it fails to come up next time. */
> 	cpu_callin_map[cpu] = 0;
> }
>
>
> drslot_chrp_cpu() still exists in drmgr:
>
>   https://github.com/ibm-power-utilities/powerpc-utils/blob/e798c4a09fbf0fa0f421e624cfa366a6c405c9fe/src/drmgr/drslot_chrp_cpu.c#L406
>
>
> I agree the comment is no longer meaningful and can be removed.

Thanks for providing this background.

> It might be good to then add a comment explaining why we need to set
> cpu_start = 0.

Sure, I can take that as a follow-up. Or perhaps it should be moved to
the online path.

> It's not immediately clear why we need to. When we bring a CPU back
> online in smp_pSeries_kick_cpu() we ask RTAS to start it and then
> immediately set cpu_start = 1, ie. there isn't a separate step that sets
> cpu_start = 1 for hotplugged CPUs.

Hmm I'm not following the distinction you seem to be drawing between
bringing a CPU back online and a hotplugged CPU. kick_cpu is used in all
cases AFAIK.