Re: [PATCH 01/13] of: dynamic: Do not use "%pOF" while holding devtree_lock

From: Rob Herring <robh@kernel.org>
To: Geert Uytterhoeven <geert+renesas@glider.be>,
	Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Frank Rowand <frowand.list@gmail.com>,
	Pantelis Antoniou <pantelis.antoniou@konsulko.com>,
	devicetree@vger.kernel.org, linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH 01/13] of: dynamic: Do not use "%pOF" while holding devtree_lock
Date: Thu, 20 Jul 2023 15:30:31 -0600	[thread overview]
Message-ID: <20230720213031.GC2939849-robh@kernel.org> (raw)
In-Reply-To: <20230719230256.GA900970-robh@kernel.org>

+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