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

[Gavin Shan <gshan@redhat.com>]

On 3/18/20 2:53 PM, Gavin Shan wrote:
> 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.
> 

Mark, I plan to post a new revision to have option#2 after thinking
about it: we will have two pointers to track the operations for boot cpu
and the secondary CPUs separately. the consumed memory isn't scaled up to
the configured CPU number. So I think it's better than option#1, which
uses two-bits as index to CPU operation array.

Thanks,
Gavin

>>>   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