Since Linux 5.19 this error is observed:
sysfs: cannot create duplicate filename '/devices/platform/of-display'
This is because multiple devices with the same name 'of-display' are
created on the same bus.
Update the code to create numbered device names for the non-boot
disaplay.
cc: [email protected]
References: https://bugzilla.kernel.org/show_bug.cgi?id=216095
Fixes: 52b1b46c39ae ("of: Create platform devices for OF framebuffers")
Reported-by: Erhard F. <[email protected]>
Suggested-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/of/platform.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 81c8c227ab6b..f2a5d679a324 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -525,6 +525,7 @@ 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 ret;
/* Check if we have a MacOS display without a node spec */
@@ -561,10 +562,15 @@ static int __init of_platform_default_populate_init(void)
boot_display = node;
break;
}
+
for_each_node_by_type(node, "display") {
+ char *buf[14];
if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
continue;
- of_platform_device_create(node, "of-display", NULL);
+ ret = snprintf(buf, "of-display-%d", display_number++);
+ if (ret >= sizeof(buf))
+ continue;
+ of_platform_device_create(node, buf, NULL);
}
} else {
--
2.35.3
On Tue, 17 Jan 2023 17:58:04 +0100, Michal Suchanek wrote:
> Since Linux 5.19 this error is observed:
>
> sysfs: cannot create duplicate filename '/devices/platform/of-display'
>
> This is because multiple devices with the same name 'of-display' are
> created on the same bus.
>
> Update the code to create numbered device names for the non-boot
> disaplay.
>
> cc: [email protected]
> References: https://bugzilla.kernel.org/show_bug.cgi?id=216095
> Fixes: 52b1b46c39ae ("of: Create platform devices for OF framebuffers")
> Reported-by: Erhard F. <[email protected]>
> Suggested-by: Thomas Zimmermann <[email protected]>
> Signed-off-by: Michal Suchanek <[email protected]>
> ---
> drivers/of/platform.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
Applied, thanks!
On Tue, 17 Jan 2023 17:58:04 +0100
Michal Suchanek <[email protected]> wrote:
> Since Linux 5.19 this error is observed:
>
> sysfs: cannot create duplicate filename '/devices/platform/of-display'
>
> This is because multiple devices with the same name 'of-display' are
> created on the same bus.
>
> Update the code to create numbered device names for the non-boot
> disaplay.
>
> cc: [email protected]
> References: https://bugzilla.kernel.org/show_bug.cgi?id=216095
> Fixes: 52b1b46c39ae ("of: Create platform devices for OF framebuffers")
> Reported-by: Erhard F. <[email protected]>
> Suggested-by: Thomas Zimmermann <[email protected]>
> Signed-off-by: Michal Suchanek <[email protected]>
> ---
> drivers/of/platform.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 81c8c227ab6b..f2a5d679a324 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -525,6 +525,7 @@ 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 ret;
>
> /* Check if we have a MacOS display without a node spec */
> @@ -561,10 +562,15 @@ static int __init of_platform_default_populate_init(void)
> boot_display = node;
> break;
> }
> +
> for_each_node_by_type(node, "display") {
> + char *buf[14];
> if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
> continue;
> - of_platform_device_create(node, "of-display", NULL);
> + ret = snprintf(buf, "of-display-%d", display_number++);
> + if (ret >= sizeof(buf))
> + continue;
> + of_platform_device_create(node, buf, NULL);
> }
>
> } else {
> --
> 2.35.3
>
Thank you for the patch Michal!
It applies on 6.2-rc4 but I get this build error with my config:
# make
CALL scripts/checksyscalls.sh
CC drivers/of/platform.o
drivers/of/platform.c: In function 'of_platform_default_populate_init':
drivers/of/platform.c:570:40: error: passing argument 1 of 'snprintf' from incompatible pointer type [-Werror=incompatible-pointer-types]
570 | ret = snprintf(buf, "of-display-%d", display_number++);
| ^~~
| |
| char **
In file included from ./arch/powerpc/include/asm/page.h:11,
from ./arch/powerpc/include/asm/thread_info.h:13,
from ./include/linux/thread_info.h:60,
from ./arch/powerpc/include/asm/ptrace.h:342,
from ./arch/powerpc/include/asm/hw_irq.h:12,
from ./arch/powerpc/include/asm/irqflags.h:12,
from ./include/linux/irqflags.h:16,
from ./include/asm-generic/cmpxchg-local.h:6,
from ./arch/powerpc/include/asm/cmpxchg.h:755,
from ./arch/powerpc/include/asm/atomic.h:11,
from ./include/linux/atomic.h:7,
from ./include/linux/mm_types_task.h:13,
from ./include/linux/mm_types.h:5,
from ./include/linux/buildid.h:5,
from ./include/linux/module.h:14,
from drivers/of/platform.c:13:
./include/linux/kernel.h:213:20: note: expected 'char *' but argument is of type 'char **'
213 | int snprintf(char *buf, size_t size, const char *fmt, ...);
| ~~~~~~^~~
drivers/of/platform.c:570:45: warning: passing argument 2 of 'snprintf' makes integer from pointer without a cast [-Wint-conversion]
570 | ret = snprintf(buf, "of-display-%d", display_number++);
| ^~~~~~~~~~~~~~~
| |
| char *
./include/linux/kernel.h:213:32: note: expected 'size_t' {aka 'unsigned int'} but argument is of type 'char *'
213 | int snprintf(char *buf, size_t size, const char *fmt, ...);
| ~~~~~~~^~~~
drivers/of/platform.c:570:76: warning: passing argument 3 of 'snprintf' makes pointer from integer without a cast [-Wint-conversion]
570 | ret = snprintf(buf, "of-display-%d", display_number++);
| ~~~~~~~~~~~~~~^~
| |
| int
./include/linux/kernel.h:213:50: note: expected 'const char *' but argument is of type 'int'
213 | int snprintf(char *buf, size_t size, const char *fmt, ...);
| ~~~~~~~~~~~~^~~
drivers/of/platform.c:573:57: error: passing argument 2 of 'of_platform_device_create' from incompatible pointer type [-Werror=incompatible-pointer-types]
573 | of_platform_device_create(node, buf, NULL);
| ^~~
| |
| char **
drivers/of/platform.c:211:57: note: expected 'const char *' but argument is of type 'char **'
211 | const char *bus_id,
| ~~~~~~~~~~~~^~~~~~
cc1: some warnings being treated as errors
make[3]: *** [scripts/Makefile.build:252: drivers/of/platform.o] Fehler 1
make[2]: *** [scripts/Makefile.build:504: drivers/of] Fehler 2
make[1]: *** [scripts/Makefile.build:504: drivers] Fehler 2
make: *** [Makefile:2008: .] Error 2
Regards,
Erhard
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.
Fixes: 2d681d6a23a1 ("of: Make of framebuffer devices unique")
Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/of/platform.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index f2a5d679a324..e9dd7371f27a 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -564,10 +564,10 @@ static int __init of_platform_default_populate_init(void)
}
for_each_node_by_type(node, "display") {
- char *buf[14];
+ 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-%d", display_number++);
if (ret >= sizeof(buf))
continue;
of_platform_device_create(node, buf, NULL);
--
2.35.3
On Wed, Jan 18, 2023 at 09:13:05PM +0100, Erhard F. wrote:
> On Tue, 17 Jan 2023 17:58:04 +0100
> Michal Suchanek <[email protected]> wrote:
>
> > Since Linux 5.19 this error is observed:
> >
> > sysfs: cannot create duplicate filename '/devices/platform/of-display'
> >
> > This is because multiple devices with the same name 'of-display' are
> > created on the same bus.
> >
> > Update the code to create numbered device names for the non-boot
> > disaplay.
> >
> > cc: [email protected]
> > References: https://bugzilla.kernel.org/show_bug.cgi?id=216095
> > Fixes: 52b1b46c39ae ("of: Create platform devices for OF framebuffers")
> > Reported-by: Erhard F. <[email protected]>
> > Suggested-by: Thomas Zimmermann <[email protected]>
> > Signed-off-by: Michal Suchanek <[email protected]>
> > ---
> > drivers/of/platform.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index 81c8c227ab6b..f2a5d679a324 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -525,6 +525,7 @@ 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 ret;
> >
> > /* Check if we have a MacOS display without a node spec */
> > @@ -561,10 +562,15 @@ static int __init of_platform_default_populate_init(void)
> > boot_display = node;
> > break;
> > }
> > +
> > for_each_node_by_type(node, "display") {
> > + char *buf[14];
> > if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
> > continue;
> > - of_platform_device_create(node, "of-display", NULL);
> > + ret = snprintf(buf, "of-display-%d", display_number++);
> > + if (ret >= sizeof(buf))
> > + continue;
> > + of_platform_device_create(node, buf, NULL);
> > }
> >
> > } else {
> > --
> > 2.35.3
> >
>
> Thank you for the patch Michal!
>
> It applies on 6.2-rc4 but I get this build error with my config:
Indeed, it's doubly bad.
Where is the kernel test robot when you need it?
It should not be that easy to miss this file but clearly it can happen.
I will send a fixup.
Sorry about the mess.
Thanks
Michal
Hi Michal,
thanks for fixing this issue. But the review time was way too short.
Please see my comments below.
Am 18.01.23 um 22:46 schrieb Michal Suchánek:
> On Wed, Jan 18, 2023 at 09:13:05PM +0100, Erhard F. wrote:
>> On Tue, 17 Jan 2023 17:58:04 +0100
>> Michal Suchanek <[email protected]> wrote:
>>
>>> Since Linux 5.19 this error is observed:
>>>
>>> sysfs: cannot create duplicate filename '/devices/platform/of-display'
>>>
>>> This is because multiple devices with the same name 'of-display' are
>>> created on the same bus.
>>>
>>> Update the code to create numbered device names for the non-boot
>>> disaplay.
>>>
>>> cc: [email protected]
>>> References: https://bugzilla.kernel.org/show_bug.cgi?id=216095
>>> Fixes: 52b1b46c39ae ("of: Create platform devices for OF framebuffers")
>>> Reported-by: Erhard F. <[email protected]>
>>> Suggested-by: Thomas Zimmermann <[email protected]>
>>> Signed-off-by: Michal Suchanek <[email protected]>
>>> ---
>>> drivers/of/platform.c | 8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>> index 81c8c227ab6b..f2a5d679a324 100644
>>> --- a/drivers/of/platform.c
>>> +++ b/drivers/of/platform.c
>>> @@ -525,6 +525,7 @@ 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 ret;
>>>
>>> /* Check if we have a MacOS display without a node spec */
>>> @@ -561,10 +562,15 @@ static int __init of_platform_default_populate_init(void)
>>> boot_display = node;
>>> break;
>>> }
>>> +
>>> for_each_node_by_type(node, "display") {
>>> + char *buf[14];
>>> if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
>>> continue;
>>> - of_platform_device_create(node, "of-display", NULL);
>>> + ret = snprintf(buf, "of-display-%d", display_number++);
Platform devices use a single dot (.) as separator before the index.
Counting starts at zero. See /sys/bus/platform/ for examples. Can we
please stick with that scheme? Generated names would then be
of-display.0, of-display.1, etc.
Best regards
Thomas
>>> + if (ret >= sizeof(buf))
>>> + continue;
>>> + of_platform_device_create(node, buf, NULL);
>>> }
>>>
>>> } else {
>>> --
>>> 2.35.3
>>>
>>
>> Thank you for the patch Michal!
>>
>> It applies on 6.2-rc4 but I get this build error with my config:
>
> Indeed, it's doubly bad.
>
> Where is the kernel test robot when you need it?
>
> It should not be that easy to miss this file but clearly it can happen.
>
> I will send a fixup.
>
> Sorry about the mess.
>
> Thanks
>
> Michal
--
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
On Thu, Jan 19, 2023 at 09:00:44AM +0100, Thomas Zimmermann wrote:
> Hi Michal,
>
> thanks for fixing this issue. But the review time was way too short. Please
> see my comments below.
>
> Am 18.01.23 um 22:46 schrieb Michal Such?nek:
> > On Wed, Jan 18, 2023 at 09:13:05PM +0100, Erhard F. wrote:
> > > On Tue, 17 Jan 2023 17:58:04 +0100
> > > Michal Suchanek <[email protected]> wrote:
> > >
> > > > Since Linux 5.19 this error is observed:
> > > >
> > > > sysfs: cannot create duplicate filename '/devices/platform/of-display'
> > > >
> > > > This is because multiple devices with the same name 'of-display' are
> > > > created on the same bus.
> > > >
> > > > Update the code to create numbered device names for the non-boot
> > > > disaplay.
> > > >
> > > > cc: [email protected]
> > > > References: https://bugzilla.kernel.org/show_bug.cgi?id=216095
> > > > Fixes: 52b1b46c39ae ("of: Create platform devices for OF framebuffers")
> > > > Reported-by: Erhard F. <[email protected]>
> > > > Suggested-by: Thomas Zimmermann <[email protected]>
> > > > Signed-off-by: Michal Suchanek <[email protected]>
> > > > ---
> > > > drivers/of/platform.c | 8 +++++++-
> > > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > > index 81c8c227ab6b..f2a5d679a324 100644
> > > > --- a/drivers/of/platform.c
> > > > +++ b/drivers/of/platform.c
> > > > @@ -525,6 +525,7 @@ 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 ret;
> > > > /* Check if we have a MacOS display without a node spec */
> > > > @@ -561,10 +562,15 @@ static int __init of_platform_default_populate_init(void)
> > > > boot_display = node;
> > > > break;
> > > > }
> > > > +
> > > > for_each_node_by_type(node, "display") {
> > > > + char *buf[14];
> > > > if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
> > > > continue;
> > > > - of_platform_device_create(node, "of-display", NULL);
> > > > + ret = snprintf(buf, "of-display-%d", display_number++);
>
> Platform devices use a single dot (.) as separator before the index.
> Counting starts at zero. See /sys/bus/platform/ for examples. Can we please
> stick with that scheme? Generated names would then be of-display.0,
> of-display.1, etc.
Yes, there was surprisingly no bikeshedding.
Do we also want to change the name of the device that did manage to
instantiate before?
This scheme changes the name only for those that did not in the past,
hence "of-display" and "of-display-%d", starting from 1.
Sure, replacing '-' with '.' is easy enough, and using the same format
for both as well.
Thanks
Michal
>
> Best regards
> Thomas
>
>
>
> > > > + if (ret >= sizeof(buf))
> > > > + continue;
> > > > + of_platform_device_create(node, buf, NULL);
> > > > }
> > > > } else {
> > > > --
> > > > 2.35.3
> > > >
> > >
> > > Thank you for the patch Michal!
> > >
> > > It applies on 6.2-rc4 but I get this build error with my config:
> >
> > Indeed, it's doubly bad.
> >
> > Where is the kernel test robot when you need it?
> >
> > It should not be that easy to miss this file but clearly it can happen.
> >
> > I will send a fixup.
> >
> > Sorry about the mess.
> >
> > Thanks
> >
> > Michal
>
> --
> 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
Hi
Am 19.01.23 um 10:01 schrieb Michal Suchánek:
> On Thu, Jan 19, 2023 at 09:00:44AM +0100, Thomas Zimmermann wrote:
>> Hi Michal,
>>
>> thanks for fixing this issue. But the review time was way too short. Please
>> see my comments below.
>>
>> Am 18.01.23 um 22:46 schrieb Michal Suchánek:
>>> On Wed, Jan 18, 2023 at 09:13:05PM +0100, Erhard F. wrote:
>>>> On Tue, 17 Jan 2023 17:58:04 +0100
>>>> Michal Suchanek <[email protected]> wrote:
>>>>
>>>>> Since Linux 5.19 this error is observed:
>>>>>
>>>>> sysfs: cannot create duplicate filename '/devices/platform/of-display'
>>>>>
>>>>> This is because multiple devices with the same name 'of-display' are
>>>>> created on the same bus.
>>>>>
>>>>> Update the code to create numbered device names for the non-boot
>>>>> disaplay.
>>>>>
>>>>> cc: [email protected]
>>>>> References: https://bugzilla.kernel.org/show_bug.cgi?id=216095
>>>>> Fixes: 52b1b46c39ae ("of: Create platform devices for OF framebuffers")
>>>>> Reported-by: Erhard F. <[email protected]>
>>>>> Suggested-by: Thomas Zimmermann <[email protected]>
>>>>> Signed-off-by: Michal Suchanek <[email protected]>
>>>>> ---
>>>>> drivers/of/platform.c | 8 +++++++-
>>>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>>>> index 81c8c227ab6b..f2a5d679a324 100644
>>>>> --- a/drivers/of/platform.c
>>>>> +++ b/drivers/of/platform.c
>>>>> @@ -525,6 +525,7 @@ 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 ret;
>>>>> /* Check if we have a MacOS display without a node spec */
>>>>> @@ -561,10 +562,15 @@ static int __init of_platform_default_populate_init(void)
>>>>> boot_display = node;
>>>>> break;
>>>>> }
>>>>> +
>>>>> for_each_node_by_type(node, "display") {
>>>>> + char *buf[14];
Another issue here: This should simply be buf[14]; not a pointer. Is 14
chars enough for the string plus a full number?
>>>>> if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
>>>>> continue;
>>>>> - of_platform_device_create(node, "of-display", NULL);
>>>>> + ret = snprintf(buf, "of-display-%d", display_number++);
The second argument to snprintf() is sizeof(buf); the number of
characters in buf.
>>
>> Platform devices use a single dot (.) as separator before the index.
>> Counting starts at zero. See /sys/bus/platform/ for examples. Can we please
>> stick with that scheme? Generated names would then be of-display.0,
>> of-display.1, etc.
>
> Yes, there was surprisingly no bikeshedding.
>
> Do we also want to change the name of the device that did manage to
> instantiate before?
>
> This scheme changes the name only for those that did not in the past,
> hence "of-display" and "of-display-%d", starting from 1.
I find that very confusing. It is is better to count all devices from 0.
I don't expect this to be an issue for userspace. But if necessary,
devfs can create softlinks to of-display.0.
Best regards
Thomas
>
> Sure, replacing '-' with '.' is easy enough, and using the same format
> for both as well.
>
> Thanks
>
> Michal
>
>>
>> Best regards
>> Thomas
>>
>>
>>
>>>>> + if (ret >= sizeof(buf))
>>>>> + continue;
>>>>> + of_platform_device_create(node, buf, NULL);
>>>>> }
>>>>> } else {
>>>>> --
>>>>> 2.35.3
>>>>>
>>>>
>>>> Thank you for the patch Michal!
>>>>
>>>> It applies on 6.2-rc4 but I get this build error with my config:
>>>
>>> Indeed, it's doubly bad.
>>>
>>> Where is the kernel test robot when you need it?
>>>
>>> It should not be that easy to miss this file but clearly it can happen.
>>>
>>> I will send a fixup.
>>>
>>> Sorry about the mess.
>>>
>>> Thanks
>>>
>>> Michal
>>
>> --
>> 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
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 <[email protected]>
---
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];
+ char *of_display_format = "of-display.%d";
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;
+ 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);
--
2.35.3
Am 19.01.23 um 10:53 schrieb Michal Suchanek:
> 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 <[email protected]>
Reviewed-by: Thomas Zimmermann <[email protected]>
Thanks again for taking care of this issue.
> ---
> 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];
> + char *of_display_format = "of-display.%d";
> 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;
> + 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
Hello,
On Thu, Jan 19, 2023 at 10:24:07AM +0000, Christophe Leroy wrote:
>
>
> 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 <[email protected]>
> > ---
> > 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 ?
No, there are two for blocks.
>
> > + char *of_display_format = "of-display.%d";
>
> Should be const ?
Yes, could be.
>
> > 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 ?
It would be a bit fragile that way.
The buffer would have to theoretically accomodate
"of-display.-9223372036854775808", and any change to the format requires
recalculating the length, by hand.
Of course, the memory would run out way before allocating that many
devices so it's kind of pointless to try and accomodate all possible
device numbers.
>
> 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
Yes, that is quite possible. Nonetheless, just like 'ret' generic
variable names also work.
Thanks
Michal
On Thu, 19 Jan 2023 10:53:23 +0100
Michal Suchanek <[email protected]> wrote:
> 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 <[email protected]>
> ---
> v2: Update the device name format
Hi Michal!
Just tested your 'of: Make of framebuffer devices unique' + 'v2 of: Fix of platform build on powerpc due to bad of disaply code' on my G4 and can confirm they fix the original issue (https://bugzilla.kernel.org/show_bug.cgi?id=216095). Thanks!
Also ofdrm gets loaded now without error messages:
# modprobe -v ofdrm
insmod /lib/modules/6.2.0-rc4-PMacG4+/kernel/drivers/video/fbdev/core/cfbcopyarea.ko
insmod /lib/modules/6.2.0-rc4-PMacG4+/kernel/drivers/video/fbdev/core/sysimgblt.ko
insmod /lib/modules/6.2.0-rc4-PMacG4+/kernel/drivers/video/fbdev/core/sysfillrect.ko
insmod /lib/modules/6.2.0-rc4-PMacG4+/kernel/drivers/video/fbdev/core/cfbimgblt.ko
insmod /lib/modules/6.2.0-rc4-PMacG4+/kernel/drivers/video/fbdev/core/syscopyarea.ko
insmod /lib/modules/6.2.0-rc4-PMacG4+/kernel/drivers/video/fbdev/core/cfbfillrect.ko
insmod /lib/modules/6.2.0-rc4-PMacG4+/kernel/drivers/gpu/drm/drm_kms_helper.ko
insmod /lib/modules/6.2.0-rc4-PMacG4+/kernel/drivers/gpu/drm/drm_shmem_helper.ko
insmod /lib/modules/6.2.0-rc4-PMacG4+/kernel/drivers/gpu/drm/tiny/ofdrm.ko
However I get no monitor output yet, despite ofdrm is loaded:
# lsmod | grep -i drm
ofdrm 9736 0
drm_shmem_helper 5704 1 ofdrm
drm_kms_helper 101736 1 ofdrm
cfbfillrect 2896 1 drm_kms_helper
syscopyarea 2400 1 drm_kms_helper
cfbimgblt 2256 1 drm_kms_helper
sysfillrect 2920 1 drm_kms_helper
sysimgblt 2296 1 drm_kms_helper
cfbcopyarea 2376 1 drm_kms_helper
drm 288960 3 drm_shmem_helper,ofdrm,drm_kms_helper
drm_panel_orientation_quirks 16 1 drm
I use DRM [=m], DRM_OFDRM [=m], DRM_RADEON [=n], DRM_FBDEV_EMULATION [=y], FB [=y] and FB_OF [=n] in my kernel test .config. As far as I understand DRM_OFDRM and FB_OF would be mutually exclusive? Also does not seem to make a difference whether FB_SIMPLE [=y] is set.
Regards,
Erhard
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 <[email protected]>
>> ---
>> 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
>
>> 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
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 <[email protected]>
> > > ---
> > > 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.
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
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 <[email protected]>
>>>> ---
>>>> 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
On Thu, Jan 19, 2023 at 3:53 AM Michal Suchanek <[email protected]> wrote:
>
> 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 <[email protected]>
> ---
> v2: Update the device name format
> ---
> drivers/of/platform.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
As this is the only fix I have queued, if you respin, send the
original fix with fixes squashed.
Rob
Hello,
On Thu, Jan 19, 2023 at 04:20:57PM +0100, Thomas Zimmermann wrote:
> 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 <[email protected]>
> > > > > ---
> > > > > 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.
This sounds a lot like an exemplar case of premature optimization.
A simplistic compiler might do exactly what you say, and allocate a slot
for the variable on the stack the moment the function is entered.
However, in real compilers there is no stack pressure from having a
local variable:
- the compiler can put the variable into a register
- it can completely omit the variable before and after it's actually
used which is that specific function call
> 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
Provided that the char pointer ever exists in the compiled code. Its
address is not taken so it does not need to.
> 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.
If it could not detect it there would be a lot of trouble all around.
Thanks
Michal
Hi
Am 20.01.23 um 12:27 schrieb Michal Suchánek:
> Hello,
>
> On Thu, Jan 19, 2023 at 04:20:57PM +0100, Thomas Zimmermann wrote:
>> 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 <[email protected]>
>>>>>> ---
>>>>>> 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.
>
> This sounds a lot like an exemplar case of premature optimization.
> A simplistic compiler might do exactly what you say, and allocate a slot
> for the variable on the stack the moment the function is entered.
>
> However, in real compilers there is no stack pressure from having a
> local variable:
> - the compiler can put the variable into a register
> - it can completely omit the variable before and after it's actually
> used which is that specific function call
>
>> 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
>
> Provided that the char pointer ever exists in the compiled code. Its
> address is not taken so it does not need to.
>
>> 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.
>
> If it could not detect it there would be a lot of trouble all around.
The issues definitely exist in userspace code. Kernel modules are
simpler, so compiler optimization is easier.
But I'm not really trying to make a technical argument. My point here is
that someone might read your code and duplicate the pattern. That's not
unreasonable: it's core Linux code, so it can be assumed to be good (or
at least not bad). But your current code teaches the reader a bad
practices, which should be avoided. It is better to do the correct
thing, even if it makes no difference to the compiled code.
Best regards
Thomas
>
> Thanks
>
> Michal
--
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
On Fri, Jan 20, 2023 at 12:39:23PM +0100, Thomas Zimmermann wrote:
> Hi
>
> Am 20.01.23 um 12:27 schrieb Michal Such?nek:
> > Hello,
> >
> > On Thu, Jan 19, 2023 at 04:20:57PM +0100, Thomas Zimmermann wrote:
> > > 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 <[email protected]>
> > > > > > > ---
> > > > > > > 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.
> >
> > This sounds a lot like an exemplar case of premature optimization.
> > A simplistic compiler might do exactly what you say, and allocate a slot
> > for the variable on the stack the moment the function is entered.
> >
> > However, in real compilers there is no stack pressure from having a
> > local variable:
> > - the compiler can put the variable into a register
> > - it can completely omit the variable before and after it's actually
> > used which is that specific function call
> >
> > > 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
> >
> > Provided that the char pointer ever exists in the compiled code. Its
> > address is not taken so it does not need to.
> >
> > > 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.
> >
> > If it could not detect it there would be a lot of trouble all around.
>
> The issues definitely exist in userspace code. Kernel modules are simpler,
> so compiler optimization is easier.
>
> But I'm not really trying to make a technical argument. My point here is
> that someone might read your code and duplicate the pattern. That's not
> unreasonable: it's core Linux code, so it can be assumed to be good (or at
> least not bad). But your current code teaches the reader a bad practices,
> which should be avoided. It is better to do the correct thing, even if it
> makes no difference to the compiled code.
The point I am trying to get across is that besides the original
objection about missing 'const' this code is not bad. Loading a string
constant address into a local variable and passing it as function call
argument is perfectly fine.
If you get any advantage by the alternate convoluted construct it's
more likely than anything else a bug in the compiler you are using.
It may be necessary to work around such bugs in performance-critical
code but not in driver probing code that runs exactly once during boot.
Thanks
Michal
On Thu, Jan 19, 2023 at 11:34:46AM +0100, Michal Such?nek wrote:
> Hello,
>
> On Thu, Jan 19, 2023 at 10:24:07AM +0000, Christophe Leroy wrote:
> >
> >
> > 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 <[email protected]>
> > > ---
> > > 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 ?
>
> No, there are two for blocks.
>
> >
> > > + char *of_display_format = "of-display.%d";
> >
> > Should be const ?
>
> Yes, could be.
>
> >
> > > 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 ?
>
> It would be a bit fragile that way.
>
> The buffer would have to theoretically accomodate
> "of-display.-9223372036854775808", and any change to the format requires
> recalculating the length, by hand.
>
> Of course, the memory would run out way before allocating that many
> devices so it's kind of pointless to try and accomodate all possible
> device numbers.
>
> >
> > 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
>
> Yes, that is quite possible. Nonetheless, just like 'ret' generic
> variable names also work.
And in fact judicious use of short generic variable names is more
readeable than naming all variables foobar_* as far as I am concerned.
Of course, YMMV.
Thanks
Michal
On Thu, Jan 19, 2023 at 3:53 AM Michal Suchanek <[email protected]> wrote:
>
> 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 <[email protected]>
> ---
> 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];
> + char *of_display_format = "of-display.%d";
static const as suggested and can we just move on please...
> 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++);
The boot display is always "of-display.0". Just use the fixed string
here. Then we can get rid of the whole debate around static const.
> + if (ret >= sizeof(buf))
> + continue;
This only happens if display_number becomes too big. Why continue on?
The next iteration will fail too.
> + 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;
Here too in the original change.
> of_platform_device_create(node, buf, NULL);
> --
> 2.35.3
>
Since Linux 5.19 this error is observed:
sysfs: cannot create duplicate filename '/devices/platform/of-display'
This is because multiple devices with the same name 'of-display' are
created on the same bus.
Update the code to create numbered device names for the non-boot
disaplay.
cc: [email protected]
References: https://bugzilla.kernel.org/show_bug.cgi?id=216095
Fixes: 52b1b46c39ae ("of: Create platform devices for OF framebuffers")
Reported-by: Erhard F. <[email protected]>
Suggested-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Michal Suchanek <[email protected]>
---
v3:
- merge fix into original patch
- Update the device name format
- add missing const
- do not continue with iterating display devices when formatting device
name fails
---
drivers/of/platform.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 81c8c227ab6b..4d3a4d9f79f2 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -525,6 +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 = 0;
+ char buf[14];
+ const char *of_display_format = "of-display.%d";
int ret;
/* Check if we have a MacOS display without a node spec */
@@ -555,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))
+ return -EOVERFLOW;
+ dev = of_platform_device_create(node, buf, NULL);
if (WARN_ON(!dev))
return -ENOMEM;
boot_display = node;
@@ -564,7 +570,10 @@ static int __init of_platform_default_populate_init(void)
for_each_node_by_type(node, "display") {
if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
continue;
- of_platform_device_create(node, "of-display", NULL);
+ ret = snprintf(buf, sizeof(buf), of_display_format, display_number++);
+ if (ret >= sizeof(buf))
+ break;
+ of_platform_device_create(node, buf, NULL);
}
} else {
--
2.35.3
Hello,
On Fri, Jan 20, 2023 at 11:23:39AM -0600, Rob Herring wrote:
> On Thu, Jan 19, 2023 at 3:53 AM Michal Suchanek <[email protected]> wrote:
> >
> > 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 <[email protected]>
> > ---
> > 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];
> > + char *of_display_format = "of-display.%d";
>
> static const as suggested and can we just move on please...
Only const, static could be dodgy
> > 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++);
>
> The boot display is always "of-display.0". Just use the fixed string
> here. Then we can get rid of the whole debate around static const.
I prefer to use the same format string when the names should be
consistent. Also it would resurrect the starting from 1 debate.
But if you really want to have two strings I do not care all that much.
>
> > + if (ret >= sizeof(buf))
> > + continue;
>
> This only happens if display_number becomes too big. Why continue on?
> The next iteration will fail too.
Yes, there is no need to continue with the loop.
Thanks
Michal
>
> > + 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;
>
> Here too in the original change.
>
> > of_platform_device_create(node, buf, NULL);
> > --
> > 2.35.3
> >
On Fri, 20 Jan 2023 19:09:57 +0100, Michal Suchanek wrote:
> Since Linux 5.19 this error is observed:
>
> sysfs: cannot create duplicate filename '/devices/platform/of-display'
>
> This is because multiple devices with the same name 'of-display' are
> created on the same bus.
>
> Update the code to create numbered device names for the non-boot
> disaplay.
>
> cc: [email protected]
> References: https://bugzilla.kernel.org/show_bug.cgi?id=216095
> Fixes: 52b1b46c39ae ("of: Create platform devices for OF framebuffers")
> Reported-by: Erhard F. <[email protected]>
> Suggested-by: Thomas Zimmermann <[email protected]>
> Signed-off-by: Michal Suchanek <[email protected]>
> ---
> v3:
> - merge fix into original patch
> - Update the device name format
> - add missing const
> - do not continue with iterating display devices when formatting device
> name fails
> ---
> drivers/of/platform.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
Applied, thanks!