Messages in this thread | | | Date | Thu, 19 Jan 2023 16:20:57 +0100 | Subject | Re: [PATCH v2] of: Fix of platform build on powerpc due to bad of disaply code | From | Thomas Zimmermann <> |
| |
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@suse.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.
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 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.
Best regards Thomas
> > Thanks > > Michal > >> >>> >>>> int ret; >>>> /* Check if we have a MacOS display without a node spec */ >>>> @@ -556,7 +558,10 @@ static int __init of_platform_default_populate_init(void) >>>> if (!of_get_property(node, "linux,opened", NULL) || >>>> !of_get_property(node, "linux,boot-display", NULL)) >>>> continue; >>>> - dev = of_platform_device_create(node, "of-display", NULL); >>>> + ret = snprintf(buf, sizeof(buf), of_display_format, display_number++); >>>> + if (ret >= sizeof(buf)) >>>> + continue; >>> >>> >>> Can you make buf big enough to avoid that ? >>> >>> And by the way could it be called something else than 'buf' ? >>> >>> See exemple here : >>> https://elixir.bootlin.com/linux/v6.1/source/drivers/fsi/fsi-occ.c#L690 >>> >>> >>>> + dev = of_platform_device_create(node, buf, NULL); >>>> if (WARN_ON(!dev)) >>>> return -ENOMEM; >>>> boot_display = node; >>>> @@ -564,10 +569,9 @@ static int __init of_platform_default_populate_init(void) >>>> } >>>> for_each_node_by_type(node, "display") { >>>> - char *buf[14]; >>>> if (!of_get_property(node, "linux,opened", NULL) || node == boot_display) >>>> continue; >>>> - ret = snprintf(buf, "of-display-%d", display_number++); >>>> + ret = snprintf(buf, sizeof(buf), of_display_format, display_number++); >>>> if (ret >= sizeof(buf)) >>>> continue; >>>> of_platform_device_create(node, buf, NULL); >> >> -- >> Thomas Zimmermann >> Graphics Driver Developer >> SUSE Software Solutions Germany GmbH >> Maxfeldstr. 5, 90409 Nürnberg, Germany >> (HRB 36809, AG Nürnberg) >> Geschäftsführer: Ivo Totev > > >
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev [unhandled content-type:application/pgp-signature] | |