lkml.org 
[lkml]   [2023]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2] of: Fix of platform build on powerpc due to bad of disaply code
From
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]
\
 
 \ /
  Last update: 2023-03-26 23:47    [W:0.078 / U:0.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site