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[] = thenWhy? 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 Michalint 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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
