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

On Fri 2020-01-03 10:35:55, Guenter Roeck wrote:

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".

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.

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?

Best Regards,
Petr