Re: [PATCH v2 2/6] console: Implement flush() function

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 5231 bytes --]

On Wed, Aug 31, 2022 at 07:46:58AM -0600, Simon Glass wrote:
> Hi Pali,
> 
> On Wed, 31 Aug 2022 at 03:13, Pali Rohár <pali@kernel.org> wrote:
> >
> > On Thursday 25 August 2022 09:08:18 Simon Glass wrote:
> > > Hi Pali,
> > >
> > > On Thu, 25 Aug 2022 at 08:20, Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Friday 12 August 2022 20:21:00 Simon Glass wrote:
> > > > > Hi Pali,
> > > > >
> > > > > On Fri, 12 Aug 2022 at 09:17, Pali Rohár <pali@kernel.org> wrote:
> > > > > >
> > > > > > On Friday 12 August 2022 09:11:10 Simon Glass wrote:
> > > > > > > Hi Pali,
> > > > > > >
> > > > > > > On Fri, 12 Aug 2022 at 03:01, Pali Rohár <pali@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Thursday 11 August 2022 18:08:46 Simon Glass wrote:
> > > > > > > > > Hi Pali,
> > > > > > > > >
> > > > > > > > > On Thu, 11 Aug 2022 at 08:50, Pali Rohár <pali@kernel.org> wrote:
> > > > > > > > > >
> > > > > > > > > > On Thursday 11 August 2022 08:47:50 Simon Glass wrote:
> > > > > > > > > > > > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > > > > > > > > > > > index 270fa2729fb2..06278366ae88 100644
> > > > > > > > > > > > --- a/include/stdio_dev.h
> > > > > > > > > > > > +++ b/include/stdio_dev.h
> > > > > > > > > > > > @@ -37,6 +37,10 @@ struct stdio_dev {
> > > > > > > > > > > >         void (*putc)(struct stdio_dev *dev, const char c);
> > > > > > > > > > > >         /* To put a string (accelerator) */
> > > > > > > > > > > >         void (*puts)(struct stdio_dev *dev, const char *s);
> > > > > > > > > > > > +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> > > > > > > > > > >
> > > > > > > > > > > I'd argue it isn't worth the #ifdef and we might as well have this
> > > > > > > > > > > member here always. Then you can drop some #ifdefs from your code.
> > > > > > > > > >
> > > > > > > > > > But then it will increase binary code size.
> > > > > > > > >
> > > > > > > > > Using the member will increase code size. But I think the only place
> > > > > > > > > you need an #ifdef for that is when you include it in the driver
> > > > > > > > > struct. You can use __maybe_unused in the other place.
> > > > > > > > >
> > > > > > > > > Having the member will only increase memory usage, not code size.
> > > > > > > >
> > > > > > > > But static memory structures are part of the u-boot.bin binary and also
> > > > > > > > their usage increase code size (required copy, etc...), so at the end it
> > > > > > > > increase code size.
> > > > > > >
> > > > > > > From what I understand stdio_dev is allocated at runtime in BSS:
> > > > > > >
> > > > > > > struct stdio_dev *stdio_devices[] = { NULL, NULL, NULL };
> > > > > > >
> > > > > > > (the NULL stuff should not be there, but does nothing, I believe)
> > > > > > >
> > > > > > > So long as no code accesses your new member, then it should only
> > > > > > > affect the BSS size.
> > > > > >
> > > > > > Code of course has to access new member. How otherwise would be able to
> > > > > > call that new callbacks? E.g. all new code in patch 2/6 or 3/6.
> > > > >
> > > > > Yes, see below. Also, do check whether it actually adds code or not.
> > > > >
> > > > > >
> > > > > > > If you are very worried about it, you could use the technique in
> > > > > > > asm-generic/global_data.h to avoid #ifdefs in the C code:
> > > > > > >
> > > > > > > #ifdef CONFIG_GENERATE_ACPI_TABLE
> > > > > > > #define gd_acpi_start() gd->acpi_start
> > > > > > > #define gd_set_acpi_start(addr) gd->acpi_start = addr
> > > > > > > #else
> > > > > > > #define gd_acpi_start() 0UL
> > > > > > > #define gd_set_acpi_start(addr)
> > > > > > > #endif
> > > > > > >
> > > > > > > Regards,
> > > > > > > Simon
> > > > >
> > > > > Regards,
> > > > > Simon
> > > >
> > > > But in this case, it is needed to introduce a new special macro and hide
> > > > assigning code into it. In my opinion such macro with side effect and
> > > > worse than writing it explicitly - open coded to show what the code is
> > > > doing.
> > >
> > > You can use an inline function if you prefer.
> >
> > But it has still same issue, hiding conditional logic indirectly to
> > additional file. Some members would be assigned directly via = operator
> > and some via macro/inlinefunction is inconsistent.
> 
> I've said everything I can say about this. If Tom has some thoughts
> I'll leave it to him.
> 
> >
> > > My main objection is to adding #ifdefs to C files. They are OK (for
> > > now) in driver struct declarations, where some members are optional.
> > > But I think it is worth going to some lengths to avoid them elsewhere.
> > >
> > > +Tom Rini for thoughts on this
> > >
> > > Regards,
> > > SImon
> >
> > Any comments on this?

Given the plethora of tools to demonstrate that approach X results in
size change Y, I'm not sure why we're pondering if something will or
won't result in increases? I would like to see this series result in the
smallest increase in binary size on platforms that don't enable the
feature. An #ifdef isn't more or less readable than using some macro
instead, and decorators like __maybe_unused have their own impact on
code readability.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]