| Message ID | 20200803212548.27765-1-xypron.glpk@gmx.de |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andes |
| Headers | show |
| Series | [1/1] riscv: fix building with CONFIG_SPL_SMP=n | expand |
On Tue, Aug 4, 2020 at 5:26 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > Building with CONFIG_SPL_SMP=n results in: > > arch/riscv/lib/spl.c: In function ‘jump_to_image_no_args’: > arch/riscv/lib/spl.c:33:6: > error: unused variable ‘ret’ [-Werror=unused-variable] > 33 | int ret; > | ^~~ > > Define the variable ret as __maybe_unused. > > Fixes: 191636e44898 ("riscv: Introduce SPL_SMP Kconfig option for U-Boot > SPL") This should be on the same line > Fixes: 8c59f2023cc8 ("riscv: add SPL support") > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > arch/riscv/lib/spl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/lib/spl.c b/arch/riscv/lib/spl.c > index c47dcd46ce..ef00ec2bcc 100644 > --- a/arch/riscv/lib/spl.c > +++ b/arch/riscv/lib/spl.c > @@ -30,7 +30,7 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) > { > typedef void __noreturn (*image_entry_riscv_t)(ulong hart, void *dtb); > void *fdt_blob; > - int ret; > + __maybe_unused int ret; > > #if CONFIG_IS_ENABLED(LOAD_FIT) || CONFIG_IS_ENABLED(LOAD_FIT_FULL) > fdt_blob = spl_image->fdt_addr; Reviewed-by: Bin Meng <bin.meng@windriver.com>
On Mon, 3 Aug 2020 at 15:26, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > Building with CONFIG_SPL_SMP=n results in: > > arch/riscv/lib/spl.c: In function ‘jump_to_image_no_args’: > arch/riscv/lib/spl.c:33:6: > error: unused variable ‘ret’ [-Werror=unused-variable] > 33 | int ret; > | ^~~ > > Define the variable ret as __maybe_unused. > > Fixes: 191636e44898 ("riscv: Introduce SPL_SMP Kconfig option for U-Boot > SPL") > Fixes: 8c59f2023cc8 ("riscv: add SPL support") > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > arch/riscv/lib/spl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Simon Glass <sjg@chromium.org>
> From: Heinrich Schuchardt [mailto:xypron.glpk@gmx.de] > Sent: Tuesday, August 04, 2020 5:26 AM > To: Rick Jian-Zhi Chen(陳建志) > Cc: Simon Glass; Anup Patel; Lukas Auer; Bin Meng; Daniel Schwierzeck; u-boot@lists.denx.de; Heinrich Schuchardt > Subject: [PATCH 1/1] riscv: fix building with CONFIG_SPL_SMP=n > > Building with CONFIG_SPL_SMP=n results in: > > arch/riscv/lib/spl.c: In function ‘jump_to_image_no_args’: > arch/riscv/lib/spl.c:33:6: > error: unused variable ‘ret’ [-Werror=unused-variable] > 33 | int ret; > | ^~~ > > Define the variable ret as __maybe_unused. > > Fixes: 191636e44898 ("riscv: Introduce SPL_SMP Kconfig option for U-Boot > SPL") > Fixes: 8c59f2023cc8 ("riscv: add SPL support") > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > arch/riscv/lib/spl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Rick Chen <rick@andestech.com> > diff --git a/arch/riscv/lib/spl.c b/arch/riscv/lib/spl.c index c47dcd46ce..ef00ec2bcc 100644 > --- a/arch/riscv/lib/spl.c > +++ b/arch/riscv/lib/spl.c > @@ -30,7 +30,7 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) { > typedef void __noreturn (*image_entry_riscv_t)(ulong hart, void *dtb); > void *fdt_blob; > - int ret; > + __maybe_unused int ret; > > #if CONFIG_IS_ENABLED(LOAD_FIT) || CONFIG_IS_ENABLED(LOAD_FIT_FULL) > fdt_blob = spl_image->fdt_addr; > -- > 2.27.0
On 04.08.20 03:46, Bin Meng wrote: > On Tue, Aug 4, 2020 at 5:26 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> Building with CONFIG_SPL_SMP=n results in: >> >> arch/riscv/lib/spl.c: In function ‘jump_to_image_no_args’: >> arch/riscv/lib/spl.c:33:6: >> error: unused variable ‘ret’ [-Werror=unused-variable] >> 33 | int ret; >> | ^~~ >> >> Define the variable ret as __maybe_unused. >> >> Fixes: 191636e44898 ("riscv: Introduce SPL_SMP Kconfig option for U-Boot >> SPL") > > This should be on the same line Commit messages should not exceed 75 characters. See scripts/checkpatch.pl: WARN("COMMIT_LOG_LONG_LINE", "Possible unwrapped commit description (prefer a maximum 75 chars per line)\n" . $herecurr); Best regards Heinrich > >> Fixes: 8c59f2023cc8 ("riscv: add SPL support") >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> arch/riscv/lib/spl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/riscv/lib/spl.c b/arch/riscv/lib/spl.c >> index c47dcd46ce..ef00ec2bcc 100644 >> --- a/arch/riscv/lib/spl.c >> +++ b/arch/riscv/lib/spl.c >> @@ -30,7 +30,7 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) >> { >> typedef void __noreturn (*image_entry_riscv_t)(ulong hart, void *dtb); >> void *fdt_blob; >> - int ret; >> + __maybe_unused int ret; >> >> #if CONFIG_IS_ENABLED(LOAD_FIT) || CONFIG_IS_ENABLED(LOAD_FIT_FULL) >> fdt_blob = spl_image->fdt_addr; > > Reviewed-by: Bin Meng <bin.meng@windriver.com> >
On Tue, Aug 4, 2020 at 7:02 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 04.08.20 03:46, Bin Meng wrote: > > On Tue, Aug 4, 2020 at 5:26 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> Building with CONFIG_SPL_SMP=n results in: > >> > >> arch/riscv/lib/spl.c: In function ‘jump_to_image_no_args’: > >> arch/riscv/lib/spl.c:33:6: > >> error: unused variable ‘ret’ [-Werror=unused-variable] > >> 33 | int ret; > >> | ^~~ > >> > >> Define the variable ret as __maybe_unused. > >> > >> Fixes: 191636e44898 ("riscv: Introduce SPL_SMP Kconfig option for U-Boot > >> SPL") > > > > This should be on the same line > > Commit messages should not exceed 75 characters. See scripts/checkpatch.pl: True, for normal commit messages. > > WARN("COMMIT_LOG_LONG_LINE", > "Possible unwrapped commit description (prefer a maximum 75 chars per > line)\n" . $herecurr); > But this Fixes tag is special. I suspect 2 lines will break some scripts that is handling this "Fixes" tag. Regards, Bin
On 04.08.20 15:15, Bin Meng wrote: > On Tue, Aug 4, 2020 at 7:02 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> On 04.08.20 03:46, Bin Meng wrote: >>> On Tue, Aug 4, 2020 at 5:26 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>>> >>>> Building with CONFIG_SPL_SMP=n results in: >>>> >>>> arch/riscv/lib/spl.c: In function ‘jump_to_image_no_args’: >>>> arch/riscv/lib/spl.c:33:6: >>>> error: unused variable ‘ret’ [-Werror=unused-variable] >>>> 33 | int ret; >>>> | ^~~ >>>> >>>> Define the variable ret as __maybe_unused. >>>> >>>> Fixes: 191636e44898 ("riscv: Introduce SPL_SMP Kconfig option for U-Boot >>>> SPL") >>> >>> This should be on the same line >> >> Commit messages should not exceed 75 characters. See scripts/checkpatch.pl: > > True, for normal commit messages. > >> >> WARN("COMMIT_LOG_LONG_LINE", >> "Possible unwrapped commit description (prefer a maximum 75 chars per >> line)\n" . $herecurr); >> > > But this Fixes tag is special. I suspect 2 lines will break some > scripts that is handling this "Fixes" tag. checkpatch.pl and patchstream.py are the only U-Boot scripts containing the string "Fixes". * checkpatch.pl does not complain. * I don't use patman. So I don't care if it has a bug. We already have patches like this by other developers and nobody complained: dcdea292d9f3 4fb2264b2848 00160cf32e6e So why should I worry? Best regards Heinrich > > Regards, > Bin >
+Andy Shevchenko On Tue, Aug 4, 2020 at 10:58 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 04.08.20 15:15, Bin Meng wrote: > > On Tue, Aug 4, 2020 at 7:02 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> On 04.08.20 03:46, Bin Meng wrote: > >>> On Tue, Aug 4, 2020 at 5:26 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >>>> > >>>> Building with CONFIG_SPL_SMP=n results in: > >>>> > >>>> arch/riscv/lib/spl.c: In function ‘jump_to_image_no_args’: > >>>> arch/riscv/lib/spl.c:33:6: > >>>> error: unused variable ‘ret’ [-Werror=unused-variable] > >>>> 33 | int ret; > >>>> | ^~~ > >>>> > >>>> Define the variable ret as __maybe_unused. > >>>> > >>>> Fixes: 191636e44898 ("riscv: Introduce SPL_SMP Kconfig option for U-Boot > >>>> SPL") > >>> > >>> This should be on the same line > >> > >> Commit messages should not exceed 75 characters. See scripts/checkpatch.pl: > > > > True, for normal commit messages. > > > >> > >> WARN("COMMIT_LOG_LONG_LINE", > >> "Possible unwrapped commit description (prefer a maximum 75 chars per > >> line)\n" . $herecurr); > >> > > > > But this Fixes tag is special. I suspect 2 lines will break some > > scripts that is handling this "Fixes" tag. > > checkpatch.pl and patchstream.py are the only U-Boot scripts containing > the string "Fixes". > > * checkpatch.pl does not complain. > * I don't use patman. So I don't care if it has a bug. > > We already have patches like this by other developers and nobody complained: IIRC, last time Andy raised the same concern. Andy, would you share some examples or best practices? > > dcdea292d9f3 > 4fb2264b2848 > 00160cf32e6e > > So why should I worry? > Regards, Bin
On Wed, Aug 5, 2020 at 3:11 AM Bin Meng <bmeng.cn@gmail.com> wrote: > On Tue, Aug 4, 2020 at 10:58 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 04.08.20 15:15, Bin Meng wrote: > > > On Tue, Aug 4, 2020 at 7:02 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > >> On 04.08.20 03:46, Bin Meng wrote: > > >>> On Tue, Aug 4, 2020 at 5:26 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: ... > > >>>> Fixes: 191636e44898 ("riscv: Introduce SPL_SMP Kconfig option for U-Boot > > >>>> SPL") > > >>> > > >>> This should be on the same line The rule of thumb is one tag == one line. Otherwise it breaks a lot of scripting here and there. The reason why is that actually Unix tools are not designed (yes, I know it's possible to do in some cases) to handle multi-line processing (like multi-line grep). So, 100% Fixes should be one line. Also this is applicable to URLs. > > >> Commit messages should not exceed 75 characters. See scripts/checkpatch.pl: > > > > > > True, for normal commit messages. > > > > > >> > > >> WARN("COMMIT_LOG_LONG_LINE", > > >> "Possible unwrapped commit description (prefer a maximum 75 chars per > > >> line)\n" . $herecurr); This warning is bogus. Fix your tools. > > > But this Fixes tag is special. I suspect 2 lines will break some > > > scripts that is handling this "Fixes" tag. Precisely! > > checkpatch.pl and patchstream.py are the only U-Boot scripts containing > > the string "Fixes". > > > > * checkpatch.pl does not complain. > > * I don't use patman. So I don't care if it has a bug. You don't but maintainers tell you what to do. And yes, tools sometimes wrong or false positive, feel free to complain about them. > > We already have patches like this by other developers and nobody complained: > > IIRC, last time Andy raised the same concern. > > Andy, would you share some examples or best practices? > > > > > dcdea292d9f3 > > 4fb2264b2848 > > 00160cf32e6e > > > > So why should I worry? I hope above clarifies. And it's generally a weak argument to point to bad examples in the past.
diff --git a/arch/riscv/lib/spl.c b/arch/riscv/lib/spl.c index c47dcd46ce..ef00ec2bcc 100644 --- a/arch/riscv/lib/spl.c +++ b/arch/riscv/lib/spl.c @@ -30,7 +30,7 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) { typedef void __noreturn (*image_entry_riscv_t)(ulong hart, void *dtb); void *fdt_blob; - int ret; + __maybe_unused int ret; #if CONFIG_IS_ENABLED(LOAD_FIT) || CONFIG_IS_ENABLED(LOAD_FIT_FULL) fdt_blob = spl_image->fdt_addr;
Building with CONFIG_SPL_SMP=n results in: arch/riscv/lib/spl.c: In function ‘jump_to_image_no_args’: arch/riscv/lib/spl.c:33:6: error: unused variable ‘ret’ [-Werror=unused-variable] 33 | int ret; | ^~~ Define the variable ret as __maybe_unused. Fixes: 191636e44898 ("riscv: Introduce SPL_SMP Kconfig option for U-Boot SPL") Fixes: 8c59f2023cc8 ("riscv: add SPL support") Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- arch/riscv/lib/spl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.27.0