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

Petr Mladek
2020-01-13T09:17:58Z

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