[PATCH 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[PATCH 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Hans de Goede]

Since simplefb nodes do not relate directly to hw typically they have been
placed in the root of the devicetree. As the represent runtime information
having them as sub-nodes of /chosen is more logical, specify this.

Also specify when to set the chosen stdout-path property to a simplefb node.

For reliable handover to a hardware specific driver, that driver needs to
know which simplefb to unregister when taking over, specify how the hw driver
can find the matching simplefb node.

Last add some advice on how to fill and use simplefb nodes from a firmware
pov.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 .../bindings/video/simple-framebuffer.txt          | 37 +++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 8f35718..95fe284 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -4,6 +4,26 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
 the bootloader, with the assumption that the display hardware has already
 been set up to scan out from the memory pointed to by the reg property.
 
+Since simplefb nodes represent runtime information they must be sub-nodes of
+the chosen node (*). The primary display node must be named framebuffer0,
+additional nodes must be called framebuffer1, etc.
+
+If a simplefb node represents the preferred console for user interaction,
+then the chosen node's stdout-path property must point to it.
+
+If the devicetree contains nodes for the display hardware used by a simplefb,
+then one of the hw nodes must have a property called "framebuffer" pointing to
+the framebuffer# node in chosen, so that the operating system knows which
+simplefb to disable when handing over control to a driver for the real
+hardware. The bindings for the hw nodes must specify which node contains the
+framebuffer property.
+
+It is advised that devicetree files contain pre-filled, disabled framebuffer#
+nodes, so that the firmware only needs to update the mode information and
+enable them. This way if e.g. later on support for more display clocks get
+added, the simplefb nodes will already contain this info and the firmware
+does not need to be updated.
+
 Required properties:
 - compatible: "simple-framebuffer"
 - reg: Should contain the location and size of the framebuffer memory.
@@ -22,11 +42,26 @@ Optional properties:
 
 Example:
 
-	framebuffer {
+chosen {
+	framebuffer0 {
 		compatible = "simple-framebuffer";
 		reg = <0x1d385000 (1600 * 1200 * 2)>;
 		width = <1600>;
 		height = <1200>;
 		stride = <(1600 * 2)>;
 		format = "r5g6b5";
+		clocks = <&ahb_gates 36>, <&ahb_gates 43>, <&ahb_gates 44>;
 	};
+};
+
+soc@01c00000 {
+	lcdc0: lcdc@1c0c000 {
+		compatible = "allwinner,sun4i-a10-lcdc";
+		framebuffer = <&framebuffer0>;
+	};
+};
+
+
+*) Older devicetree files may have a compatible = "simple-framebuffer" node
+in a different place, operating systems must first enumerate any framebuffer#
+nodes found under chosen and then check for other compatible nodes.
-- 
2.1.0

[PATCH 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Hans de Goede]

Since simplefb nodes do not relate directly to hw typically they have been
placed in the root of the devicetree. As the represent runtime information
having them as sub-nodes of /chosen is more logical, specify this.

Also specify when to set the chosen stdout-path property to a simplefb node.

For reliable handover to a hardware specific driver, that driver needs to
know which simplefb to unregister when taking over, specify how the hw driver
can find the matching simplefb node.

Last add some advice on how to fill and use simplefb nodes from a firmware
pov.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../bindings/video/simple-framebuffer.txt          | 37 +++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 8f35718..95fe284 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -4,6 +4,26 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
 the bootloader, with the assumption that the display hardware has already
 been set up to scan out from the memory pointed to by the reg property.
 
+Since simplefb nodes represent runtime information they must be sub-nodes of
+the chosen node (*). The primary display node must be named framebuffer0,
+additional nodes must be called framebuffer1, etc.
+
+If a simplefb node represents the preferred console for user interaction,
+then the chosen node's stdout-path property must point to it.
+
+If the devicetree contains nodes for the display hardware used by a simplefb,
+then one of the hw nodes must have a property called "framebuffer" pointing to
+the framebuffer# node in chosen, so that the operating system knows which
+simplefb to disable when handing over control to a driver for the real
+hardware. The bindings for the hw nodes must specify which node contains the
+framebuffer property.
+
+It is advised that devicetree files contain pre-filled, disabled framebuffer#
+nodes, so that the firmware only needs to update the mode information and
+enable them. This way if e.g. later on support for more display clocks get
+added, the simplefb nodes will already contain this info and the firmware
+does not need to be updated.
+
 Required properties:
 - compatible: "simple-framebuffer"
 - reg: Should contain the location and size of the framebuffer memory.
@@ -22,11 +42,26 @@ Optional properties:
 
 Example:
 
-	framebuffer {
+chosen {
+	framebuffer0 {
 		compatible = "simple-framebuffer";
 		reg = <0x1d385000 (1600 * 1200 * 2)>;
 		width = <1600>;
 		height = <1200>;
 		stride = <(1600 * 2)>;
 		format = "r5g6b5";
+		clocks = <&ahb_gates 36>, <&ahb_gates 43>, <&ahb_gates 44>;
 	};
+};
+
+soc@01c00000 {
+	lcdc0: lcdc@1c0c000 {
+		compatible = "allwinner,sun4i-a10-lcdc";
+		framebuffer = <&framebuffer0>;
+	};
+};
+
+
+*) Older devicetree files may have a compatible = "simple-framebuffer" node
+in a different place, operating systems must first enumerate any framebuffer#
+nodes found under chosen and then check for other compatible nodes.
-- 
2.1.0

[PATCH 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Hans de Goede]

Since simplefb nodes do not relate directly to hw typically they have been
placed in the root of the devicetree. As the represent runtime information
having them as sub-nodes of /chosen is more logical, specify this.

Also specify when to set the chosen stdout-path property to a simplefb node.

For reliable handover to a hardware specific driver, that driver needs to
know which simplefb to unregister when taking over, specify how the hw driver
can find the matching simplefb node.

Last add some advice on how to fill and use simplefb nodes from a firmware
pov.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../bindings/video/simple-framebuffer.txt          | 37 +++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 8f35718..95fe284 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -4,6 +4,26 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
 the bootloader, with the assumption that the display hardware has already
 been set up to scan out from the memory pointed to by the reg property.
 
+Since simplefb nodes represent runtime information they must be sub-nodes of
+the chosen node (*). The primary display node must be named framebuffer0,
+additional nodes must be called framebuffer1, etc.
+
+If a simplefb node represents the preferred console for user interaction,
+then the chosen node's stdout-path property must point to it.
+
+If the devicetree contains nodes for the display hardware used by a simplefb,
+then one of the hw nodes must have a property called "framebuffer" pointing to
+the framebuffer# node in chosen, so that the operating system knows which
+simplefb to disable when handing over control to a driver for the real
+hardware. The bindings for the hw nodes must specify which node contains the
+framebuffer property.
+
+It is advised that devicetree files contain pre-filled, disabled framebuffer#
+nodes, so that the firmware only needs to update the mode information and
+enable them. This way if e.g. later on support for more display clocks get
+added, the simplefb nodes will already contain this info and the firmware
+does not need to be updated.
+
 Required properties:
 - compatible: "simple-framebuffer"
 - reg: Should contain the location and size of the framebuffer memory.
@@ -22,11 +42,26 @@ Optional properties:
 
 Example:
 
-	framebuffer {
+chosen {
+	framebuffer0 {
 		compatible = "simple-framebuffer";
 		reg = <0x1d385000 (1600 * 1200 * 2)>;
 		width = <1600>;
 		height = <1200>;
 		stride = <(1600 * 2)>;
 		format = "r5g6b5";
+		clocks = <&ahb_gates 36>, <&ahb_gates 43>, <&ahb_gates 44>;
 	};
+};
+
+soc at 01c00000 {
+	lcdc0: lcdc at 1c0c000 {
+		compatible = "allwinner,sun4i-a10-lcdc";
+		framebuffer = <&framebuffer0>;
+	};
+};
+
+
+*) Older devicetree files may have a compatible = "simple-framebuffer" node
+in a different place, operating systems must first enumerate any framebuffer#
+nodes found under chosen and then check for other compatible nodes.
-- 
2.1.0

[PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Hans de Goede]

Update simplefb to support the new preferred location for simplefb dt nodes
under /chosen.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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);
+		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-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>");
 MODULE_DESCRIPTION("Simple framebuffer driver");
-- 
2.1.0

[PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Hans de Goede]

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);
+		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");
-- 
2.1.0

[PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Hans de Goede]

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);
+		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");
-- 
2.1.0

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

One of the reasons for having the simplefb nodes in /chosen, and doing
explicit enumeration of the nodes there, is too allow enumerating them sooner,
so that we get a console earlier on.

Doing this earlier then fs_initcall is not useful, since the fb only turns into
a console when fbcon intializes, which is a fs_initcall too.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/video/fbdev/simplefb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index be7d288..8c0c972 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
 	platform_driver_unregister(&simplefb_driver);
 }
 
-module_init(simplefb_init);
+/*
+ * While this can be a module, if builtin it's most likely the console
+ * So let's leave module_exit but move module_init to an earlier place
+ */
+fs_initcall(simplefb_init);
 module_exit(simplefb_exit);
 
 MODULE_AUTHOR("Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>");
-- 
2.1.0

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

One of the reasons for having the simplefb nodes in /chosen, and doing
explicit enumeration of the nodes there, is too allow enumerating them sooner,
so that we get a console earlier on.

Doing this earlier then fs_initcall is not useful, since the fb only turns into
a console when fbcon intializes, which is a fs_initcall too.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/video/fbdev/simplefb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index be7d288..8c0c972 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
 	platform_driver_unregister(&simplefb_driver);
 }
 
-module_init(simplefb_init);
+/*
+ * While this can be a module, if builtin it's most likely the console
+ * So let's leave module_exit but move module_init to an earlier place
+ */
+fs_initcall(simplefb_init);
 module_exit(simplefb_exit);
 
 MODULE_AUTHOR("Stephen Warren <swarren@wwwdotorg.org>");
-- 
2.1.0

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

One of the reasons for having the simplefb nodes in /chosen, and doing
explicit enumeration of the nodes there, is too allow enumerating them sooner,
so that we get a console earlier on.

Doing this earlier then fs_initcall is not useful, since the fb only turns into
a console when fbcon intializes, which is a fs_initcall too.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/video/fbdev/simplefb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index be7d288..8c0c972 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
 	platform_driver_unregister(&simplefb_driver);
 }
 
-module_init(simplefb_init);
+/*
+ * While this can be a module, if builtin it's most likely the console
+ * So let's leave module_exit but move module_init to an earlier place
+ */
+fs_initcall(simplefb_init);
 module_exit(simplefb_exit);
 
 MODULE_AUTHOR("Stephen Warren <swarren@wwwdotorg.org>");
-- 
2.1.0

[PATCH 4/4] fbcon: Change fbcon_init from module_init to fs_initcall

[Hans de Goede]

Various fb drivers register themselves before module_init so as to have a
console as early as possible, this is of little use if fbcon does not initialze
early too.

Fbcon cannot initialize earlier then fs_initcall, because then the creation
of /sys/class/graphics/fbcon will fail.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/video/console/fbcon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index eb976ee..ea43724 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -3624,7 +3624,7 @@ static int __init fb_console_init(void)
 	return 0;
 }
 
-module_init(fb_console_init);
+fs_initcall(fb_console_init);
 
 #ifdef MODULE
 
-- 
2.1.0

[PATCH 4/4] fbcon: Change fbcon_init from module_init to fs_initcall

[Hans de Goede]

Various fb drivers register themselves before module_init so as to have a
console as early as possible, this is of little use if fbcon does not initialze
early too.

Fbcon cannot initialize earlier then fs_initcall, because then the creation
of /sys/class/graphics/fbcon will fail.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/video/console/fbcon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index eb976ee..ea43724 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -3624,7 +3624,7 @@ static int __init fb_console_init(void)
 	return 0;
 }
 
-module_init(fb_console_init);
+fs_initcall(fb_console_init);
 
 #ifdef MODULE
 
-- 
2.1.0

[PATCH 4/4] fbcon: Change fbcon_init from module_init to fs_initcall

[Hans de Goede]

Various fb drivers register themselves before module_init so as to have a
console as early as possible, this is of little use if fbcon does not initialze
early too.

Fbcon cannot initialize earlier then fs_initcall, because then the creation
of /sys/class/graphics/fbcon will fail.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/video/console/fbcon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index eb976ee..ea43724 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -3624,7 +3624,7 @@ static int __init fb_console_init(void)
 	return 0;
 }
 
-module_init(fb_console_init);
+fs_initcall(fb_console_init);
 
 #ifdef MODULE
 
-- 
2.1.0

Re: [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Julian Calaby]

Hi Hans,

On Thu, Nov 13, 2014 at 9:08 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Update simplefb to support the new preferred location for simplefb dt nodes
> under /chosen.
>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  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,

-- 
Julian Calaby

Email: julian.calaby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Profile: http://www.google.com/profiles/julian.calaby/

Re: [linux-sunxi] [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Julian Calaby]

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,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

[linux-sunxi] [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Julian Calaby]

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,

-- 
Julian Calaby

Email: julian.calaby at gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Geert Uytterhoeven]

On Wed, Nov 12, 2014 at 11:08 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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

Re: [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Geert Uytterhoeven]

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

[PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Geert Uytterhoeven]

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 at 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

Re: [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Geert Uytterhoeven]

On Thu, Nov 13, 2014 at 9:15 AM, Geert Uytterhoeven
<geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> On Wed, Nov 12, 2014 at 11:08 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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

Re: [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Geert Uytterhoeven]

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

[PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Geert Uytterhoeven]

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 at 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

Re: [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Hans de Goede]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> Update simplefb to support the new preferred location for simplefb dt nodes
>> under /chosen.
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  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

Re: [linux-sunxi] [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Hans de Goede]

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

[linux-sunxi] [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Hans de Goede]

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

Re: [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Hans de Goede]

Hi Geert,

On 11/13/2014 09:16 AM, Geert Uytterhoeven wrote:
> On Thu, Nov 13, 2014 at 9:15 AM, Geert Uytterhoeven
> <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>> On Wed, Nov 12, 2014 at 11:08 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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

Re: [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Hans de Goede]

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

[PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Hans de Goede]

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

Re: [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Geert Uytterhoeven]

Hi Hans,

On Thu, Nov 13, 2014 at 9:27 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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

Re: [linux-sunxi] [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Geert Uytterhoeven]

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

[linux-sunxi] [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Geert Uytterhoeven]

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 at 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

Re: [PATCH 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Geert Uytterhoeven]

On Wed, Nov 12, 2014 at 11:08 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Since simplefb nodes do not relate directly to hw typically they have been
> placed in the root of the devicetree. As the represent runtime information
> having them as sub-nodes of /chosen is more logical, specify this.
>
> Also specify when to set the chosen stdout-path property to a simplefb node.
>
> For reliable handover to a hardware specific driver, that driver needs to
> know which simplefb to unregister when taking over, specify how the hw driver
> can find the matching simplefb node.
>
> Last add some advice on how to fill and use simplefb nodes from a firmware
> pov.
>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>

> ---
>  .../bindings/video/simple-framebuffer.txt          | 37 +++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> index 8f35718..95fe284 100644
> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> @@ -4,6 +4,26 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
>  the bootloader, with the assumption that the display hardware has already
>  been set up to scan out from the memory pointed to by the reg property.
>
> +Since simplefb nodes represent runtime information they must be sub-nodes of
> +the chosen node (*). The primary display node must be named framebuffer0,
> +additional nodes must be called framebuffer1, etc.
> +
> +If a simplefb node represents the preferred console for user interaction,
> +then the chosen node's stdout-path property must point to it.

You may want to add an stdout-path property to the example.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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

Re: [PATCH 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Geert Uytterhoeven]

On Wed, Nov 12, 2014 at 11:08 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Since simplefb nodes do not relate directly to hw typically they have been
> placed in the root of the devicetree. As the represent runtime information
> having them as sub-nodes of /chosen is more logical, specify this.
>
> Also specify when to set the chosen stdout-path property to a simplefb node.
>
> For reliable handover to a hardware specific driver, that driver needs to
> know which simplefb to unregister when taking over, specify how the hw driver
> can find the matching simplefb node.
>
> Last add some advice on how to fill and use simplefb nodes from a firmware
> pov.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

> ---
>  .../bindings/video/simple-framebuffer.txt          | 37 +++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> index 8f35718..95fe284 100644
> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> @@ -4,6 +4,26 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
>  the bootloader, with the assumption that the display hardware has already
>  been set up to scan out from the memory pointed to by the reg property.
>
> +Since simplefb nodes represent runtime information they must be sub-nodes of
> +the chosen node (*). The primary display node must be named framebuffer0,
> +additional nodes must be called framebuffer1, etc.
> +
> +If a simplefb node represents the preferred console for user interaction,
> +then the chosen node's stdout-path property must point to it.

You may want to add an stdout-path property to the example.

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

[PATCH 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Geert Uytterhoeven]

On Wed, Nov 12, 2014 at 11:08 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Since simplefb nodes do not relate directly to hw typically they have been
> placed in the root of the devicetree. As the represent runtime information
> having them as sub-nodes of /chosen is more logical, specify this.
>
> Also specify when to set the chosen stdout-path property to a simplefb node.
>
> For reliable handover to a hardware specific driver, that driver needs to
> know which simplefb to unregister when taking over, specify how the hw driver
> can find the matching simplefb node.
>
> Last add some advice on how to fill and use simplefb nodes from a firmware
> pov.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

> ---
>  .../bindings/video/simple-framebuffer.txt          | 37 +++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> index 8f35718..95fe284 100644
> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> @@ -4,6 +4,26 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
>  the bootloader, with the assumption that the display hardware has already
>  been set up to scan out from the memory pointed to by the reg property.
>
> +Since simplefb nodes represent runtime information they must be sub-nodes of
> +the chosen node (*). The primary display node must be named framebuffer0,
> +additional nodes must be called framebuffer1, etc.
> +
> +If a simplefb node represents the preferred console for user interaction,
> +then the chosen node's stdout-path property must point to it.

You may want to add an stdout-path property to the example.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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

Re: [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Hans de Goede]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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

Re: [linux-sunxi] [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Hans de Goede]

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

[linux-sunxi] [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Hans de Goede]

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

Re: [PATCH 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Hans de Goede]

Hi,

On 11/13/2014 09:38 AM, Geert Uytterhoeven wrote:
> On Wed, Nov 12, 2014 at 11:08 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> Since simplefb nodes do not relate directly to hw typically they have been
>> placed in the root of the devicetree. As the represent runtime information
>> having them as sub-nodes of /chosen is more logical, specify this.
>>
>> Also specify when to set the chosen stdout-path property to a simplefb node.
>>
>> For reliable handover to a hardware specific driver, that driver needs to
>> know which simplefb to unregister when taking over, specify how the hw driver
>> can find the matching simplefb node.
>>
>> Last add some advice on how to fill and use simplefb nodes from a firmware
>> pov.
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>

Thanks.

> 
>> ---
>>  .../bindings/video/simple-framebuffer.txt          | 37 +++++++++++++++++++++-
>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> index 8f35718..95fe284 100644
>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> @@ -4,6 +4,26 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
>>  the bootloader, with the assumption that the display hardware has already
>>  been set up to scan out from the memory pointed to by the reg property.
>>
>> +Since simplefb nodes represent runtime information they must be sub-nodes of
>> +the chosen node (*). The primary display node must be named framebuffer0,
>> +additional nodes must be called framebuffer1, etc.
>> +
>> +If a simplefb node represents the preferred console for user interaction,
>> +then the chosen node's stdout-path property must point to it.
> 
> You may want to add an stdout-path property to the example.

That is a good idea, done for v2.

Regards,

Hans

Re: [PATCH 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Hans de Goede]

Hi,

On 11/13/2014 09:38 AM, Geert Uytterhoeven wrote:
> On Wed, Nov 12, 2014 at 11:08 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Since simplefb nodes do not relate directly to hw typically they have been
>> placed in the root of the devicetree. As the represent runtime information
>> having them as sub-nodes of /chosen is more logical, specify this.
>>
>> Also specify when to set the chosen stdout-path property to a simplefb node.
>>
>> For reliable handover to a hardware specific driver, that driver needs to
>> know which simplefb to unregister when taking over, specify how the hw driver
>> can find the matching simplefb node.
>>
>> Last add some advice on how to fill and use simplefb nodes from a firmware
>> pov.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Thanks.

> 
>> ---
>>  .../bindings/video/simple-framebuffer.txt          | 37 +++++++++++++++++++++-
>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> index 8f35718..95fe284 100644
>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> @@ -4,6 +4,26 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
>>  the bootloader, with the assumption that the display hardware has already
>>  been set up to scan out from the memory pointed to by the reg property.
>>
>> +Since simplefb nodes represent runtime information they must be sub-nodes of
>> +the chosen node (*). The primary display node must be named framebuffer0,
>> +additional nodes must be called framebuffer1, etc.
>> +
>> +If a simplefb node represents the preferred console for user interaction,
>> +then the chosen node's stdout-path property must point to it.
> 
> You may want to add an stdout-path property to the example.

That is a good idea, done for v2.

Regards,

Hans

[PATCH 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Hans de Goede]

Hi,

On 11/13/2014 09:38 AM, Geert Uytterhoeven wrote:
> On Wed, Nov 12, 2014 at 11:08 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Since simplefb nodes do not relate directly to hw typically they have been
>> placed in the root of the devicetree. As the represent runtime information
>> having them as sub-nodes of /chosen is more logical, specify this.
>>
>> Also specify when to set the chosen stdout-path property to a simplefb node.
>>
>> For reliable handover to a hardware specific driver, that driver needs to
>> know which simplefb to unregister when taking over, specify how the hw driver
>> can find the matching simplefb node.
>>
>> Last add some advice on how to fill and use simplefb nodes from a firmware
>> pov.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Thanks.

> 
>> ---
>>  .../bindings/video/simple-framebuffer.txt          | 37 +++++++++++++++++++++-
>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> index 8f35718..95fe284 100644
>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> @@ -4,6 +4,26 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
>>  the bootloader, with the assumption that the display hardware has already
>>  been set up to scan out from the memory pointed to by the reg property.
>>
>> +Since simplefb nodes represent runtime information they must be sub-nodes of
>> +the chosen node (*). The primary display node must be named framebuffer0,
>> +additional nodes must be called framebuffer1, etc.
>> +
>> +If a simplefb node represents the preferred console for user interaction,
>> +then the chosen node's stdout-path property must point to it.
> 
> You may want to add an stdout-path property to the example.

That is a good idea, done for v2.

Regards,

Hans

Re: [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Julian Calaby]

Hi Hans,

On Thu, Nov 13, 2014 at 7:40 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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,

-- 
Julian Calaby

Email: julian.calaby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Profile: http://www.google.com/profiles/julian.calaby/

Re: [linux-sunxi] [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Julian Calaby]

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,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

[linux-sunxi] [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Julian Calaby]

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,

-- 
Julian Calaby

Email: julian.calaby at gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1394 bytes --]

On Wed, Nov 12, 2014 at 11:08:43PM +0100, Hans de Goede wrote:
> One of the reasons for having the simplefb nodes in /chosen, and doing
> explicit enumeration of the nodes there, is too allow enumerating them sooner,
> so that we get a console earlier on.
> 
> Doing this earlier then fs_initcall is not useful, since the fb only turns into
> a console when fbcon intializes, which is a fs_initcall too.
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/video/fbdev/simplefb.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index be7d288..8c0c972 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
>  	platform_driver_unregister(&simplefb_driver);
>  }
>  
> -module_init(simplefb_init);
> +/*
> + * While this can be a module, if builtin it's most likely the console
> + * So let's leave module_exit but move module_init to an earlier place
> + */

Not really related to this patch itself, but do we want to support
simplefb as a module? It seems like it's going to be most of the time
broken.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1365 bytes --]

On Wed, Nov 12, 2014 at 11:08:43PM +0100, Hans de Goede wrote:
> One of the reasons for having the simplefb nodes in /chosen, and doing
> explicit enumeration of the nodes there, is too allow enumerating them sooner,
> so that we get a console earlier on.
> 
> Doing this earlier then fs_initcall is not useful, since the fb only turns into
> a console when fbcon intializes, which is a fs_initcall too.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/video/fbdev/simplefb.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index be7d288..8c0c972 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
>  	platform_driver_unregister(&simplefb_driver);
>  }
>  
> -module_init(simplefb_init);
> +/*
> + * While this can be a module, if builtin it's most likely the console
> + * So let's leave module_exit but move module_init to an earlier place
> + */

Not really related to this patch itself, but do we want to support
simplefb as a module? It seems like it's going to be most of the time
broken.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Maxime Ripard]

On Wed, Nov 12, 2014 at 11:08:43PM +0100, Hans de Goede wrote:
> One of the reasons for having the simplefb nodes in /chosen, and doing
> explicit enumeration of the nodes there, is too allow enumerating them sooner,
> so that we get a console earlier on.
> 
> Doing this earlier then fs_initcall is not useful, since the fb only turns into
> a console when fbcon intializes, which is a fs_initcall too.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/video/fbdev/simplefb.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index be7d288..8c0c972 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
>  	platform_driver_unregister(&simplefb_driver);
>  }
>  
> -module_init(simplefb_init);
> +/*
> + * While this can be a module, if builtin it's most likely the console
> + * So let's leave module_exit but move module_init to an earlier place
> + */

Not really related to this patch itself, but do we want to support
simplefb as a module? It seems like it's going to be most of the time
broken.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141113/6baced6e/attachment-0001.sig>

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Geert Uytterhoeven]

Hi Maxime,

On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> -module_init(simplefb_init);
>> +/*
>> + * While this can be a module, if builtin it's most likely the console
>> + * So let's leave module_exit but move module_init to an earlier place
>> + */
>
> Not really related to this patch itself, but do we want to support
> simplefb as a module? It seems like it's going to be most of the time
> broken.

If it depends on clocks, it won't work as a module, as CCF will have disabled
all unused clocks at that point.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Geert Uytterhoeven]

Hi Maxime,

On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
>> -module_init(simplefb_init);
>> +/*
>> + * While this can be a module, if builtin it's most likely the console
>> + * So let's leave module_exit but move module_init to an earlier place
>> + */
>
> Not really related to this patch itself, but do we want to support
> simplefb as a module? It seems like it's going to be most of the time
> broken.

If it depends on clocks, it won't work as a module, as CCF will have disabled
all unused clocks at that point.

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

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Geert Uytterhoeven]

Hi Maxime,

On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
>> -module_init(simplefb_init);
>> +/*
>> + * While this can be a module, if builtin it's most likely the console
>> + * So let's leave module_exit but move module_init to an earlier place
>> + */
>
> Not really related to this patch itself, but do we want to support
> simplefb as a module? It seems like it's going to be most of the time
> broken.

If it depends on clocks, it won't work as a module, as CCF will have disabled
all unused clocks at that point.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/13/2014 09:52 AM, Maxime Ripard wrote:
> On Wed, Nov 12, 2014 at 11:08:43PM +0100, Hans de Goede wrote:
>> One of the reasons for having the simplefb nodes in /chosen, and doing
>> explicit enumeration of the nodes there, is too allow enumerating them sooner,
>> so that we get a console earlier on.
>>
>> Doing this earlier then fs_initcall is not useful, since the fb only turns into
>> a console when fbcon intializes, which is a fs_initcall too.
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/video/fbdev/simplefb.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>> index be7d288..8c0c972 100644
>> --- a/drivers/video/fbdev/simplefb.c
>> +++ b/drivers/video/fbdev/simplefb.c
>> @@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
>>  	platform_driver_unregister(&simplefb_driver);
>>  }
>>  
>> -module_init(simplefb_init);
>> +/*
>> + * While this can be a module, if builtin it's most likely the console
>> + * So let's leave module_exit but move module_init to an earlier place
>> + */
> 
> Not really related to this patch itself, but do we want to support
> simplefb as a module? It seems like it's going to be most of the time
> broken.

A valid point, my mean reasoning here is that some may see not being able to
use it as a module as a regression, so I just kept things as is, but I do
agree that it is advisable to just build it in.

Regards,

Hans

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/13/2014 09:52 AM, Maxime Ripard wrote:
> On Wed, Nov 12, 2014 at 11:08:43PM +0100, Hans de Goede wrote:
>> One of the reasons for having the simplefb nodes in /chosen, and doing
>> explicit enumeration of the nodes there, is too allow enumerating them sooner,
>> so that we get a console earlier on.
>>
>> Doing this earlier then fs_initcall is not useful, since the fb only turns into
>> a console when fbcon intializes, which is a fs_initcall too.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/video/fbdev/simplefb.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>> index be7d288..8c0c972 100644
>> --- a/drivers/video/fbdev/simplefb.c
>> +++ b/drivers/video/fbdev/simplefb.c
>> @@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
>>  	platform_driver_unregister(&simplefb_driver);
>>  }
>>  
>> -module_init(simplefb_init);
>> +/*
>> + * While this can be a module, if builtin it's most likely the console
>> + * So let's leave module_exit but move module_init to an earlier place
>> + */
> 
> Not really related to this patch itself, but do we want to support
> simplefb as a module? It seems like it's going to be most of the time
> broken.

A valid point, my mean reasoning here is that some may see not being able to
use it as a module as a regression, so I just kept things as is, but I do
agree that it is advisable to just build it in.

Regards,

Hans

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/13/2014 09:52 AM, Maxime Ripard wrote:
> On Wed, Nov 12, 2014 at 11:08:43PM +0100, Hans de Goede wrote:
>> One of the reasons for having the simplefb nodes in /chosen, and doing
>> explicit enumeration of the nodes there, is too allow enumerating them sooner,
>> so that we get a console earlier on.
>>
>> Doing this earlier then fs_initcall is not useful, since the fb only turns into
>> a console when fbcon intializes, which is a fs_initcall too.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/video/fbdev/simplefb.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>> index be7d288..8c0c972 100644
>> --- a/drivers/video/fbdev/simplefb.c
>> +++ b/drivers/video/fbdev/simplefb.c
>> @@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
>>  	platform_driver_unregister(&simplefb_driver);
>>  }
>>  
>> -module_init(simplefb_init);
>> +/*
>> + * While this can be a module, if builtin it's most likely the console
>> + * So let's leave module_exit but move module_init to an earlier place
>> + */
> 
> Not really related to this patch itself, but do we want to support
> simplefb as a module? It seems like it's going to be most of the time
> broken.

A valid point, my mean reasoning here is that some may see not being able to
use it as a module as a regression, so I just kept things as is, but I do
agree that it is advisable to just build it in.

Regards,

Hans

Re: [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Grant Likely]

On Wed, Nov 12, 2014 at 11:39 PM, Julian Calaby <julian.calaby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Hans,
>
> On Thu, Nov 13, 2014 at 9:08 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> Update simplefb to support the new preferred location for simplefb dt nodes
>> under /chosen.
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  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.

Re: [linux-sunxi] [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Grant Likely]

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.

[linux-sunxi] [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Grant Likely]

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.

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Grant Likely]

On Thu, Nov 13, 2014 at 9:24 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Hi,
>
> On 11/13/2014 09:52 AM, Maxime Ripard wrote:
>> On Wed, Nov 12, 2014 at 11:08:43PM +0100, Hans de Goede wrote:
>>> One of the reasons for having the simplefb nodes in /chosen, and doing
>>> explicit enumeration of the nodes there, is too allow enumerating them sooner,
>>> so that we get a console earlier on.
>>>
>>> Doing this earlier then fs_initcall is not useful, since the fb only turns into
>>> a console when fbcon intializes, which is a fs_initcall too.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  drivers/video/fbdev/simplefb.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>>> index be7d288..8c0c972 100644
>>> --- a/drivers/video/fbdev/simplefb.c
>>> +++ b/drivers/video/fbdev/simplefb.c
>>> @@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
>>>      platform_driver_unregister(&simplefb_driver);
>>>  }
>>>
>>> -module_init(simplefb_init);
>>> +/*
>>> + * While this can be a module, if builtin it's most likely the console
>>> + * So let's leave module_exit but move module_init to an earlier place
>>> + */
>>
>> Not really related to this patch itself, but do we want to support
>> simplefb as a module? It seems like it's going to be most of the time
>> broken.
>
> A valid point, my mean reasoning here is that some may see not being able to
> use it as a module as a regression, so I just kept things as is, but I do
> agree that it is advisable to just build it in.

Like a lot of things, if it is made a modules, and it breaks for the
user, the user gets to keep the pieces. There are potentially some
valid scenarios where it is fine to have it as a module. I don't
recommend changing this unless is actually starts causing problems.

g.

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Grant Likely]

On Thu, Nov 13, 2014 at 9:24 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 11/13/2014 09:52 AM, Maxime Ripard wrote:
>> On Wed, Nov 12, 2014 at 11:08:43PM +0100, Hans de Goede wrote:
>>> One of the reasons for having the simplefb nodes in /chosen, and doing
>>> explicit enumeration of the nodes there, is too allow enumerating them sooner,
>>> so that we get a console earlier on.
>>>
>>> Doing this earlier then fs_initcall is not useful, since the fb only turns into
>>> a console when fbcon intializes, which is a fs_initcall too.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  drivers/video/fbdev/simplefb.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>>> index be7d288..8c0c972 100644
>>> --- a/drivers/video/fbdev/simplefb.c
>>> +++ b/drivers/video/fbdev/simplefb.c
>>> @@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
>>>      platform_driver_unregister(&simplefb_driver);
>>>  }
>>>
>>> -module_init(simplefb_init);
>>> +/*
>>> + * While this can be a module, if builtin it's most likely the console
>>> + * So let's leave module_exit but move module_init to an earlier place
>>> + */
>>
>> Not really related to this patch itself, but do we want to support
>> simplefb as a module? It seems like it's going to be most of the time
>> broken.
>
> A valid point, my mean reasoning here is that some may see not being able to
> use it as a module as a regression, so I just kept things as is, but I do
> agree that it is advisable to just build it in.

Like a lot of things, if it is made a modules, and it breaks for the
user, the user gets to keep the pieces. There are potentially some
valid scenarios where it is fine to have it as a module. I don't
recommend changing this unless is actually starts causing problems.

g.

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Grant Likely]

On Thu, Nov 13, 2014 at 9:24 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 11/13/2014 09:52 AM, Maxime Ripard wrote:
>> On Wed, Nov 12, 2014 at 11:08:43PM +0100, Hans de Goede wrote:
>>> One of the reasons for having the simplefb nodes in /chosen, and doing
>>> explicit enumeration of the nodes there, is too allow enumerating them sooner,
>>> so that we get a console earlier on.
>>>
>>> Doing this earlier then fs_initcall is not useful, since the fb only turns into
>>> a console when fbcon intializes, which is a fs_initcall too.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  drivers/video/fbdev/simplefb.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>>> index be7d288..8c0c972 100644
>>> --- a/drivers/video/fbdev/simplefb.c
>>> +++ b/drivers/video/fbdev/simplefb.c
>>> @@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
>>>      platform_driver_unregister(&simplefb_driver);
>>>  }
>>>
>>> -module_init(simplefb_init);
>>> +/*
>>> + * While this can be a module, if builtin it's most likely the console
>>> + * So let's leave module_exit but move module_init to an earlier place
>>> + */
>>
>> Not really related to this patch itself, but do we want to support
>> simplefb as a module? It seems like it's going to be most of the time
>> broken.
>
> A valid point, my mean reasoning here is that some may see not being able to
> use it as a module as a regression, so I just kept things as is, but I do
> agree that it is advisable to just build it in.

Like a lot of things, if it is made a modules, and it breaks for the
user, the user gets to keep the pieces. There are potentially some
valid scenarios where it is fine to have it as a module. I don't
recommend changing this unless is actually starts causing problems.

g.

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Grant Likely]

On Thu, Nov 13, 2014 at 10:29 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Nov 13, 2014 at 9:24 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> Hi,
>>
>> On 11/13/2014 09:52 AM, Maxime Ripard wrote:
>>> On Wed, Nov 12, 2014 at 11:08:43PM +0100, Hans de Goede wrote:
>>>> One of the reasons for having the simplefb nodes in /chosen, and doing
>>>> explicit enumeration of the nodes there, is too allow enumerating them sooner,
>>>> so that we get a console earlier on.
>>>>
>>>> Doing this earlier then fs_initcall is not useful, since the fb only turns into
>>>> a console when fbcon intializes, which is a fs_initcall too.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>>  drivers/video/fbdev/simplefb.c | 6 +++++-
>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>>>> index be7d288..8c0c972 100644
>>>> --- a/drivers/video/fbdev/simplefb.c
>>>> +++ b/drivers/video/fbdev/simplefb.c
>>>> @@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
>>>>      platform_driver_unregister(&simplefb_driver);
>>>>  }
>>>>
>>>> -module_init(simplefb_init);
>>>> +/*
>>>> + * While this can be a module, if builtin it's most likely the console
>>>> + * So let's leave module_exit but move module_init to an earlier place
>>>> + */
>>>
>>> Not really related to this patch itself, but do we want to support
>>> simplefb as a module? It seems like it's going to be most of the time
>>> broken.
>>
>> A valid point, my mean reasoning here is that some may see not being able to
>> use it as a module as a regression, so I just kept things as is, but I do
>> agree that it is advisable to just build it in.
>
> Like a lot of things, if it is made a modules, and it breaks for the
> user, the user gets to keep the pieces. There are potentially some
> valid scenarios where it is fine to have it as a module. I don't
> recommend changing this unless is actually starts causing problems.

I assume that you've tested this and it actually makes a difference,
correct? If so,

Acked-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Grant Likely]

On Thu, Nov 13, 2014 at 10:29 AM, Grant Likely <grant.likely@linaro.org> wrote:
> On Thu, Nov 13, 2014 at 9:24 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 11/13/2014 09:52 AM, Maxime Ripard wrote:
>>> On Wed, Nov 12, 2014 at 11:08:43PM +0100, Hans de Goede wrote:
>>>> One of the reasons for having the simplefb nodes in /chosen, and doing
>>>> explicit enumeration of the nodes there, is too allow enumerating them sooner,
>>>> so that we get a console earlier on.
>>>>
>>>> Doing this earlier then fs_initcall is not useful, since the fb only turns into
>>>> a console when fbcon intializes, which is a fs_initcall too.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/video/fbdev/simplefb.c | 6 +++++-
>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>>>> index be7d288..8c0c972 100644
>>>> --- a/drivers/video/fbdev/simplefb.c
>>>> +++ b/drivers/video/fbdev/simplefb.c
>>>> @@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
>>>>      platform_driver_unregister(&simplefb_driver);
>>>>  }
>>>>
>>>> -module_init(simplefb_init);
>>>> +/*
>>>> + * While this can be a module, if builtin it's most likely the console
>>>> + * So let's leave module_exit but move module_init to an earlier place
>>>> + */
>>>
>>> Not really related to this patch itself, but do we want to support
>>> simplefb as a module? It seems like it's going to be most of the time
>>> broken.
>>
>> A valid point, my mean reasoning here is that some may see not being able to
>> use it as a module as a regression, so I just kept things as is, but I do
>> agree that it is advisable to just build it in.
>
> Like a lot of things, if it is made a modules, and it breaks for the
> user, the user gets to keep the pieces. There are potentially some
> valid scenarios where it is fine to have it as a module. I don't
> recommend changing this unless is actually starts causing problems.

I assume that you've tested this and it actually makes a difference,
correct? If so,

Acked-by: Grant Likely <grant.likely@linaro.org>

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Grant Likely]

On Thu, Nov 13, 2014 at 10:29 AM, Grant Likely <grant.likely@linaro.org> wrote:
> On Thu, Nov 13, 2014 at 9:24 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 11/13/2014 09:52 AM, Maxime Ripard wrote:
>>> On Wed, Nov 12, 2014 at 11:08:43PM +0100, Hans de Goede wrote:
>>>> One of the reasons for having the simplefb nodes in /chosen, and doing
>>>> explicit enumeration of the nodes there, is too allow enumerating them sooner,
>>>> so that we get a console earlier on.
>>>>
>>>> Doing this earlier then fs_initcall is not useful, since the fb only turns into
>>>> a console when fbcon intializes, which is a fs_initcall too.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/video/fbdev/simplefb.c | 6 +++++-
>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>>>> index be7d288..8c0c972 100644
>>>> --- a/drivers/video/fbdev/simplefb.c
>>>> +++ b/drivers/video/fbdev/simplefb.c
>>>> @@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
>>>>      platform_driver_unregister(&simplefb_driver);
>>>>  }
>>>>
>>>> -module_init(simplefb_init);
>>>> +/*
>>>> + * While this can be a module, if builtin it's most likely the console
>>>> + * So let's leave module_exit but move module_init to an earlier place
>>>> + */
>>>
>>> Not really related to this patch itself, but do we want to support
>>> simplefb as a module? It seems like it's going to be most of the time
>>> broken.
>>
>> A valid point, my mean reasoning here is that some may see not being able to
>> use it as a module as a regression, so I just kept things as is, but I do
>> agree that it is advisable to just build it in.
>
> Like a lot of things, if it is made a modules, and it breaks for the
> user, the user gets to keep the pieces. There are potentially some
> valid scenarios where it is fine to have it as a module. I don't
> recommend changing this unless is actually starts causing problems.

I assume that you've tested this and it actually makes a difference,
correct? If so,

Acked-by: Grant Likely <grant.likely@linaro.org>

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1672 bytes --]

On Thu, Nov 13, 2014 at 10:29:52AM +0000, Grant Likely wrote:
> On Thu, Nov 13, 2014 at 9:24 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On 11/13/2014 09:52 AM, Maxime Ripard wrote:
> >>> +/*
> >>> + * While this can be a module, if builtin it's most likely the console
> >>> + * So let's leave module_exit but move module_init to an earlier place
> >>> + */
> >>
> >> Not really related to this patch itself, but do we want to support
> >> simplefb as a module? It seems like it's going to be most of the time
> >> broken.
> >
> > A valid point, my mean reasoning here is that some may see not being able to
> > use it as a module as a regression, so I just kept things as is, but I do
> > agree that it is advisable to just build it in.
> 
> Like a lot of things, if it is made a modules, and it breaks for the
> user, the user gets to keep the pieces. There are potentially some
> valid scenarios where it is fine to have it as a module. I don't
> recommend changing this unless is actually starts causing problems.

I don't really agree here. If it's broken because the clocks, reset,
memory, or whatever resource has been reclaimed by the kernel before
the module even had a chance to probe, the only thing that the user
will get is that there's no chance it's ever going to work, and that
it's just unreliable.

If we know that it's going to break, and that there's no way it can be
reliable (as in on all the SoCs reliable), keeping it as a module is
just asking for trouble.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1643 bytes --]

On Thu, Nov 13, 2014 at 10:29:52AM +0000, Grant Likely wrote:
> On Thu, Nov 13, 2014 at 9:24 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> > On 11/13/2014 09:52 AM, Maxime Ripard wrote:
> >>> +/*
> >>> + * While this can be a module, if builtin it's most likely the console
> >>> + * So let's leave module_exit but move module_init to an earlier place
> >>> + */
> >>
> >> Not really related to this patch itself, but do we want to support
> >> simplefb as a module? It seems like it's going to be most of the time
> >> broken.
> >
> > A valid point, my mean reasoning here is that some may see not being able to
> > use it as a module as a regression, so I just kept things as is, but I do
> > agree that it is advisable to just build it in.
> 
> Like a lot of things, if it is made a modules, and it breaks for the
> user, the user gets to keep the pieces. There are potentially some
> valid scenarios where it is fine to have it as a module. I don't
> recommend changing this unless is actually starts causing problems.

I don't really agree here. If it's broken because the clocks, reset,
memory, or whatever resource has been reclaimed by the kernel before
the module even had a chance to probe, the only thing that the user
will get is that there's no chance it's ever going to work, and that
it's just unreliable.

If we know that it's going to break, and that there's no way it can be
reliable (as in on all the SoCs reliable), keeping it as a module is
just asking for trouble.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Maxime Ripard]

On Thu, Nov 13, 2014 at 10:29:52AM +0000, Grant Likely wrote:
> On Thu, Nov 13, 2014 at 9:24 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> > On 11/13/2014 09:52 AM, Maxime Ripard wrote:
> >>> +/*
> >>> + * While this can be a module, if builtin it's most likely the console
> >>> + * So let's leave module_exit but move module_init to an earlier place
> >>> + */
> >>
> >> Not really related to this patch itself, but do we want to support
> >> simplefb as a module? It seems like it's going to be most of the time
> >> broken.
> >
> > A valid point, my mean reasoning here is that some may see not being able to
> > use it as a module as a regression, so I just kept things as is, but I do
> > agree that it is advisable to just build it in.
> 
> Like a lot of things, if it is made a modules, and it breaks for the
> user, the user gets to keep the pieces. There are potentially some
> valid scenarios where it is fine to have it as a module. I don't
> recommend changing this unless is actually starts causing problems.

I don't really agree here. If it's broken because the clocks, reset,
memory, or whatever resource has been reclaimed by the kernel before
the module even had a chance to probe, the only thing that the user
will get is that there's no chance it's ever going to work, and that
it's just unreliable.

If we know that it's going to break, and that there's no way it can be
reliable (as in on all the SoCs reliable), keeping it as a module is
just asking for trouble.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141113/27f6b3c0/attachment-0001.sig>

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/13/2014 11:31 AM, Grant Likely wrote:
> On Thu, Nov 13, 2014 at 10:29 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On Thu, Nov 13, 2014 at 9:24 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>> Hi,
>>>
>>> On 11/13/2014 09:52 AM, Maxime Ripard wrote:
>>>> On Wed, Nov 12, 2014 at 11:08:43PM +0100, Hans de Goede wrote:
>>>>> One of the reasons for having the simplefb nodes in /chosen, and doing
>>>>> explicit enumeration of the nodes there, is too allow enumerating them sooner,
>>>>> so that we get a console earlier on.
>>>>>
>>>>> Doing this earlier then fs_initcall is not useful, since the fb only turns into
>>>>> a console when fbcon intializes, which is a fs_initcall too.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>>> ---
>>>>>  drivers/video/fbdev/simplefb.c | 6 +++++-
>>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>>>>> index be7d288..8c0c972 100644
>>>>> --- a/drivers/video/fbdev/simplefb.c
>>>>> +++ b/drivers/video/fbdev/simplefb.c
>>>>> @@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
>>>>>      platform_driver_unregister(&simplefb_driver);
>>>>>  }
>>>>>
>>>>> -module_init(simplefb_init);
>>>>> +/*
>>>>> + * While this can be a module, if builtin it's most likely the console
>>>>> + * So let's leave module_exit but move module_init to an earlier place
>>>>> + */
>>>>
>>>> Not really related to this patch itself, but do we want to support
>>>> simplefb as a module? It seems like it's going to be most of the time
>>>> broken.
>>>
>>> A valid point, my mean reasoning here is that some may see not being able to
>>> use it as a module as a regression, so I just kept things as is, but I do
>>> agree that it is advisable to just build it in.
>>
>> Like a lot of things, if it is made a modules, and it breaks for the
>> user, the user gets to keep the pieces. There are potentially some
>> valid scenarios where it is fine to have it as a module. I don't
>> recommend changing this unless is actually starts causing problems.
> 
> I assume that you've tested this and it actually makes a difference,
> correct? If so,
> 
> Acked-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Yes I've tested the entire set on 4 boards / 4 SoCs (A10, A10s, A20 and A31),
and yes it makes a difference.

Regards,

Hans

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/13/2014 11:31 AM, Grant Likely wrote:
> On Thu, Nov 13, 2014 at 10:29 AM, Grant Likely <grant.likely@linaro.org> wrote:
>> On Thu, Nov 13, 2014 at 9:24 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi,
>>>
>>> On 11/13/2014 09:52 AM, Maxime Ripard wrote:
>>>> On Wed, Nov 12, 2014 at 11:08:43PM +0100, Hans de Goede wrote:
>>>>> One of the reasons for having the simplefb nodes in /chosen, and doing
>>>>> explicit enumeration of the nodes there, is too allow enumerating them sooner,
>>>>> so that we get a console earlier on.
>>>>>
>>>>> Doing this earlier then fs_initcall is not useful, since the fb only turns into
>>>>> a console when fbcon intializes, which is a fs_initcall too.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>>  drivers/video/fbdev/simplefb.c | 6 +++++-
>>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>>>>> index be7d288..8c0c972 100644
>>>>> --- a/drivers/video/fbdev/simplefb.c
>>>>> +++ b/drivers/video/fbdev/simplefb.c
>>>>> @@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
>>>>>      platform_driver_unregister(&simplefb_driver);
>>>>>  }
>>>>>
>>>>> -module_init(simplefb_init);
>>>>> +/*
>>>>> + * While this can be a module, if builtin it's most likely the console
>>>>> + * So let's leave module_exit but move module_init to an earlier place
>>>>> + */
>>>>
>>>> Not really related to this patch itself, but do we want to support
>>>> simplefb as a module? It seems like it's going to be most of the time
>>>> broken.
>>>
>>> A valid point, my mean reasoning here is that some may see not being able to
>>> use it as a module as a regression, so I just kept things as is, but I do
>>> agree that it is advisable to just build it in.
>>
>> Like a lot of things, if it is made a modules, and it breaks for the
>> user, the user gets to keep the pieces. There are potentially some
>> valid scenarios where it is fine to have it as a module. I don't
>> recommend changing this unless is actually starts causing problems.
> 
> I assume that you've tested this and it actually makes a difference,
> correct? If so,
> 
> Acked-by: Grant Likely <grant.likely@linaro.org>

Yes I've tested the entire set on 4 boards / 4 SoCs (A10, A10s, A20 and A31),
and yes it makes a difference.

Regards,

Hans

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/13/2014 11:31 AM, Grant Likely wrote:
> On Thu, Nov 13, 2014 at 10:29 AM, Grant Likely <grant.likely@linaro.org> wrote:
>> On Thu, Nov 13, 2014 at 9:24 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi,
>>>
>>> On 11/13/2014 09:52 AM, Maxime Ripard wrote:
>>>> On Wed, Nov 12, 2014 at 11:08:43PM +0100, Hans de Goede wrote:
>>>>> One of the reasons for having the simplefb nodes in /chosen, and doing
>>>>> explicit enumeration of the nodes there, is too allow enumerating them sooner,
>>>>> so that we get a console earlier on.
>>>>>
>>>>> Doing this earlier then fs_initcall is not useful, since the fb only turns into
>>>>> a console when fbcon intializes, which is a fs_initcall too.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>>  drivers/video/fbdev/simplefb.c | 6 +++++-
>>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>>>>> index be7d288..8c0c972 100644
>>>>> --- a/drivers/video/fbdev/simplefb.c
>>>>> +++ b/drivers/video/fbdev/simplefb.c
>>>>> @@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
>>>>>      platform_driver_unregister(&simplefb_driver);
>>>>>  }
>>>>>
>>>>> -module_init(simplefb_init);
>>>>> +/*
>>>>> + * While this can be a module, if builtin it's most likely the console
>>>>> + * So let's leave module_exit but move module_init to an earlier place
>>>>> + */
>>>>
>>>> Not really related to this patch itself, but do we want to support
>>>> simplefb as a module? It seems like it's going to be most of the time
>>>> broken.
>>>
>>> A valid point, my mean reasoning here is that some may see not being able to
>>> use it as a module as a regression, so I just kept things as is, but I do
>>> agree that it is advisable to just build it in.
>>
>> Like a lot of things, if it is made a modules, and it breaks for the
>> user, the user gets to keep the pieces. There are potentially some
>> valid scenarios where it is fine to have it as a module. I don't
>> recommend changing this unless is actually starts causing problems.
> 
> I assume that you've tested this and it actually makes a difference,
> correct? If so,
> 
> Acked-by: Grant Likely <grant.likely@linaro.org>

Yes I've tested the entire set on 4 boards / 4 SoCs (A10, A10s, A20 and A31),
and yes it makes a difference.

Regards,

Hans

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Stefan Monnier]

> Like a lot of things, if it is made a modules, and it breaks for the
> user, the user gets to keep the pieces.  There are potentially some

But the option's description should probably include some blurb about
the fact that it only works as a module with some combinations of
SoC+kernel.


        Stefan

Re: Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/13/2014 05:13 PM, Stefan Monnier wrote:
>> Like a lot of things, if it is made a modules, and it breaks for the
>> user, the user gets to keep the pieces.  There are potentially some
> 
> But the option's description should probably include some blurb about
> the fact that it only works as a module with some combinations of
> SoC+kernel.

So I was about to add an update of the Kconfig text along this line
while I was respinning the patch anyways, and guess what simplefb
currently cannot be build as a module, so this is a non-issue.

I'll update my "simplefb: Add support for enumerating simplefb dt nodes in /chosen"
patch to not have a module_exit in the next version, since that is
not really useful if it cannot be build as a module anyways.

Regards,

Hans

[linux-sunxi] Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/13/2014 05:13 PM, Stefan Monnier wrote:
>> Like a lot of things, if it is made a modules, and it breaks for the
>> user, the user gets to keep the pieces.  There are potentially some
> 
> But the option's description should probably include some blurb about
> the fact that it only works as a module with some combinations of
> SoC+kernel.

So I was about to add an update of the Kconfig text along this line
while I was respinning the patch anyways, and guess what simplefb
currently cannot be build as a module, so this is a non-issue.

I'll update my "simplefb: Add support for enumerating simplefb dt nodes in /chosen"
patch to not have a module_exit in the next version, since that is
not really useful if it cannot be build as a module anyways.

Regards,

Hans

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]

On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
> Hi Maxime,
> 
> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >> -module_init(simplefb_init);
> >> +/*
> >> + * While this can be a module, if builtin it's most likely the console
> >> + * So let's leave module_exit but move module_init to an earlier place
> >> + */
> >
> > Not really related to this patch itself, but do we want to support
> > simplefb as a module? It seems like it's going to be most of the time
> > broken.
> 
> If it depends on clocks, it won't work as a module, as CCF will have disabled
> all unused clocks at that point.

If it does depend on anything beyond clocks it won't work at all. Clocks
are special because they get set up very early at boot time. If it turns
out that a simplefb ever needs a regulator to remain on, and that's even
quite likely to happen eventually, it's going to fail miserably, because
those regulators will typically be provided by a PMIC on an I2C bus. The
regulator won't be registered until very late into the boot process and
a regulator_get() call will almost certainly cause the simplefb driver
to defer probing.

Now deferring probing is a real showstopper for simplefb, because not
only does it make the framebuffer useless as early boot console, once
probing is attempted again the clocks that it would have needed to
acquire to keep going will already have been switched off, too.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1504 bytes --]

On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
> Hi Maxime,
> 
> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> >> -module_init(simplefb_init);
> >> +/*
> >> + * While this can be a module, if builtin it's most likely the console
> >> + * So let's leave module_exit but move module_init to an earlier place
> >> + */
> >
> > Not really related to this patch itself, but do we want to support
> > simplefb as a module? It seems like it's going to be most of the time
> > broken.
> 
> If it depends on clocks, it won't work as a module, as CCF will have disabled
> all unused clocks at that point.

If it does depend on anything beyond clocks it won't work at all. Clocks
are special because they get set up very early at boot time. If it turns
out that a simplefb ever needs a regulator to remain on, and that's even
quite likely to happen eventually, it's going to fail miserably, because
those regulators will typically be provided by a PMIC on an I2C bus. The
regulator won't be registered until very late into the boot process and
a regulator_get() call will almost certainly cause the simplefb driver
to defer probing.

Now deferring probing is a real showstopper for simplefb, because not
only does it make the framebuffer useless as early boot console, once
probing is attempted again the clocks that it would have needed to
acquire to keep going will already have been switched off, too.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Thierry Reding]

On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
> Hi Maxime,
> 
> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> >> -module_init(simplefb_init);
> >> +/*
> >> + * While this can be a module, if builtin it's most likely the console
> >> + * So let's leave module_exit but move module_init to an earlier place
> >> + */
> >
> > Not really related to this patch itself, but do we want to support
> > simplefb as a module? It seems like it's going to be most of the time
> > broken.
> 
> If it depends on clocks, it won't work as a module, as CCF will have disabled
> all unused clocks at that point.

If it does depend on anything beyond clocks it won't work at all. Clocks
are special because they get set up very early at boot time. If it turns
out that a simplefb ever needs a regulator to remain on, and that's even
quite likely to happen eventually, it's going to fail miserably, because
those regulators will typically be provided by a PMIC on an I2C bus. The
regulator won't be registered until very late into the boot process and
a regulator_get() call will almost certainly cause the simplefb driver
to defer probing.

Now deferring probing is a real showstopper for simplefb, because not
only does it make the framebuffer useless as early boot console, once
probing is attempted again the clocks that it would have needed to
acquire to keep going will already have been switched off, too.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141118/bc68db5b/attachment.sig>

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/18/2014 11:19 AM, Thierry Reding wrote:
> On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
>> Hi Maxime,
>>
>> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>> -module_init(simplefb_init);
>>>> +/*
>>>> + * While this can be a module, if builtin it's most likely the console
>>>> + * So let's leave module_exit but move module_init to an earlier place
>>>> + */
>>>
>>> Not really related to this patch itself, but do we want to support
>>> simplefb as a module? It seems like it's going to be most of the time
>>> broken.
>>
>> If it depends on clocks, it won't work as a module, as CCF will have disabled
>> all unused clocks at that point.
> 
> If it does depend on anything beyond clocks it won't work at all. Clocks
> are special because they get set up very early at boot time. If it turns
> out that a simplefb ever needs a regulator to remain on, and that's even
> quite likely to happen eventually, it's going to fail miserably, because
> those regulators will typically be provided by a PMIC on an I2C bus. The
> regulator won't be registered until very late into the boot process and
> a regulator_get() call will almost certainly cause the simplefb driver
> to defer probing.

Right, this has been discussed already and the plan is to have simplefb
continue its probe function and return success from it if it encounters
any -eprobe_defer errors, while tracking which resources it misses.

And then have a late_initcall which will claim any resources which failed
with -eprobe beforehand.

> Now deferring probing is a real showstopper for simplefb, because not
> only does it make the framebuffer useless as early boot console, once
> probing is attempted again the clocks that it would have needed to
> acquire to keep going will already have been switched off, too.

That is not true, even with the current implementation, if all necessary
drivers are built in, then simplefb will come up later, but it will still
come up before the late_initcall which disables the clocks.

Once we do the split probing described above (which is something which
we plan to do when it becomes necessary), then simplefb will still come
up early.

Regards,

Hans

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/18/2014 11:19 AM, Thierry Reding wrote:
> On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
>> Hi Maxime,
>>
>> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>>>> -module_init(simplefb_init);
>>>> +/*
>>>> + * While this can be a module, if builtin it's most likely the console
>>>> + * So let's leave module_exit but move module_init to an earlier place
>>>> + */
>>>
>>> Not really related to this patch itself, but do we want to support
>>> simplefb as a module? It seems like it's going to be most of the time
>>> broken.
>>
>> If it depends on clocks, it won't work as a module, as CCF will have disabled
>> all unused clocks at that point.
> 
> If it does depend on anything beyond clocks it won't work at all. Clocks
> are special because they get set up very early at boot time. If it turns
> out that a simplefb ever needs a regulator to remain on, and that's even
> quite likely to happen eventually, it's going to fail miserably, because
> those regulators will typically be provided by a PMIC on an I2C bus. The
> regulator won't be registered until very late into the boot process and
> a regulator_get() call will almost certainly cause the simplefb driver
> to defer probing.

Right, this has been discussed already and the plan is to have simplefb
continue its probe function and return success from it if it encounters
any -eprobe_defer errors, while tracking which resources it misses.

And then have a late_initcall which will claim any resources which failed
with -eprobe beforehand.

> Now deferring probing is a real showstopper for simplefb, because not
> only does it make the framebuffer useless as early boot console, once
> probing is attempted again the clocks that it would have needed to
> acquire to keep going will already have been switched off, too.

That is not true, even with the current implementation, if all necessary
drivers are built in, then simplefb will come up later, but it will still
come up before the late_initcall which disables the clocks.

Once we do the split probing described above (which is something which
we plan to do when it becomes necessary), then simplefb will still come
up early.

Regards,

Hans

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/18/2014 11:19 AM, Thierry Reding wrote:
> On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
>> Hi Maxime,
>>
>> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>>>> -module_init(simplefb_init);
>>>> +/*
>>>> + * While this can be a module, if builtin it's most likely the console
>>>> + * So let's leave module_exit but move module_init to an earlier place
>>>> + */
>>>
>>> Not really related to this patch itself, but do we want to support
>>> simplefb as a module? It seems like it's going to be most of the time
>>> broken.
>>
>> If it depends on clocks, it won't work as a module, as CCF will have disabled
>> all unused clocks at that point.
> 
> If it does depend on anything beyond clocks it won't work at all. Clocks
> are special because they get set up very early at boot time. If it turns
> out that a simplefb ever needs a regulator to remain on, and that's even
> quite likely to happen eventually, it's going to fail miserably, because
> those regulators will typically be provided by a PMIC on an I2C bus. The
> regulator won't be registered until very late into the boot process and
> a regulator_get() call will almost certainly cause the simplefb driver
> to defer probing.

Right, this has been discussed already and the plan is to have simplefb
continue its probe function and return success from it if it encounters
any -eprobe_defer errors, while tracking which resources it misses.

And then have a late_initcall which will claim any resources which failed
with -eprobe beforehand.

> Now deferring probing is a real showstopper for simplefb, because not
> only does it make the framebuffer useless as early boot console, once
> probing is attempted again the clocks that it would have needed to
> acquire to keep going will already have been switched off, too.

That is not true, even with the current implementation, if all necessary
drivers are built in, then simplefb will come up later, but it will still
come up before the late_initcall which disables the clocks.

Once we do the split probing described above (which is something which
we plan to do when it becomes necessary), then simplefb will still come
up early.

Regards,

Hans

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3406 bytes --]

On Tue, Nov 18, 2014 at 12:01:08PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/18/2014 11:19 AM, Thierry Reding wrote:
> > On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
> >> Hi Maxime,
> >>
> >> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
> >> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >>>> -module_init(simplefb_init);
> >>>> +/*
> >>>> + * While this can be a module, if builtin it's most likely the console
> >>>> + * So let's leave module_exit but move module_init to an earlier place
> >>>> + */
> >>>
> >>> Not really related to this patch itself, but do we want to support
> >>> simplefb as a module? It seems like it's going to be most of the time
> >>> broken.
> >>
> >> If it depends on clocks, it won't work as a module, as CCF will have disabled
> >> all unused clocks at that point.
> > 
> > If it does depend on anything beyond clocks it won't work at all. Clocks
> > are special because they get set up very early at boot time. If it turns
> > out that a simplefb ever needs a regulator to remain on, and that's even
> > quite likely to happen eventually, it's going to fail miserably, because
> > those regulators will typically be provided by a PMIC on an I2C bus. The
> > regulator won't be registered until very late into the boot process and
> > a regulator_get() call will almost certainly cause the simplefb driver
> > to defer probing.
> 
> Right, this has been discussed already and the plan is to have simplefb
> continue its probe function and return success from it if it encounters
> any -eprobe_defer errors, while tracking which resources it misses.
> 
> And then have a late_initcall which will claim any resources which failed
> with -eprobe beforehand.

How do you ensure that the late_initcall gets run before any of the
other late_initcalls that disable the resources? Also my recollection is
that deferred probing will first be triggered the first time from a
late_initcall, so chances aren't very high that all resources have shown
up by that time.

> > Now deferring probing is a real showstopper for simplefb, because not
> > only does it make the framebuffer useless as early boot console, once
> > probing is attempted again the clocks that it would have needed to
> > acquire to keep going will already have been switched off, too.
> 
> That is not true, even with the current implementation, if all necessary
> drivers are built in, then simplefb will come up later, but it will still
> come up before the late_initcall which disables the clocks.

Yes, in the current implementation because clocks typically are
registered very early and thus you don't hit the deferred probe. The
same is not true for other types of resources where it's actually quite
common to hit deferred probing (regulators is a very notorious one).

It doesn't matter whether a driver is built-in or not, once you hit
deferred probing you lose.

> Once we do the split probing described above (which is something which
> we plan to do when it becomes necessary), then simplefb will still come
> up early.

It will come up early but won't have acquired all the resources that it
needs, so unless you somehow manage to order late_initcalls in exactly
the way that you need them, the frameworks will still turn off what you
haven't managed to claim yet.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3375 bytes --]

On Tue, Nov 18, 2014 at 12:01:08PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/18/2014 11:19 AM, Thierry Reding wrote:
> > On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
> >> Hi Maxime,
> >>
> >> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >>>> -module_init(simplefb_init);
> >>>> +/*
> >>>> + * While this can be a module, if builtin it's most likely the console
> >>>> + * So let's leave module_exit but move module_init to an earlier place
> >>>> + */
> >>>
> >>> Not really related to this patch itself, but do we want to support
> >>> simplefb as a module? It seems like it's going to be most of the time
> >>> broken.
> >>
> >> If it depends on clocks, it won't work as a module, as CCF will have disabled
> >> all unused clocks at that point.
> > 
> > If it does depend on anything beyond clocks it won't work at all. Clocks
> > are special because they get set up very early at boot time. If it turns
> > out that a simplefb ever needs a regulator to remain on, and that's even
> > quite likely to happen eventually, it's going to fail miserably, because
> > those regulators will typically be provided by a PMIC on an I2C bus. The
> > regulator won't be registered until very late into the boot process and
> > a regulator_get() call will almost certainly cause the simplefb driver
> > to defer probing.
> 
> Right, this has been discussed already and the plan is to have simplefb
> continue its probe function and return success from it if it encounters
> any -eprobe_defer errors, while tracking which resources it misses.
> 
> And then have a late_initcall which will claim any resources which failed
> with -eprobe beforehand.

How do you ensure that the late_initcall gets run before any of the
other late_initcalls that disable the resources? Also my recollection is
that deferred probing will first be triggered the first time from a
late_initcall, so chances aren't very high that all resources have shown
up by that time.

> > Now deferring probing is a real showstopper for simplefb, because not
> > only does it make the framebuffer useless as early boot console, once
> > probing is attempted again the clocks that it would have needed to
> > acquire to keep going will already have been switched off, too.
> 
> That is not true, even with the current implementation, if all necessary
> drivers are built in, then simplefb will come up later, but it will still
> come up before the late_initcall which disables the clocks.

Yes, in the current implementation because clocks typically are
registered very early and thus you don't hit the deferred probe. The
same is not true for other types of resources where it's actually quite
common to hit deferred probing (regulators is a very notorious one).

It doesn't matter whether a driver is built-in or not, once you hit
deferred probing you lose.

> Once we do the split probing described above (which is something which
> we plan to do when it becomes necessary), then simplefb will still come
> up early.

It will come up early but won't have acquired all the resources that it
needs, so unless you somehow manage to order late_initcalls in exactly
the way that you need them, the frameworks will still turn off what you
haven't managed to claim yet.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Thierry Reding]

On Tue, Nov 18, 2014 at 12:01:08PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/18/2014 11:19 AM, Thierry Reding wrote:
> > On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
> >> Hi Maxime,
> >>
> >> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >>>> -module_init(simplefb_init);
> >>>> +/*
> >>>> + * While this can be a module, if builtin it's most likely the console
> >>>> + * So let's leave module_exit but move module_init to an earlier place
> >>>> + */
> >>>
> >>> Not really related to this patch itself, but do we want to support
> >>> simplefb as a module? It seems like it's going to be most of the time
> >>> broken.
> >>
> >> If it depends on clocks, it won't work as a module, as CCF will have disabled
> >> all unused clocks at that point.
> > 
> > If it does depend on anything beyond clocks it won't work at all. Clocks
> > are special because they get set up very early at boot time. If it turns
> > out that a simplefb ever needs a regulator to remain on, and that's even
> > quite likely to happen eventually, it's going to fail miserably, because
> > those regulators will typically be provided by a PMIC on an I2C bus. The
> > regulator won't be registered until very late into the boot process and
> > a regulator_get() call will almost certainly cause the simplefb driver
> > to defer probing.
> 
> Right, this has been discussed already and the plan is to have simplefb
> continue its probe function and return success from it if it encounters
> any -eprobe_defer errors, while tracking which resources it misses.
> 
> And then have a late_initcall which will claim any resources which failed
> with -eprobe beforehand.

How do you ensure that the late_initcall gets run before any of the
other late_initcalls that disable the resources? Also my recollection is
that deferred probing will first be triggered the first time from a
late_initcall, so chances aren't very high that all resources have shown
up by that time.

> > Now deferring probing is a real showstopper for simplefb, because not
> > only does it make the framebuffer useless as early boot console, once
> > probing is attempted again the clocks that it would have needed to
> > acquire to keep going will already have been switched off, too.
> 
> That is not true, even with the current implementation, if all necessary
> drivers are built in, then simplefb will come up later, but it will still
> come up before the late_initcall which disables the clocks.

Yes, in the current implementation because clocks typically are
registered very early and thus you don't hit the deferred probe. The
same is not true for other types of resources where it's actually quite
common to hit deferred probing (regulators is a very notorious one).

It doesn't matter whether a driver is built-in or not, once you hit
deferred probing you lose.

> Once we do the split probing described above (which is something which
> we plan to do when it becomes necessary), then simplefb will still come
> up early.

It will come up early but won't have acquired all the resources that it
needs, so unless you somehow manage to order late_initcalls in exactly
the way that you need them, the frameworks will still turn off what you
haven't managed to claim yet.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141118/8e76abb6/attachment.sig>

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/18/2014 12:21 PM, Thierry Reding wrote:
> On Tue, Nov 18, 2014 at 12:01:08PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11/18/2014 11:19 AM, Thierry Reding wrote:
>>> On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
>>>> Hi Maxime,
>>>>
>>>> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
>>>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>>>> -module_init(simplefb_init);
>>>>>> +/*
>>>>>> + * While this can be a module, if builtin it's most likely the console
>>>>>> + * So let's leave module_exit but move module_init to an earlier place
>>>>>> + */
>>>>>
>>>>> Not really related to this patch itself, but do we want to support
>>>>> simplefb as a module? It seems like it's going to be most of the time
>>>>> broken.
>>>>
>>>> If it depends on clocks, it won't work as a module, as CCF will have disabled
>>>> all unused clocks at that point.
>>>
>>> If it does depend on anything beyond clocks it won't work at all. Clocks
>>> are special because they get set up very early at boot time. If it turns
>>> out that a simplefb ever needs a regulator to remain on, and that's even
>>> quite likely to happen eventually, it's going to fail miserably, because
>>> those regulators will typically be provided by a PMIC on an I2C bus. The
>>> regulator won't be registered until very late into the boot process and
>>> a regulator_get() call will almost certainly cause the simplefb driver
>>> to defer probing.
>>
>> Right, this has been discussed already and the plan is to have simplefb
>> continue its probe function and return success from it if it encounters
>> any -eprobe_defer errors, while tracking which resources it misses.
>>
>> And then have a late_initcall which will claim any resources which failed
>> with -eprobe beforehand.
> 
> How do you ensure that the late_initcall gets run before any of the
> other late_initcalls that disable the resources?

> Also my recollection is
> that deferred probing will first be triggered the first time from a
> late_initcall, so chances aren't very high that all resources have shown
> up by that time.

So I just looked up the relevant code, and your right, this means that
the whole model of "disable unused resources once probing is done" which
we use for e.g. clocks, is already somewhat broken since there is
no guarantee probing is really done when the cleanup code runs.

>>> Now deferring probing is a real showstopper for simplefb, because not
>>> only does it make the framebuffer useless as early boot console, once
>>> probing is attempted again the clocks that it would have needed to
>>> acquire to keep going will already have been switched off, too.
>>
>> That is not true, even with the current implementation, if all necessary
>> drivers are built in, then simplefb will come up later, but it will still
>> come up before the late_initcall which disables the clocks.
> 
> Yes, in the current implementation because clocks typically are
> registered very early and thus you don't hit the deferred probe. The
> same is not true for other types of resources where it's actually quite
> common to hit deferred probing (regulators is a very notorious one).
> 
> It doesn't matter whether a driver is built-in or not, once you hit
> deferred probing you lose.
> 
>> Once we do the split probing described above (which is something which
>> we plan to do when it becomes necessary), then simplefb will still come
>> up early.
> 
> It will come up early but won't have acquired all the resources that it
> needs, so unless you somehow manage to order late_initcalls in exactly
> the way that you need them, the frameworks will still turn off what you
> haven't managed to claim yet.

If it is a resource which only shows up as a result of deferred probing,
then it may very well not have been probed & registered yet, when the
framework cleanup functions runs, and thus will not get turned off...

So yes you're right that deferred probing may cause issues, but it seems
that this is not something simplefb specific, but rather a generic problem
with deferred-probing vs subsys cleanup functions.

My view on this is simple, lets worry about this when we actually have
a board which hits these issues, and then we'll see from there.

Regards,

Hans

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/18/2014 12:21 PM, Thierry Reding wrote:
> On Tue, Nov 18, 2014 at 12:01:08PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11/18/2014 11:19 AM, Thierry Reding wrote:
>>> On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
>>>> Hi Maxime,
>>>>
>>>> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
>>>> <maxime.ripard@free-electrons.com> wrote:
>>>>>> -module_init(simplefb_init);
>>>>>> +/*
>>>>>> + * While this can be a module, if builtin it's most likely the console
>>>>>> + * So let's leave module_exit but move module_init to an earlier place
>>>>>> + */
>>>>>
>>>>> Not really related to this patch itself, but do we want to support
>>>>> simplefb as a module? It seems like it's going to be most of the time
>>>>> broken.
>>>>
>>>> If it depends on clocks, it won't work as a module, as CCF will have disabled
>>>> all unused clocks at that point.
>>>
>>> If it does depend on anything beyond clocks it won't work at all. Clocks
>>> are special because they get set up very early at boot time. If it turns
>>> out that a simplefb ever needs a regulator to remain on, and that's even
>>> quite likely to happen eventually, it's going to fail miserably, because
>>> those regulators will typically be provided by a PMIC on an I2C bus. The
>>> regulator won't be registered until very late into the boot process and
>>> a regulator_get() call will almost certainly cause the simplefb driver
>>> to defer probing.
>>
>> Right, this has been discussed already and the plan is to have simplefb
>> continue its probe function and return success from it if it encounters
>> any -eprobe_defer errors, while tracking which resources it misses.
>>
>> And then have a late_initcall which will claim any resources which failed
>> with -eprobe beforehand.
> 
> How do you ensure that the late_initcall gets run before any of the
> other late_initcalls that disable the resources?

> Also my recollection is
> that deferred probing will first be triggered the first time from a
> late_initcall, so chances aren't very high that all resources have shown
> up by that time.

So I just looked up the relevant code, and your right, this means that
the whole model of "disable unused resources once probing is done" which
we use for e.g. clocks, is already somewhat broken since there is
no guarantee probing is really done when the cleanup code runs.

>>> Now deferring probing is a real showstopper for simplefb, because not
>>> only does it make the framebuffer useless as early boot console, once
>>> probing is attempted again the clocks that it would have needed to
>>> acquire to keep going will already have been switched off, too.
>>
>> That is not true, even with the current implementation, if all necessary
>> drivers are built in, then simplefb will come up later, but it will still
>> come up before the late_initcall which disables the clocks.
> 
> Yes, in the current implementation because clocks typically are
> registered very early and thus you don't hit the deferred probe. The
> same is not true for other types of resources where it's actually quite
> common to hit deferred probing (regulators is a very notorious one).
> 
> It doesn't matter whether a driver is built-in or not, once you hit
> deferred probing you lose.
> 
>> Once we do the split probing described above (which is something which
>> we plan to do when it becomes necessary), then simplefb will still come
>> up early.
> 
> It will come up early but won't have acquired all the resources that it
> needs, so unless you somehow manage to order late_initcalls in exactly
> the way that you need them, the frameworks will still turn off what you
> haven't managed to claim yet.

If it is a resource which only shows up as a result of deferred probing,
then it may very well not have been probed & registered yet, when the
framework cleanup functions runs, and thus will not get turned off...

So yes you're right that deferred probing may cause issues, but it seems
that this is not something simplefb specific, but rather a generic problem
with deferred-probing vs subsys cleanup functions.

My view on this is simple, lets worry about this when we actually have
a board which hits these issues, and then we'll see from there.

Regards,

Hans

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/18/2014 12:21 PM, Thierry Reding wrote:
> On Tue, Nov 18, 2014 at 12:01:08PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11/18/2014 11:19 AM, Thierry Reding wrote:
>>> On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
>>>> Hi Maxime,
>>>>
>>>> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
>>>> <maxime.ripard@free-electrons.com> wrote:
>>>>>> -module_init(simplefb_init);
>>>>>> +/*
>>>>>> + * While this can be a module, if builtin it's most likely the console
>>>>>> + * So let's leave module_exit but move module_init to an earlier place
>>>>>> + */
>>>>>
>>>>> Not really related to this patch itself, but do we want to support
>>>>> simplefb as a module? It seems like it's going to be most of the time
>>>>> broken.
>>>>
>>>> If it depends on clocks, it won't work as a module, as CCF will have disabled
>>>> all unused clocks at that point.
>>>
>>> If it does depend on anything beyond clocks it won't work at all. Clocks
>>> are special because they get set up very early at boot time. If it turns
>>> out that a simplefb ever needs a regulator to remain on, and that's even
>>> quite likely to happen eventually, it's going to fail miserably, because
>>> those regulators will typically be provided by a PMIC on an I2C bus. The
>>> regulator won't be registered until very late into the boot process and
>>> a regulator_get() call will almost certainly cause the simplefb driver
>>> to defer probing.
>>
>> Right, this has been discussed already and the plan is to have simplefb
>> continue its probe function and return success from it if it encounters
>> any -eprobe_defer errors, while tracking which resources it misses.
>>
>> And then have a late_initcall which will claim any resources which failed
>> with -eprobe beforehand.
> 
> How do you ensure that the late_initcall gets run before any of the
> other late_initcalls that disable the resources?

> Also my recollection is
> that deferred probing will first be triggered the first time from a
> late_initcall, so chances aren't very high that all resources have shown
> up by that time.

So I just looked up the relevant code, and your right, this means that
the whole model of "disable unused resources once probing is done" which
we use for e.g. clocks, is already somewhat broken since there is
no guarantee probing is really done when the cleanup code runs.

>>> Now deferring probing is a real showstopper for simplefb, because not
>>> only does it make the framebuffer useless as early boot console, once
>>> probing is attempted again the clocks that it would have needed to
>>> acquire to keep going will already have been switched off, too.
>>
>> That is not true, even with the current implementation, if all necessary
>> drivers are built in, then simplefb will come up later, but it will still
>> come up before the late_initcall which disables the clocks.
> 
> Yes, in the current implementation because clocks typically are
> registered very early and thus you don't hit the deferred probe. The
> same is not true for other types of resources where it's actually quite
> common to hit deferred probing (regulators is a very notorious one).
> 
> It doesn't matter whether a driver is built-in or not, once you hit
> deferred probing you lose.
> 
>> Once we do the split probing described above (which is something which
>> we plan to do when it becomes necessary), then simplefb will still come
>> up early.
> 
> It will come up early but won't have acquired all the resources that it
> needs, so unless you somehow manage to order late_initcalls in exactly
> the way that you need them, the frameworks will still turn off what you
> haven't managed to claim yet.

If it is a resource which only shows up as a result of deferred probing,
then it may very well not have been probed & registered yet, when the
framework cleanup functions runs, and thus will not get turned off...

So yes you're right that deferred probing may cause issues, but it seems
that this is not something simplefb specific, but rather a generic problem
with deferred-probing vs subsys cleanup functions.

My view on this is simple, lets worry about this when we actually have
a board which hits these issues, and then we'll see from there.

Regards,

Hans

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/18/2014 12:46 PM, Hans de Goede wrote:
> Hi,
> 
> On 11/18/2014 12:21 PM, Thierry Reding wrote:
>> On Tue, Nov 18, 2014 at 12:01:08PM +0100, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 11/18/2014 11:19 AM, Thierry Reding wrote:
>>>> On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
>>>>> Hi Maxime,
>>>>>
>>>>> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
>>>>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>>>>> -module_init(simplefb_init);
>>>>>>> +/*
>>>>>>> + * While this can be a module, if builtin it's most likely the console
>>>>>>> + * So let's leave module_exit but move module_init to an earlier place
>>>>>>> + */
>>>>>>
>>>>>> Not really related to this patch itself, but do we want to support
>>>>>> simplefb as a module? It seems like it's going to be most of the time
>>>>>> broken.
>>>>>
>>>>> If it depends on clocks, it won't work as a module, as CCF will have disabled
>>>>> all unused clocks at that point.
>>>>
>>>> If it does depend on anything beyond clocks it won't work at all. Clocks
>>>> are special because they get set up very early at boot time. If it turns
>>>> out that a simplefb ever needs a regulator to remain on, and that's even
>>>> quite likely to happen eventually, it's going to fail miserably, because
>>>> those regulators will typically be provided by a PMIC on an I2C bus. The
>>>> regulator won't be registered until very late into the boot process and
>>>> a regulator_get() call will almost certainly cause the simplefb driver
>>>> to defer probing.
>>>
>>> Right, this has been discussed already and the plan is to have simplefb
>>> continue its probe function and return success from it if it encounters
>>> any -eprobe_defer errors, while tracking which resources it misses.
>>>
>>> And then have a late_initcall which will claim any resources which failed
>>> with -eprobe beforehand.
>>
>> How do you ensure that the late_initcall gets run before any of the
>> other late_initcalls that disable the resources?
> 
>> Also my recollection is
>> that deferred probing will first be triggered the first time from a
>> late_initcall, so chances aren't very high that all resources have shown
>> up by that time.
> 
> So I just looked up the relevant code, and your right, this means that
> the whole model of "disable unused resources once probing is done" which
> we use for e.g. clocks, is already somewhat broken since there is
> no guarantee probing is really done when the cleanup code runs.
> 
>>>> Now deferring probing is a real showstopper for simplefb, because not
>>>> only does it make the framebuffer useless as early boot console, once
>>>> probing is attempted again the clocks that it would have needed to
>>>> acquire to keep going will already have been switched off, too.
>>>
>>> That is not true, even with the current implementation, if all necessary
>>> drivers are built in, then simplefb will come up later, but it will still
>>> come up before the late_initcall which disables the clocks.
>>
>> Yes, in the current implementation because clocks typically are
>> registered very early and thus you don't hit the deferred probe. The
>> same is not true for other types of resources where it's actually quite
>> common to hit deferred probing (regulators is a very notorious one).
>>
>> It doesn't matter whether a driver is built-in or not, once you hit
>> deferred probing you lose.
>>
>>> Once we do the split probing described above (which is something which
>>> we plan to do when it becomes necessary), then simplefb will still come
>>> up early.
>>
>> It will come up early but won't have acquired all the resources that it
>> needs, so unless you somehow manage to order late_initcalls in exactly
>> the way that you need them, the frameworks will still turn off what you
>> haven't managed to claim yet.
> 
> If it is a resource which only shows up as a result of deferred probing,
> then it may very well not have been probed & registered yet, when the
> framework cleanup functions runs, and thus will not get turned off...
> 
> So yes you're right that deferred probing may cause issues, but it seems
> that this is not something simplefb specific, but rather a generic problem
> with deferred-probing vs subsys cleanup functions.
> 
> My view on this is simple, lets worry about this when we actually have
> a board which hits these issues, and then we'll see from there.

So thinking more about this, I think this is not that hard to fix.

First lets fix the generic conflict between eprobedefer and subsys cleanup
functions. This can be done by:

1) Having a linked list of subsys cleanup functions
to call in drivers/base/dd.c

2) Have subsystems register their cleanup function rather then using
late_initcall to get it called

3) Have deferred_probe_work_func iterate over the list and call the cleanup
functions once deferred_probe_active_list goes empty. It should also remove
them once called, so that they only get called the first time
deferred_probe_active_list goes empty (iow when the deferred probing of'
build-in drivers is done).

This way cleanup functions actually get run when all probing of (buildin)
drivers is done.

Then when we move simplefb to a 2 fase probe, we can simply register the
framebuffer at the first probe call, and claim any resources we get, but
return -eprobe_defer if some resources returned that themselves.

Then when simplefb_probe gets re-called, it can first check if it did not
already register a fb, and if it did, it can use that and see which
resources are missing, and only try to claim those. If some resources
still return probe_defer, return eprobe_defer again, otherwise success.

This way simplefb_probe will get called as long as resources are missing
and other drivers are successfully completing deferred probes (if no
driver successfully completes a deferred probe, deferred_probe_active_list
will go empty).

And then when the subsys cleanup functions run, simplefb will have been
able to claim any resources registered by buildin drivers.

Regards,

Hans

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/18/2014 12:46 PM, Hans de Goede wrote:
> Hi,
> 
> On 11/18/2014 12:21 PM, Thierry Reding wrote:
>> On Tue, Nov 18, 2014 at 12:01:08PM +0100, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 11/18/2014 11:19 AM, Thierry Reding wrote:
>>>> On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
>>>>> Hi Maxime,
>>>>>
>>>>> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
>>>>> <maxime.ripard@free-electrons.com> wrote:
>>>>>>> -module_init(simplefb_init);
>>>>>>> +/*
>>>>>>> + * While this can be a module, if builtin it's most likely the console
>>>>>>> + * So let's leave module_exit but move module_init to an earlier place
>>>>>>> + */
>>>>>>
>>>>>> Not really related to this patch itself, but do we want to support
>>>>>> simplefb as a module? It seems like it's going to be most of the time
>>>>>> broken.
>>>>>
>>>>> If it depends on clocks, it won't work as a module, as CCF will have disabled
>>>>> all unused clocks at that point.
>>>>
>>>> If it does depend on anything beyond clocks it won't work at all. Clocks
>>>> are special because they get set up very early at boot time. If it turns
>>>> out that a simplefb ever needs a regulator to remain on, and that's even
>>>> quite likely to happen eventually, it's going to fail miserably, because
>>>> those regulators will typically be provided by a PMIC on an I2C bus. The
>>>> regulator won't be registered until very late into the boot process and
>>>> a regulator_get() call will almost certainly cause the simplefb driver
>>>> to defer probing.
>>>
>>> Right, this has been discussed already and the plan is to have simplefb
>>> continue its probe function and return success from it if it encounters
>>> any -eprobe_defer errors, while tracking which resources it misses.
>>>
>>> And then have a late_initcall which will claim any resources which failed
>>> with -eprobe beforehand.
>>
>> How do you ensure that the late_initcall gets run before any of the
>> other late_initcalls that disable the resources?
> 
>> Also my recollection is
>> that deferred probing will first be triggered the first time from a
>> late_initcall, so chances aren't very high that all resources have shown
>> up by that time.
> 
> So I just looked up the relevant code, and your right, this means that
> the whole model of "disable unused resources once probing is done" which
> we use for e.g. clocks, is already somewhat broken since there is
> no guarantee probing is really done when the cleanup code runs.
> 
>>>> Now deferring probing is a real showstopper for simplefb, because not
>>>> only does it make the framebuffer useless as early boot console, once
>>>> probing is attempted again the clocks that it would have needed to
>>>> acquire to keep going will already have been switched off, too.
>>>
>>> That is not true, even with the current implementation, if all necessary
>>> drivers are built in, then simplefb will come up later, but it will still
>>> come up before the late_initcall which disables the clocks.
>>
>> Yes, in the current implementation because clocks typically are
>> registered very early and thus you don't hit the deferred probe. The
>> same is not true for other types of resources where it's actually quite
>> common to hit deferred probing (regulators is a very notorious one).
>>
>> It doesn't matter whether a driver is built-in or not, once you hit
>> deferred probing you lose.
>>
>>> Once we do the split probing described above (which is something which
>>> we plan to do when it becomes necessary), then simplefb will still come
>>> up early.
>>
>> It will come up early but won't have acquired all the resources that it
>> needs, so unless you somehow manage to order late_initcalls in exactly
>> the way that you need them, the frameworks will still turn off what you
>> haven't managed to claim yet.
> 
> If it is a resource which only shows up as a result of deferred probing,
> then it may very well not have been probed & registered yet, when the
> framework cleanup functions runs, and thus will not get turned off...
> 
> So yes you're right that deferred probing may cause issues, but it seems
> that this is not something simplefb specific, but rather a generic problem
> with deferred-probing vs subsys cleanup functions.
> 
> My view on this is simple, lets worry about this when we actually have
> a board which hits these issues, and then we'll see from there.

So thinking more about this, I think this is not that hard to fix.

First lets fix the generic conflict between eprobedefer and subsys cleanup
functions. This can be done by:

1) Having a linked list of subsys cleanup functions
to call in drivers/base/dd.c

2) Have subsystems register their cleanup function rather then using
late_initcall to get it called

3) Have deferred_probe_work_func iterate over the list and call the cleanup
functions once deferred_probe_active_list goes empty. It should also remove
them once called, so that they only get called the first time
deferred_probe_active_list goes empty (iow when the deferred probing of'
build-in drivers is done).

This way cleanup functions actually get run when all probing of (buildin)
drivers is done.

Then when we move simplefb to a 2 fase probe, we can simply register the
framebuffer at the first probe call, and claim any resources we get, but
return -eprobe_defer if some resources returned that themselves.

Then when simplefb_probe gets re-called, it can first check if it did not
already register a fb, and if it did, it can use that and see which
resources are missing, and only try to claim those. If some resources
still return probe_defer, return eprobe_defer again, otherwise success.

This way simplefb_probe will get called as long as resources are missing
and other drivers are successfully completing deferred probes (if no
driver successfully completes a deferred probe, deferred_probe_active_list
will go empty).

And then when the subsys cleanup functions run, simplefb will have been
able to claim any resources registered by buildin drivers.

Regards,

Hans

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

Hi,

On 11/18/2014 12:46 PM, Hans de Goede wrote:
> Hi,
> 
> On 11/18/2014 12:21 PM, Thierry Reding wrote:
>> On Tue, Nov 18, 2014 at 12:01:08PM +0100, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 11/18/2014 11:19 AM, Thierry Reding wrote:
>>>> On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
>>>>> Hi Maxime,
>>>>>
>>>>> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
>>>>> <maxime.ripard@free-electrons.com> wrote:
>>>>>>> -module_init(simplefb_init);
>>>>>>> +/*
>>>>>>> + * While this can be a module, if builtin it's most likely the console
>>>>>>> + * So let's leave module_exit but move module_init to an earlier place
>>>>>>> + */
>>>>>>
>>>>>> Not really related to this patch itself, but do we want to support
>>>>>> simplefb as a module? It seems like it's going to be most of the time
>>>>>> broken.
>>>>>
>>>>> If it depends on clocks, it won't work as a module, as CCF will have disabled
>>>>> all unused clocks at that point.
>>>>
>>>> If it does depend on anything beyond clocks it won't work at all. Clocks
>>>> are special because they get set up very early at boot time. If it turns
>>>> out that a simplefb ever needs a regulator to remain on, and that's even
>>>> quite likely to happen eventually, it's going to fail miserably, because
>>>> those regulators will typically be provided by a PMIC on an I2C bus. The
>>>> regulator won't be registered until very late into the boot process and
>>>> a regulator_get() call will almost certainly cause the simplefb driver
>>>> to defer probing.
>>>
>>> Right, this has been discussed already and the plan is to have simplefb
>>> continue its probe function and return success from it if it encounters
>>> any -eprobe_defer errors, while tracking which resources it misses.
>>>
>>> And then have a late_initcall which will claim any resources which failed
>>> with -eprobe beforehand.
>>
>> How do you ensure that the late_initcall gets run before any of the
>> other late_initcalls that disable the resources?
> 
>> Also my recollection is
>> that deferred probing will first be triggered the first time from a
>> late_initcall, so chances aren't very high that all resources have shown
>> up by that time.
> 
> So I just looked up the relevant code, and your right, this means that
> the whole model of "disable unused resources once probing is done" which
> we use for e.g. clocks, is already somewhat broken since there is
> no guarantee probing is really done when the cleanup code runs.
> 
>>>> Now deferring probing is a real showstopper for simplefb, because not
>>>> only does it make the framebuffer useless as early boot console, once
>>>> probing is attempted again the clocks that it would have needed to
>>>> acquire to keep going will already have been switched off, too.
>>>
>>> That is not true, even with the current implementation, if all necessary
>>> drivers are built in, then simplefb will come up later, but it will still
>>> come up before the late_initcall which disables the clocks.
>>
>> Yes, in the current implementation because clocks typically are
>> registered very early and thus you don't hit the deferred probe. The
>> same is not true for other types of resources where it's actually quite
>> common to hit deferred probing (regulators is a very notorious one).
>>
>> It doesn't matter whether a driver is built-in or not, once you hit
>> deferred probing you lose.
>>
>>> Once we do the split probing described above (which is something which
>>> we plan to do when it becomes necessary), then simplefb will still come
>>> up early.
>>
>> It will come up early but won't have acquired all the resources that it
>> needs, so unless you somehow manage to order late_initcalls in exactly
>> the way that you need them, the frameworks will still turn off what you
>> haven't managed to claim yet.
> 
> If it is a resource which only shows up as a result of deferred probing,
> then it may very well not have been probed & registered yet, when the
> framework cleanup functions runs, and thus will not get turned off...
> 
> So yes you're right that deferred probing may cause issues, but it seems
> that this is not something simplefb specific, but rather a generic problem
> with deferred-probing vs subsys cleanup functions.
> 
> My view on this is simple, lets worry about this when we actually have
> a board which hits these issues, and then we'll see from there.

So thinking more about this, I think this is not that hard to fix.

First lets fix the generic conflict between eprobedefer and subsys cleanup
functions. This can be done by:

1) Having a linked list of subsys cleanup functions
to call in drivers/base/dd.c

2) Have subsystems register their cleanup function rather then using
late_initcall to get it called

3) Have deferred_probe_work_func iterate over the list and call the cleanup
functions once deferred_probe_active_list goes empty. It should also remove
them once called, so that they only get called the first time
deferred_probe_active_list goes empty (iow when the deferred probing of'
build-in drivers is done).

This way cleanup functions actually get run when all probing of (buildin)
drivers is done.

Then when we move simplefb to a 2 fase probe, we can simply register the
framebuffer at the first probe call, and claim any resources we get, but
return -eprobe_defer if some resources returned that themselves.

Then when simplefb_probe gets re-called, it can first check if it did not
already register a fb, and if it did, it can use that and see which
resources are missing, and only try to claim those. If some resources
still return probe_defer, return eprobe_defer again, otherwise success.

This way simplefb_probe will get called as long as resources are missing
and other drivers are successfully completing deferred probes (if no
driver successfully completes a deferred probe, deferred_probe_active_list
will go empty).

And then when the subsys cleanup functions run, simplefb will have been
able to claim any resources registered by buildin drivers.

Regards,

Hans

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6549 bytes --]

On Tue, Nov 18, 2014 at 01:44:44PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/18/2014 12:46 PM, Hans de Goede wrote:
> > Hi,
> > 
> > On 11/18/2014 12:21 PM, Thierry Reding wrote:
> >> On Tue, Nov 18, 2014 at 12:01:08PM +0100, Hans de Goede wrote:
> >>> Hi,
> >>>
> >>> On 11/18/2014 11:19 AM, Thierry Reding wrote:
> >>>> On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
> >>>>> Hi Maxime,
> >>>>>
> >>>>> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
> >>>>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >>>>>>> -module_init(simplefb_init);
> >>>>>>> +/*
> >>>>>>> + * While this can be a module, if builtin it's most likely the console
> >>>>>>> + * So let's leave module_exit but move module_init to an earlier place
> >>>>>>> + */
> >>>>>>
> >>>>>> Not really related to this patch itself, but do we want to support
> >>>>>> simplefb as a module? It seems like it's going to be most of the time
> >>>>>> broken.
> >>>>>
> >>>>> If it depends on clocks, it won't work as a module, as CCF will have disabled
> >>>>> all unused clocks at that point.
> >>>>
> >>>> If it does depend on anything beyond clocks it won't work at all. Clocks
> >>>> are special because they get set up very early at boot time. If it turns
> >>>> out that a simplefb ever needs a regulator to remain on, and that's even
> >>>> quite likely to happen eventually, it's going to fail miserably, because
> >>>> those regulators will typically be provided by a PMIC on an I2C bus. The
> >>>> regulator won't be registered until very late into the boot process and
> >>>> a regulator_get() call will almost certainly cause the simplefb driver
> >>>> to defer probing.
> >>>
> >>> Right, this has been discussed already and the plan is to have simplefb
> >>> continue its probe function and return success from it if it encounters
> >>> any -eprobe_defer errors, while tracking which resources it misses.
> >>>
> >>> And then have a late_initcall which will claim any resources which failed
> >>> with -eprobe beforehand.
> >>
> >> How do you ensure that the late_initcall gets run before any of the
> >> other late_initcalls that disable the resources?
> > 
> >> Also my recollection is
> >> that deferred probing will first be triggered the first time from a
> >> late_initcall, so chances aren't very high that all resources have shown
> >> up by that time.
> > 
> > So I just looked up the relevant code, and your right, this means that
> > the whole model of "disable unused resources once probing is done" which
> > we use for e.g. clocks, is already somewhat broken since there is
> > no guarantee probing is really done when the cleanup code runs.
> > 
> >>>> Now deferring probing is a real showstopper for simplefb, because not
> >>>> only does it make the framebuffer useless as early boot console, once
> >>>> probing is attempted again the clocks that it would have needed to
> >>>> acquire to keep going will already have been switched off, too.
> >>>
> >>> That is not true, even with the current implementation, if all necessary
> >>> drivers are built in, then simplefb will come up later, but it will still
> >>> come up before the late_initcall which disables the clocks.
> >>
> >> Yes, in the current implementation because clocks typically are
> >> registered very early and thus you don't hit the deferred probe. The
> >> same is not true for other types of resources where it's actually quite
> >> common to hit deferred probing (regulators is a very notorious one).
> >>
> >> It doesn't matter whether a driver is built-in or not, once you hit
> >> deferred probing you lose.
> >>
> >>> Once we do the split probing described above (which is something which
> >>> we plan to do when it becomes necessary), then simplefb will still come
> >>> up early.
> >>
> >> It will come up early but won't have acquired all the resources that it
> >> needs, so unless you somehow manage to order late_initcalls in exactly
> >> the way that you need them, the frameworks will still turn off what you
> >> haven't managed to claim yet.
> > 
> > If it is a resource which only shows up as a result of deferred probing,
> > then it may very well not have been probed & registered yet, when the
> > framework cleanup functions runs, and thus will not get turned off...
> > 
> > So yes you're right that deferred probing may cause issues, but it seems
> > that this is not something simplefb specific, but rather a generic problem
> > with deferred-probing vs subsys cleanup functions.
> > 
> > My view on this is simple, lets worry about this when we actually have
> > a board which hits these issues, and then we'll see from there.
> 
> So thinking more about this, I think this is not that hard to fix.
> 
> First lets fix the generic conflict between eprobedefer and subsys cleanup
> functions. This can be done by:
> 
> 1) Having a linked list of subsys cleanup functions
> to call in drivers/base/dd.c
> 
> 2) Have subsystems register their cleanup function rather then using
> late_initcall to get it called
> 
> 3) Have deferred_probe_work_func iterate over the list and call the cleanup
> functions once deferred_probe_active_list goes empty. It should also remove
> them once called, so that they only get called the first time
> deferred_probe_active_list goes empty (iow when the deferred probing of'
> build-in drivers is done).
> 
> This way cleanup functions actually get run when all probing of (buildin)
> drivers is done.

That sounds like a reasonable solution to me.

> Then when we move simplefb to a 2 fase probe, we can simply register the
> framebuffer at the first probe call, and claim any resources we get, but
> return -eprobe_defer if some resources returned that themselves.
> 
> Then when simplefb_probe gets re-called, it can first check if it did not
> already register a fb, and if it did, it can use that and see which
> resources are missing, and only try to claim those. If some resources
> still return probe_defer, return eprobe_defer again, otherwise success.

I'm not sure you can check that the fb was already registered. When the
driver probe fails the driver core will pretty much remove any trace of
the driver, so checking for an already-reagistered framebuffer isn't
going to be easy.

But if you can manage to make that work it should give us a pretty good
solution that should be easy to make use of by other drivers as well.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6518 bytes --]

On Tue, Nov 18, 2014 at 01:44:44PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/18/2014 12:46 PM, Hans de Goede wrote:
> > Hi,
> > 
> > On 11/18/2014 12:21 PM, Thierry Reding wrote:
> >> On Tue, Nov 18, 2014 at 12:01:08PM +0100, Hans de Goede wrote:
> >>> Hi,
> >>>
> >>> On 11/18/2014 11:19 AM, Thierry Reding wrote:
> >>>> On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
> >>>>> Hi Maxime,
> >>>>>
> >>>>> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
> >>>>> <maxime.ripard@free-electrons.com> wrote:
> >>>>>>> -module_init(simplefb_init);
> >>>>>>> +/*
> >>>>>>> + * While this can be a module, if builtin it's most likely the console
> >>>>>>> + * So let's leave module_exit but move module_init to an earlier place
> >>>>>>> + */
> >>>>>>
> >>>>>> Not really related to this patch itself, but do we want to support
> >>>>>> simplefb as a module? It seems like it's going to be most of the time
> >>>>>> broken.
> >>>>>
> >>>>> If it depends on clocks, it won't work as a module, as CCF will have disabled
> >>>>> all unused clocks at that point.
> >>>>
> >>>> If it does depend on anything beyond clocks it won't work at all. Clocks
> >>>> are special because they get set up very early at boot time. If it turns
> >>>> out that a simplefb ever needs a regulator to remain on, and that's even
> >>>> quite likely to happen eventually, it's going to fail miserably, because
> >>>> those regulators will typically be provided by a PMIC on an I2C bus. The
> >>>> regulator won't be registered until very late into the boot process and
> >>>> a regulator_get() call will almost certainly cause the simplefb driver
> >>>> to defer probing.
> >>>
> >>> Right, this has been discussed already and the plan is to have simplefb
> >>> continue its probe function and return success from it if it encounters
> >>> any -eprobe_defer errors, while tracking which resources it misses.
> >>>
> >>> And then have a late_initcall which will claim any resources which failed
> >>> with -eprobe beforehand.
> >>
> >> How do you ensure that the late_initcall gets run before any of the
> >> other late_initcalls that disable the resources?
> > 
> >> Also my recollection is
> >> that deferred probing will first be triggered the first time from a
> >> late_initcall, so chances aren't very high that all resources have shown
> >> up by that time.
> > 
> > So I just looked up the relevant code, and your right, this means that
> > the whole model of "disable unused resources once probing is done" which
> > we use for e.g. clocks, is already somewhat broken since there is
> > no guarantee probing is really done when the cleanup code runs.
> > 
> >>>> Now deferring probing is a real showstopper for simplefb, because not
> >>>> only does it make the framebuffer useless as early boot console, once
> >>>> probing is attempted again the clocks that it would have needed to
> >>>> acquire to keep going will already have been switched off, too.
> >>>
> >>> That is not true, even with the current implementation, if all necessary
> >>> drivers are built in, then simplefb will come up later, but it will still
> >>> come up before the late_initcall which disables the clocks.
> >>
> >> Yes, in the current implementation because clocks typically are
> >> registered very early and thus you don't hit the deferred probe. The
> >> same is not true for other types of resources where it's actually quite
> >> common to hit deferred probing (regulators is a very notorious one).
> >>
> >> It doesn't matter whether a driver is built-in or not, once you hit
> >> deferred probing you lose.
> >>
> >>> Once we do the split probing described above (which is something which
> >>> we plan to do when it becomes necessary), then simplefb will still come
> >>> up early.
> >>
> >> It will come up early but won't have acquired all the resources that it
> >> needs, so unless you somehow manage to order late_initcalls in exactly
> >> the way that you need them, the frameworks will still turn off what you
> >> haven't managed to claim yet.
> > 
> > If it is a resource which only shows up as a result of deferred probing,
> > then it may very well not have been probed & registered yet, when the
> > framework cleanup functions runs, and thus will not get turned off...
> > 
> > So yes you're right that deferred probing may cause issues, but it seems
> > that this is not something simplefb specific, but rather a generic problem
> > with deferred-probing vs subsys cleanup functions.
> > 
> > My view on this is simple, lets worry about this when we actually have
> > a board which hits these issues, and then we'll see from there.
> 
> So thinking more about this, I think this is not that hard to fix.
> 
> First lets fix the generic conflict between eprobedefer and subsys cleanup
> functions. This can be done by:
> 
> 1) Having a linked list of subsys cleanup functions
> to call in drivers/base/dd.c
> 
> 2) Have subsystems register their cleanup function rather then using
> late_initcall to get it called
> 
> 3) Have deferred_probe_work_func iterate over the list and call the cleanup
> functions once deferred_probe_active_list goes empty. It should also remove
> them once called, so that they only get called the first time
> deferred_probe_active_list goes empty (iow when the deferred probing of'
> build-in drivers is done).
> 
> This way cleanup functions actually get run when all probing of (buildin)
> drivers is done.

That sounds like a reasonable solution to me.

> Then when we move simplefb to a 2 fase probe, we can simply register the
> framebuffer at the first probe call, and claim any resources we get, but
> return -eprobe_defer if some resources returned that themselves.
> 
> Then when simplefb_probe gets re-called, it can first check if it did not
> already register a fb, and if it did, it can use that and see which
> resources are missing, and only try to claim those. If some resources
> still return probe_defer, return eprobe_defer again, otherwise success.

I'm not sure you can check that the fb was already registered. When the
driver probe fails the driver core will pretty much remove any trace of
the driver, so checking for an already-reagistered framebuffer isn't
going to be easy.

But if you can manage to make that work it should give us a pretty good
solution that should be easy to make use of by other drivers as well.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Thierry Reding]

On Tue, Nov 18, 2014 at 01:44:44PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/18/2014 12:46 PM, Hans de Goede wrote:
> > Hi,
> > 
> > On 11/18/2014 12:21 PM, Thierry Reding wrote:
> >> On Tue, Nov 18, 2014 at 12:01:08PM +0100, Hans de Goede wrote:
> >>> Hi,
> >>>
> >>> On 11/18/2014 11:19 AM, Thierry Reding wrote:
> >>>> On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
> >>>>> Hi Maxime,
> >>>>>
> >>>>> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
> >>>>> <maxime.ripard@free-electrons.com> wrote:
> >>>>>>> -module_init(simplefb_init);
> >>>>>>> +/*
> >>>>>>> + * While this can be a module, if builtin it's most likely the console
> >>>>>>> + * So let's leave module_exit but move module_init to an earlier place
> >>>>>>> + */
> >>>>>>
> >>>>>> Not really related to this patch itself, but do we want to support
> >>>>>> simplefb as a module? It seems like it's going to be most of the time
> >>>>>> broken.
> >>>>>
> >>>>> If it depends on clocks, it won't work as a module, as CCF will have disabled
> >>>>> all unused clocks at that point.
> >>>>
> >>>> If it does depend on anything beyond clocks it won't work at all. Clocks
> >>>> are special because they get set up very early at boot time. If it turns
> >>>> out that a simplefb ever needs a regulator to remain on, and that's even
> >>>> quite likely to happen eventually, it's going to fail miserably, because
> >>>> those regulators will typically be provided by a PMIC on an I2C bus. The
> >>>> regulator won't be registered until very late into the boot process and
> >>>> a regulator_get() call will almost certainly cause the simplefb driver
> >>>> to defer probing.
> >>>
> >>> Right, this has been discussed already and the plan is to have simplefb
> >>> continue its probe function and return success from it if it encounters
> >>> any -eprobe_defer errors, while tracking which resources it misses.
> >>>
> >>> And then have a late_initcall which will claim any resources which failed
> >>> with -eprobe beforehand.
> >>
> >> How do you ensure that the late_initcall gets run before any of the
> >> other late_initcalls that disable the resources?
> > 
> >> Also my recollection is
> >> that deferred probing will first be triggered the first time from a
> >> late_initcall, so chances aren't very high that all resources have shown
> >> up by that time.
> > 
> > So I just looked up the relevant code, and your right, this means that
> > the whole model of "disable unused resources once probing is done" which
> > we use for e.g. clocks, is already somewhat broken since there is
> > no guarantee probing is really done when the cleanup code runs.
> > 
> >>>> Now deferring probing is a real showstopper for simplefb, because not
> >>>> only does it make the framebuffer useless as early boot console, once
> >>>> probing is attempted again the clocks that it would have needed to
> >>>> acquire to keep going will already have been switched off, too.
> >>>
> >>> That is not true, even with the current implementation, if all necessary
> >>> drivers are built in, then simplefb will come up later, but it will still
> >>> come up before the late_initcall which disables the clocks.
> >>
> >> Yes, in the current implementation because clocks typically are
> >> registered very early and thus you don't hit the deferred probe. The
> >> same is not true for other types of resources where it's actually quite
> >> common to hit deferred probing (regulators is a very notorious one).
> >>
> >> It doesn't matter whether a driver is built-in or not, once you hit
> >> deferred probing you lose.
> >>
> >>> Once we do the split probing described above (which is something which
> >>> we plan to do when it becomes necessary), then simplefb will still come
> >>> up early.
> >>
> >> It will come up early but won't have acquired all the resources that it
> >> needs, so unless you somehow manage to order late_initcalls in exactly
> >> the way that you need them, the frameworks will still turn off what you
> >> haven't managed to claim yet.
> > 
> > If it is a resource which only shows up as a result of deferred probing,
> > then it may very well not have been probed & registered yet, when the
> > framework cleanup functions runs, and thus will not get turned off...
> > 
> > So yes you're right that deferred probing may cause issues, but it seems
> > that this is not something simplefb specific, but rather a generic problem
> > with deferred-probing vs subsys cleanup functions.
> > 
> > My view on this is simple, lets worry about this when we actually have
> > a board which hits these issues, and then we'll see from there.
> 
> So thinking more about this, I think this is not that hard to fix.
> 
> First lets fix the generic conflict between eprobedefer and subsys cleanup
> functions. This can be done by:
> 
> 1) Having a linked list of subsys cleanup functions
> to call in drivers/base/dd.c
> 
> 2) Have subsystems register their cleanup function rather then using
> late_initcall to get it called
> 
> 3) Have deferred_probe_work_func iterate over the list and call the cleanup
> functions once deferred_probe_active_list goes empty. It should also remove
> them once called, so that they only get called the first time
> deferred_probe_active_list goes empty (iow when the deferred probing of'
> build-in drivers is done).
> 
> This way cleanup functions actually get run when all probing of (buildin)
> drivers is done.

That sounds like a reasonable solution to me.

> Then when we move simplefb to a 2 fase probe, we can simply register the
> framebuffer at the first probe call, and claim any resources we get, but
> return -eprobe_defer if some resources returned that themselves.
> 
> Then when simplefb_probe gets re-called, it can first check if it did not
> already register a fb, and if it did, it can use that and see which
> resources are missing, and only try to claim those. If some resources
> still return probe_defer, return eprobe_defer again, otherwise success.

I'm not sure you can check that the fb was already registered. When the
driver probe fails the driver core will pretty much remove any trace of
the driver, so checking for an already-reagistered framebuffer isn't
going to be easy.

But if you can manage to make that work it should give us a pretty good
solution that should be easy to make use of by other drivers as well.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141118/f3066bf5/attachment.sig>

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 7068 bytes --]

On Tue, Nov 18, 2014 at 04:16:04PM +0100, Thierry Reding wrote:
> On Tue, Nov 18, 2014 at 01:44:44PM +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 11/18/2014 12:46 PM, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 11/18/2014 12:21 PM, Thierry Reding wrote:
> > >> On Tue, Nov 18, 2014 at 12:01:08PM +0100, Hans de Goede wrote:
> > >>> Hi,
> > >>>
> > >>> On 11/18/2014 11:19 AM, Thierry Reding wrote:
> > >>>> On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
> > >>>>> Hi Maxime,
> > >>>>>
> > >>>>> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
> > >>>>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > >>>>>>> -module_init(simplefb_init);
> > >>>>>>> +/*
> > >>>>>>> + * While this can be a module, if builtin it's most likely the console
> > >>>>>>> + * So let's leave module_exit but move module_init to an earlier place
> > >>>>>>> + */
> > >>>>>>
> > >>>>>> Not really related to this patch itself, but do we want to support
> > >>>>>> simplefb as a module? It seems like it's going to be most of the time
> > >>>>>> broken.
> > >>>>>
> > >>>>> If it depends on clocks, it won't work as a module, as CCF will have disabled
> > >>>>> all unused clocks at that point.
> > >>>>
> > >>>> If it does depend on anything beyond clocks it won't work at all. Clocks
> > >>>> are special because they get set up very early at boot time. If it turns
> > >>>> out that a simplefb ever needs a regulator to remain on, and that's even
> > >>>> quite likely to happen eventually, it's going to fail miserably, because
> > >>>> those regulators will typically be provided by a PMIC on an I2C bus. The
> > >>>> regulator won't be registered until very late into the boot process and
> > >>>> a regulator_get() call will almost certainly cause the simplefb driver
> > >>>> to defer probing.
> > >>>
> > >>> Right, this has been discussed already and the plan is to have simplefb
> > >>> continue its probe function and return success from it if it encounters
> > >>> any -eprobe_defer errors, while tracking which resources it misses.
> > >>>
> > >>> And then have a late_initcall which will claim any resources which failed
> > >>> with -eprobe beforehand.
> > >>
> > >> How do you ensure that the late_initcall gets run before any of the
> > >> other late_initcalls that disable the resources?
> > > 
> > >> Also my recollection is
> > >> that deferred probing will first be triggered the first time from a
> > >> late_initcall, so chances aren't very high that all resources have shown
> > >> up by that time.
> > > 
> > > So I just looked up the relevant code, and your right, this means that
> > > the whole model of "disable unused resources once probing is done" which
> > > we use for e.g. clocks, is already somewhat broken since there is
> > > no guarantee probing is really done when the cleanup code runs.
> > > 
> > >>>> Now deferring probing is a real showstopper for simplefb, because not
> > >>>> only does it make the framebuffer useless as early boot console, once
> > >>>> probing is attempted again the clocks that it would have needed to
> > >>>> acquire to keep going will already have been switched off, too.
> > >>>
> > >>> That is not true, even with the current implementation, if all necessary
> > >>> drivers are built in, then simplefb will come up later, but it will still
> > >>> come up before the late_initcall which disables the clocks.
> > >>
> > >> Yes, in the current implementation because clocks typically are
> > >> registered very early and thus you don't hit the deferred probe. The
> > >> same is not true for other types of resources where it's actually quite
> > >> common to hit deferred probing (regulators is a very notorious one).
> > >>
> > >> It doesn't matter whether a driver is built-in or not, once you hit
> > >> deferred probing you lose.
> > >>
> > >>> Once we do the split probing described above (which is something which
> > >>> we plan to do when it becomes necessary), then simplefb will still come
> > >>> up early.
> > >>
> > >> It will come up early but won't have acquired all the resources that it
> > >> needs, so unless you somehow manage to order late_initcalls in exactly
> > >> the way that you need them, the frameworks will still turn off what you
> > >> haven't managed to claim yet.
> > > 
> > > If it is a resource which only shows up as a result of deferred probing,
> > > then it may very well not have been probed & registered yet, when the
> > > framework cleanup functions runs, and thus will not get turned off...
> > > 
> > > So yes you're right that deferred probing may cause issues, but it seems
> > > that this is not something simplefb specific, but rather a generic problem
> > > with deferred-probing vs subsys cleanup functions.
> > > 
> > > My view on this is simple, lets worry about this when we actually have
> > > a board which hits these issues, and then we'll see from there.
> > 
> > So thinking more about this, I think this is not that hard to fix.
> > 
> > First lets fix the generic conflict between eprobedefer and subsys cleanup
> > functions. This can be done by:
> > 
> > 1) Having a linked list of subsys cleanup functions
> > to call in drivers/base/dd.c
> > 
> > 2) Have subsystems register their cleanup function rather then using
> > late_initcall to get it called
> > 
> > 3) Have deferred_probe_work_func iterate over the list and call the cleanup
> > functions once deferred_probe_active_list goes empty. It should also remove
> > them once called, so that they only get called the first time
> > deferred_probe_active_list goes empty (iow when the deferred probing of'
> > build-in drivers is done).
> > 
> > This way cleanup functions actually get run when all probing of (buildin)
> > drivers is done.
> 
> That sounds like a reasonable solution to me.
> 
> > Then when we move simplefb to a 2 fase probe, we can simply register the
> > framebuffer at the first probe call, and claim any resources we get, but
> > return -eprobe_defer if some resources returned that themselves.
> > 
> > Then when simplefb_probe gets re-called, it can first check if it did not
> > already register a fb, and if it did, it can use that and see which
> > resources are missing, and only try to claim those. If some resources
> > still return probe_defer, return eprobe_defer again, otherwise success.
> 
> I'm not sure you can check that the fb was already registered. When the
> driver probe fails the driver core will pretty much remove any trace of
> the driver, so checking for an already-reagistered framebuffer isn't
> going to be easy.
> 
> But if you can manage to make that work it should give us a pretty good
> solution that should be easy to make use of by other drivers as well.

What about handling priorities in the suggested cleanup mechanism, and
just add the second phase as the highest priority cleanup callback?

That way, we would be guaranteed to be run just before any cleanup
function.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 7185 bytes --]

On Tue, Nov 18, 2014 at 04:16:04PM +0100, Thierry Reding wrote:
> On Tue, Nov 18, 2014 at 01:44:44PM +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 11/18/2014 12:46 PM, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 11/18/2014 12:21 PM, Thierry Reding wrote:
> > >> On Tue, Nov 18, 2014 at 12:01:08PM +0100, Hans de Goede wrote:
> > >>> Hi,
> > >>>
> > >>> On 11/18/2014 11:19 AM, Thierry Reding wrote:
> > >>>> On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
> > >>>>> Hi Maxime,
> > >>>>>
> > >>>>> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
> > >>>>> <maxime.ripard@free-electrons.com> wrote:
> > >>>>>>> -module_init(simplefb_init);
> > >>>>>>> +/*
> > >>>>>>> + * While this can be a module, if builtin it's most likely the console
> > >>>>>>> + * So let's leave module_exit but move module_init to an earlier place
> > >>>>>>> + */
> > >>>>>>
> > >>>>>> Not really related to this patch itself, but do we want to support
> > >>>>>> simplefb as a module? It seems like it's going to be most of the time
> > >>>>>> broken.
> > >>>>>
> > >>>>> If it depends on clocks, it won't work as a module, as CCF will have disabled
> > >>>>> all unused clocks at that point.
> > >>>>
> > >>>> If it does depend on anything beyond clocks it won't work at all. Clocks
> > >>>> are special because they get set up very early at boot time. If it turns
> > >>>> out that a simplefb ever needs a regulator to remain on, and that's even
> > >>>> quite likely to happen eventually, it's going to fail miserably, because
> > >>>> those regulators will typically be provided by a PMIC on an I2C bus. The
> > >>>> regulator won't be registered until very late into the boot process and
> > >>>> a regulator_get() call will almost certainly cause the simplefb driver
> > >>>> to defer probing.
> > >>>
> > >>> Right, this has been discussed already and the plan is to have simplefb
> > >>> continue its probe function and return success from it if it encounters
> > >>> any -eprobe_defer errors, while tracking which resources it misses.
> > >>>
> > >>> And then have a late_initcall which will claim any resources which failed
> > >>> with -eprobe beforehand.
> > >>
> > >> How do you ensure that the late_initcall gets run before any of the
> > >> other late_initcalls that disable the resources?
> > > 
> > >> Also my recollection is
> > >> that deferred probing will first be triggered the first time from a
> > >> late_initcall, so chances aren't very high that all resources have shown
> > >> up by that time.
> > > 
> > > So I just looked up the relevant code, and your right, this means that
> > > the whole model of "disable unused resources once probing is done" which
> > > we use for e.g. clocks, is already somewhat broken since there is
> > > no guarantee probing is really done when the cleanup code runs.
> > > 
> > >>>> Now deferring probing is a real showstopper for simplefb, because not
> > >>>> only does it make the framebuffer useless as early boot console, once
> > >>>> probing is attempted again the clocks that it would have needed to
> > >>>> acquire to keep going will already have been switched off, too.
> > >>>
> > >>> That is not true, even with the current implementation, if all necessary
> > >>> drivers are built in, then simplefb will come up later, but it will still
> > >>> come up before the late_initcall which disables the clocks.
> > >>
> > >> Yes, in the current implementation because clocks typically are
> > >> registered very early and thus you don't hit the deferred probe. The
> > >> same is not true for other types of resources where it's actually quite
> > >> common to hit deferred probing (regulators is a very notorious one).
> > >>
> > >> It doesn't matter whether a driver is built-in or not, once you hit
> > >> deferred probing you lose.
> > >>
> > >>> Once we do the split probing described above (which is something which
> > >>> we plan to do when it becomes necessary), then simplefb will still come
> > >>> up early.
> > >>
> > >> It will come up early but won't have acquired all the resources that it
> > >> needs, so unless you somehow manage to order late_initcalls in exactly
> > >> the way that you need them, the frameworks will still turn off what you
> > >> haven't managed to claim yet.
> > > 
> > > If it is a resource which only shows up as a result of deferred probing,
> > > then it may very well not have been probed & registered yet, when the
> > > framework cleanup functions runs, and thus will not get turned off...
> > > 
> > > So yes you're right that deferred probing may cause issues, but it seems
> > > that this is not something simplefb specific, but rather a generic problem
> > > with deferred-probing vs subsys cleanup functions.
> > > 
> > > My view on this is simple, lets worry about this when we actually have
> > > a board which hits these issues, and then we'll see from there.
> > 
> > So thinking more about this, I think this is not that hard to fix.
> > 
> > First lets fix the generic conflict between eprobedefer and subsys cleanup
> > functions. This can be done by:
> > 
> > 1) Having a linked list of subsys cleanup functions
> > to call in drivers/base/dd.c
> > 
> > 2) Have subsystems register their cleanup function rather then using
> > late_initcall to get it called
> > 
> > 3) Have deferred_probe_work_func iterate over the list and call the cleanup
> > functions once deferred_probe_active_list goes empty. It should also remove
> > them once called, so that they only get called the first time
> > deferred_probe_active_list goes empty (iow when the deferred probing of'
> > build-in drivers is done).
> > 
> > This way cleanup functions actually get run when all probing of (buildin)
> > drivers is done.
> 
> That sounds like a reasonable solution to me.
> 
> > Then when we move simplefb to a 2 fase probe, we can simply register the
> > framebuffer at the first probe call, and claim any resources we get, but
> > return -eprobe_defer if some resources returned that themselves.
> > 
> > Then when simplefb_probe gets re-called, it can first check if it did not
> > already register a fb, and if it did, it can use that and see which
> > resources are missing, and only try to claim those. If some resources
> > still return probe_defer, return eprobe_defer again, otherwise success.
> 
> I'm not sure you can check that the fb was already registered. When the
> driver probe fails the driver core will pretty much remove any trace of
> the driver, so checking for an already-reagistered framebuffer isn't
> going to be easy.
> 
> But if you can manage to make that work it should give us a pretty good
> solution that should be easy to make use of by other drivers as well.

What about handling priorities in the suggested cleanup mechanism, and
just add the second phase as the highest priority cleanup callback?

That way, we would be guaranteed to be run just before any cleanup
function.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Maxime Ripard]

On Tue, Nov 18, 2014 at 04:16:04PM +0100, Thierry Reding wrote:
> On Tue, Nov 18, 2014 at 01:44:44PM +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 11/18/2014 12:46 PM, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 11/18/2014 12:21 PM, Thierry Reding wrote:
> > >> On Tue, Nov 18, 2014 at 12:01:08PM +0100, Hans de Goede wrote:
> > >>> Hi,
> > >>>
> > >>> On 11/18/2014 11:19 AM, Thierry Reding wrote:
> > >>>> On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
> > >>>>> Hi Maxime,
> > >>>>>
> > >>>>> On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
> > >>>>> <maxime.ripard@free-electrons.com> wrote:
> > >>>>>>> -module_init(simplefb_init);
> > >>>>>>> +/*
> > >>>>>>> + * While this can be a module, if builtin it's most likely the console
> > >>>>>>> + * So let's leave module_exit but move module_init to an earlier place
> > >>>>>>> + */
> > >>>>>>
> > >>>>>> Not really related to this patch itself, but do we want to support
> > >>>>>> simplefb as a module? It seems like it's going to be most of the time
> > >>>>>> broken.
> > >>>>>
> > >>>>> If it depends on clocks, it won't work as a module, as CCF will have disabled
> > >>>>> all unused clocks at that point.
> > >>>>
> > >>>> If it does depend on anything beyond clocks it won't work at all. Clocks
> > >>>> are special because they get set up very early at boot time. If it turns
> > >>>> out that a simplefb ever needs a regulator to remain on, and that's even
> > >>>> quite likely to happen eventually, it's going to fail miserably, because
> > >>>> those regulators will typically be provided by a PMIC on an I2C bus. The
> > >>>> regulator won't be registered until very late into the boot process and
> > >>>> a regulator_get() call will almost certainly cause the simplefb driver
> > >>>> to defer probing.
> > >>>
> > >>> Right, this has been discussed already and the plan is to have simplefb
> > >>> continue its probe function and return success from it if it encounters
> > >>> any -eprobe_defer errors, while tracking which resources it misses.
> > >>>
> > >>> And then have a late_initcall which will claim any resources which failed
> > >>> with -eprobe beforehand.
> > >>
> > >> How do you ensure that the late_initcall gets run before any of the
> > >> other late_initcalls that disable the resources?
> > > 
> > >> Also my recollection is
> > >> that deferred probing will first be triggered the first time from a
> > >> late_initcall, so chances aren't very high that all resources have shown
> > >> up by that time.
> > > 
> > > So I just looked up the relevant code, and your right, this means that
> > > the whole model of "disable unused resources once probing is done" which
> > > we use for e.g. clocks, is already somewhat broken since there is
> > > no guarantee probing is really done when the cleanup code runs.
> > > 
> > >>>> Now deferring probing is a real showstopper for simplefb, because not
> > >>>> only does it make the framebuffer useless as early boot console, once
> > >>>> probing is attempted again the clocks that it would have needed to
> > >>>> acquire to keep going will already have been switched off, too.
> > >>>
> > >>> That is not true, even with the current implementation, if all necessary
> > >>> drivers are built in, then simplefb will come up later, but it will still
> > >>> come up before the late_initcall which disables the clocks.
> > >>
> > >> Yes, in the current implementation because clocks typically are
> > >> registered very early and thus you don't hit the deferred probe. The
> > >> same is not true for other types of resources where it's actually quite
> > >> common to hit deferred probing (regulators is a very notorious one).
> > >>
> > >> It doesn't matter whether a driver is built-in or not, once you hit
> > >> deferred probing you lose.
> > >>
> > >>> Once we do the split probing described above (which is something which
> > >>> we plan to do when it becomes necessary), then simplefb will still come
> > >>> up early.
> > >>
> > >> It will come up early but won't have acquired all the resources that it
> > >> needs, so unless you somehow manage to order late_initcalls in exactly
> > >> the way that you need them, the frameworks will still turn off what you
> > >> haven't managed to claim yet.
> > > 
> > > If it is a resource which only shows up as a result of deferred probing,
> > > then it may very well not have been probed & registered yet, when the
> > > framework cleanup functions runs, and thus will not get turned off...
> > > 
> > > So yes you're right that deferred probing may cause issues, but it seems
> > > that this is not something simplefb specific, but rather a generic problem
> > > with deferred-probing vs subsys cleanup functions.
> > > 
> > > My view on this is simple, lets worry about this when we actually have
> > > a board which hits these issues, and then we'll see from there.
> > 
> > So thinking more about this, I think this is not that hard to fix.
> > 
> > First lets fix the generic conflict between eprobedefer and subsys cleanup
> > functions. This can be done by:
> > 
> > 1) Having a linked list of subsys cleanup functions
> > to call in drivers/base/dd.c
> > 
> > 2) Have subsystems register their cleanup function rather then using
> > late_initcall to get it called
> > 
> > 3) Have deferred_probe_work_func iterate over the list and call the cleanup
> > functions once deferred_probe_active_list goes empty. It should also remove
> > them once called, so that they only get called the first time
> > deferred_probe_active_list goes empty (iow when the deferred probing of'
> > build-in drivers is done).
> > 
> > This way cleanup functions actually get run when all probing of (buildin)
> > drivers is done.
> 
> That sounds like a reasonable solution to me.
> 
> > Then when we move simplefb to a 2 fase probe, we can simply register the
> > framebuffer at the first probe call, and claim any resources we get, but
> > return -eprobe_defer if some resources returned that themselves.
> > 
> > Then when simplefb_probe gets re-called, it can first check if it did not
> > already register a fb, and if it did, it can use that and see which
> > resources are missing, and only try to claim those. If some resources
> > still return probe_defer, return eprobe_defer again, otherwise success.
> 
> I'm not sure you can check that the fb was already registered. When the
> driver probe fails the driver core will pretty much remove any trace of
> the driver, so checking for an already-reagistered framebuffer isn't
> going to be easy.
> 
> But if you can manage to make that work it should give us a pretty good
> solution that should be easy to make use of by other drivers as well.

What about handling priorities in the suggested cleanup mechanism, and
just add the second phase as the highest priority cleanup callback?

That way, we would be guaranteed to be run just before any cleanup
function.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141118/40726180/attachment.sig>