[PATCH] drivers/of: Fix depth for sub-tree blob in unflatten_dt_nodes()

[PATCH] drivers/of: Fix depth for sub-tree blob in unflatten_dt_nodes()

[Gavin Shan]

The function is unflattening device sub-tree blob if @dad passed to
the function is valid. Currently, this functionality is used by PPC
PowerNV PCI hotplug driver only. There are possibly multiple nodes
in the first level of depth, fdt_next_node() bails immediately when
@depth becomes negative before the second device node can be probed
successfully. It leads to the device nodes except the first one won't
be unflattened successfully.

This fixes the issue by setting the initial depth (@inital_depth) to
1 when this function is called to unflatten device sub-tree blob. No
logic changes when this function is used to unflatten non-sub-tree
blob.

Cc: Rhyland Klein <rklein-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Fixes: 78c44d910 ("drivers/of: Fix depth when unflattening devicetree")
Signed-off-by: Gavin Shan <gwshan-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/of/fdt.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 14f2f8c..9d70316 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -395,7 +395,7 @@ static int unflatten_dt_nodes(const void *blob,
 			      struct device_node **nodepp)
 {
 	struct device_node *root;
-	int offset = 0, depth = 0;
+	int offset = 0, depth = 0, initial_depth = 0;
 #define FDT_MAX_DEPTH	64
 	unsigned int fpsizes[FDT_MAX_DEPTH];
 	struct device_node *nps[FDT_MAX_DEPTH];
@@ -408,8 +408,19 @@ static int unflatten_dt_nodes(const void *blob,
 	root = dad;
 	fpsizes[depth] = dad ? strlen(of_node_full_name(dad)) : 0;
 	nps[depth] = dad;
+
+	/*
+	 * We're unflattening device sub-tree if @dad is valid. There are
+	 * possibly multiple nodes in the first level of depth. We need
+	 * set @depth to 1 to make fdt_next_node() happy as it bails
+	 * immediately when negative @depth is found. Otherwise, the device
+	 * nodes except the first one won't be unflattened successfully.
+	 */
+	if (dad)
+		depth = initial_depth = 1;
+
 	for (offset = 0;
-	     offset >= 0 && depth >= 0;
+	     offset >= 0 && depth >= initial_depth;
 	     offset = fdt_next_node(blob, offset, &depth)) {
 		if (WARN_ON_ONCE(depth >= FDT_MAX_DEPTH))
 			continue;
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drivers/of: Fix depth for sub-tree blob in unflatten_dt_nodes()

[Andrew Donnellan]

On 08/06/16 16:50, Gavin Shan wrote:
> The function is unflattening device sub-tree blob if @dad passed to
> the function is valid. Currently, this functionality is used by PPC
> PowerNV PCI hotplug driver only. There are possibly multiple nodes
> in the first level of depth, fdt_next_node() bails immediately when
> @depth becomes negative before the second device node can be probed
> successfully. It leads to the device nodes except the first one won't
> be unflattened successfully.
>
> This fixes the issue by setting the initial depth (@inital_depth) to
> 1 when this function is called to unflatten device sub-tree blob. No
> logic changes when this function is used to unflatten non-sub-tree
> blob.
>
> Cc: Rhyland Klein <rklein-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Fixes: 78c44d910 ("drivers/of: Fix depth when unflattening devicetree")
> Signed-off-by: Gavin Shan <gwshan-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

This doesn't appear to work for me - in my cxl use case, at first glance 
it looks like we're getting an empty unflattened device tree returned.

Will investigate further.

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan-8fk3Idey6ehBDgjK7y7TUQ@public.gmane.org  IBM Australia Limited

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drivers/of: Fix depth for sub-tree blob in unflatten_dt_nodes()

[Andrew Donnellan]

On 08/06/16 18:02, Andrew Donnellan wrote:
> On 08/06/16 16:50, Gavin Shan wrote:
>> The function is unflattening device sub-tree blob if @dad passed to
>> the function is valid. Currently, this functionality is used by PPC
>> PowerNV PCI hotplug driver only. There are possibly multiple nodes
>> in the first level of depth, fdt_next_node() bails immediately when
>> @depth becomes negative before the second device node can be probed
>> successfully. It leads to the device nodes except the first one won't
>> be unflattened successfully.
>>
>> This fixes the issue by setting the initial depth (@inital_depth) to
>> 1 when this function is called to unflatten device sub-tree blob. No
>> logic changes when this function is used to unflatten non-sub-tree
>> blob.
>>
>> Cc: Rhyland Klein <rklein-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> Fixes: 78c44d910 ("drivers/of: Fix depth when unflattening devicetree")
>> Signed-off-by: Gavin Shan <gwshan-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>
> This doesn't appear to work for me - in my cxl use case, at first glance
> it looks like we're getting an empty unflattened device tree returned.
>
> Will investigate further.

Can confirm though that that I was observing the original problem here 
with 78c44d9 when hotplugging a PCI device and only getting 1 PCI 
function added to the devicetree.

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan-8fk3Idey6ehBDgjK7y7TUQ@public.gmane.org  IBM Australia Limited

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drivers/of: Fix depth for sub-tree blob in unflatten_dt_nodes()

[Gavin Shan]

On Wed, Jun 08, 2016 at 06:04:33PM +1000, Andrew Donnellan wrote:
>On 08/06/16 18:02, Andrew Donnellan wrote:
>>On 08/06/16 16:50, Gavin Shan wrote:
>>>The function is unflattening device sub-tree blob if @dad passed to
>>>the function is valid. Currently, this functionality is used by PPC
>>>PowerNV PCI hotplug driver only. There are possibly multiple nodes
>>>in the first level of depth, fdt_next_node() bails immediately when
>>>@depth becomes negative before the second device node can be probed
>>>successfully. It leads to the device nodes except the first one won't
>>>be unflattened successfully.
>>>
>>>This fixes the issue by setting the initial depth (@inital_depth) to
>>>1 when this function is called to unflatten device sub-tree blob. No
>>>logic changes when this function is used to unflatten non-sub-tree
>>>blob.
>>>
>>>Cc: Rhyland Klein <rklein-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>Fixes: 78c44d910 ("drivers/of: Fix depth when unflattening devicetree")
>>>Signed-off-by: Gavin Shan <gwshan-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>>
>>This doesn't appear to work for me - in my cxl use case, at first glance
>>it looks like we're getting an empty unflattened device tree returned.
>>
>>Will investigate further.
>
>Can confirm though that that I was observing the original problem here with
>78c44d9 when hotplugging a PCI device and only getting 1 PCI function added
>to the devicetree.
>

Andrew, thanks for testing it for CAPI adapter. Without the fix, the
device node for PCI device 0003:09:00.0 is populated and the other 3
device nodes for 0003:09:00.1/2/3 are missed. With the fix, all 4 device
nodes are populated properly.

Andrew, the patch isn't related to the empty FDT blob. please check the
firmware log if the newly enabled (3) CAPI functions have been probed by
firmware. Otherwise, empty FDT blob will be returned. I guess you probably
need a power cycle to retest? :)

Thanks,
Gavin

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drivers/of: Fix depth for sub-tree blob in unflatten_dt_nodes()

[Rhyland Klein]

On 6/8/2016 2:50 AM, Gavin Shan wrote:
> The function is unflattening device sub-tree blob if @dad passed to
> the function is valid. Currently, this functionality is used by PPC
> PowerNV PCI hotplug driver only. There are possibly multiple nodes
> in the first level of depth, fdt_next_node() bails immediately when
> @depth becomes negative before the second device node can be probed
> successfully. It leads to the device nodes except the first one won't
> be unflattened successfully.
> 
> This fixes the issue by setting the initial depth (@inital_depth) to
> 1 when this function is called to unflatten device sub-tree blob. No
> logic changes when this function is used to unflatten non-sub-tree
> blob.
> 
> Cc: Rhyland Klein <rklein-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Fixes: 78c44d910 ("drivers/of: Fix depth when unflattening devicetree")
> Signed-off-by: Gavin Shan <gwshan-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/of/fdt.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 14f2f8c..9d70316 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -395,7 +395,7 @@ static int unflatten_dt_nodes(const void *blob,
>  			      struct device_node **nodepp)
>  {
>  	struct device_node *root;
> -	int offset = 0, depth = 0;
> +	int offset = 0, depth = 0, initial_depth = 0;
>  #define FDT_MAX_DEPTH	64
>  	unsigned int fpsizes[FDT_MAX_DEPTH];
>  	struct device_node *nps[FDT_MAX_DEPTH];
> @@ -408,8 +408,19 @@ static int unflatten_dt_nodes(const void *blob,
>  	root = dad;
>  	fpsizes[depth] = dad ? strlen(of_node_full_name(dad)) : 0;
>  	nps[depth] = dad;
> +
> +	/*
> +	 * We're unflattening device sub-tree if @dad is valid. There are
> +	 * possibly multiple nodes in the first level of depth. We need
> +	 * set @depth to 1 to make fdt_next_node() happy as it bails
> +	 * immediately when negative @depth is found. Otherwise, the device
> +	 * nodes except the first one won't be unflattened successfully.
> +	 */
> +	if (dad)
> +		depth = initial_depth = 1;
> +
>  	for (offset = 0;
> -	     offset >= 0 && depth >= 0;
> +	     offset >= 0 && depth >= initial_depth;
>  	     offset = fdt_next_node(blob, offset, &depth)) {
>  		if (WARN_ON_ONCE(depth >= FDT_MAX_DEPTH))
>  			continue;
> 

I tested this and it seems to be working fine for me.

Tested-by: Rhyland Klein <rklein-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

-rhyland

-- 
nvpublic
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drivers/of: Fix depth for sub-tree blob in unflatten_dt_nodes()

[Rob Herring]

On Wed, Jun 8, 2016 at 1:50 AM, Gavin Shan <gwshan-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
> The function is unflattening device sub-tree blob if @dad passed to
> the function is valid. Currently, this functionality is used by PPC
> PowerNV PCI hotplug driver only. There are possibly multiple nodes
> in the first level of depth, fdt_next_node() bails immediately when
> @depth becomes negative before the second device node can be probed
> successfully. It leads to the device nodes except the first one won't
> be unflattened successfully.
>
> This fixes the issue by setting the initial depth (@inital_depth) to
> 1 when this function is called to unflatten device sub-tree blob. No
> logic changes when this function is used to unflatten non-sub-tree
> blob.

Does this affect anything besides your PCI hotplug stuff? I'd think
overlays/changesets would be broken, but the unittests aren't failing
that I recall.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drivers/of: Fix depth for sub-tree blob in unflatten_dt_nodes()

[Gavin Shan]

On Wed, Jun 08, 2016 at 11:46:41AM -0500, Rob Herring wrote:
>On Wed, Jun 8, 2016 at 1:50 AM, Gavin Shan <gwshan-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
>> The function is unflattening device sub-tree blob if @dad passed to
>> the function is valid. Currently, this functionality is used by PPC
>> PowerNV PCI hotplug driver only. There are possibly multiple nodes
>> in the first level of depth, fdt_next_node() bails immediately when
>> @depth becomes negative before the second device node can be probed
>> successfully. It leads to the device nodes except the first one won't
>> be unflattened successfully.
>>
>> This fixes the issue by setting the initial depth (@inital_depth) to
>> 1 when this function is called to unflatten device sub-tree blob. No
>> logic changes when this function is used to unflatten non-sub-tree
>> blob.
>
>Does this affect anything besides your PCI hotplug stuff? I'd think
>overlays/changesets would be broken, but the unittests aren't failing
>that I recall.
>

Rob, it affects my PCI hotplug stuff only. There are two paths how
unflatten_dt_nodes() is called. No valid @dad specified in (A). PCI
hotplug is the only user specifying valid @dad in (B) and all other
users have invalid @dad.

          A                          B

   unflatten_device_tree     of_fdt_unflatten_tree
   __unflatten_device_tree   __unflatten_device_tree
   unflatten_dt_nodes        unflatten_dt_nodes

With or without this patch, I got same otuput from unittest as below:

### dt-test ### start of unittest - you will see error messages
/testcase-data/phandle-tests/consumer-a: could not get #phandle-cells-missing for /testcase-data/phandle-tests/provider1
/testcase-data/phandle-tests/consumer-a: could not get #phandle-cells-missing for /testcase-data/phandle-tests/provider1
/testcase-data/phandle-tests/consumer-a: could not find phandle
/testcase-data/phandle-tests/consumer-a: could not find phandle
/testcase-data/phandle-tests/consumer-a: arguments longer than property
/testcase-data/phandle-tests/consumer-a: arguments longer than property
irq: XICS didn't like hwirq-0x1 to VIRQ27 mapping (rc=-22)
irq: XICS didn't like hwirq-0x1 to VIRQ27 mapping (rc=-22)
### dt-test ### FAIL of_unittest_platform_populate():782 device deferred probe failed - 0
overlay_is_topmost: #5 clashes #6 @/testcase-data/overlay-node/test-bus/test-unittest8
overlay_removal_is_ok: overlay #5 is not topmost
of_overlay_destroy: removal check failed for overlay #5
### dt-test ### end of unittest - 147 passed, 1 failed


>Rob
>

Thanks,
Gavin

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drivers/of: Fix depth for sub-tree blob in unflatten_dt_nodes()

[Gavin Shan]

On Wed, Jun 08, 2016 at 04:50:54PM +1000, Gavin Shan wrote:
>The function is unflattening device sub-tree blob if @dad passed to
>the function is valid. Currently, this functionality is used by PPC
>PowerNV PCI hotplug driver only. There are possibly multiple nodes
>in the first level of depth, fdt_next_node() bails immediately when
>@depth becomes negative before the second device node can be probed
>successfully. It leads to the device nodes except the first one won't
>be unflattened successfully.
>
>This fixes the issue by setting the initial depth (@inital_depth) to
>1 when this function is called to unflatten device sub-tree blob. No
>logic changes when this function is used to unflatten non-sub-tree
>blob.
>
>Cc: Rhyland Klein <rklein-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>Fixes: 78c44d910 ("drivers/of: Fix depth when unflattening devicetree")
>Signed-off-by: Gavin Shan <gwshan-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>---
> drivers/of/fdt.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>index 14f2f8c..9d70316 100644
>--- a/drivers/of/fdt.c
>+++ b/drivers/of/fdt.c
>@@ -395,7 +395,7 @@ static int unflatten_dt_nodes(const void *blob,
> 			      struct device_node **nodepp)
> {
> 	struct device_node *root;
>-	int offset = 0, depth = 0;
>+	int offset = 0, depth = 0, initial_depth = 0;
> #define FDT_MAX_DEPTH	64
> 	unsigned int fpsizes[FDT_MAX_DEPTH];
> 	struct device_node *nps[FDT_MAX_DEPTH];
>@@ -408,8 +408,19 @@ static int unflatten_dt_nodes(const void *blob,
> 	root = dad;
> 	fpsizes[depth] = dad ? strlen(of_node_full_name(dad)) : 0;
> 	nps[depth] = dad;
>+
>+	/*
>+	 * We're unflattening device sub-tree if @dad is valid. There are
>+	 * possibly multiple nodes in the first level of depth. We need
>+	 * set @depth to 1 to make fdt_next_node() happy as it bails
>+	 * immediately when negative @depth is found. Otherwise, the device
>+	 * nodes except the first one won't be unflattened successfully.
>+	 */
>+	if (dad)
>+		depth = initial_depth = 1;
>+

The change to @depth should happen before @fpsizes[depth] and @nps[depth]
are initialized. Otherwise, the parent device node and its full name length
are lost. It's why this didn't work for Andrew's hotplug case, but it did
work for me (with luck). I will post v2 to correct it.

Thanks,
Gavin

> 	for (offset = 0;
>-	     offset >= 0 && depth >= 0;
>+	     offset >= 0 && depth >= initial_depth;
> 	     offset = fdt_next_node(blob, offset, &depth)) {
> 		if (WARN_ON_ONCE(depth >= FDT_MAX_DEPTH))
> 			continue;
>-- 
>2.1.0
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drivers/of: Fix depth for sub-tree blob in unflatten_dt_nodes()

[Gavin Shan]

On Wed, Jun 08, 2016 at 11:53:26AM -0400, Rhyland Klein wrote:
>On 6/8/2016 2:50 AM, Gavin Shan wrote:
>> The function is unflattening device sub-tree blob if @dad passed to
>> the function is valid. Currently, this functionality is used by PPC
>> PowerNV PCI hotplug driver only. There are possibly multiple nodes
>> in the first level of depth, fdt_next_node() bails immediately when
>> @depth becomes negative before the second device node can be probed
>> successfully. It leads to the device nodes except the first one won't
>> be unflattened successfully.
>> 
>> This fixes the issue by setting the initial depth (@inital_depth) to
>> 1 when this function is called to unflatten device sub-tree blob. No
>> logic changes when this function is used to unflatten non-sub-tree
>> blob.
>> 
>> Cc: Rhyland Klein <rklein-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> Fixes: 78c44d910 ("drivers/of: Fix depth when unflattening devicetree")
>> Signed-off-by: Gavin Shan <gwshan-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>> ---
>>  drivers/of/fdt.c | 15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index 14f2f8c..9d70316 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -395,7 +395,7 @@ static int unflatten_dt_nodes(const void *blob,
>>  			      struct device_node **nodepp)
>>  {
>>  	struct device_node *root;
>> -	int offset = 0, depth = 0;
>> +	int offset = 0, depth = 0, initial_depth = 0;
>>  #define FDT_MAX_DEPTH	64
>>  	unsigned int fpsizes[FDT_MAX_DEPTH];
>>  	struct device_node *nps[FDT_MAX_DEPTH];
>> @@ -408,8 +408,19 @@ static int unflatten_dt_nodes(const void *blob,
>>  	root = dad;
>>  	fpsizes[depth] = dad ? strlen(of_node_full_name(dad)) : 0;
>>  	nps[depth] = dad;
>> +
>> +	/*
>> +	 * We're unflattening device sub-tree if @dad is valid. There are
>> +	 * possibly multiple nodes in the first level of depth. We need
>> +	 * set @depth to 1 to make fdt_next_node() happy as it bails
>> +	 * immediately when negative @depth is found. Otherwise, the device
>> +	 * nodes except the first one won't be unflattened successfully.
>> +	 */
>> +	if (dad)
>> +		depth = initial_depth = 1;
>> +
>>  	for (offset = 0;
>> -	     offset >= 0 && depth >= 0;
>> +	     offset >= 0 && depth >= initial_depth;
>>  	     offset = fdt_next_node(blob, offset, &depth)) {
>>  		if (WARN_ON_ONCE(depth >= FDT_MAX_DEPTH))
>>  			continue;
>> 
>
>I tested this and it seems to be working fine for me.
>
>Tested-by: Rhyland Klein <rklein-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>

Rhyland, thanks for your testing. I need send a v2 to adjust the @depth
before @fpsizes[depth] and @nps[depth] are initialized, which doesn't
affect your case and v2 should work for you. Your tested-by will be
included in v2.

Thanks,
Gavin

>-rhyland
>
>-- 
>nvpublic
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html