Re: [PATCH v2] cpufreq: powernv: fix stack bloat and NR_CPUS limitation

[Michael Ellerman]

John Hubbard <jhubbard@xxxxxxxxxx> writes:
> On 10/30/19 7:39 PM, Michael Ellerman wrote:
>> Hi John,
>> 
>> Sorry I didn't reply to this sooner, too many patches :/
>> 
>> John Hubbard <jhubbard@xxxxxxxxxx> writes:
>>> The following build warning occurred on powerpc 64-bit builds:
>>>
>>> drivers/cpufreq/powernv-cpufreq.c: In function 'init_chip_info':
>>> drivers/cpufreq/powernv-cpufreq.c:1070:1: warning: the frame size of 1040 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>> 
>> Oddly I don't see that warning in my builds, eg with GCC9:
>> 
>>    https://travis-ci.org/linuxppc/linux/jobs/604870722
>
> This is with a cross-compiler based on gcc 8.1.0, which I got from:
>    https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/8.1.0/
>
> I'll put that in the v3 commit description.
>
>> 
>>> This is due to putting 1024 bytes on the stack:
>>>
>>>      unsigned int chip[256];
>>>
>>> ...and while looking at this, it also has a bug: it fails with a stack
>>> overrun, if CONFIG_NR_CPUS > 256.
>> 
>> It _probably_ doesn't, because it only increments the index when the
>> chip_id of the CPU changes, ie. it doesn't create a chip for every CPU.
>> But I agree it's flaky the way it's written.
>
> I'll soften up the wording accordingly.
>
>> 
>>> Fix both problems by dynamically allocating based on CONFIG_NR_CPUS.
>> 
>> Shouldn't it use num_possible_cpus() ?
>> 
>> Given the for loop is over possible CPUs that seems like the upper
>> bound. In practice it should be lower because some CPUs will share a
>> chip.
>> 
>
> OK, I see, that's more consistent with the code, I'll change to that.

Thanks.

cheers