iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 1/2] iommu: pass cell_count = -1 to of_for_each_phandle with cells_name
@ 2019-08-24 13:28 Uwe Kleine-König
  2019-08-24 13:28 ` [PATCH v1 2/2] of: Let of_for_each_phandle fallback to non-negative cell_count Uwe Kleine-König
  2019-09-03 12:52 ` [PATCH v1 1/2] iommu: pass cell_count = -1 to of_for_each_phandle with cells_name Joerg Roedel
  0 siblings, 2 replies; 13+ messages in thread
From: Uwe Kleine-König @ 2019-08-24 13:28 UTC (permalink / raw)
  To: Rob Herring, Frank Rowand, Will Deacon, Robin Murphy,
	Joerg Roedel, Matthias Brugger
  Cc: devicetree, linux-kernel, iommu, linux-mediatek, kernel,
	linux-arm-kernel

Currently of_for_each_phandle ignores the cell_count parameter when a
cells_name is given. I intend to change that and let the iterator fall
back to a non-negative cell_count if the cells_name property is missing
in the referenced node.

To not change how existing of_for_each_phandle's users iterate, fix them
to pass cell_count = -1 when also cells_name is given which yields the
expected behaviour with and without my change.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/iommu/arm-smmu.c     | 2 +-
 drivers/iommu/mtk_iommu_v1.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 64977c131ee6..81b7734654b3 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -333,7 +333,7 @@ static int __find_legacy_master_phandle(struct device *dev, void *data)
 	int err;
 
 	of_for_each_phandle(it, err, dev->of_node, "mmu-masters",
-			    "#stream-id-cells", 0)
+			    "#stream-id-cells", -1)
 		if (it->node == np) {
 			*(void **)data = dev;
 			return 1;
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index abeeac488372..68d1de70de0c 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -426,7 +426,7 @@ static int mtk_iommu_add_device(struct device *dev)
 	int err;
 
 	of_for_each_phandle(&it, err, dev->of_node, "iommus",
-			"#iommu-cells", 0) {
+			"#iommu-cells", -1) {
 		int count = of_phandle_iterator_args(&it, iommu_spec.args,
 					MAX_PHANDLE_ARGS);
 		iommu_spec.np = of_node_get(it.node);
-- 
2.20.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v1 2/2] of: Let of_for_each_phandle fallback to non-negative cell_count
  2019-08-24 13:28 [PATCH v1 1/2] iommu: pass cell_count = -1 to of_for_each_phandle with cells_name Uwe Kleine-König
@ 2019-08-24 13:28 ` Uwe Kleine-König
  2019-09-02 14:26   ` Rob Herring
  2019-09-13 21:58   ` Rob Herring
  2019-09-03 12:52 ` [PATCH v1 1/2] iommu: pass cell_count = -1 to of_for_each_phandle with cells_name Joerg Roedel
  1 sibling, 2 replies; 13+ messages in thread
From: Uwe Kleine-König @ 2019-08-24 13:28 UTC (permalink / raw)
  To: Rob Herring, Frank Rowand
  Cc: devicetree, Robin Murphy, linux-kernel, iommu, linux-mediatek,
	kernel, Matthias Brugger, Will Deacon, linux-arm-kernel

Referencing device tree nodes from a property allows to pass arguments.
This is for example used for referencing gpios. This looks as follows:

	gpio_ctrl: gpio-controller {
		#gpio-cells = <2>
		...
	}

	someothernode {
		gpios = <&gpio_ctrl 5 0 &gpio_ctrl 3 0>;
		...
	}

To know the number of arguments this must be either fixed, or the
referenced node is checked for a $cells_name (here: "#gpio-cells")
property and with this information the start of the second reference can
be determined.

Currently regulators are referenced with no additional arguments. To
allow some optional arguments without having to change all referenced
nodes this change introduces a way to specify a default cell_count. So
when a phandle is parsed we check for the $cells_name property and use
it as before if present. If it is not present we fall back to
cells_count if non-negative and only fail if cells_count is smaller than
zero.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/of/base.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 55e7f5bb0549..2f25d2dfecfa 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1335,11 +1335,20 @@ int of_phandle_iterator_next(struct of_phandle_iterator *it)
 
 			if (of_property_read_u32(it->node, it->cells_name,
 						 &count)) {
-				pr_err("%pOF: could not get %s for %pOF\n",
-				       it->parent,
-				       it->cells_name,
-				       it->node);
-				goto err;
+				/*
+				 * If both cell_count and cells_name is given,
+				 * fall back to cell_count in absence
+				 * of the cells_name property
+				 */
+				if (it->cell_count >= 0) {
+					count = it->cell_count;
+				} else {
+					pr_err("%pOF: could not get %s for %pOF\n",
+					       it->parent,
+					       it->cells_name,
+					       it->node);
+					goto err;
+				}
 			}
 		} else {
 			count = it->cell_count;
@@ -1505,7 +1514,7 @@ int of_parse_phandle_with_args(const struct device_node *np, const char *list_na
 {
 	if (index < 0)
 		return -EINVAL;
-	return __of_parse_phandle_with_args(np, list_name, cells_name, 0,
+	return __of_parse_phandle_with_args(np, list_name, cells_name, -1,
 					    index, out_args);
 }
 EXPORT_SYMBOL(of_parse_phandle_with_args);
@@ -1588,7 +1597,7 @@ int of_parse_phandle_with_args_map(const struct device_node *np,
 	if (!pass_name)
 		goto free;
 
-	ret = __of_parse_phandle_with_args(np, list_name, cells_name, 0, index,
+	ret = __of_parse_phandle_with_args(np, list_name, cells_name, -1, index,
 					   out_args);
 	if (ret)
 		goto free;
@@ -1756,7 +1765,7 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
 	struct of_phandle_iterator it;
 	int rc, cur_index = 0;
 
-	rc = of_phandle_iterator_init(&it, np, list_name, cells_name, 0);
+	rc = of_phandle_iterator_init(&it, np, list_name, cells_name, -1);
 	if (rc)
 		return rc;
 
-- 
2.20.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/2] of: Let of_for_each_phandle fallback to non-negative cell_count
  2019-08-24 13:28 ` [PATCH v1 2/2] of: Let of_for_each_phandle fallback to non-negative cell_count Uwe Kleine-König
@ 2019-09-02 14:26   ` Rob Herring
  2019-09-13 21:58   ` Rob Herring
  1 sibling, 0 replies; 13+ messages in thread
From: Rob Herring @ 2019-09-02 14:26 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: devicetree, Will Deacon, linux-kernel, iommu, linux-mediatek,
	kernel, Matthias Brugger, Frank Rowand, linux-arm-kernel,
	Robin Murphy

On Sat, Aug 24, 2019 at 03:28:46PM +0200, Uwe Kleine-König wrote:
> Referencing device tree nodes from a property allows to pass arguments.
> This is for example used for referencing gpios. This looks as follows:
> 
> 	gpio_ctrl: gpio-controller {
> 		#gpio-cells = <2>
> 		...
> 	}
> 
> 	someothernode {
> 		gpios = <&gpio_ctrl 5 0 &gpio_ctrl 3 0>;
> 		...
> 	}
> 
> To know the number of arguments this must be either fixed, or the
> referenced node is checked for a $cells_name (here: "#gpio-cells")
> property and with this information the start of the second reference can
> be determined.
> 
> Currently regulators are referenced with no additional arguments. To
> allow some optional arguments without having to change all referenced
> nodes this change introduces a way to specify a default cell_count. So
> when a phandle is parsed we check for the $cells_name property and use
> it as before if present. If it is not present we fall back to
> cells_count if non-negative and only fail if cells_count is smaller than
> zero.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/of/base.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)

Looks fine to me. I can apply with an ack from the iommu folks on patch 
1.

Rob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 1/2] iommu: pass cell_count = -1 to of_for_each_phandle with cells_name
  2019-08-24 13:28 [PATCH v1 1/2] iommu: pass cell_count = -1 to of_for_each_phandle with cells_name Uwe Kleine-König
  2019-08-24 13:28 ` [PATCH v1 2/2] of: Let of_for_each_phandle fallback to non-negative cell_count Uwe Kleine-König
@ 2019-09-03 12:52 ` Joerg Roedel
  2019-09-12  7:43   ` Uwe Kleine-König
  1 sibling, 1 reply; 13+ messages in thread
From: Joerg Roedel @ 2019-09-03 12:52 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel, iommu,
	Rob Herring, linux-mediatek, kernel, Matthias Brugger,
	Will Deacon, linux-arm-kernel

On Sat, Aug 24, 2019 at 03:28:45PM +0200, Uwe Kleine-König wrote:
> Currently of_for_each_phandle ignores the cell_count parameter when a
> cells_name is given. I intend to change that and let the iterator fall
> back to a non-negative cell_count if the cells_name property is missing
> in the referenced node.
> 
> To not change how existing of_for_each_phandle's users iterate, fix them
> to pass cell_count = -1 when also cells_name is given which yields the
> expected behaviour with and without my change.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/iommu/arm-smmu.c     | 2 +-
>  drivers/iommu/mtk_iommu_v1.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Acked-by: Joerg Roedel <jroedel@suse.de>

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 1/2] iommu: pass cell_count = -1 to of_for_each_phandle with cells_name
  2019-09-03 12:52 ` [PATCH v1 1/2] iommu: pass cell_count = -1 to of_for_each_phandle with cells_name Joerg Roedel
@ 2019-09-12  7:43   ` Uwe Kleine-König
  2019-09-13  7:37     ` Joerg Roedel
  0 siblings, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2019-09-12  7:43 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: devicetree, Will Deacon, Robin Murphy, linux-kernel, iommu,
	Rob Herring, linux-mediatek, kernel, Matthias Brugger,
	Frank Rowand, linux-arm-kernel

On Tue, Sep 03, 2019 at 02:52:10PM +0200, Joerg Roedel wrote:
> On Sat, Aug 24, 2019 at 03:28:45PM +0200, Uwe Kleine-König wrote:
> > Currently of_for_each_phandle ignores the cell_count parameter when a
> > cells_name is given. I intend to change that and let the iterator fall
> > back to a non-negative cell_count if the cells_name property is missing
> > in the referenced node.
> > 
> > To not change how existing of_for_each_phandle's users iterate, fix them
> > to pass cell_count = -1 when also cells_name is given which yields the
> > expected behaviour with and without my change.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/iommu/arm-smmu.c     | 2 +-
> >  drivers/iommu/mtk_iommu_v1.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> Acked-by: Joerg Roedel <jroedel@suse.de>

Does this ack mean that Rob is expected to apply this together with
patch 2?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 1/2] iommu: pass cell_count = -1 to of_for_each_phandle with cells_name
  2019-09-12  7:43   ` Uwe Kleine-König
@ 2019-09-13  7:37     ` Joerg Roedel
  0 siblings, 0 replies; 13+ messages in thread
From: Joerg Roedel @ 2019-09-13  7:37 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: devicetree, Will Deacon, Robin Murphy, linux-kernel, iommu,
	Rob Herring, linux-mediatek, kernel, Matthias Brugger,
	Frank Rowand, linux-arm-kernel

On Thu, Sep 12, 2019 at 09:43:53AM +0200, Uwe Kleine-König wrote:
> On Tue, Sep 03, 2019 at 02:52:10PM +0200, Joerg Roedel wrote:
> > Acked-by: Joerg Roedel <jroedel@suse.de>
> 
> Does this ack mean that Rob is expected to apply this together with
> patch 2?

"Expected" is a strong word. I'd more phrase it like I am fine with this
patch going through his tree.

Regards,

	Joerg
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/2] of: Let of_for_each_phandle fallback to non-negative cell_count
  2019-08-24 13:28 ` [PATCH v1 2/2] of: Let of_for_each_phandle fallback to non-negative cell_count Uwe Kleine-König
  2019-09-02 14:26   ` Rob Herring
@ 2019-09-13 21:58   ` Rob Herring
  2019-09-17  9:40     ` Geert Uytterhoeven
  1 sibling, 1 reply; 13+ messages in thread
From: Rob Herring @ 2019-09-13 21:58 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: devicetree, Will Deacon, linux-kernel, iommu, linux-mediatek,
	kernel, Matthias Brugger, Frank Rowand, linux-arm-kernel,
	Robin Murphy

On Sat, 24 Aug 2019 15:28:46 +0200, =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?=          wrote:
> Referencing device tree nodes from a property allows to pass arguments.
> This is for example used for referencing gpios. This looks as follows:
> 
> 	gpio_ctrl: gpio-controller {
> 		#gpio-cells = <2>
> 		...
> 	}
> 
> 	someothernode {
> 		gpios = <&gpio_ctrl 5 0 &gpio_ctrl 3 0>;
> 		...
> 	}
> 
> To know the number of arguments this must be either fixed, or the
> referenced node is checked for a $cells_name (here: "#gpio-cells")
> property and with this information the start of the second reference can
> be determined.
> 
> Currently regulators are referenced with no additional arguments. To
> allow some optional arguments without having to change all referenced
> nodes this change introduces a way to specify a default cell_count. So
> when a phandle is parsed we check for the $cells_name property and use
> it as before if present. If it is not present we fall back to
> cells_count if non-negative and only fail if cells_count is smaller than
> zero.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/of/base.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 

Applied both patches, thanks.

Rob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/2] of: Let of_for_each_phandle fallback to non-negative cell_count
  2019-09-13 21:58   ` Rob Herring
@ 2019-09-17  9:40     ` Geert Uytterhoeven
  2019-09-17 10:13       ` Uwe Kleine-König
  0 siblings, 1 reply; 13+ messages in thread
From: Geert Uytterhoeven @ 2019-09-17  9:40 UTC (permalink / raw)
  To: Rob Herring, Uwe Kleine-König
  Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Frank Rowand, Linux Kernel Mailing List, Linux-Renesas,
	Wolfram Sang, Linux IOMMU, linux-mediatek, Linux I2C,
	Sascha Hauer, Matthias Brugger, Will Deacon, Linux ARM,
	Robin Murphy

Hi Rob, Uwe,

On Fri, Sep 13, 2019 at 11:58 PM Rob Herring <robh@kernel.org> wrote:
> On Sat, 24 Aug 2019 15:28:46 +0200, =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?=          wrote:
> > Referencing device tree nodes from a property allows to pass arguments.
> > This is for example used for referencing gpios. This looks as follows:
> >
> >       gpio_ctrl: gpio-controller {
> >               #gpio-cells = <2>
> >               ...
> >       }
> >
> >       someothernode {
> >               gpios = <&gpio_ctrl 5 0 &gpio_ctrl 3 0>;
> >               ...
> >       }
> >
> > To know the number of arguments this must be either fixed, or the
> > referenced node is checked for a $cells_name (here: "#gpio-cells")
> > property and with this information the start of the second reference can
> > be determined.
> >
> > Currently regulators are referenced with no additional arguments. To
> > allow some optional arguments without having to change all referenced
> > nodes this change introduces a way to specify a default cell_count. So
> > when a phandle is parsed we check for the $cells_name property and use
> > it as before if present. If it is not present we fall back to
> > cells_count if non-negative and only fail if cells_count is smaller than
> > zero.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

This is now commit e42ee61017f58cd9 ("of: Let of_for_each_phandle fallback
to non-negative cell_count") in robh/for-next, which causes a lock-up when
booting a shmobile_defconfig kernel on r8a7791/koelsch:

rcu: INFO: rcu_sched self-detected stall on CPU
rcu:     0-....: (2099 ticks this GP) idle=6fe/1/0x40000002
softirq=29/29 fqs=1050
 (t=2100 jiffies g=-1131 q=0)
NMI backtrace for cpu 0
CPU: 0 PID: 1 Comm: swapper/0 Not tainted
5.3.0-rc2-shmobile-00050-ge42ee61017f58cd9 #376
Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
[<c010f8ac>] (unwind_backtrace) from [<c010b620>] (show_stack+0x10/0x14)
[<c010b620>] (show_stack) from [<c073d038>] (dump_stack+0x7c/0x9c)
[<c073d038>] (dump_stack) from [<c0742e80>] (nmi_cpu_backtrace+0xa0/0xb8)
[<c0742e80>] (nmi_cpu_backtrace) from [<c0742f1c>]
(nmi_trigger_cpumask_backtrace+0x84/0x114)
[<c0742f1c>] (nmi_trigger_cpumask_backtrace) from [<c017d684>]
(rcu_dump_cpu_stacks+0xac/0xc8)
[<c017d684>] (rcu_dump_cpu_stacks) from [<c017a598>]
(rcu_sched_clock_irq+0x2ac/0x6b4)
[<c017a598>] (rcu_sched_clock_irq) from [<c0183980>]
(update_process_times+0x30/0x5c)
[<c0183980>] (update_process_times) from [<c01941a8>]
(tick_nohz_handler+0xcc/0x120)
[<c01941a8>] (tick_nohz_handler) from [<c05b1d40>]
(arch_timer_handler_virt+0x28/0x30)
[<c05b1d40>] (arch_timer_handler_virt) from [<c016c9e0>]
(handle_percpu_devid_irq+0xe8/0x21c)
[<c016c9e0>] (handle_percpu_devid_irq) from [<c0167a8c>]
(generic_handle_irq+0x18/0x28)
[<c0167a8c>] (generic_handle_irq) from [<c0167b3c>]
(__handle_domain_irq+0xa0/0xb4)
[<c0167b3c>] (__handle_domain_irq) from [<c03673ec>] (gic_handle_irq+0x58/0x90)
[<c03673ec>] (gic_handle_irq) from [<c0101a8c>] (__irq_svc+0x6c/0x90)
Exception stack(0xeb08dd30 to 0xeb08dd78)
dd20:                                     c0cc7514 20000013 00000005 00003b27
dd40: eb7c4020 c0cc750c 00000051 00000051 20000013 c0c66b08 eb1cdc00 00000018
dd60: 00000000 eb08dd80 c05c1a38 c0756c00 20000013 ffffffff
[<c0101a8c>] (__irq_svc) from [<c0756c00>]
(_raw_spin_unlock_irqrestore+0x1c/0x20)
[<c0756c00>] (_raw_spin_unlock_irqrestore) from [<c05c1a38>]
(of_find_node_by_phandle+0xcc/0xf0)
[<c05c1a38>] (of_find_node_by_phandle) from [<c05c1bb8>]
(of_phandle_iterator_next+0x68/0x178)
[<c05c1bb8>] (of_phandle_iterator_next) from [<c05c22bc>]
(of_count_phandle_with_args+0x5c/0x7c)
[<c05c22bc>] (of_count_phandle_with_args) from [<c053fc38>]
(i2c_demux_pinctrl_probe+0x24/0x1fc)
[<c053fc38>] (i2c_demux_pinctrl_probe) from [<c04463c4>]
(platform_drv_probe+0x48/0x94)
[<c04463c4>] (platform_drv_probe) from [<c0444a20>] (really_probe+0x1f0/0x2b8)
[<c0444a20>] (really_probe) from [<c0444e68>] (driver_probe_device+0x140/0x158)
[<c0444e68>] (driver_probe_device) from [<c0444ff0>]
(device_driver_attach+0x44/0x5c)
[<c0444ff0>] (device_driver_attach) from [<c04450b4>]
(__driver_attach+0xac/0xb4)
[<c04450b4>] (__driver_attach) from [<c0443178>] (bus_for_each_dev+0x64/0xa0)
[<c0443178>] (bus_for_each_dev) from [<c04438a8>] (bus_add_driver+0x148/0x1a8)
[<c04438a8>] (bus_add_driver) from [<c0445ad0>] (driver_register+0xac/0xf0)
[<c0445ad0>] (driver_register) from [<c0b010b0>] (do_one_initcall+0xa8/0x1d4)
[<c0b010b0>] (do_one_initcall) from [<c0b01448>]
(kernel_init_freeable+0x26c/0x2c8)
[<c0b01448>] (kernel_init_freeable) from [<c0751c70>] (kernel_init+0x8/0x10c)
[<c0751c70>] (kernel_init) from [<c01010e8>] (ret_from_fork+0x14/0x2c)
Exception stack(0xeb08dfb0 to 0xeb08dff8)
dfa0:                                     00000000 00000000 00000000 00000000
dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
dfe0: 00000000 00000000 00000000 00000000 00000013 00000000

Presumably it loops forever, due to a conversion of -1 to unsigned
somewhere?

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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/2] of: Let of_for_each_phandle fallback to non-negative cell_count
  2019-09-17  9:40     ` Geert Uytterhoeven
@ 2019-09-17 10:13       ` Uwe Kleine-König
  2019-09-17 11:25         ` Peter Rosin
  0 siblings, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2019-09-17 10:13 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Rob Herring,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Frank Rowand, Linux Kernel Mailing List, Linux-Renesas,
	Wolfram Sang, Linux IOMMU, linux-mediatek, Linux I2C,
	Sascha Hauer, Matthias Brugger, Will Deacon, Linux ARM,
	Robin Murphy

Hello Geert,

On Tue, Sep 17, 2019 at 11:40:25AM +0200, Geert Uytterhoeven wrote:
> Hi Rob, Uwe,
> 
> On Fri, Sep 13, 2019 at 11:58 PM Rob Herring <robh@kernel.org> wrote:
> > On Sat, 24 Aug 2019 15:28:46 +0200, =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?=          wrote:
> > > Referencing device tree nodes from a property allows to pass arguments.
> > > This is for example used for referencing gpios. This looks as follows:
> > >
> > >       gpio_ctrl: gpio-controller {
> > >               #gpio-cells = <2>
> > >               ...
> > >       }
> > >
> > >       someothernode {
> > >               gpios = <&gpio_ctrl 5 0 &gpio_ctrl 3 0>;
> > >               ...
> > >       }
> > >
> > > To know the number of arguments this must be either fixed, or the
> > > referenced node is checked for a $cells_name (here: "#gpio-cells")
> > > property and with this information the start of the second reference can
> > > be determined.
> > >
> > > Currently regulators are referenced with no additional arguments. To
> > > allow some optional arguments without having to change all referenced
> > > nodes this change introduces a way to specify a default cell_count. So
> > > when a phandle is parsed we check for the $cells_name property and use
> > > it as before if present. If it is not present we fall back to
> > > cells_count if non-negative and only fail if cells_count is smaller than
> > > zero.
> > >
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> This is now commit e42ee61017f58cd9 ("of: Let of_for_each_phandle fallback
> to non-negative cell_count") in robh/for-next, which causes a lock-up when
> booting a shmobile_defconfig kernel on r8a7791/koelsch:
> 
> rcu: INFO: rcu_sched self-detected stall on CPU
> rcu:     0-....: (2099 ticks this GP) idle=6fe/1/0x40000002
> softirq=29/29 fqs=1050
>  (t=2100 jiffies g=-1131 q=0)
> NMI backtrace for cpu 0
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted
> 5.3.0-rc2-shmobile-00050-ge42ee61017f58cd9 #376
> Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> [<c010f8ac>] (unwind_backtrace) from [<c010b620>] (show_stack+0x10/0x14)
> [<c010b620>] (show_stack) from [<c073d038>] (dump_stack+0x7c/0x9c)
> [<c073d038>] (dump_stack) from [<c0742e80>] (nmi_cpu_backtrace+0xa0/0xb8)
> [<c0742e80>] (nmi_cpu_backtrace) from [<c0742f1c>] (nmi_trigger_cpumask_backtrace+0x84/0x114)
> [<c0742f1c>] (nmi_trigger_cpumask_backtrace) from [<c017d684>] (rcu_dump_cpu_stacks+0xac/0xc8)
> [<c017d684>] (rcu_dump_cpu_stacks) from [<c017a598>] (rcu_sched_clock_irq+0x2ac/0x6b4)
> [<c017a598>] (rcu_sched_clock_irq) from [<c0183980>] (update_process_times+0x30/0x5c)
> [<c0183980>] (update_process_times) from [<c01941a8>] (tick_nohz_handler+0xcc/0x120)
> [<c01941a8>] (tick_nohz_handler) from [<c05b1d40>] (arch_timer_handler_virt+0x28/0x30)
> [<c05b1d40>] (arch_timer_handler_virt) from [<c016c9e0>] (handle_percpu_devid_irq+0xe8/0x21c)
> [<c016c9e0>] (handle_percpu_devid_irq) from [<c0167a8c>] (generic_handle_irq+0x18/0x28)
> [<c0167a8c>] (generic_handle_irq) from [<c0167b3c>] (__handle_domain_irq+0xa0/0xb4)
> [<c0167b3c>] (__handle_domain_irq) from [<c03673ec>] (gic_handle_irq+0x58/0x90)
> [<c03673ec>] (gic_handle_irq) from [<c0101a8c>] (__irq_svc+0x6c/0x90)
> Exception stack(0xeb08dd30 to 0xeb08dd78)
> dd20:                                     c0cc7514 20000013 00000005 00003b27
> dd40: eb7c4020 c0cc750c 00000051 00000051 20000013 c0c66b08 eb1cdc00 00000018
> dd60: 00000000 eb08dd80 c05c1a38 c0756c00 20000013 ffffffff
> [<c0101a8c>] (__irq_svc) from [<c0756c00>] (_raw_spin_unlock_irqrestore+0x1c/0x20)
> [<c0756c00>] (_raw_spin_unlock_irqrestore) from [<c05c1a38>] (of_find_node_by_phandle+0xcc/0xf0)
> [<c05c1a38>] (of_find_node_by_phandle) from [<c05c1bb8>] (of_phandle_iterator_next+0x68/0x178)
> [<c05c1bb8>] (of_phandle_iterator_next) from [<c05c22bc>] (of_count_phandle_with_args+0x5c/0x7c)
> [<c05c22bc>] (of_count_phandle_with_args) from [<c053fc38>] (i2c_demux_pinctrl_probe+0x24/0x1fc)
> [<c053fc38>] (i2c_demux_pinctrl_probe) from [<c04463c4>] (platform_drv_probe+0x48/0x94)
> [<c04463c4>] (platform_drv_probe) from [<c0444a20>] (really_probe+0x1f0/0x2b8)
> [<c0444a20>] (really_probe) from [<c0444e68>] (driver_probe_device+0x140/0x158)
> [<c0444e68>] (driver_probe_device) from [<c0444ff0>] (device_driver_attach+0x44/0x5c)
> [<c0444ff0>] (device_driver_attach) from [<c04450b4>] (__driver_attach+0xac/0xb4)
> [<c04450b4>] (__driver_attach) from [<c0443178>] (bus_for_each_dev+0x64/0xa0)
> [<c0443178>] (bus_for_each_dev) from [<c04438a8>] (bus_add_driver+0x148/0x1a8)
> [<c04438a8>] (bus_add_driver) from [<c0445ad0>] (driver_register+0xac/0xf0)
> [<c0445ad0>] (driver_register) from [<c0b010b0>] (do_one_initcall+0xa8/0x1d4)
> [<c0b010b0>] (do_one_initcall) from [<c0b01448>] (kernel_init_freeable+0x26c/0x2c8)
> [<c0b01448>] (kernel_init_freeable) from [<c0751c70>] (kernel_init+0x8/0x10c)
> [<c0751c70>] (kernel_init) from [<c01010e8>] (ret_from_fork+0x14/0x2c)
> Exception stack(0xeb08dfb0 to 0xeb08dff8)
> dfa0:                                     00000000 00000000 00000000 00000000
> dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> 
> Presumably it loops forever, due to a conversion of -1 to unsigned
> somewhere?

Hmm, I fail to see the culprit. i2c_demux_pinctrl_probe calls
of_count_phandle_with_args with cells_name=NULL. With that I don't see
how my patch changes anything as the only change is in an if
(it->cells_name) block that shouldn't be relevant in your case.

Can you please verify that the loop in of_count_phandle_with_args is
indeed not terminating, e.g. with

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 2f25d2dfecfa..2425a6d26038 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1769,8 +1769,13 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
 	if (rc)
 		return rc;
 
-	while ((rc = of_phandle_iterator_next(&it)) == 0)
+	pr_err("%s: enter loop (np=%pOF, list_name=%s, cells_name=%s)\n",
+	       __func__, np, list_name, cells_name);
+	while ((rc = of_phandle_iterator_next(&it)) == 0) {
+		pr_err("%s: it.node = %pOF cur_index=%d\n", __func__, it.node, cur_index);
 		cur_index += 1;
+	}
+	pr_err("%s: exit loop\n", __func__);
 
 	if (rc != -ENOENT)
 		return rc;

Thanks
Uwe
-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/2] of: Let of_for_each_phandle fallback to non-negative cell_count
  2019-09-17 10:13       ` Uwe Kleine-König
@ 2019-09-17 11:25         ` Peter Rosin
  2019-09-17 12:25           ` Uwe Kleine-König
  0 siblings, 1 reply; 13+ messages in thread
From: Peter Rosin @ 2019-09-17 11:25 UTC (permalink / raw)
  To: Uwe Kleine-König, Geert Uytterhoeven
  Cc: Rob Herring,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Frank Rowand, Linux Kernel Mailing List, Linux-Renesas,
	Wolfram Sang, Linux IOMMU, linux-mediatek, Linux I2C,
	Sascha Hauer, Matthias Brugger, Will Deacon, Linux ARM,
	Robin Murphy

On 2019-09-17 12:13, Uwe Kleine-König wrote:
> Hello Geert,
> 
> On Tue, Sep 17, 2019 at 11:40:25AM +0200, Geert Uytterhoeven wrote:
>> Hi Rob, Uwe,
>>
>> On Fri, Sep 13, 2019 at 11:58 PM Rob Herring <robh@kernel.org> wrote:
>>> On Sat, 24 Aug 2019 15:28:46 +0200, =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?=          wrote:
>>>> Referencing device tree nodes from a property allows to pass arguments.
>>>> This is for example used for referencing gpios. This looks as follows:
>>>>
>>>>       gpio_ctrl: gpio-controller {
>>>>               #gpio-cells = <2>
>>>>               ...
>>>>       }
>>>>
>>>>       someothernode {
>>>>               gpios = <&gpio_ctrl 5 0 &gpio_ctrl 3 0>;
>>>>               ...
>>>>       }
>>>>
>>>> To know the number of arguments this must be either fixed, or the
>>>> referenced node is checked for a $cells_name (here: "#gpio-cells")
>>>> property and with this information the start of the second reference can
>>>> be determined.
>>>>
>>>> Currently regulators are referenced with no additional arguments. To
>>>> allow some optional arguments without having to change all referenced
>>>> nodes this change introduces a way to specify a default cell_count. So
>>>> when a phandle is parsed we check for the $cells_name property and use
>>>> it as before if present. If it is not present we fall back to
>>>> cells_count if non-negative and only fail if cells_count is smaller than
>>>> zero.
>>>>
>>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>
>> This is now commit e42ee61017f58cd9 ("of: Let of_for_each_phandle fallback
>> to non-negative cell_count") in robh/for-next, which causes a lock-up when
>> booting a shmobile_defconfig kernel on r8a7791/koelsch:
>>
>> rcu: INFO: rcu_sched self-detected stall on CPU
>> rcu:     0-....: (2099 ticks this GP) idle=6fe/1/0x40000002
>> softirq=29/29 fqs=1050
>>  (t=2100 jiffies g=-1131 q=0)
>> NMI backtrace for cpu 0
>> CPU: 0 PID: 1 Comm: swapper/0 Not tainted
>> 5.3.0-rc2-shmobile-00050-ge42ee61017f58cd9 #376
>> Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
>> [<c010f8ac>] (unwind_backtrace) from [<c010b620>] (show_stack+0x10/0x14)
>> [<c010b620>] (show_stack) from [<c073d038>] (dump_stack+0x7c/0x9c)
>> [<c073d038>] (dump_stack) from [<c0742e80>] (nmi_cpu_backtrace+0xa0/0xb8)
>> [<c0742e80>] (nmi_cpu_backtrace) from [<c0742f1c>] (nmi_trigger_cpumask_backtrace+0x84/0x114)
>> [<c0742f1c>] (nmi_trigger_cpumask_backtrace) from [<c017d684>] (rcu_dump_cpu_stacks+0xac/0xc8)
>> [<c017d684>] (rcu_dump_cpu_stacks) from [<c017a598>] (rcu_sched_clock_irq+0x2ac/0x6b4)
>> [<c017a598>] (rcu_sched_clock_irq) from [<c0183980>] (update_process_times+0x30/0x5c)
>> [<c0183980>] (update_process_times) from [<c01941a8>] (tick_nohz_handler+0xcc/0x120)
>> [<c01941a8>] (tick_nohz_handler) from [<c05b1d40>] (arch_timer_handler_virt+0x28/0x30)
>> [<c05b1d40>] (arch_timer_handler_virt) from [<c016c9e0>] (handle_percpu_devid_irq+0xe8/0x21c)
>> [<c016c9e0>] (handle_percpu_devid_irq) from [<c0167a8c>] (generic_handle_irq+0x18/0x28)
>> [<c0167a8c>] (generic_handle_irq) from [<c0167b3c>] (__handle_domain_irq+0xa0/0xb4)
>> [<c0167b3c>] (__handle_domain_irq) from [<c03673ec>] (gic_handle_irq+0x58/0x90)
>> [<c03673ec>] (gic_handle_irq) from [<c0101a8c>] (__irq_svc+0x6c/0x90)
>> Exception stack(0xeb08dd30 to 0xeb08dd78)
>> dd20:                                     c0cc7514 20000013 00000005 00003b27
>> dd40: eb7c4020 c0cc750c 00000051 00000051 20000013 c0c66b08 eb1cdc00 00000018
>> dd60: 00000000 eb08dd80 c05c1a38 c0756c00 20000013 ffffffff
>> [<c0101a8c>] (__irq_svc) from [<c0756c00>] (_raw_spin_unlock_irqrestore+0x1c/0x20)
>> [<c0756c00>] (_raw_spin_unlock_irqrestore) from [<c05c1a38>] (of_find_node_by_phandle+0xcc/0xf0)
>> [<c05c1a38>] (of_find_node_by_phandle) from [<c05c1bb8>] (of_phandle_iterator_next+0x68/0x178)
>> [<c05c1bb8>] (of_phandle_iterator_next) from [<c05c22bc>] (of_count_phandle_with_args+0x5c/0x7c)
>> [<c05c22bc>] (of_count_phandle_with_args) from [<c053fc38>] (i2c_demux_pinctrl_probe+0x24/0x1fc)
>> [<c053fc38>] (i2c_demux_pinctrl_probe) from [<c04463c4>] (platform_drv_probe+0x48/0x94)
>> [<c04463c4>] (platform_drv_probe) from [<c0444a20>] (really_probe+0x1f0/0x2b8)
>> [<c0444a20>] (really_probe) from [<c0444e68>] (driver_probe_device+0x140/0x158)
>> [<c0444e68>] (driver_probe_device) from [<c0444ff0>] (device_driver_attach+0x44/0x5c)
>> [<c0444ff0>] (device_driver_attach) from [<c04450b4>] (__driver_attach+0xac/0xb4)
>> [<c04450b4>] (__driver_attach) from [<c0443178>] (bus_for_each_dev+0x64/0xa0)
>> [<c0443178>] (bus_for_each_dev) from [<c04438a8>] (bus_add_driver+0x148/0x1a8)
>> [<c04438a8>] (bus_add_driver) from [<c0445ad0>] (driver_register+0xac/0xf0)
>> [<c0445ad0>] (driver_register) from [<c0b010b0>] (do_one_initcall+0xa8/0x1d4)
>> [<c0b010b0>] (do_one_initcall) from [<c0b01448>] (kernel_init_freeable+0x26c/0x2c8)
>> [<c0b01448>] (kernel_init_freeable) from [<c0751c70>] (kernel_init+0x8/0x10c)
>> [<c0751c70>] (kernel_init) from [<c01010e8>] (ret_from_fork+0x14/0x2c)
>> Exception stack(0xeb08dfb0 to 0xeb08dff8)
>> dfa0:                                     00000000 00000000 00000000 00000000
>> dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
>>
>> Presumably it loops forever, due to a conversion of -1 to unsigned
>> somewhere?
> 
> Hmm, I fail to see the culprit. i2c_demux_pinctrl_probe calls
> of_count_phandle_with_args with cells_name=NULL. With that I don't see
> how my patch changes anything as the only change is in an if
> (it->cells_name) block that shouldn't be relevant in your case.
> 
> Can you please verify that the loop in of_count_phandle_with_args is
> indeed not terminating, e.g. with

The below indicated else-branch was not touched by e42ee61017f58cd9,
which ends up setting the count to -1 (aka 0xff...ff in this case).
No?

int of_phandle_iterator_next(struct of_phandle_iterator *it)
{

	...

		if (it->cells_name) {

			...

		} else {
			count = it->cell_count;    /* <---- SUSPECT!!! */
		}

		...
	}

	...
}

Cheers,
Peter
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/2] of: Let of_for_each_phandle fallback to non-negative cell_count
  2019-09-17 11:25         ` Peter Rosin
@ 2019-09-17 12:25           ` Uwe Kleine-König
       [not found]             ` <CGME20190917124409eucas1p2211d232e6833a44a9ad5dbf72457197c@eucas1p2.samsung.com>
  2019-09-17 12:52             ` Geert Uytterhoeven
  0 siblings, 2 replies; 13+ messages in thread
From: Uwe Kleine-König @ 2019-09-17 12:25 UTC (permalink / raw)
  To: Peter Rosin
  Cc: Rob Herring,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Frank Rowand, Linux Kernel Mailing List, Linux-Renesas,
	Wolfram Sang, Linux IOMMU, Geert Uytterhoeven, Linux I2C,
	Sascha Hauer, Matthias Brugger, linux-mediatek, Will Deacon,
	Linux ARM, Robin Murphy

On Tue, Sep 17, 2019 at 11:25:46AM +0000, Peter Rosin wrote:
> On 2019-09-17 12:13, Uwe Kleine-König wrote:
> > Hello Geert,
> > 
> > On Tue, Sep 17, 2019 at 11:40:25AM +0200, Geert Uytterhoeven wrote:
> >> Hi Rob, Uwe,
> >>
> >> On Fri, Sep 13, 2019 at 11:58 PM Rob Herring <robh@kernel.org> wrote:
> >>> On Sat, 24 Aug 2019 15:28:46 +0200, =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?=          wrote:
> >>>> Referencing device tree nodes from a property allows to pass arguments.
> >>>> This is for example used for referencing gpios. This looks as follows:
> >>>>
> >>>>       gpio_ctrl: gpio-controller {
> >>>>               #gpio-cells = <2>
> >>>>               ...
> >>>>       }
> >>>>
> >>>>       someothernode {
> >>>>               gpios = <&gpio_ctrl 5 0 &gpio_ctrl 3 0>;
> >>>>               ...
> >>>>       }
> >>>>
> >>>> To know the number of arguments this must be either fixed, or the
> >>>> referenced node is checked for a $cells_name (here: "#gpio-cells")
> >>>> property and with this information the start of the second reference can
> >>>> be determined.
> >>>>
> >>>> Currently regulators are referenced with no additional arguments. To
> >>>> allow some optional arguments without having to change all referenced
> >>>> nodes this change introduces a way to specify a default cell_count. So
> >>>> when a phandle is parsed we check for the $cells_name property and use
> >>>> it as before if present. If it is not present we fall back to
> >>>> cells_count if non-negative and only fail if cells_count is smaller than
> >>>> zero.
> >>>>
> >>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >>
> >> This is now commit e42ee61017f58cd9 ("of: Let of_for_each_phandle fallback
> >> to non-negative cell_count") in robh/for-next, which causes a lock-up when
> >> booting a shmobile_defconfig kernel on r8a7791/koelsch:
> >>
> >> rcu: INFO: rcu_sched self-detected stall on CPU
> >> rcu:     0-....: (2099 ticks this GP) idle=6fe/1/0x40000002
> >> softirq=29/29 fqs=1050
> >>  (t=2100 jiffies g=-1131 q=0)
> >> NMI backtrace for cpu 0
> >> CPU: 0 PID: 1 Comm: swapper/0 Not tainted
> >> 5.3.0-rc2-shmobile-00050-ge42ee61017f58cd9 #376
> >> Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> >> [<c010f8ac>] (unwind_backtrace) from [<c010b620>] (show_stack+0x10/0x14)
> >> [<c010b620>] (show_stack) from [<c073d038>] (dump_stack+0x7c/0x9c)
> >> [<c073d038>] (dump_stack) from [<c0742e80>] (nmi_cpu_backtrace+0xa0/0xb8)
> >> [<c0742e80>] (nmi_cpu_backtrace) from [<c0742f1c>] (nmi_trigger_cpumask_backtrace+0x84/0x114)
> >> [<c0742f1c>] (nmi_trigger_cpumask_backtrace) from [<c017d684>] (rcu_dump_cpu_stacks+0xac/0xc8)
> >> [<c017d684>] (rcu_dump_cpu_stacks) from [<c017a598>] (rcu_sched_clock_irq+0x2ac/0x6b4)
> >> [<c017a598>] (rcu_sched_clock_irq) from [<c0183980>] (update_process_times+0x30/0x5c)
> >> [<c0183980>] (update_process_times) from [<c01941a8>] (tick_nohz_handler+0xcc/0x120)
> >> [<c01941a8>] (tick_nohz_handler) from [<c05b1d40>] (arch_timer_handler_virt+0x28/0x30)
> >> [<c05b1d40>] (arch_timer_handler_virt) from [<c016c9e0>] (handle_percpu_devid_irq+0xe8/0x21c)
> >> [<c016c9e0>] (handle_percpu_devid_irq) from [<c0167a8c>] (generic_handle_irq+0x18/0x28)
> >> [<c0167a8c>] (generic_handle_irq) from [<c0167b3c>] (__handle_domain_irq+0xa0/0xb4)
> >> [<c0167b3c>] (__handle_domain_irq) from [<c03673ec>] (gic_handle_irq+0x58/0x90)
> >> [<c03673ec>] (gic_handle_irq) from [<c0101a8c>] (__irq_svc+0x6c/0x90)
> >> Exception stack(0xeb08dd30 to 0xeb08dd78)
> >> dd20:                                     c0cc7514 20000013 00000005 00003b27
> >> dd40: eb7c4020 c0cc750c 00000051 00000051 20000013 c0c66b08 eb1cdc00 00000018
> >> dd60: 00000000 eb08dd80 c05c1a38 c0756c00 20000013 ffffffff
> >> [<c0101a8c>] (__irq_svc) from [<c0756c00>] (_raw_spin_unlock_irqrestore+0x1c/0x20)
> >> [<c0756c00>] (_raw_spin_unlock_irqrestore) from [<c05c1a38>] (of_find_node_by_phandle+0xcc/0xf0)
> >> [<c05c1a38>] (of_find_node_by_phandle) from [<c05c1bb8>] (of_phandle_iterator_next+0x68/0x178)
> >> [<c05c1bb8>] (of_phandle_iterator_next) from [<c05c22bc>] (of_count_phandle_with_args+0x5c/0x7c)
> >> [<c05c22bc>] (of_count_phandle_with_args) from [<c053fc38>] (i2c_demux_pinctrl_probe+0x24/0x1fc)
> >> [<c053fc38>] (i2c_demux_pinctrl_probe) from [<c04463c4>] (platform_drv_probe+0x48/0x94)
> >> [<c04463c4>] (platform_drv_probe) from [<c0444a20>] (really_probe+0x1f0/0x2b8)
> >> [<c0444a20>] (really_probe) from [<c0444e68>] (driver_probe_device+0x140/0x158)
> >> [<c0444e68>] (driver_probe_device) from [<c0444ff0>] (device_driver_attach+0x44/0x5c)
> >> [<c0444ff0>] (device_driver_attach) from [<c04450b4>] (__driver_attach+0xac/0xb4)
> >> [<c04450b4>] (__driver_attach) from [<c0443178>] (bus_for_each_dev+0x64/0xa0)
> >> [<c0443178>] (bus_for_each_dev) from [<c04438a8>] (bus_add_driver+0x148/0x1a8)
> >> [<c04438a8>] (bus_add_driver) from [<c0445ad0>] (driver_register+0xac/0xf0)
> >> [<c0445ad0>] (driver_register) from [<c0b010b0>] (do_one_initcall+0xa8/0x1d4)
> >> [<c0b010b0>] (do_one_initcall) from [<c0b01448>] (kernel_init_freeable+0x26c/0x2c8)
> >> [<c0b01448>] (kernel_init_freeable) from [<c0751c70>] (kernel_init+0x8/0x10c)
> >> [<c0751c70>] (kernel_init) from [<c01010e8>] (ret_from_fork+0x14/0x2c)
> >> Exception stack(0xeb08dfb0 to 0xeb08dff8)
> >> dfa0:                                     00000000 00000000 00000000 00000000
> >> dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> >> dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> >>
> >> Presumably it loops forever, due to a conversion of -1 to unsigned
> >> somewhere?
> > 
> > Hmm, I fail to see the culprit. i2c_demux_pinctrl_probe calls
> > of_count_phandle_with_args with cells_name=NULL. With that I don't see
> > how my patch changes anything as the only change is in an if
> > (it->cells_name) block that shouldn't be relevant in your case.
> > 
> > Can you please verify that the loop in of_count_phandle_with_args is
> > indeed not terminating, e.g. with
> 
> The below indicated else-branch was not touched by e42ee61017f58cd9,
> which ends up setting the count to -1 (aka 0xff...ff in this case).
> No?
> 
> int of_phandle_iterator_next(struct of_phandle_iterator *it)
> {
> 
> 	...
> 
> 		if (it->cells_name) {
> 
> 			...
> 
> 		} else {
> 			count = it->cell_count;    /* <---- SUSPECT!!! */
> 		}

Oh yeah, you're right. I'm a bit disappointed that I didn't spot this
myself :-|

Untested patch to fix this problem:

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 2f25d2dfecfa..26f7a21d7187 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1284,6 +1284,13 @@ int of_phandle_iterator_init(struct of_phandle_iterator *it,
 	const __be32 *list;
 	int size;
 
+	/*
+	 * one of cell_count or cells_name must be provided to determine the
+	 * argument length.
+	 */
+	if (cell_count < 0 && !cells_name)
+		return -EINVAL;
+
 	memset(it, 0, sizeof(*it));
 
 	list = of_get_property(np, list_name, &size);
@@ -1765,6 +1772,18 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
 	struct of_phandle_iterator it;
 	int rc, cur_index = 0;
 
+	/* If cells_name is NULL we assume an cell_count of 0 */
+	if (cells_name == NULL) {
+		const __be32 *list;
+		int size;
+
+		list = of_get_property(np, list_name, &size);
+		if (!list)
+			return -ENOENT;
+
+		return size / sizeof(*list);
+	}
+
 	rc = of_phandle_iterator_init(&it, np, list_name, cells_name, -1);
 	if (rc)
 		return rc;

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/2] of: Let of_for_each_phandle fallback to non-negative cell_count
       [not found]             ` <CGME20190917124409eucas1p2211d232e6833a44a9ad5dbf72457197c@eucas1p2.samsung.com>
@ 2019-09-17 12:44               ` Marek Szyprowski
  0 siblings, 0 replies; 13+ messages in thread
From: Marek Szyprowski @ 2019-09-17 12:44 UTC (permalink / raw)
  To: Uwe Kleine-König, Peter Rosin
  Cc: Rob Herring,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Frank Rowand, Linux Kernel Mailing List, Linux-Renesas,
	Wolfram Sang, Linux IOMMU, Geert Uytterhoeven, Linux I2C,
	Sascha Hauer, Matthias Brugger, linux-mediatek, Will Deacon,
	Linux ARM, Robin Murphy

Hi Uwe,

On 17.09.2019 14:25, Uwe Kleine-König wrote:
> On Tue, Sep 17, 2019 at 11:25:46AM +0000, Peter Rosin wrote:
>> On 2019-09-17 12:13, Uwe Kleine-König wrote:
>>> Hello Geert,
>>>
>>> On Tue, Sep 17, 2019 at 11:40:25AM +0200, Geert Uytterhoeven wrote:
>>>> Hi Rob, Uwe,
>>>>
>>>> On Fri, Sep 13, 2019 at 11:58 PM Rob Herring <robh@kernel.org> wrote:
>>>>> On Sat, 24 Aug 2019 15:28:46 +0200, =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?=          wrote:
>>>>>> Referencing device tree nodes from a property allows to pass arguments.
>>>>>> This is for example used for referencing gpios. This looks as follows:
>>>>>>
>>>>>>        gpio_ctrl: gpio-controller {
>>>>>>                #gpio-cells = <2>
>>>>>>                ...
>>>>>>        }
>>>>>>
>>>>>>        someothernode {
>>>>>>                gpios = <&gpio_ctrl 5 0 &gpio_ctrl 3 0>;
>>>>>>                ...
>>>>>>        }
>>>>>>
>>>>>> To know the number of arguments this must be either fixed, or the
>>>>>> referenced node is checked for a $cells_name (here: "#gpio-cells")
>>>>>> property and with this information the start of the second reference can
>>>>>> be determined.
>>>>>>
>>>>>> Currently regulators are referenced with no additional arguments. To
>>>>>> allow some optional arguments without having to change all referenced
>>>>>> nodes this change introduces a way to specify a default cell_count. So
>>>>>> when a phandle is parsed we check for the $cells_name property and use
>>>>>> it as before if present. If it is not present we fall back to
>>>>>> cells_count if non-negative and only fail if cells_count is smaller than
>>>>>> zero.
>>>>>>
>>>>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>>> This is now commit e42ee61017f58cd9 ("of: Let of_for_each_phandle fallback
>>>> to non-negative cell_count") in robh/for-next, which causes a lock-up when
>>>> booting a shmobile_defconfig kernel on r8a7791/koelsch:
>>>>
>>>> rcu: INFO: rcu_sched self-detected stall on CPU
>>>> rcu:     0-....: (2099 ticks this GP) idle=6fe/1/0x40000002
>>>> softirq=29/29 fqs=1050
>>>>   (t=2100 jiffies g=-1131 q=0)
>>>> NMI backtrace for cpu 0
>>>> CPU: 0 PID: 1 Comm: swapper/0 Not tainted
>>>> 5.3.0-rc2-shmobile-00050-ge42ee61017f58cd9 #376
>>>> Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
>>>> [<c010f8ac>] (unwind_backtrace) from [<c010b620>] (show_stack+0x10/0x14)
>>>> [<c010b620>] (show_stack) from [<c073d038>] (dump_stack+0x7c/0x9c)
>>>> [<c073d038>] (dump_stack) from [<c0742e80>] (nmi_cpu_backtrace+0xa0/0xb8)
>>>> [<c0742e80>] (nmi_cpu_backtrace) from [<c0742f1c>] (nmi_trigger_cpumask_backtrace+0x84/0x114)
>>>> [<c0742f1c>] (nmi_trigger_cpumask_backtrace) from [<c017d684>] (rcu_dump_cpu_stacks+0xac/0xc8)
>>>> [<c017d684>] (rcu_dump_cpu_stacks) from [<c017a598>] (rcu_sched_clock_irq+0x2ac/0x6b4)
>>>> [<c017a598>] (rcu_sched_clock_irq) from [<c0183980>] (update_process_times+0x30/0x5c)
>>>> [<c0183980>] (update_process_times) from [<c01941a8>] (tick_nohz_handler+0xcc/0x120)
>>>> [<c01941a8>] (tick_nohz_handler) from [<c05b1d40>] (arch_timer_handler_virt+0x28/0x30)
>>>> [<c05b1d40>] (arch_timer_handler_virt) from [<c016c9e0>] (handle_percpu_devid_irq+0xe8/0x21c)
>>>> [<c016c9e0>] (handle_percpu_devid_irq) from [<c0167a8c>] (generic_handle_irq+0x18/0x28)
>>>> [<c0167a8c>] (generic_handle_irq) from [<c0167b3c>] (__handle_domain_irq+0xa0/0xb4)
>>>> [<c0167b3c>] (__handle_domain_irq) from [<c03673ec>] (gic_handle_irq+0x58/0x90)
>>>> [<c03673ec>] (gic_handle_irq) from [<c0101a8c>] (__irq_svc+0x6c/0x90)
>>>> Exception stack(0xeb08dd30 to 0xeb08dd78)
>>>> dd20:                                     c0cc7514 20000013 00000005 00003b27
>>>> dd40: eb7c4020 c0cc750c 00000051 00000051 20000013 c0c66b08 eb1cdc00 00000018
>>>> dd60: 00000000 eb08dd80 c05c1a38 c0756c00 20000013 ffffffff
>>>> [<c0101a8c>] (__irq_svc) from [<c0756c00>] (_raw_spin_unlock_irqrestore+0x1c/0x20)
>>>> [<c0756c00>] (_raw_spin_unlock_irqrestore) from [<c05c1a38>] (of_find_node_by_phandle+0xcc/0xf0)
>>>> [<c05c1a38>] (of_find_node_by_phandle) from [<c05c1bb8>] (of_phandle_iterator_next+0x68/0x178)
>>>> [<c05c1bb8>] (of_phandle_iterator_next) from [<c05c22bc>] (of_count_phandle_with_args+0x5c/0x7c)
>>>> [<c05c22bc>] (of_count_phandle_with_args) from [<c053fc38>] (i2c_demux_pinctrl_probe+0x24/0x1fc)
>>>> [<c053fc38>] (i2c_demux_pinctrl_probe) from [<c04463c4>] (platform_drv_probe+0x48/0x94)
>>>> [<c04463c4>] (platform_drv_probe) from [<c0444a20>] (really_probe+0x1f0/0x2b8)
>>>> [<c0444a20>] (really_probe) from [<c0444e68>] (driver_probe_device+0x140/0x158)
>>>> [<c0444e68>] (driver_probe_device) from [<c0444ff0>] (device_driver_attach+0x44/0x5c)
>>>> [<c0444ff0>] (device_driver_attach) from [<c04450b4>] (__driver_attach+0xac/0xb4)
>>>> [<c04450b4>] (__driver_attach) from [<c0443178>] (bus_for_each_dev+0x64/0xa0)
>>>> [<c0443178>] (bus_for_each_dev) from [<c04438a8>] (bus_add_driver+0x148/0x1a8)
>>>> [<c04438a8>] (bus_add_driver) from [<c0445ad0>] (driver_register+0xac/0xf0)
>>>> [<c0445ad0>] (driver_register) from [<c0b010b0>] (do_one_initcall+0xa8/0x1d4)
>>>> [<c0b010b0>] (do_one_initcall) from [<c0b01448>] (kernel_init_freeable+0x26c/0x2c8)
>>>> [<c0b01448>] (kernel_init_freeable) from [<c0751c70>] (kernel_init+0x8/0x10c)
>>>> [<c0751c70>] (kernel_init) from [<c01010e8>] (ret_from_fork+0x14/0x2c)
>>>> Exception stack(0xeb08dfb0 to 0xeb08dff8)
>>>> dfa0:                                     00000000 00000000 00000000 00000000
>>>> dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>>>> dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
>>>>
>>>> Presumably it loops forever, due to a conversion of -1 to unsigned
>>>> somewhere?
>>> Hmm, I fail to see the culprit. i2c_demux_pinctrl_probe calls
>>> of_count_phandle_with_args with cells_name=NULL. With that I don't see
>>> how my patch changes anything as the only change is in an if
>>> (it->cells_name) block that shouldn't be relevant in your case.
>>>
>>> Can you please verify that the loop in of_count_phandle_with_args is
>>> indeed not terminating, e.g. with
>> The below indicated else-branch was not touched by e42ee61017f58cd9,
>> which ends up setting the count to -1 (aka 0xff...ff in this case).
>> No?
>>
>> int of_phandle_iterator_next(struct of_phandle_iterator *it)
>> {
>>
>> 	...
>>
>> 		if (it->cells_name) {
>>
>> 			...
>>
>> 		} else {
>> 			count = it->cell_count;    /* <---- SUSPECT!!! */
>> 		}
> Oh yeah, you're right. I'm a bit disappointed that I didn't spot this
> myself :-|
>
> Untested patch to fix this problem:

Yesterday I've noticed that sound driver fails to initialize on TM2(e) 
board (arm64) and today I've bisected to this commit. Nice to see that 
the issue has been already investigated.

> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 2f25d2dfecfa..26f7a21d7187 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1284,6 +1284,13 @@ int of_phandle_iterator_init(struct of_phandle_iterator *it,
>   	const __be32 *list;
>   	int size;
>   
> +	/*
> +	 * one of cell_count or cells_name must be provided to determine the
> +	 * argument length.
> +	 */
> +	if (cell_count < 0 && !cells_name)
> +		return -EINVAL;
> +
>   	memset(it, 0, sizeof(*it));
>   
>   	list = of_get_property(np, list_name, &size);
> @@ -1765,6 +1772,18 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
>   	struct of_phandle_iterator it;
>   	int rc, cur_index = 0;
>   
> +	/* If cells_name is NULL we assume an cell_count of 0 */
> +	if (cells_name == NULL) {
> +		const __be32 *list;
> +		int size;
> +
> +		list = of_get_property(np, list_name, &size);
> +		if (!list)
> +			return -ENOENT;
> +
> +		return size / sizeof(*list);
> +	}
> +
>   	rc = of_phandle_iterator_init(&it, np, list_name, cells_name, -1);
>   	if (rc)
>   		return rc;
>
I confirm that the above code works. The patch fixes my TM2(e) sound 
issue, feel free to add:

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/2] of: Let of_for_each_phandle fallback to non-negative cell_count
  2019-09-17 12:25           ` Uwe Kleine-König
       [not found]             ` <CGME20190917124409eucas1p2211d232e6833a44a9ad5dbf72457197c@eucas1p2.samsung.com>
@ 2019-09-17 12:52             ` Geert Uytterhoeven
  1 sibling, 0 replies; 13+ messages in thread
From: Geert Uytterhoeven @ 2019-09-17 12:52 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Rob Herring,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Frank Rowand, Linux Kernel Mailing List, Linux-Renesas,
	Wolfram Sang, Linux IOMMU, linux-mediatek, Linux I2C,
	Sascha Hauer, Matthias Brugger, Will Deacon, Peter Rosin,
	Linux ARM, Robin Murphy

Hi Uwe,

On Tue, Sep 17, 2019 at 2:25 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Tue, Sep 17, 2019 at 11:25:46AM +0000, Peter Rosin wrote:
> > On 2019-09-17 12:13, Uwe Kleine-König wrote:
> > > On Tue, Sep 17, 2019 at 11:40:25AM +0200, Geert Uytterhoeven wrote:
> > >> On Fri, Sep 13, 2019 at 11:58 PM Rob Herring <robh@kernel.org> wrote:
> > >>> On Sat, 24 Aug 2019 15:28:46 +0200, =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?=          wrote:
> > >>>> Referencing device tree nodes from a property allows to pass arguments.
> > >>>> This is for example used for referencing gpios. This looks as follows:
> > >>>>
> > >>>>       gpio_ctrl: gpio-controller {
> > >>>>               #gpio-cells = <2>
> > >>>>               ...
> > >>>>       }
> > >>>>
> > >>>>       someothernode {
> > >>>>               gpios = <&gpio_ctrl 5 0 &gpio_ctrl 3 0>;
> > >>>>               ...
> > >>>>       }
> > >>>>
> > >>>> To know the number of arguments this must be either fixed, or the
> > >>>> referenced node is checked for a $cells_name (here: "#gpio-cells")
> > >>>> property and with this information the start of the second reference can
> > >>>> be determined.
> > >>>>
> > >>>> Currently regulators are referenced with no additional arguments. To
> > >>>> allow some optional arguments without having to change all referenced
> > >>>> nodes this change introduces a way to specify a default cell_count. So
> > >>>> when a phandle is parsed we check for the $cells_name property and use
> > >>>> it as before if present. If it is not present we fall back to
> > >>>> cells_count if non-negative and only fail if cells_count is smaller than
> > >>>> zero.
> > >>>>
> > >>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > >>
> > >> This is now commit e42ee61017f58cd9 ("of: Let of_for_each_phandle fallback
> > >> to non-negative cell_count") in robh/for-next, which causes a lock-up when
> > >> booting a shmobile_defconfig kernel on r8a7791/koelsch:
> > >>
> > >> rcu: INFO: rcu_sched self-detected stall on CPU

> Oh yeah, you're right. I'm a bit disappointed that I didn't spot this
> myself :-|
>
> Untested patch to fix this problem:
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 2f25d2dfecfa..26f7a21d7187 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1284,6 +1284,13 @@ int of_phandle_iterator_init(struct of_phandle_iterator *it,
>         const __be32 *list;
>         int size;
>
> +       /*
> +        * one of cell_count or cells_name must be provided to determine the
> +        * argument length.
> +        */
> +       if (cell_count < 0 && !cells_name)
> +               return -EINVAL;
> +
>         memset(it, 0, sizeof(*it));
>
>         list = of_get_property(np, list_name, &size);
> @@ -1765,6 +1772,18 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
>         struct of_phandle_iterator it;
>         int rc, cur_index = 0;
>
> +       /* If cells_name is NULL we assume an cell_count of 0 */

a cell count

> +       if (cells_name == NULL) {
> +               const __be32 *list;
> +               int size;
> +
> +               list = of_get_property(np, list_name, &size);
> +               if (!list)
> +                       return -ENOENT;
> +
> +               return size / sizeof(*list);
> +       }
> +
>         rc = of_phandle_iterator_init(&it, np, list_name, cells_name, -1);
>         if (rc)
>                 return rc;

Thanks, that fixes the boot for me!

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-09-17 12:52 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-24 13:28 [PATCH v1 1/2] iommu: pass cell_count = -1 to of_for_each_phandle with cells_name Uwe Kleine-König
2019-08-24 13:28 ` [PATCH v1 2/2] of: Let of_for_each_phandle fallback to non-negative cell_count Uwe Kleine-König
2019-09-02 14:26   ` Rob Herring
2019-09-13 21:58   ` Rob Herring
2019-09-17  9:40     ` Geert Uytterhoeven
2019-09-17 10:13       ` Uwe Kleine-König
2019-09-17 11:25         ` Peter Rosin
2019-09-17 12:25           ` Uwe Kleine-König
     [not found]             ` <CGME20190917124409eucas1p2211d232e6833a44a9ad5dbf72457197c@eucas1p2.samsung.com>
2019-09-17 12:44               ` Marek Szyprowski
2019-09-17 12:52             ` Geert Uytterhoeven
2019-09-03 12:52 ` [PATCH v1 1/2] iommu: pass cell_count = -1 to of_for_each_phandle with cells_name Joerg Roedel
2019-09-12  7:43   ` Uwe Kleine-König
2019-09-13  7:37     ` Joerg Roedel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).