Hi Tom, On Fri, 6 Oct 2023 at 19:01, Tom Rini <tr...@konsulko.com> wrote: > > On Fri, Oct 06, 2023 at 04:42:44PM -0600, Simon Glass wrote: > > Hi Tom, > > > > On Fri, 6 Oct 2023 at 10:55, Tom Rini <tr...@konsulko.com> wrote: > > > > > > On Fri, Oct 06, 2023 at 07:03:17AM -0600, Simon Glass wrote: > > > > Hi Tom, > > > > > > > > On Thu, 5 Oct 2023 at 20:16, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > > > On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote: > > > > > > Hi Tom, > > > > > > > > > > > > On Thu, 5 Oct 2023 at 08:53, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > > > > > > > On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote: > > > > > > > > Hi Tom, > > > > > > > > > > > > > > > > On Sun, 24 Sept 2023 at 17:27, Tom Rini <tr...@konsulko.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote: > > > > > > > > > > Some commands assume that CONFIG_SYS_LONGHELP is always > > > > > > > > > > defined. > > > > > > > > > > Declaration of long help should be bracketed by an #ifdef > > > > > > > > > > to avoid an > > > > > > > > > > 'unused variable' warning. > > > > > > > > > > > > > > > > > > > > Fix this treewide. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Simon Glass <s...@chromium.org> > > > > > > > > > [snip] > > > > > > > > > > diff --git a/arch/arm/mach-imx/cmd_dek.c > > > > > > > > > > b/arch/arm/mach-imx/cmd_dek.c > > > > > > > > > > index 6fa5b41fcd38..25ea7d3b37da 100644 > > > > > > > > > > --- a/arch/arm/mach-imx/cmd_dek.c > > > > > > > > > > +++ b/arch/arm/mach-imx/cmd_dek.c > > > > > > > > > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl > > > > > > > > > > *cmdtp, int flag, int argc, > > > > > > > > > > return blob_encap_dek(src_addr, dst_addr, len); > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > -/***************************************************/ > > > > > > > > > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP) > > > > > > > > > > static char dek_blob_help_text[] = > > > > > > > > > > "src dst len - Encapsulate and create blob > > > > > > > > > > of data\n" > > > > > > > > > > " $len bits long at address > > > > > > > > > > $src and\n" > > > > > > > > > > " store the result at address > > > > > > > > > > $dst.\n"; > > > > > > > > > > +#endif > > > > > > > > > > > > > > > > > > > > U_BOOT_CMD( > > > > > > > > > > dek_blob, 4, 1, do_dek_blob, > > > > > > > > > > > > > > > > > > This really doesn't read nicely. I would rather (globally > > > > > > > > > and fix > > > > > > > > > existing users) __maybe_unused this instead. I think there's > > > > > > > > > just one > > > > > > > > > example today that isn't "foo_help_text". > > > > > > > > > > > > > > > > Hmm, what do you think about adding a __longhelp symbol to > > > > > > > > cause the > > > > > > > > linker to discard it when not needed? > > > > > > > > > > > > > > Well, I don't think we need linker list magic when __maybe_unused > > > > > > > will > > > > > > > just have them be discarded normally. > > > > > > > > > > > > Yes, perhaps things are in a better state than they used to be, but > > > > > > there is a linker discard for commands at present. > > > > > > > > > > Yes, but __maybe_unused means we don't get a warning about it, and > > > > > it's > > > > > literally discarded as part of --gc-sections as it done everywhere > > > > > with > > > > > maybe 3 exceptions? > > > > > > > > Actually with this series we get a lot closer to that. The problem > > > > with the status quo is that disabling CMDLINE doesn't disable most > > > > commands, relying instead on discarding them at link time. > > > > > > I don't follow you here. We're talking only about the long help. > > > > I was actually talking about how the command code is removed. This > > series allows that to happen via Kconfig rather than needing a > > linker-script discard rule, something I only just fully appreciated. > > OK. But this series as-is has a lot of problems and I don't see it as > more than a proof of concept.
Probably I need to send a few version as this discussion is becoming a bit theoretical. > > > > There's already an option to enable/disable it. When disabled all of > > > the long help text should be discarded, because we reference it via > > > U_BOOT_CMD macro which in turn cares about it, or not. What's missing > > > is a U_BOOT_LONGHELP macro so that instead of: > > > #ifdef CONFIG_SYS_LONGHELP > > > static char cat_help_text[] = > > > "<interface> <dev[:part]> <file>\n" > > > " - Print file from 'dev' on 'interface' to standard output\n"; > > > #endif > > > > > > We do: > > > U_BOOT_LONGHELP(cat, > > > "<interface> <dev[:part]> <file>\n" > > > " - Print file from 'dev' on 'interface' to standard output\n" > > > ); > > > > > > Which then does: > > > static __maybe_unused char cat_help_text[] = > > > ... > > > ; > > > > > > And we discard the text automatically due to --gc-sections. We possibly > > > haven't already been doing this since back when we first started using > > > --gc-sections there was a bug in binutils that caused text like the > > > above to still get combined in to other objects and not discarded. > > > That's been fixed for ages. > > > > > > And the above macro would also let us clean up U_BOOT_CMD macro slightly > > > to just omit the longhelp option and instead always do _CMD_HELP(_name) > > > inside the macros. It'll also make it harder to add new commands > > > without a long help by accident. > > > > Gosh this is complicated. > > > > At present the U_BOOT_CMD() macro just drops the strings...the problem > > with the unused var only happens in a small number of cases where an > > explicit var is used. I don't see why creating a var (when none is > > there today) helps anything? It doesn't detect missing longhelp, since > > this is already an error (missing argument). Sure, "" can be > > provided, but your macro doesn't stop that either. > > The problem today is that 95% of the cases surround the help text with > #ifdef CONFIG_SYS_LONGHELP ... #endif. That's why the rest of the > macros are the way they are today. And that in turn is due to (in part > at least) old compiler bugs. I see about 46 #idefs for that and nearly 300 commands that don't have one. > > > If you are suggesting a new U_BOOT_LONGHELP() macro, bear in mind that > > we already have quite a lot...each new 'top-level' macro results in > > more combinations. > > It really should just be a single macro. I think I'll make an attempt > at this, to show what I'm talking about. OK thanks. > > > > > With this series, it looks like we can in fact do that, so I should > > > > remove the discards as well. > > > > > > > > The one proviso is that this does drop a lot of things we want...e.g. > > > > BOOTSTD_DEFAULTS cannot be enabled due to its use of 'select', meaning > > > > that common filesystems are dropped. So we'll need more effort after > > > > this, to allow (for example) bootmeths that don't need commands to > > > > work correctly. But I think this series at least provides a better > > > > starting point for teasing things apart. > > > > > > > > So OK I'll go with __maybe_unused for the ones that need it, which > > > > isn't too many given that many of the strings are just included > > > > directly in the macro. It is considerably easier than trying to be to > > > > clever about things. > > > > > > Yes, this series itself has a lot of problems that need to be addressed > > > before reposting because I don't like "hiding" that network stuff no > > > longer works, and I also saw that DFU also silently failing. > > > > Yes I saw your other comments. But I think this patch is the big area > > of confusion. > > This isn't the tricky part of the series that I'm saying has problems, > that part is all of the functionality that's not being untangled and I > think leads to the question of what exactly is the use case where we do > remove the command line but don't need some of that functionality still. For security and code-size reasons it is useful to disable commands, perhaps all of them. Quite a few features don't need it, but it certainly would take more effort to get a usable version. The goal of this series is to avoid people adding Kconfig mistakes which need to be cleaned up later. Regards, Simon