[bug report] arch_topology: Build cacheinfo from primary CPU

[bug report] arch_topology: Build cacheinfo from primary CPU

[Dan Carpenter]

Hello Pierre Gondois,

The patch 5944ce092b97: "arch_topology: Build cacheinfo from primary
CPU" from Jan 4, 2023, leads to the following Smatch static checker
warning:

drivers/base/cacheinfo.c:440 fetch_cache_info()	error: uninitialized symbol 'levels'.
drivers/base/cacheinfo.c:447 fetch_cache_info() error: uninitialized symbol 'split_levels'.

drivers/base/cacheinfo.c
    424 int fetch_cache_info(unsigned int cpu)
    425 {
    426         struct cpu_cacheinfo *this_cpu_ci;
    427         unsigned int levels, split_levels;
    428         int ret;
    429 
    430         if (acpi_disabled) {
    431                 ret = init_of_cache_level(cpu);
    432                 if (ret < 0)
    433                         return ret;
    434         } else {
    435                 ret = acpi_get_cache_info(cpu, &levels, &split_levels);
    436                 if (ret < 0)
    437                         return ret;

Apparently, I must have CONFIG_ACPI_PPTT disabled.

    438 
    439                 this_cpu_ci = get_cpu_cacheinfo(cpu);
--> 440                 this_cpu_ci->num_levels = levels;
                                                  ^^^^^^
Unititialized.

    441                 /*
    442                  * This assumes that:
    443                  * - there cannot be any split caches (data/instruction)
    444                  *   above a unified cache
    445                  * - data/instruction caches come by pair
    446                  */
    447                 this_cpu_ci->num_leaves = levels + split_levels;
    448         }
    449         if (!cache_leaves(cpu))
    450                 return -ENOENT;
    451 
    452         return allocate_cache_info(cpu);
    453 }

regards,
dan carpenter

Re: [bug report] arch_topology: Build cacheinfo from primary CPU

[Pierre Gondois]

Hello Dan,
The 'levels' and 'split_levels' variables are initialized through their
addresses when necessary, so I believe the warning can be ignored.

If you still want to have the variables initialized, please let me know and I
will send a patch,

Regards,
Pierre

On 1/23/23 16:07, Dan Carpenter wrote:
> Hello Pierre Gondois,
> 
> The patch 5944ce092b97: "arch_topology: Build cacheinfo from primary
> CPU" from Jan 4, 2023, leads to the following Smatch static checker
> warning:
> 
> drivers/base/cacheinfo.c:440 fetch_cache_info()	error: uninitialized symbol 'levels'.
> drivers/base/cacheinfo.c:447 fetch_cache_info() error: uninitialized symbol 'split_levels'.
> 
> drivers/base/cacheinfo.c
>      424 int fetch_cache_info(unsigned int cpu)
>      425 {
>      426         struct cpu_cacheinfo *this_cpu_ci;
>      427         unsigned int levels, split_levels;
>      428         int ret;
>      429
>      430         if (acpi_disabled) {
>      431                 ret = init_of_cache_level(cpu);
>      432                 if (ret < 0)
>      433                         return ret;
>      434         } else {
>      435                 ret = acpi_get_cache_info(cpu, &levels, &split_levels);
>      436                 if (ret < 0)
>      437                         return ret;
> 
> Apparently, I must have CONFIG_ACPI_PPTT disabled.
> 
>      438
>      439                 this_cpu_ci = get_cpu_cacheinfo(cpu);
> --> 440                 this_cpu_ci->num_levels = levels;
>                                                    ^^^^^^
> Unititialized.
> 
>      441                 /*
>      442                  * This assumes that:
>      443                  * - there cannot be any split caches (data/instruction)
>      444                  *   above a unified cache
>      445                  * - data/instruction caches come by pair
>      446                  */
>      447                 this_cpu_ci->num_leaves = levels + split_levels;
>      448         }
>      449         if (!cache_leaves(cpu))
>      450                 return -ENOENT;
>      451
>      452         return allocate_cache_info(cpu);
>      453 }
> 
> regards,
> dan carpenter

Re: [bug report] arch_topology: Build cacheinfo from primary CPU

[Dan Carpenter]

On Mon, Jan 23, 2023 at 04:14:56PM +0100, Pierre Gondois wrote:
> Hello Dan,
> The 'levels' and 'split_levels' variables are initialized through their
> addresses when necessary, so I believe the warning can be ignored.

I don't understand what "initialized through their addresses when
necessary" means.  If you have CONFIG_ACPI_PPTT turned off as I do then
acpi_get_cache_info() is a no-op so it's not initializing anything.

> 
> If you still want to have the variables initialized, please let me know and I
> will send a patch,

I feel like I have this kind of discussion a lot.  I don't know why
people don't want to initialize their variables to zero.

1) It doesn't affect runtime on modern distros because they use the
   CONFIG_ option to zero stack variables.

2) If they don't then syzbot will detect it at runtime.  It will probably
   take a year or two.  That will set off a bunch of emails.  Meanwhile
   you will have joined an exciting new start up and won't be around.
   No one else will remember how this code works.

regards,
dan carpenter

Re: [bug report] arch_topology: Build cacheinfo from primary CPU

[Pierre Gondois]

On 1/23/23 16:44, Dan Carpenter wrote:
> On Mon, Jan 23, 2023 at 04:14:56PM +0100, Pierre Gondois wrote:
>> Hello Dan,
>> The 'levels' and 'split_levels' variables are initialized through their
>> addresses when necessary, so I believe the warning can be ignored.
> 
> I don't understand what "initialized through their addresses when
> necessary" means.  If you have CONFIG_ACPI_PPTT turned off as I do then
> acpi_get_cache_info() is a no-op so it's not initializing anything.

Ok yes indeed. I will send a fix shortly.
Sorry for the trouble,
Pierre

> 
>>
>> If you still want to have the variables initialized, please let me know and I
>> will send a patch,
> 
> I feel like I have this kind of discussion a lot.  I don't know why
> people don't want to initialize their variables to zero.
> 
> 1) It doesn't affect runtime on modern distros because they use the
>     CONFIG_ option to zero stack variables.
> 
> 2) If they don't then syzbot will detect it at runtime.  It will probably
>     take a year or two.  That will set off a bunch of emails.  Meanwhile
>     you will have joined an exciting new start up and won't be around.
>     No one else will remember how this code works.
> 
> regards,
> dan carpenter
> 
>