* [PATCH v2] arm64/cache: fix -Woverride-init compiler warnings
@ 2019-08-06 19:34 Qian Cai
2019-08-07 10:53 ` Mark Rutland
2019-08-07 10:56 ` Will Deacon
0 siblings, 2 replies; 5+ messages in thread
From: Qian Cai @ 2019-08-06 19:34 UTC (permalink / raw)
To: will, catalin.marinas
Cc: mark.rutland, linux-arm-kernel, linux-kernel, Qian Cai
The commit 155433cb365e ("arm64: cache: Remove support for ASID-tagged
VIVT I-caches") introduced some compiation warnings from GCC (and
Clang),
arch/arm64/kernel/cpuinfo.c:38:26: warning: initialized field
overwritten [-Woverride-init]
[ICACHE_POLICY_VIPT] = "VIPT",
^~~~~~
arch/arm64/kernel/cpuinfo.c:38:26: note: (near initialization for
'icache_policy_str[2]')
arch/arm64/kernel/cpuinfo.c:39:26: warning: initialized field
overwritten [-Woverride-init]
[ICACHE_POLICY_PIPT] = "PIPT",
^~~~~~
arch/arm64/kernel/cpuinfo.c:39:26: note: (near initialization for
'icache_policy_str[3]')
arch/arm64/kernel/cpuinfo.c:40:27: warning: initialized field
overwritten [-Woverride-init]
[ICACHE_POLICY_VPIPT] = "VPIPT",
^~~~~~~
arch/arm64/kernel/cpuinfo.c:40:27: note: (near initialization for
'icache_policy_str[0]')
because it initializes icache_policy_str[0 ... 3] twice. Since the array
is only used in cpuinfo_detect_icache_policy(), fix it by initializing
a specific field there just before using.
Fixes: 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches")
Signed-off-by: Qian Cai <cai@lca.pw>
---
v2: Initialize a specific field in cpuinfo_detect_icache_policy().
arch/arm64/kernel/cpuinfo.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 876055e37352..a0c495a3f4fd 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -34,10 +34,7 @@ DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data);
static struct cpuinfo_arm64 boot_cpu_data;
static char *icache_policy_str[] = {
- [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN",
- [ICACHE_POLICY_VIPT] = "VIPT",
- [ICACHE_POLICY_PIPT] = "PIPT",
- [ICACHE_POLICY_VPIPT] = "VPIPT",
+ [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN"
};
unsigned long __icache_flags;
@@ -310,13 +307,16 @@ static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info)
switch (l1ip) {
case ICACHE_POLICY_PIPT:
+ icache_policy_str[ICACHE_POLICY_PIPT] = "PIPT";
break;
case ICACHE_POLICY_VPIPT:
+ icache_policy_str[ICACHE_POLICY_VPIPT] = "VPIPT";
set_bit(ICACHEF_VPIPT, &__icache_flags);
break;
default:
/* Fallthrough */
case ICACHE_POLICY_VIPT:
+ icache_policy_str[ICACHE_POLICY_VIPT] = "VIPT";
/* Assume aliasing */
set_bit(ICACHEF_ALIASING, &__icache_flags);
}
--
2.20.1 (Apple Git-117)
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] arm64/cache: fix -Woverride-init compiler warnings
2019-08-06 19:34 [PATCH v2] arm64/cache: fix -Woverride-init compiler warnings Qian Cai
@ 2019-08-07 10:53 ` Mark Rutland
2019-08-07 10:56 ` Will Deacon
1 sibling, 0 replies; 5+ messages in thread
From: Mark Rutland @ 2019-08-07 10:53 UTC (permalink / raw)
To: Qian Cai; +Cc: will, catalin.marinas, linux-arm-kernel, linux-kernel
On Tue, Aug 06, 2019 at 03:34:34PM -0400, Qian Cai wrote:
> The commit 155433cb365e ("arm64: cache: Remove support for ASID-tagged
> VIVT I-caches") introduced some compiation warnings from GCC (and
> Clang),
>
> arch/arm64/kernel/cpuinfo.c:38:26: warning: initialized field
> overwritten [-Woverride-init]
> [ICACHE_POLICY_VIPT] = "VIPT",
> ^~~~~~
> arch/arm64/kernel/cpuinfo.c:38:26: note: (near initialization for
> 'icache_policy_str[2]')
> arch/arm64/kernel/cpuinfo.c:39:26: warning: initialized field
> overwritten [-Woverride-init]
> [ICACHE_POLICY_PIPT] = "PIPT",
> ^~~~~~
> arch/arm64/kernel/cpuinfo.c:39:26: note: (near initialization for
> 'icache_policy_str[3]')
> arch/arm64/kernel/cpuinfo.c:40:27: warning: initialized field
> overwritten [-Woverride-init]
> [ICACHE_POLICY_VPIPT] = "VPIPT",
> ^~~~~~~
> arch/arm64/kernel/cpuinfo.c:40:27: note: (near initialization for
> 'icache_policy_str[0]')
>
> because it initializes icache_policy_str[0 ... 3] twice. Since the array
> is only used in cpuinfo_detect_icache_policy(), fix it by initializing
> a specific field there just before using.
>
> Fixes: 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches")
> Signed-off-by: Qian Cai <cai@lca.pw>
Rather than trying to "fix" correct code like this (and making it harder
to read), could you instead look into where/whether the warning is
actually useful?
I had a look at an arm64 defconfig, where I see:
[mark@lakrids:~/src/linux]% grep override-init err.log | grep -o '^[^[:space:]:]\+' | sort | uniq -c | sort -rn
434 arch/arm64/kernel/sys32.c // all benign
291 arch/arm64/kernel/sys.c // all benign
48 ./arch/arm64/include/asm/perf_event.h // all benign
37 arch/arm64/kernel/traps.c // all benign
21 arch/arm64/kvm/handle_exit.c // all benign
12 drivers/ata/ahci.h // all benign
6 arch/arm64/kernel/perf_event.c // all benign
4 kernel/time/hrtimer.c // all benign
3 arch/arm64/kernel/cpuinfo.c // all benign
2 drivers/pinctrl/tegra/pinctrl-tegra194.c // unclear to me
1 ./include/linux/blkdev.h // all benign
1 drivers/gpu/drm/sun4i/sun4i_drv.c // all benign
1 drivers/ata/sata_sil24.c // all benign
1 ./arch/arm64/include/asm/mmu.h // all benign
... so that's 862 warnings where at least 860 are unhelpful (and I
suspect those last two are also fine, but I haven't untangled the set of
macros).
Given that, what's the point in enabling this warning? It forces us to
write worse code that's harder to maintain, and it doesn't spot anything
useful.
> ---
>
> v2: Initialize a specific field in cpuinfo_detect_icache_policy().
>
> arch/arm64/kernel/cpuinfo.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index 876055e37352..a0c495a3f4fd 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -34,10 +34,7 @@ DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data);
> static struct cpuinfo_arm64 boot_cpu_data;
>
> static char *icache_policy_str[] = {
> - [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN",
> - [ICACHE_POLICY_VIPT] = "VIPT",
> - [ICACHE_POLICY_PIPT] = "PIPT",
> - [ICACHE_POLICY_VPIPT] = "VPIPT",
> + [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN"
> };
>
> unsigned long __icache_flags;
> @@ -310,13 +307,16 @@ static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info)
>
> switch (l1ip) {
> case ICACHE_POLICY_PIPT:
> + icache_policy_str[ICACHE_POLICY_PIPT] = "PIPT";
> break;
> case ICACHE_POLICY_VPIPT:
> + icache_policy_str[ICACHE_POLICY_VPIPT] = "VPIPT";
> set_bit(ICACHEF_VPIPT, &__icache_flags);
> break;
> default:
> /* Fallthrough */
> case ICACHE_POLICY_VIPT:
> + icache_policy_str[ICACHE_POLICY_VIPT] = "VIPT";
> /* Assume aliasing */
> set_bit(ICACHEF_ALIASING, &__icache_flags);
NAK to this. Please leave this code as-is.
Thanks,
Mark.
> }
> --
> 2.20.1 (Apple Git-117)
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] arm64/cache: fix -Woverride-init compiler warnings
2019-08-06 19:34 [PATCH v2] arm64/cache: fix -Woverride-init compiler warnings Qian Cai
2019-08-07 10:53 ` Mark Rutland
@ 2019-08-07 10:56 ` Will Deacon
2019-08-07 11:50 ` Qian Cai
1 sibling, 1 reply; 5+ messages in thread
From: Will Deacon @ 2019-08-07 10:56 UTC (permalink / raw)
To: Qian Cai; +Cc: catalin.marinas, mark.rutland, linux-arm-kernel, linux-kernel
On Tue, Aug 06, 2019 at 03:34:34PM -0400, Qian Cai wrote:
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index 876055e37352..a0c495a3f4fd 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -34,10 +34,7 @@ DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data);
> static struct cpuinfo_arm64 boot_cpu_data;
>
> static char *icache_policy_str[] = {
> - [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN",
> - [ICACHE_POLICY_VIPT] = "VIPT",
> - [ICACHE_POLICY_PIPT] = "PIPT",
> - [ICACHE_POLICY_VPIPT] = "VPIPT",
> + [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN"
> };
>
> unsigned long __icache_flags;
> @@ -310,13 +307,16 @@ static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info)
>
> switch (l1ip) {
> case ICACHE_POLICY_PIPT:
> + icache_policy_str[ICACHE_POLICY_PIPT] = "PIPT";
> break;
> case ICACHE_POLICY_VPIPT:
> + icache_policy_str[ICACHE_POLICY_VPIPT] = "VPIPT";
> set_bit(ICACHEF_VPIPT, &__icache_flags);
> break;
> default:
> /* Fallthrough */
> case ICACHE_POLICY_VIPT:
> + icache_policy_str[ICACHE_POLICY_VIPT] = "VIPT";
> /* Assume aliasing */
> set_bit(ICACHEF_ALIASING, &__icache_flags);
I still think this is worse than the code in mainline. I don't think
-Woverride-init should warn when overriding a field from a GCC range
designated initialiser, since it makes them considerably less useful
imo.
Will
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] arm64/cache: fix -Woverride-init compiler warnings
2019-08-07 10:56 ` Will Deacon
@ 2019-08-07 11:50 ` Qian Cai
2019-08-07 11:59 ` Will Deacon
0 siblings, 1 reply; 5+ messages in thread
From: Qian Cai @ 2019-08-07 11:50 UTC (permalink / raw)
To: Will Deacon; +Cc: Catalin Marinas, Mark Rutland, linux-arm-kernel, linux-kernel
> On Aug 7, 2019, at 6:56 AM, Will Deacon <will@kernel.org> wrote:
>
> On Tue, Aug 06, 2019 at 03:34:34PM -0400, Qian Cai wrote:
>> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
>> index 876055e37352..a0c495a3f4fd 100644
>> --- a/arch/arm64/kernel/cpuinfo.c
>> +++ b/arch/arm64/kernel/cpuinfo.c
>> @@ -34,10 +34,7 @@ DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data);
>> static struct cpuinfo_arm64 boot_cpu_data;
>>
>> static char *icache_policy_str[] = {
>> - [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN",
>> - [ICACHE_POLICY_VIPT] = "VIPT",
>> - [ICACHE_POLICY_PIPT] = "PIPT",
>> - [ICACHE_POLICY_VPIPT] = "VPIPT",
>> + [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN"
>> };
>>
>> unsigned long __icache_flags;
>> @@ -310,13 +307,16 @@ static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info)
>>
>> switch (l1ip) {
>> case ICACHE_POLICY_PIPT:
>> + icache_policy_str[ICACHE_POLICY_PIPT] = "PIPT";
>> break;
>> case ICACHE_POLICY_VPIPT:
>> + icache_policy_str[ICACHE_POLICY_VPIPT] = "VPIPT";
>> set_bit(ICACHEF_VPIPT, &__icache_flags);
>> break;
>> default:
>> /* Fallthrough */
>> case ICACHE_POLICY_VIPT:
>> + icache_policy_str[ICACHE_POLICY_VIPT] = "VIPT";
>> /* Assume aliasing */
>> set_bit(ICACHEF_ALIASING, &__icache_flags);
>
> I still think this is worse than the code in mainline. I don't think
> -Woverride-init should warn when overriding a field from a GCC range
> designated initialiser, since it makes them considerably less useful
> imo.
Unfortunately, compiler people are moving into a different direction as
Clang would warn those kind of usage too.
It actually prove that those warnings are useful to find real issues. See,
Fae5e033d65a (“mfd: rk808: Fix RK818_IRQ_DISCHG_ILIM initializer”)
32df34d875bb (“[media] rc: img-ir: jvc: Remove unused no-leader timings”)
Especially, to find redundant initializations in large structures. e.g.,
e6ea0b917875 (“[media] dvb_frontend: Don't declare values twice at a table”)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] arm64/cache: fix -Woverride-init compiler warnings
2019-08-07 11:50 ` Qian Cai
@ 2019-08-07 11:59 ` Will Deacon
0 siblings, 0 replies; 5+ messages in thread
From: Will Deacon @ 2019-08-07 11:59 UTC (permalink / raw)
To: Qian Cai; +Cc: Catalin Marinas, Mark Rutland, linux-arm-kernel, linux-kernel
On Wed, Aug 07, 2019 at 07:50:43AM -0400, Qian Cai wrote:
>
>
> > On Aug 7, 2019, at 6:56 AM, Will Deacon <will@kernel.org> wrote:
> >
> > On Tue, Aug 06, 2019 at 03:34:34PM -0400, Qian Cai wrote:
> >> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> >> index 876055e37352..a0c495a3f4fd 100644
> >> --- a/arch/arm64/kernel/cpuinfo.c
> >> +++ b/arch/arm64/kernel/cpuinfo.c
> >> @@ -34,10 +34,7 @@ DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data);
> >> static struct cpuinfo_arm64 boot_cpu_data;
> >>
> >> static char *icache_policy_str[] = {
> >> - [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN",
> >> - [ICACHE_POLICY_VIPT] = "VIPT",
> >> - [ICACHE_POLICY_PIPT] = "PIPT",
> >> - [ICACHE_POLICY_VPIPT] = "VPIPT",
> >> + [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN"
> >> };
> >>
> >> unsigned long __icache_flags;
> >> @@ -310,13 +307,16 @@ static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info)
> >>
> >> switch (l1ip) {
> >> case ICACHE_POLICY_PIPT:
> >> + icache_policy_str[ICACHE_POLICY_PIPT] = "PIPT";
> >> break;
> >> case ICACHE_POLICY_VPIPT:
> >> + icache_policy_str[ICACHE_POLICY_VPIPT] = "VPIPT";
> >> set_bit(ICACHEF_VPIPT, &__icache_flags);
> >> break;
> >> default:
> >> /* Fallthrough */
> >> case ICACHE_POLICY_VIPT:
> >> + icache_policy_str[ICACHE_POLICY_VIPT] = "VIPT";
> >> /* Assume aliasing */
> >> set_bit(ICACHEF_ALIASING, &__icache_flags);
> >
> > I still think this is worse than the code in mainline. I don't think
> > -Woverride-init should warn when overriding a field from a GCC range
> > designated initialiser, since it makes them considerably less useful
> > imo.
>
> Unfortunately, compiler people are moving into a different direction as
> Clang would warn those kind of usage too.
>
> It actually prove that those warnings are useful to find real issues. See,
>
> Fae5e033d65a (“mfd: rk808: Fix RK818_IRQ_DISCHG_ILIM initializer”)
> 32df34d875bb (“[media] rc: img-ir: jvc: Remove unused no-leader timings”)
>
> Especially, to find redundant initializations in large structures. e.g.,
>
> e6ea0b917875 (“[media] dvb_frontend: Don't declare values twice at a table”)
None of these appear to use the range initialisers I was referring to, so I
don't see why this is relevant to the discussion at hand.
Will
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-08-07 11:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-06 19:34 [PATCH v2] arm64/cache: fix -Woverride-init compiler warnings Qian Cai
2019-08-07 10:53 ` Mark Rutland
2019-08-07 10:56 ` Will Deacon
2019-08-07 11:50 ` Qian Cai
2019-08-07 11:59 ` Will Deacon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).