[PATCH 1/2] ralink: Introduce fw_passed_dtb to arch/mips/ralink

[PATCH 1/2] ralink: Introduce fw_passed_dtb to arch/mips/ralink

[Tobias Wolf]

This patch adds fw_passed_dtb to arch/mips/ralink to support 
CONFIG_MIPS_RAW_APPENDED_DTB. Furthermore it adds a check that __dtb_start is 
not the same address as __dtb_end.

Signed-off-by: Tobias Wolf <dev-NTEO@vplace.de>
---
--- a/arch/mips/ralink/of.c
+++ b/arch/mips/ralink/of.c
@@ -66,13 +66,21 @@
 
 void __init plat_mem_setup(void)
 {
+	void *dtb;
+
 	set_io_port_base(KSEG1);
 
 	/*
 	 * Load the builtin devicetree. This causes the chosen node to be
 	 * parsed resulting in our memory appearing
 	 */
-	__dt_setup_arch(__dtb_start);
+	if (fw_passed_dtb) /* used by CONFIG_MIPS_APPENDED_RAW_DTB as well */
+		dtb = (void *)fw_passed_dtb;
+	else if (__dtb_start != __dtb_end)
+		dtb = (void *)__dtb_start;
+
+	if (dtb)
+		__dt_setup_arch(dtb);
 
 	of_scan_flat_dt(early_init_dt_find_memory, NULL);
 	if (memory_dtb)

[PATCH 2/2] of: Add check to of_scan_flat_dt() before accessing initial_boot_params

[Tobias Wolf]

An empty __dtb_start to __dtb_end section might result in initial_boot_params 
being null for arch/mips/ralink. This showed that the boot process hangs 
indefinitely in of_scan_flat_dt().

Signed-off-by: Tobias Wolf <dev-NTEO@vplace.de>
---
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -628,6 +628,9 @@
 				     void *data),
 			   void *data)
 {
+	if (!initial_boot_params)
+		return;
+
 	const void *blob = initial_boot_params;
 	const char *pathp;
 	int offset, rc = 0, depth = -1;

Re: [PATCH 2/2] of: Add check to of_scan_flat_dt() before accessing initial_boot_params

[Sergei Shtylyov]

On 11/21/2016 07:23 PM, Tobias Wolf wrote:

> An empty __dtb_start to __dtb_end section might result in initial_boot_params
> being null for arch/mips/ralink. This showed that the boot process hangs
> indefinitely in of_scan_flat_dt().
>
> Signed-off-by: Tobias Wolf <dev-NTEO@vplace.de>
> ---
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -628,6 +628,9 @@
>  				     void *data),
>  			   void *data)
>  {
> +	if (!initial_boot_params)
> +		return;
> +
>  	const void *blob = initial_boot_params;
>  	const char *pathp;
>  	int offset, rc = 0, depth = -1;

   CC      drivers/of/fdt.o
drivers/of/fdt.c: In function ‘of_scan_flat_dt’:
drivers/of/fdt.c:738:3: warning: ‘return’ with no value, in function returning 
non-void [-Wreturn-type]
drivers/of/fdt.c:740:2: warning: ISO C90 forbids mixed declarations and code 
[-Wdeclaration-after-statement]

MBR, Sergei

Re: [PATCH 2/2] of: Add check to of_scan_flat_dt() before accessing initial_boot_params

[Tobias Wolf]

Am Montag, 21. November 2016, 20:21:03 CET schrieb Sergei Shtylyov:
> On 11/21/2016 07:23 PM, Tobias Wolf wrote:
>    CC      drivers/of/fdt.o
> drivers/of/fdt.c: In function ‘of_scan_flat_dt’:
> drivers/of/fdt.c:738:3: warning: ‘return’ with no value, in function
> returning non-void [-Wreturn-type]
> drivers/of/fdt.c:740:2: warning: ISO C90 forbids mixed declarations and code
> [-Wdeclaration-after-statement]
> 
> MBR, Sergei

Dear Sergei,

Thanks for that note. In fact it returns whatever it provides. Question is, 
what would be the right value.

int offset, rc = 0, depth = -1;

rc defaults to 0 but something like EINVAL would better reflect that something 
is wrong.

Best regards
Tobias

[PATCH 2/2 v2] of: Add check to of_scan_flat_dt() before accessing initial_boot_params

[Tobias Wolf]

An empty __dtb_start to __dtb_end section might result in initial_boot_params 
being null for arch/mips/ralink. This showed that the boot process hangs 
indefinitely in of_scan_flat_dt().

Signed-off-by: Tobias Wolf <dev-NTEO@vplace.de>
---
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -628,6 +628,9 @@
 				     void *data),
 			   void *data)
 {
+	if (!initial_boot_params)
+		return 0;
+
 	const void *blob = initial_boot_params;
 	const char *pathp;
 	int offset, rc = 0, depth = -1;
---

Dear Sergei,

After checking the use of "of_scan_flat_dt()" I revised the patch to return 0 
as any other value would most likely break code in:

Best regards
Tobias

Re: [PATCH 2/2 v2] of: Add check to of_scan_flat_dt() before accessing initial_boot_params

[Sergei Shtylyov]

Hello.

On 11/23/2016 9:11 AM, Tobias Wolf wrote:

> An empty __dtb_start to __dtb_end section might result in initial_boot_params
> being null for arch/mips/ralink. This showed that the boot process hangs
> indefinitely in of_scan_flat_dt().
>
> Signed-off-by: Tobias Wolf <dev-NTEO@vplace.de>
> ---
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -628,6 +628,9 @@
>  				     void *data),
>  			   void *data)
>  {
> +	if (!initial_boot_params)
> +		return 0;
> +
>  	const void *blob = initial_boot_params;
>  	const char *pathp;
>  	int offset, rc = 0, depth = -1;
> ---
>
> Dear Sergei,
>
> After checking the use of "of_scan_flat_dt()" I revised the patch to return 0
> as any other value would most likely break code in:

    It still causes a compiler warning -- you can't mix code and declarations 
in C90.

> Best regards
> Tobias

MBR, Sergei

[PATCH 2/2 v3] of: Add check to of_scan_flat_dt() before accessing initial_boot_params

[Tobias Wolf]

An empty __dtb_start to __dtb_end section might result in initial_boot_params 
being null for arch/mips/ralink. This showed that the boot process hangs 
indefinitely in of_scan_flat_dt().

Signed-off-by: Tobias Wolf <dev-NTEO@vplace.de>
---
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -738,9 +738,12 @@
 	const char *pathp;
 	int offset, rc = 0, depth = -1;
 
-        for (offset = fdt_next_node(blob, -1, &depth);
-             offset >= 0 && depth >= 0 && !rc;
-             offset = fdt_next_node(blob, offset, &depth)) {
+	if (!blob)
+		return 0;
+
+	for (offset = fdt_next_node(blob, -1, &depth);
+	     offset >= 0 && depth >= 0 && !rc;
+	     offset = fdt_next_node(blob, offset, &depth)) {
 
 		pathp = fdt_get_name(blob, offset, NULL);
 		if (*pathp == '/')

Dear Sergei,

Missed that warning completely during compilation of a testable image for my 
device. I regenerated the patch based on your input (for 4.9-rc6 this time) 
and based the check on the local blob variable this time.

Haven't seen any warnings this time.

Hope it's correct that I reference the new patch version each time in the 
subject line.

Best regards
Tobias

Btw.: Last e-mail I wanted to list occurrences EINVAL would break existing 
code. One is kernel/prom.c in arch/microblaze.

[PATCH 1/2 v2] ralink: Introduce fw_passed_dtb to arch/mips/ralink

[Tobias Wolf]

This patch adds fw_passed_dtb to arch/mips/ralink to support 
CONFIG_MIPS_RAW_APPENDED_DTB. Furthermore it adds a check that __dtb_start is 
not the same address as __dtb_end.

Signed-off-by: Tobias Wolf <dev-NTEO@vplace.de>
---
--- a/arch/mips/ralink/of.c
+++ b/arch/mips/ralink/of.c
@@ -66,13 +66,21 @@
 
 void __init plat_mem_setup(void)
 {
+	void *dtb = NULL;
+
 	set_io_port_base(KSEG1);
 
 	/*
 	 * Load the builtin devicetree. This causes the chosen node to be
-	 * parsed resulting in our memory appearing
+	 * parsed resulting in our memory appearing. fw_passed_dtb is used
+	 * by CONFIG_MIPS_APPENDED_RAW_DTB as well.
 	 */
-	__dt_setup_arch(__dtb_start);
+	if (fw_passed_dtb)
+		dtb = (void *)fw_passed_dtb;
+	else if (__dtb_start != __dtb_end)
+		dtb = (void *)__dtb_start;
+
+	__dt_setup_arch(dtb);
 
 	of_scan_flat_dt(early_init_dt_find_memory, NULL);
 	if (memory_dtb)

---
This version has been cleaned up based on feedback [1] of John Crispin for the 
LEDE Project.

[1] https://github.com/lede-project/source/pull/582#discussion_r90778573

Best regards
Tobias

Re: [PATCH 1/2 v2] ralink: Introduce fw_passed_dtb to arch/mips/ralink

[John Crispin]

On 13/12/2016 11:46, Tobias Wolf wrote:
> This patch adds fw_passed_dtb to arch/mips/ralink to support 
> CONFIG_MIPS_RAW_APPENDED_DTB. Furthermore it adds a check that __dtb_start is 
> not the same address as __dtb_end.
> 
> Signed-off-by: Tobias Wolf <dev-NTEO@vplace.de>
> ---

there should be a chunk here explaining the changes between v1 and v2
... anyhow

Acked-by: John Crispin <john@phrozen.org>



> --- a/arch/mips/ralink/of.c
> +++ b/arch/mips/ralink/of.c
> @@ -66,13 +66,21 @@
>  
>  void __init plat_mem_setup(void)
>  {
> +	void *dtb = NULL;
> +
>  	set_io_port_base(KSEG1);
>  
>  	/*
>  	 * Load the builtin devicetree. This causes the chosen node to be
> -	 * parsed resulting in our memory appearing
> +	 * parsed resulting in our memory appearing. fw_passed_dtb is used
> +	 * by CONFIG_MIPS_APPENDED_RAW_DTB as well.
>  	 */
> -	__dt_setup_arch(__dtb_start);
> +	if (fw_passed_dtb)
> +		dtb = (void *)fw_passed_dtb;
> +	else if (__dtb_start != __dtb_end)
> +		dtb = (void *)__dtb_start;
> +
> +	__dt_setup_arch(dtb);
>  
>  	of_scan_flat_dt(early_init_dt_find_memory, NULL);
>  	if (memory_dtb)
> 
> ---
> This version has been cleaned up based on feedback [1] of John Crispin for the 
> LEDE Project.
> 
> [1] https://github.com/lede-project/source/pull/582#discussion_r90778573
> 
> Best regards
> Tobias
>