[PATCH v2] of: Fix of platform build on powerpc due to bad of disaply code

[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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20230119/a7b7118f/attachment.sig>