* [bug report] arch_topology: Build cacheinfo from primary CPU
@ 2023-01-23 15:07 Dan Carpenter
2023-01-23 15:14 ` Pierre Gondois
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2023-01-23 15:07 UTC (permalink / raw)
To: pierre.gondois; +Cc: linux-acpi
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] arch_topology: Build cacheinfo from primary CPU
2023-01-23 15:07 [bug report] arch_topology: Build cacheinfo from primary CPU Dan Carpenter
@ 2023-01-23 15:14 ` Pierre Gondois
2023-01-23 15:44 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Pierre Gondois @ 2023-01-23 15:14 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-acpi
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] arch_topology: Build cacheinfo from primary CPU
2023-01-23 15:14 ` Pierre Gondois
@ 2023-01-23 15:44 ` Dan Carpenter
2023-01-23 16:20 ` Pierre Gondois
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2023-01-23 15:44 UTC (permalink / raw)
To: Pierre Gondois; +Cc: linux-acpi
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] arch_topology: Build cacheinfo from primary CPU
2023-01-23 15:44 ` Dan Carpenter
@ 2023-01-23 16:20 ` Pierre Gondois
0 siblings, 0 replies; 4+ messages in thread
From: Pierre Gondois @ 2023-01-23 16:20 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-acpi
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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-23 16:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-23 15:07 [bug report] arch_topology: Build cacheinfo from primary CPU Dan Carpenter
2023-01-23 15:14 ` Pierre Gondois
2023-01-23 15:44 ` Dan Carpenter
2023-01-23 16:20 ` Pierre Gondois
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.