[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <190c1c68-0249-a291-f2ab-45c9a7f716d7@suse.de>
Date: Thu, 19 Jan 2023 16:20:57 +0100
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Michal Suchánek <msuchanek@...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
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.
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
Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)
Powered by blists - more mailing lists