Re: [PATCH v4 4/5] arm64: Remove CPU operations dereferencing array

From: Gavin Shan <gshan@redhat.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: lorenzo.pieralisi@arm.com, catalin.marinas@arm.com,
	robin.murphy@arm.com, shan.gavin@gmail.com, sudeep.holla@arm.com,
	will@kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4 4/5] arm64: Remove CPU operations dereferencing array
Date: Wed, 18 Mar 2020 14:53:42 +1100	[thread overview]
Message-ID: <a92d9a3a-7bc3-81f0-f4a3-9e1da3623e16@redhat.com> (raw)
In-Reply-To: <20200317105626.GF8831@lakrids.cambridge.arm.com>

On 3/17/20 9:56 PM, Mark Rutland wrote:
> On Wed, Feb 26, 2020 at 11:23:55AM +1100, Gavin Shan wrote:
>> One CPU operations is maintained through array @cpu_ops[NR_CPUS]. 2KB
>> memory is consumed when CONFIG_NR_CPUS is set to 256. It seems too
>> much memory has been used for this. Also, all CPUs must use same CPU
>> operations and we shouldn't bring up the broken CPU, as Lorenzo Pieralisi
>> pointed out.
> 
> I suspect there might be some pre PSCIv0.2 platforms where the boot CPU
> doesn't have PSCI, but others do. On those platforms, this could be
> because CPU0 cannot be hotplugged out, and we must avoid doing so.
> 
> Can you check the in-kernel DTs to see if any of those exist?
> 
> Other than that, I agree that mandating uniformity is the best approach
> here.
> 

Thanks for the comments, Mark. There are platforms where the CPU#0 and other
CPUs have different "enable-method" specified. More specificly, CPU#0 doesn't
have "enable-method" while other CPUs have "psci" specified:

    lg/lg1312.dtsi
    lg/lg1313.dtsi
    mediatek/mt2712e.dtsi

In order to support two enable methods, I think we have two options here:
(1) Revert the code to what we had in v2. Two bits consumed by one CPU,
which is taken as index to a CPU operation array. The code can be found in
link [1] (2) Two CPU operation pointers are maintained. One is used to track
the CPU#0's operations and other one is for other cpus.

[1] https://patchwork.kernel.org/patch/11363745/

Please let me know which one is better.

>>   int __init init_cpu_ops(int cpu)
>>   {
>> -	const char *enable_method = cpu_read_enable_method(cpu);
>> -
>> -	if (!enable_method)
>> -		return -ENODEV;
>> +	const struct cpu_operations *ops = get_cpu_method(cpu);
>>   
>> -	cpu_ops[cpu] = cpu_get_ops(enable_method);
>> -	if (!cpu_ops[cpu]) {
>> -		pr_warn("Unsupported enable-method: %s\n", enable_method);
>> +	if (!ops)
>>   		return -EOPNOTSUPP;
>> +
>> +	/* Update global CPU operations if it's not initialized yet */
>> +	if (!cpu_ops) {
>> +		cpu_ops = ops;
>> +		return 0;
>> +	}
> 
> As above, I don't think this is quite right. If we're going to mandate
> uniformity, we should init the ops from the boot CPU, and then verify
> that every other CPU matches that. The initialization of the global ops
> should not be conditional.> 
I think you're correct because CPU#0's "enable-method" can be unspecified.
In this case, the CPU#0's operations will be set to same one as other CPUs,
which isn't correct. I will see how to handle this comments when the above
comments is resolved. This might become invalid after the above comments get
resolved.

Thanks,
Gavin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel