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@xxxxxxx>
> > > > > > > ---
> > > > > > > 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
