| Message ID | 416d1ea056bb2d7ec6f21d8919b96a3d48099344.1689776064.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | of: overlay/unittest: Miscellaneous fixes and improvements | expand |
On Wed, Jul 19, 2023 at 05:00:01PM +0200, Geert Uytterhoeven wrote: > Formatting strings using "%pOF" while holding devtree_lock causes a > deadlock. Lockdep reports: > > of_get_parent from of_fwnode_get_parent+0x18/0x24 > ^^^^^^^^^^^^^ I'm wondering if we really need the lock in there. We never unset or change the parent. It gets detached, but we're not checking for that. The node could get freed, but the lock is not for that, refcounts are. > of_fwnode_get_parent from fwnode_count_parents+0xc/0x28 count parents? Huh? Isn't it always 1? > fwnode_count_parents from fwnode_full_name_string+0x18/0xac > fwnode_full_name_string from device_node_string+0x1a0/0x404 > device_node_string from pointer+0x3c0/0x534 > pointer from vsnprintf+0x248/0x36c > vsnprintf from vprintk_store+0x130/0x3b4 > > Fix this by making the locking cover only the parts that really need it. > > Fixes: 0d638a07d3a1e98a ("of: Convert to using %pOF instead of full_name") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/of/dynamic.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c > index e311d406b1705306..eae45a1c673ee05f 100644 > --- a/drivers/of/dynamic.c > +++ b/drivers/of/dynamic.c > @@ -601,13 +601,16 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) > > __of_changeset_entry_dump(ce); > > - raw_spin_lock_irqsave(&devtree_lock, flags); > switch (ce->action) { > case OF_RECONFIG_ATTACH_NODE: > + raw_spin_lock_irqsave(&devtree_lock, flags); > __of_attach_node(ce->np); > + raw_spin_unlock_irqrestore(&devtree_lock, flags); I think you could just move the spinlock into __of_attach_node(). The only other caller looks like this. > break; > case OF_RECONFIG_DETACH_NODE: > + raw_spin_lock_irqsave(&devtree_lock, flags); > __of_detach_node(ce->np); > + raw_spin_unlock_irqrestore(&devtree_lock, flags); > break; > case OF_RECONFIG_ADD_PROPERTY: > /* If the property is in deadprops then it must be removed */ > @@ -619,7 +622,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) > } > } > > + raw_spin_lock_irqsave(&devtree_lock, flags); > ret = __of_add_property(ce->np, ce->prop); > + raw_spin_unlock_irqrestore(&devtree_lock, flags); > if (ret) { > pr_err("changeset: add_property failed @%pOF/%s\n", > ce->np, > @@ -628,7 +633,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) > } > break; > case OF_RECONFIG_REMOVE_PROPERTY: > + raw_spin_lock_irqsave(&devtree_lock, flags); > ret = __of_remove_property(ce->np, ce->prop); > + raw_spin_unlock_irqrestore(&devtree_lock, flags); > if (ret) { > pr_err("changeset: remove_property failed @%pOF/%s\n", > ce->np, > @@ -647,7 +654,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) > } > } > > + raw_spin_lock_irqsave(&devtree_lock, flags); > ret = __of_update_property(ce->np, ce->prop, &old_prop); > + raw_spin_unlock_irqrestore(&devtree_lock, flags); > if (ret) { > pr_err("changeset: update_property failed @%pOF/%s\n", > ce->np, > @@ -658,7 +667,6 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) > default: > ret = -EINVAL; > } > - raw_spin_unlock_irqrestore(&devtree_lock, flags); > > if (ret) > return ret; > -- > 2.34.1 >
+Sakari On Wed, Jul 19, 2023 at 05:02:56PM -0600, Rob Herring wrote: > On Wed, Jul 19, 2023 at 05:00:01PM +0200, Geert Uytterhoeven wrote: > > Formatting strings using "%pOF" while holding devtree_lock causes a > > deadlock. Lockdep reports: > > > > of_get_parent from of_fwnode_get_parent+0x18/0x24 > > ^^^^^^^^^^^^^ > > I'm wondering if we really need the lock in there. We never unset or > change the parent. It gets detached, but we're not checking for that. > The node could get freed, but the lock is not for that, refcounts are. The lock existed since 2.6.12 for powerpc. It's not clear to me whether it was really ever needed. There's lots of places we just access 'parent' without a lock. Not to say that's right. The lock doesn't even help in this case because we release the lock on each count and between counting and getting the names. If the tree changes, the lock isn't going to help. > > of_fwnode_get_parent from fwnode_count_parents+0xc/0x28 > > count parents? Huh? Isn't it always 1? > > > fwnode_count_parents from fwnode_full_name_string+0x18/0xac > > fwnode_full_name_string from device_node_string+0x1a0/0x404 > > device_node_string from pointer+0x3c0/0x534 > > pointer from vsnprintf+0x248/0x36c > > vsnprintf from vprintk_store+0x130/0x3b4 > > > > Fix this by making the locking cover only the parts that really need it. > > > > Fixes: 0d638a07d3a1e98a ("of: Convert to using %pOF instead of full_name") That's the wrong commit. My implementation in vsprintf.c worked with this. It's commit a92eb7621b9f ("lib/vsprintf: Make use of fwnode API to obtain node names and separators") which broke it. It came 2 years later. The fwnode based implementation looks like the wrong level of abstraction to me. Why not just push 'give me the full name' down to the fwnode backends? The functions defined are *only* used by vsprintf.c. I don't really understand the "let's change everything to use fwnode" even for things which will never be anything but DT. %pOF is DT only. </rant> Rob
Hi Rob, On Thu, Jul 20, 2023 at 1:03 AM Rob Herring <robh@kernel.org> wrote: > On Wed, Jul 19, 2023 at 05:00:01PM +0200, Geert Uytterhoeven wrote: > > Formatting strings using "%pOF" while holding devtree_lock causes a > > deadlock. Lockdep reports: > > > > of_get_parent from of_fwnode_get_parent+0x18/0x24 > > ^^^^^^^^^^^^^ > > of_fwnode_get_parent from fwnode_count_parents+0xc/0x28 > > fwnode_count_parents from fwnode_full_name_string+0x18/0xac > > fwnode_full_name_string from device_node_string+0x1a0/0x404 > > device_node_string from pointer+0x3c0/0x534 > > pointer from vsnprintf+0x248/0x36c > > vsnprintf from vprintk_store+0x130/0x3b4 > > > > Fix this by making the locking cover only the parts that really need it. > > > > Fixes: 0d638a07d3a1e98a ("of: Convert to using %pOF instead of full_name") > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > drivers/of/dynamic.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c > > index e311d406b1705306..eae45a1c673ee05f 100644 > > --- a/drivers/of/dynamic.c > > +++ b/drivers/of/dynamic.c > > @@ -601,13 +601,16 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) > > > > __of_changeset_entry_dump(ce); > > > > - raw_spin_lock_irqsave(&devtree_lock, flags); > > switch (ce->action) { > > case OF_RECONFIG_ATTACH_NODE: > > + raw_spin_lock_irqsave(&devtree_lock, flags); > > __of_attach_node(ce->np); > > + raw_spin_unlock_irqrestore(&devtree_lock, flags); > > I think you could just move the spinlock into __of_attach_node(). The > only other caller looks like this. I'd rather not do that, as the double underscore is typically used to indicate that this function does not take the lock. Cfr. of_find_property() vs. __of_find_property(). Gr{oetje,eeting}s, Geert
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c index e311d406b1705306..eae45a1c673ee05f 100644 --- a/drivers/of/dynamic.c +++ b/drivers/of/dynamic.c @@ -601,13 +601,16 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) __of_changeset_entry_dump(ce); - raw_spin_lock_irqsave(&devtree_lock, flags); switch (ce->action) { case OF_RECONFIG_ATTACH_NODE: + raw_spin_lock_irqsave(&devtree_lock, flags); __of_attach_node(ce->np); + raw_spin_unlock_irqrestore(&devtree_lock, flags); break; case OF_RECONFIG_DETACH_NODE: + raw_spin_lock_irqsave(&devtree_lock, flags); __of_detach_node(ce->np); + raw_spin_unlock_irqrestore(&devtree_lock, flags); break; case OF_RECONFIG_ADD_PROPERTY: /* If the property is in deadprops then it must be removed */ @@ -619,7 +622,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) } } + raw_spin_lock_irqsave(&devtree_lock, flags); ret = __of_add_property(ce->np, ce->prop); + raw_spin_unlock_irqrestore(&devtree_lock, flags); if (ret) { pr_err("changeset: add_property failed @%pOF/%s\n", ce->np, @@ -628,7 +633,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) } break; case OF_RECONFIG_REMOVE_PROPERTY: + raw_spin_lock_irqsave(&devtree_lock, flags); ret = __of_remove_property(ce->np, ce->prop); + raw_spin_unlock_irqrestore(&devtree_lock, flags); if (ret) { pr_err("changeset: remove_property failed @%pOF/%s\n", ce->np, @@ -647,7 +654,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) } } + raw_spin_lock_irqsave(&devtree_lock, flags); ret = __of_update_property(ce->np, ce->prop, &old_prop); + raw_spin_unlock_irqrestore(&devtree_lock, flags); if (ret) { pr_err("changeset: update_property failed @%pOF/%s\n", ce->np, @@ -658,7 +667,6 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) default: ret = -EINVAL; } - raw_spin_unlock_irqrestore(&devtree_lock, flags); if (ret) return ret;
Formatting strings using "%pOF" while holding devtree_lock causes a deadlock. Lockdep reports: of_get_parent from of_fwnode_get_parent+0x18/0x24 ^^^^^^^^^^^^^ of_fwnode_get_parent from fwnode_count_parents+0xc/0x28 fwnode_count_parents from fwnode_full_name_string+0x18/0xac fwnode_full_name_string from device_node_string+0x1a0/0x404 device_node_string from pointer+0x3c0/0x534 pointer from vsnprintf+0x248/0x36c vsnprintf from vprintk_store+0x130/0x3b4 Fix this by making the locking cover only the parts that really need it. Fixes: 0d638a07d3a1e98a ("of: Convert to using %pOF instead of full_name") Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/of/dynamic.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)