Re: [PATCH v9 09/12] lib/vsprintf: Make use of fwnode API to obtain node names and separators

From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Guenter Roeck <linux@roeck-us.net>,
	linux-kernel@vger.kernel.org, rafael@kernel.org,
	Rob Herring <robh@kernel.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	linux-acpi@vger.kernel.org, devicetree@vger.kernel.org,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	Joe Perches <joe@perches.com>
Subject: Re: [PATCH v9 09/12] lib/vsprintf: Make use of fwnode API to obtain node names and separators
Date: Fri, 17 Jan 2020 16:23:04 +0200	[thread overview]
Message-ID: <20200117142304.GQ5440@paasikivi.fi.intel.com> (raw)
In-Reply-To: <20200113091751.d63u7jbyh6p2rj23@pathway.suse.cz>

Hi Petr,

On Mon, Jan 13, 2020 at 10:17:51AM +0100, Petr Mladek wrote:
> On Fri 2020-01-03 10:35:55, Guenter Roeck wrote:
> > On Fri, Jan 03, 2020 at 03:42:53PM +0100, Petr Mladek wrote:
> > > On Fri 2020-01-03 13:21:45, Sakari Ailus wrote:
> > > > Hi Guenter,
> > > > 
> > > > On Thu, Jan 02, 2020 at 02:20:41PM -0800, Guenter Roeck wrote:
> > > > > Hi,
> > > > > 
> > > > > On Thu, Oct 03, 2019 at 03:32:16PM +0300, Sakari Ailus wrote:
> > > > > > Instead of implementing our own means of discovering parent nodes, node
> > > > > > names or counting how many parents a node has, use the newly added
> > > > > > functions in the fwnode API to obtain that information.
> > > > > > 
> > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > > > > > ---
> > > > > 
> > > > > This patch results in a lockdep splat when running one of my qemu
> > > > > emulations. See below for log and bisect results. A complete log
> > > > > is available at
> > > > > https://kerneltests.org/builders/qemu-arm-master/builds/1408/steps/qemubuildcommand/logs/stdio
> > > > > 
> > > > > Guenter
> > > > 
> > > > Thank you for reporting this.
> > > > 
> > > > I looked into the issue, and indeed I can conform the patch introduces this
> > > > as it takes the devtree_lock for printing the name of the fwnode. There is
> > > 
> > > I guess that you meant "is not".

Right, that's what I meant. Sometimes small words can make a big
difference. :-)

> > > 
> > > 
> > > > however chance of a deadlock in practice as the code in mm/slub.c does not
> > > > deal with fwnodes (in which case acquiring devtree_lock could be possible),
> > > > maybe for other reasons as well. The patch however introduces an unpleasant
> > > > source of such warnings.
> > > 
> > > I agree that it is a false positive. alloc/free is called in OF code
> > > under devtree_lock. But OF code is not called from alloc/free (slub.c)
> > > and it should not happen.
> > > 
> > 
> > Assuming that memory allocation is indeed called from code holding
> > devtree_lock: The problem, as I see it, is that the order of acquiring
> > locks is different. In OF code, the order is
> > 	devtree_lock
> > 	(&n->list_lock)->rlock
> 
> Yes, this happens when alloc is called in OF code under devtree_lock.
> 
> > Elsewhere, in %pOF print sequences, it is
> > 	(&n->list_lock)->rlock
> > 	devtree_lock
> 
> I believe that this order does not exist in reality. lockep "just"
> connected this the two locks via logbuf_lock. When printk() is
> called in the allocator:
> 
> 	(&n->list_lock)->rlock
> 	logbuf_lock
> 
> and when %pOF is used in printk():
> 
> 	logbuf_lock
> 	devtree_lock
> 
> From this two lockdep assumes that it might be possible to
> use %pOF in printk() from allocator code:
> 
> 	(&n->list_lock)->rlock
> 	logbuf_lock
> 	devtree_lock
> 
> But I believe that this does not make sense and never happen reality.
> 
> That said, I would still prefer when %pOF could be implemented
> without the lock. It would make it usable anywhere without any
> risk of deadlock.
> 
> Sakari, what is your opinion, please?

The reason taking that lock is to be able to traverse the device tree data
structure to find the names of the nodes upto the root node. This happens
only when the full node name is printed.

Traversing the tree takes place through the fwnode framework, and currently
the framework uses only the name of the fwnode itself to print the full
path. It *could* use the full name directly, but that way removing the
full_name field (taking up some memory right now) would not be possible.
That said, I don't know if there are plans to do so. A quick look at one
system tells the size of this memory is around 20 kB.

ACPI does not use such locks in traversing to the tree, but it might do
that in the future, and avoiding the lock there would also require copying
the full node name to each node in the system.

If there are plans to avoid having logbuf_lock, then the problem disappears
with that.

How about disabling the lockdep warning now, and if it seems we're not
getting rid of the logbuf_lock in a reasonable timeframe, then look at the
alternatives, such as using the full_name in printing node names instead?

-- 
Kind regards,

Sakari Ailus