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:

quoted

On Fri, Jan 03, 2020 at 03:42:53PM +0100, Petr Mladek wrote:

quoted

On Fri 2020-01-03 13:21:45, Sakari Ailus wrote:

quoted

Hi Guenter,

On Thu, Jan 02, 2020 at 02:20:41PM -0800, Guenter Roeck wrote:

quoted

Hi,

On Thu, Oct 03, 2019 at 03:32:16PM +0300, Sakari Ailus wrote:

quoted

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. :-)

quoted

quoted

quoted

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.

quoted

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