Received-SPF: pass (google.com: domain of linux-kernel+bounces-90838-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <93d0adf0ee6f3737af4d482dc206fe152f762482.camel@gmail.com>
Subject: Re: [PATCH v3 2/2] of: overlay: Synchronize of_overlay_remove()
 with the devlink removals
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: Rob Herring <robh@kernel.org>
Cc: Herve Codina <herve.codina@bootlin.com>, Greg Kroah-Hartman
 <gregkh@linuxfoundation.org>, "Rafael J. Wysocki" <rafael@kernel.org>,
 Frank Rowand <frowand.list@gmail.com>, Lizhi Hou <lizhi.hou@amd.com>, Max
 Zhen <max.zhen@amd.com>,  Sonal Santan <sonal.santan@amd.com>, Stefano
 Stabellini <stefano.stabellini@xilinx.com>, Jonathan Cameron
 <Jonathan.Cameron@huawei.com>, linux-kernel@vger.kernel.org, 
 devicetree@vger.kernel.org, Allan Nielsen <allan.nielsen@microchip.com>, 
 Horatiu Vultur <horatiu.vultur@microchip.com>, Steen Hegelund
 <steen.hegelund@microchip.com>, Luca Ceresoli <luca.ceresoli@bootlin.com>,
 Nuno Sa <nuno.sa@analog.com>, Thomas Petazzoni
 <thomas.petazzoni@bootlin.com>, stable@vger.kernel.org
Date: Mon, 04 Mar 2024 16:36:15 +0100
In-Reply-To: <20240304152202.GA222088-robh@kernel.org>
References: <20240229105204.720717-1-herve.codina@bootlin.com>
	 <20240229105204.720717-3-herve.codina@bootlin.com>
	 <acb69aa8c1a4c4e9849123ef538b9646a71507a0.camel@gmail.com>
	 <20240304152202.GA222088-robh@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 
Precedence: bulk
MIME-Version: 1.0

On Mon, 2024-03-04 at 09:22 -0600, Rob Herring wrote:
> On Thu, Feb 29, 2024 at 12:18:49PM +0100, Nuno S=C3=A1 wrote:
> > On Thu, 2024-02-29 at 11:52 +0100, Herve Codina wrote:
> > > In the following sequence:
> > > =C2=A0 1) of_platform_depopulate()
> > > =C2=A0 2) of_overlay_remove()
> > >=20
> > > During the step 1, devices are destroyed and devlinks are removed.
> > > During the step 2, OF nodes are destroyed but
> > > __of_changeset_entry_destroy() can raise warnings related to missing
> > > of_node_put():
> > > =C2=A0 ERROR: memory leak, expected refcount 1 instead of 2 ...
> > >=20
> > > Indeed, during the devlink removals performed at step 1, the removal
> > > itself releasing the device (and the attached of_node) is done by a j=
ob
> > > queued in a workqueue and so, it is done asynchronously with respect =
to
> > > function calls.
> > > When the warning is present, of_node_put() will be called but wrongly
> > > too late from the workqueue job.
> > >=20
> > > In order to be sure that any ongoing devlink removals are done before
> > > the of_node destruction, synchronize the of_overlay_remove() with the
> > > devlink removals.
> > >=20
> > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > ---
> > > =C2=A0drivers/of/overlay.c | 10 +++++++++-
> > > =C2=A01 file changed, 9 insertions(+), 1 deletion(-)
> > >=20
> > > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > > index 2ae7e9d24a64..7a010a62b9d8 100644
> > > --- a/drivers/of/overlay.c
> > > +++ b/drivers/of/overlay.c
> > > @@ -8,6 +8,7 @@
> > > =C2=A0
> > > =C2=A0#define pr_fmt(fmt)	"OF: overlay: " fmt
> > > =C2=A0
> > > +#include <linux/device.h>
> >=20
> > This is clearly up to the DT maintainers to decide but, IMHO, I would v=
ery
> > much
> > prefer to see fwnode.h included in here rather than directly device.h (=
so
> > yeah,
> > renaming the function to fwnode_*).
>=20
> IMO, the DT code should know almost nothing about fwnode because that's=
=20
> the layer above it. But then overlay stuff is kind of a layer above the=
=20
> core DT code too.

Yeah, my reasoning is just that it may be better than knowing about device.=
h
code... But maybe I'm wrong :)

>=20
> > But yeah, I might be biased by own series :)
> >=20
> > > =C2=A0#include <linux/kernel.h>
> > > =C2=A0#include <linux/module.h>
> > > =C2=A0#include <linux/of.h>
> > > @@ -853,6 +854,14 @@ static void free_overlay_changeset(struct
> > > overlay_changeset *ovcs)
> > > =C2=A0{
> > > =C2=A0	int i;
> > > =C2=A0
> > > +	/*
> > > +	 * Wait for any ongoing device link removals before removing some
> > > of
> > > +	 * nodes. Drop the global lock while waiting
> > > +	 */
> > > +	mutex_unlock(&of_mutex);
> > > +	device_link_wait_removal();
> > > +	mutex_lock(&of_mutex);
> >=20
> > I'm still not convinced we need to drop the lock. What happens if someo=
ne
> > else
> > grabs the lock while we are in device_link_wait_removal()? Can we guara=
ntee
> > that
> > we can't screw things badly?
>=20
> It is also just ugly because it's the callers of=20
> free_overlay_changeset() that hold the lock and now we're releasing it=
=20
> behind their back.
>=20
> As device_link_wait_removal() is called before we touch anything, can't=
=20
> it be called before we take the lock? And do we need to call it if=20
> applying the overlay fails?
>=20

My natural feeling was to put it right before checking the node refcount...=
 and
I would like to still see proof that there's any potential deadlock. I did =
not
checked the code but the issue with calling it before we take the lock is t=
hat
likely the device links wont be removed because the overlay removal path (w=
hich
unbinds devices from drivers) needs to run under the lock?

- Nuno S=C3=A1