| Message ID | 1415830124-28787-2-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Hans, On Thu, Nov 13, 2014 at 9:08 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Update simplefb to support the new preferred location for simplefb dt nodes > under /chosen. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/video/fbdev/simplefb.c | 33 ++++++++++++++++++++++++++++++++- > 1 file changed, 32 insertions(+), 1 deletion(-) > > diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c > index cd96edd..be7d288 100644 > --- a/drivers/video/fbdev/simplefb.c > +++ b/drivers/video/fbdev/simplefb.c > @@ -27,6 +27,7 @@ > #include <linux/platform_data/simplefb.h> > #include <linux/platform_device.h> > #include <linux/clk-provider.h> > +#include <linux/of_platform.h> > > static struct fb_fix_screeninfo simplefb_fix = { > .id = "simple", > @@ -385,7 +386,37 @@ static struct platform_driver simplefb_driver = { > .probe = simplefb_probe, > .remove = simplefb_remove, > }; > -module_platform_driver(simplefb_driver); > + > +static int __init simplefb_init(void) > +{ > + int i, ret; > + char name[16]; > + struct device_node *np; > + > + ret = platform_driver_register(&simplefb_driver); > + if (ret) > + return ret; > + > + for (i = 0; ; i++) { > + snprintf(name, sizeof(name), "framebuffer%d", i); This smells like an infinite loop: we can be pretty sure that no hardware will ever exist with more than 9999 (I think?) framebuffers, however if that ever happens this'll loop until it runs out of RAM. Maybe add a suitably high limit to the for loop? Thanks,
On Wed, Nov 12, 2014 at 11:08 PM, Hans de Goede <hdegoede@redhat.com> wrote: > --- a/drivers/video/fbdev/simplefb.c > +++ b/drivers/video/fbdev/simplefb.c > @@ -385,7 +386,37 @@ static struct platform_driver simplefb_driver = { > .probe = simplefb_probe, > .remove = simplefb_remove, > }; > -module_platform_driver(simplefb_driver); > + > +static int __init simplefb_init(void) > +{ > + int i, ret; unsigned int i; > + char name[16]; > + struct device_node *np; > + > + ret = platform_driver_register(&simplefb_driver); > + if (ret) > + return ret; > + > + for (i = 0; ; i++) { Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 13, 2014 at 9:15 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Wed, Nov 12, 2014 at 11:08 PM, Hans de Goede <hdegoede@redhat.com> wrote: >> --- a/drivers/video/fbdev/simplefb.c >> +++ b/drivers/video/fbdev/simplefb.c > >> @@ -385,7 +386,37 @@ static struct platform_driver simplefb_driver = { >> .probe = simplefb_probe, >> .remove = simplefb_remove, >> }; >> -module_platform_driver(simplefb_driver); >> + >> +static int __init simplefb_init(void) >> +{ >> + int i, ret; > > unsigned int i; > >> + char name[16]; >> + struct device_node *np; >> + >> + ret = platform_driver_register(&simplefb_driver); >> + if (ret) >> + return ret; >> + >> + for (i = 0; ; i++) { >> + snprintf(name, sizeof(name), "framebuffer%d", i); and %u, of course. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 11/13/2014 12:39 AM, Julian Calaby wrote: > Hi Hans, > > On Thu, Nov 13, 2014 at 9:08 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> Update simplefb to support the new preferred location for simplefb dt nodes >> under /chosen. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/video/fbdev/simplefb.c | 33 ++++++++++++++++++++++++++++++++- >> 1 file changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c >> index cd96edd..be7d288 100644 >> --- a/drivers/video/fbdev/simplefb.c >> +++ b/drivers/video/fbdev/simplefb.c >> @@ -27,6 +27,7 @@ >> #include <linux/platform_data/simplefb.h> >> #include <linux/platform_device.h> >> #include <linux/clk-provider.h> >> +#include <linux/of_platform.h> >> >> static struct fb_fix_screeninfo simplefb_fix = { >> .id = "simple", >> @@ -385,7 +386,37 @@ static struct platform_driver simplefb_driver = { >> .probe = simplefb_probe, >> .remove = simplefb_remove, >> }; >> -module_platform_driver(simplefb_driver); >> + >> +static int __init simplefb_init(void) >> +{ >> + int i, ret; >> + char name[16]; >> + struct device_node *np; >> + >> + ret = platform_driver_register(&simplefb_driver); >> + if (ret) >> + return ret; >> + >> + for (i = 0; ; i++) { >> + snprintf(name, sizeof(name), "framebuffer%d", i); > > This smells like an infinite loop: we can be pretty sure that no > hardware will ever exist with more than 9999 (I think?) framebuffers, > however if that ever happens this'll loop until it runs out of RAM. > Maybe add a suitably high limit to the for loop? The loop will stop as soon as there are no more framebuffer# nodes in chosen, so the loop is only infinite if there are infinite nodes in the devicetree, which would make the devicetree infinitely large, so this will never happen. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On 11/13/2014 09:16 AM, Geert Uytterhoeven wrote: > On Thu, Nov 13, 2014 at 9:15 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Wed, Nov 12, 2014 at 11:08 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>> --- a/drivers/video/fbdev/simplefb.c >>> +++ b/drivers/video/fbdev/simplefb.c >> >>> @@ -385,7 +386,37 @@ static struct platform_driver simplefb_driver = { >>> .probe = simplefb_probe, >>> .remove = simplefb_remove, >>> }; >>> -module_platform_driver(simplefb_driver); >>> + >>> +static int __init simplefb_init(void) >>> +{ >>> + int i, ret; >> >> unsigned int i; Although you're right that it will never become negative, making this unsigned really gains us nothing, and it is normal C programming practice to use signed ints to loop over arrays even if the array index never becomes negative. So I see no reason to change this. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Thu, Nov 13, 2014 at 9:27 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>> + char name[16]; >>> + struct device_node *np; >>> + >>> + ret = platform_driver_register(&simplefb_driver); >>> + if (ret) >>> + return ret; >>> + >>> + for (i = 0; ; i++) { >>> + snprintf(name, sizeof(name), "framebuffer%d", i); >> >> This smells like an infinite loop: we can be pretty sure that no >> hardware will ever exist with more than 9999 (I think?) framebuffers, >> however if that ever happens this'll loop until it runs out of RAM. >> Maybe add a suitably high limit to the for loop? > > The loop will stop as soon as there are no more framebuffer# nodes in chosen, > so the loop is only infinite if there are infinite nodes in the devicetree, > which would make the devicetree infinitely large, so this will never happen. If there are 10000 frame buffers, the loop will continue beyond that, as the index will be truncated to 4 digits due to the size of name[]. It will stop when (signed) i becomes negative, though ;-) One solution is to increase the size of name[], another to use kasprintf(). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 11/13/2014 09:34 AM, Geert Uytterhoeven wrote: > Hi Hans, > > On Thu, Nov 13, 2014 at 9:27 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> + char name[16]; >>>> + struct device_node *np; >>>> + >>>> + ret = platform_driver_register(&simplefb_driver); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + for (i = 0; ; i++) { >>>> + snprintf(name, sizeof(name), "framebuffer%d", i); >>> >>> This smells like an infinite loop: we can be pretty sure that no >>> hardware will ever exist with more than 9999 (I think?) framebuffers, >>> however if that ever happens this'll loop until it runs out of RAM. >>> Maybe add a suitably high limit to the for loop? >> >> The loop will stop as soon as there are no more framebuffer# nodes in chosen, >> so the loop is only infinite if there are infinite nodes in the devicetree, >> which would make the devicetree infinitely large, so this will never happen. > > If there are 10000 frame buffers, the loop will continue beyond that, > as the index will be truncated to 4 digits due to the size of name[]. > It will stop when (signed) i becomes negative, though ;-) > > One solution is to increase the size of name[], This is probably never going to happen, but I'll increase the size of name in v2 anyways. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Thu, Nov 13, 2014 at 7:40 PM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 11/13/2014 09:34 AM, Geert Uytterhoeven wrote: >> Hi Hans, >> >> On Thu, Nov 13, 2014 at 9:27 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>> + char name[16]; >>>>> + struct device_node *np; >>>>> + >>>>> + ret = platform_driver_register(&simplefb_driver); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + for (i = 0; ; i++) { >>>>> + snprintf(name, sizeof(name), "framebuffer%d", i); >>>> >>>> This smells like an infinite loop: we can be pretty sure that no >>>> hardware will ever exist with more than 9999 (I think?) framebuffers, >>>> however if that ever happens this'll loop until it runs out of RAM. >>>> Maybe add a suitably high limit to the for loop? >>> >>> The loop will stop as soon as there are no more framebuffer# nodes in chosen, >>> so the loop is only infinite if there are infinite nodes in the devicetree, >>> which would make the devicetree infinitely large, so this will never happen. >> >> If there are 10000 frame buffers, the loop will continue beyond that, >> as the index will be truncated to 4 digits due to the size of name[]. >> It will stop when (signed) i becomes negative, though ;-) >> >> One solution is to increase the size of name[], > > This is probably never going to happen, but I'll increase the size of name in v2 > anyways. You could also just limit it to 100 or something like that. Thanks,
On Wed, Nov 12, 2014 at 11:39 PM, Julian Calaby <julian.calaby@gmail.com> wrote: > Hi Hans, > > On Thu, Nov 13, 2014 at 9:08 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> Update simplefb to support the new preferred location for simplefb dt nodes >> under /chosen. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/video/fbdev/simplefb.c | 33 ++++++++++++++++++++++++++++++++- >> 1 file changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c >> index cd96edd..be7d288 100644 >> --- a/drivers/video/fbdev/simplefb.c >> +++ b/drivers/video/fbdev/simplefb.c >> @@ -27,6 +27,7 @@ >> #include <linux/platform_data/simplefb.h> >> #include <linux/platform_device.h> >> #include <linux/clk-provider.h> >> +#include <linux/of_platform.h> >> >> static struct fb_fix_screeninfo simplefb_fix = { >> .id = "simple", >> @@ -385,7 +386,37 @@ static struct platform_driver simplefb_driver = { >> .probe = simplefb_probe, >> .remove = simplefb_remove, >> }; >> -module_platform_driver(simplefb_driver); >> + >> +static int __init simplefb_init(void) >> +{ >> + int i, ret; >> + char name[16]; >> + struct device_node *np; >> + >> + ret = platform_driver_register(&simplefb_driver); >> + if (ret) >> + return ret; >> + >> + for (i = 0; ; i++) { >> + snprintf(name, sizeof(name), "framebuffer%d", i); > > This smells like an infinite loop: we can be pretty sure that no > hardware will ever exist with more than 9999 (I think?) framebuffers, > however if that ever happens this'll loop until it runs out of RAM. > Maybe add a suitably high limit to the for loop? Unlikely, but the loop is wrong anyway. The loop should be: for_each_child_of_node(of_chosen, child) if (of_device_is_compatible(child, "simple-framebuffer"); of_platform_device_create(np, NULL, NULL); Then make the probe hook choose an appropriate FB number. It looks like you structured the code the way you did to get the framebuffers to register in a particular order, and therefore implicitly get the right numbers, but that is a fragile way to go about it. Using /aliases really is the right way to get specific framebuffer numbers. We already use it for UARTs, so we should do the same here. Use of_alias_get_id(, "framebuffer") to get the framebuffer number. g. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c index cd96edd..be7d288 100644 --- a/drivers/video/fbdev/simplefb.c +++ b/drivers/video/fbdev/simplefb.c @@ -27,6 +27,7 @@ #include <linux/platform_data/simplefb.h> #include <linux/platform_device.h> #include <linux/clk-provider.h> +#include <linux/of_platform.h> static struct fb_fix_screeninfo simplefb_fix = { .id = "simple", @@ -385,7 +386,37 @@ static struct platform_driver simplefb_driver = { .probe = simplefb_probe, .remove = simplefb_remove, }; -module_platform_driver(simplefb_driver); + +static int __init simplefb_init(void) +{ + int i, ret; + char name[16]; + struct device_node *np; + + ret = platform_driver_register(&simplefb_driver); + if (ret) + return ret; + + for (i = 0; ; i++) { + snprintf(name, sizeof(name), "framebuffer%d", i); + np = of_find_node_by_name(of_chosen, name); + if (!np) + break; + + /* of_platform_device_create will check status for us */ + of_platform_device_create(np, NULL, NULL); + } + + return 0; +} + +static void __exit simplefb_exit(void) +{ + platform_driver_unregister(&simplefb_driver); +} + +module_init(simplefb_init); +module_exit(simplefb_exit); MODULE_AUTHOR("Stephen Warren <swarren@wwwdotorg.org>"); MODULE_DESCRIPTION("Simple framebuffer driver");
Update simplefb to support the new preferred location for simplefb dt nodes under /chosen. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/video/fbdev/simplefb.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)