+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
