| Message ID | 20171125094127.7536-1-ynorov@caviumnetworks.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Checked call sites, looks like no one tries to modify these pointers.
I too prefer to make things as const as possible, though I recently
had a coworker refer to the propagation of const in a code base as a
"Gordian knot," which I thought was eloquent.
checkpatch mentions that there's one line that's over 80 chars, and
that there's M$ DOS style line endings (but that may just be my client
gmail, or yours (exchange?)). If you fix up that long line, then:
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
On Mon, Nov 27, 2017 at 09:15:50AM -0800, Nick Desaulniers wrote: > Checked call sites, looks like no one tries to modify these pointers. > I too prefer to make things as const as possible, though I recently > had a coworker refer to the propagation of const in a code base as a > "Gordian knot," which I thought was eloquent. > > checkpatch mentions that there's one line that's over 80 chars, and > that there's M$ DOS style line endings (but that may just be my client > gmail, or yours (exchange?)). If you fix up that long line, then: I happily use mutt, but my company's mail server is MS Outlook, so I have to send everything with msmtp. It usually works. In fact, this is the first complain for 2 years. My checkpatch: yury:linux$ scripts/checkpatch.pl const.patch total: 0 errors, 0 warnings, 32 lines checked const.patch has no obvious style problems and is ready for submission. I guess, because 1 line is exactly 80 chars length, and because MS format adds another symbol as line delimiter, we end up with 81 chars in line, which makes checkpatch worrying. CC Joe Perches and Andy Whitcroft for it. > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Thanks
On Sat, Nov 25, 2017 at 12:41:27PM +0300, Yury Norov wrote:
> It was discovered during LTO-enabled compilation with gcc/ld.bfd.
What was discovered? Could you provide a bit more information in the
changelog, please? I'm happy to take this as a fix if it's actually fixing
something.
Will
On Tue, Nov 28, 2017 at 06:33:55PM +0000, Will Deacon wrote: > On Sat, Nov 25, 2017 at 12:41:27PM +0300, Yury Norov wrote: > > It was discovered during LTO-enabled compilation with gcc/ld.bfd. > > What was discovered? Could you provide a bit more information in the > changelog, please? I'm happy to take this as a fix if it's actually fixing > something. Yes it does. There's inconsistency in variable declaration and section type. GCC doesn't throw error for usual build, but if LTO enabled, build becomes broken, like this: mm/percpu.c:2168:20: error: pcpu_fc_names causes a section type conflict with dt_supported_cpu_ops const char * const pcpu_fc_names[PCPU_FC_NR] __initconst = { ^ arch/arm64/kernel/cpu_ops.c:34:37: note: ‘dt_supported_cpu_ops’ was declared here static const struct cpu_operations *dt_supported_cpu_ops[] __initconst = { And so on. Let me know if you need full error log, then I'll resend the patch with it. You can also try it yourself, the very dirty and unfinished branch is here: https://github.com/norov/linux/tree/lto Yury
On Wed, Nov 29, 2017 at 12:32:25PM +0300, Yury Norov wrote: > On Tue, Nov 28, 2017 at 06:33:55PM +0000, Will Deacon wrote: > > On Sat, Nov 25, 2017 at 12:41:27PM +0300, Yury Norov wrote: > > > It was discovered during LTO-enabled compilation with gcc/ld.bfd. > > > > What was discovered? Could you provide a bit more information in the > > changelog, please? I'm happy to take this as a fix if it's actually fixing > > something. > > Yes it does. There's inconsistency in variable declaration and > section type. GCC doesn't throw error for usual build, but if LTO > enabled, build becomes broken, like this: > > mm/percpu.c:2168:20: error: pcpu_fc_names causes a section type conflict > with dt_supported_cpu_ops > const char * const pcpu_fc_names[PCPU_FC_NR] __initconst = { > ^ > arch/arm64/kernel/cpu_ops.c:34:37: note: ‘dt_supported_cpu_ops’ was declared here > static const struct cpu_operations *dt_supported_cpu_ops[] __initconst = { > > And so on. Let me know if you need full error log, then I'll resend the > patch with it. I think it would be helpful to include one of the errors in the commit message. Please send a v2 with that. Will
diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c index d16978213c5b..36178002f0c3 100644 --- a/arch/arm64/kernel/cpu_ops.c +++ b/arch/arm64/kernel/cpu_ops.c @@ -31,13 +31,14 @@ extern const struct cpu_operations cpu_psci_ops; const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init; -static const struct cpu_operations *dt_supported_cpu_ops[] __initconst = { +static const struct cpu_operations *const dt_supported_cpu_ops[] __initconst = { &smp_spin_table_ops, &cpu_psci_ops, NULL, }; -static const struct cpu_operations *acpi_supported_cpu_ops[] __initconst = { +static const struct cpu_operations + *const acpi_supported_cpu_ops[] __initconst = { #ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL &acpi_parking_protocol_ops, #endif @@ -47,7 +48,7 @@ static const struct cpu_operations *acpi_supported_cpu_ops[] __initconst = { static const struct cpu_operations * __init cpu_get_ops(const char *name) { - const struct cpu_operations **ops; + const struct cpu_operations *const *ops; ops = acpi_disabled ? dt_supported_cpu_ops : acpi_supported_cpu_ops; diff --git a/drivers/clk/hisilicon/crg-hi3516cv300.c b/drivers/clk/hisilicon/crg-hi3516cv300.c index 2007123832bb..53450b651e4c 100644 --- a/drivers/clk/hisilicon/crg-hi3516cv300.c +++ b/drivers/clk/hisilicon/crg-hi3516cv300.c @@ -204,7 +204,7 @@ static const struct hisi_crg_funcs hi3516cv300_crg_funcs = { /* hi3516CV300 sysctrl CRG */ #define HI3516CV300_SYSCTRL_NR_CLKS 16 -static const char *wdt_mux_p[] __initconst = { "3m", "apb" }; +static const char *const wdt_mux_p[] __initconst = { "3m", "apb" }; static u32 wdt_mux_table[] = {0, 1}; static const struct hisi_mux_clock hi3516cv300_sysctrl_mux_clks[] = {
It was discovered during LTO-enabled compilation with gcc/ld.bfd. Signed-off-by: Yury Norov <ynorov@caviumnetworks.com> --- arch/arm64/kernel/cpu_ops.c | 7 ++++--- drivers/clk/hisilicon/crg-hi3516cv300.c | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-)