Message-ID: <20230120115604.GT16547@kitsune.suse.cz>
Date:   Fri, 20 Jan 2023 12:56:04 +0100
From:   Michal Suchánek <msuchanek@...e.de>
To:     Thomas Zimmermann <tzimmermann@...e.de>
Cc:     Christophe Leroy <christophe.leroy@...roup.eu>,
        "Erhard F." <erhard_f@...lbox.org>,
        Rob Herring <robh+dt@...nel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE" 
        <devicetree@...r.kernel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Javier Martinez Canillas <javierm@...hat.com>,
        open list <linux-kernel@...r.kernel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [PATCH v2] of: Fix of platform build on powerpc due to bad of
 disaply code

On Fri, Jan 20, 2023 at 12:39:23PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 20.01.23 um 12:27 schrieb Michal Suchánek:
> > Hello,
> > 
> > On Thu, Jan 19, 2023 at 04:20:57PM +0100, Thomas Zimmermann wrote:
> > > Hi
> > > 
> > > Am 19.01.23 um 14:23 schrieb Michal Suchánek:
> > > > On Thu, Jan 19, 2023 at 02:11:13PM +0100, Thomas Zimmermann wrote:
> > > > > Hi
> > > > > 
> > > > > Am 19.01.23 um 11:24 schrieb Christophe Leroy:
> > > > > > 
> > > > > > 
> > > > > > Le 19/01/2023 à 10:53, Michal Suchanek a écrit :
> > > > > > > The commit 2d681d6a23a1 ("of: Make of framebuffer devices unique")
> > > > > > > breaks build because of wrong argument to snprintf. That certainly
> > > > > > > avoids the runtime error but is not the intended outcome.
> > > > > > > 
> > > > > > > Also use standard device name format of-display.N for all created
> > > > > > > devices.
> > > > > > > 
> > > > > > > Fixes: 2d681d6a23a1 ("of: Make of framebuffer devices unique")
> > > > > > > Signed-off-by: Michal Suchanek <msuchanek@...e.de>
> > > > > > > ---
> > > > > > > v2: Update the device name format
> > > > > > > ---
> > > > > > >      drivers/of/platform.c | 12 ++++++++----
> > > > > > >      1 file changed, 8 insertions(+), 4 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > > > > > index f2a5d679a324..8c1b1de22036 100644
> > > > > > > --- a/drivers/of/platform.c
> > > > > > > +++ b/drivers/of/platform.c
> > > > > > > @@ -525,7 +525,9 @@ static int __init of_platform_default_populate_init(void)
> > > > > > >      	if (IS_ENABLED(CONFIG_PPC)) {
> > > > > > >      		struct device_node *boot_display = NULL;
> > > > > > >      		struct platform_device *dev;
> > > > > > > -		int display_number = 1;
> > > > > > > +		int display_number = 0;
> > > > > > > +		char buf[14];
> > > > > > 
> > > > > > Can you declare that in the for block where it is used instead ?
> > > > > > 
> > > > > > > +		char *of_display_format = "of-display.%d";
> > > > > > 
> > > > > > Should be const ?
> > > > > 
> > > > > That should be static const of_display_format[] = then
> > > > 
> > > > Why? It sounds completely fine to have a const pointer to a string
> > > > constatnt.
> > > 
> > > Generally speaking:
> > > 
> > > 'static' because your const pointer is then not a local variable, so it
> > > takes pressure off the stack. For global variables, you don't want them to
> > > show up in any linker symbol tables.
> > 
> > This sounds a lot like an exemplar case of premature optimization.
> > A simplistic compiler might do exactly what you say, and allocate a slot
> > for the variable on the stack the moment the function is entered.
> > 
> > However, in real compilers there is no stack pressure from having a
> > local variable:
> >   - the compiler can put the variable into a register
> >   - it can completely omit the variable before and after it's actually
> >     used which is that specific function call
> > 
> > > The string "of-display.%d" is stored as an array in the ELF data section.
> > > And your char pointer is a reference to that array. For static pointers,
> > > these indirections take CPU cycles to update when the loader has to relocate
> > 
> > Provided that the char pointer ever exists in the compiled code. Its
> > address is not taken so it does not need to.
> > 
> > > sections. If you declare of_display_format[] directly as array, you avoid
> > > the reference and work directly with the array.
> > > 
> > > Of course, this is a kernel module and the string is self-contained within
> > > the function. So the compiler can probably detect that and optimize the code
> > > to be like the 'static const []' version. It's still good to follow best
> > > practices, as someone might copy from this function.
> > 
> > If it could not detect it there would be a lot of trouble all around.
> 
> The issues definitely exist in userspace code. Kernel modules are simpler,
> so compiler optimization is easier.
> 
> But I'm not really trying to make a technical argument. My point here is
> that someone might read your code and duplicate the pattern. That's not
> unreasonable: it's core Linux code, so it can be assumed to be good (or at
> least not bad). But your current code teaches the reader a bad practices,
> which should be avoided. It is better to do the correct thing, even if it
> makes no difference to the compiled code.

The point I am trying to get across is that besides the original
objection about missing 'const' this code is not bad. Loading a string
constant address into a local variable and passing it as function call
argument is perfectly fine.

If you get any advantage by the alternate convoluted construct it's
more likely than anything else a bug in the compiler you are using.

It may be necessary to work around such bugs in performance-critical
code but not in driver probing code that runs exactly once during boot.

Thanks

Michal

Powered by blists - more mailing lists