Received-SPF: pass (google.com: domain of linux-kernel+bounces-83659-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <86f2262d059db84070745e299d96dde3e6078220.camel@gmail.com>
Subject: Re: [PATCH 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: Herve Codina <herve.codina@bootlin.com>, Saravana Kannan
	 <saravanak@google.com>, Luca Ceresoli <luca.ceresoli@bootlin.com>, Nuno Sa
	 <nuno.sa@analog.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael J. Wysocki"
 <rafael@kernel.org>, Rob Herring <robh+dt@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>, Thomas Petazzoni
 <thomas.petazzoni@bootlin.com>, Android Kernel Team
 <kernel-team@android.com>
Date: Tue, 27 Feb 2024 17:55:07 +0100
In-Reply-To: <20240227162422.76a00f11@bootlin.com>
References: <20231130174126.688486-1-herve.codina@bootlin.com>
	 <20231130174126.688486-3-herve.codina@bootlin.com>
	 <CAGETcx_zB95nyTpi-_kYW_VqnPqMEc8mS9sewSwRNVr0x=7+kA@mail.gmail.com>
	 <20240227162422.76a00f11@bootlin.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

On Tue, 2024-02-27 at 16:24 +0100, Herve Codina wrote:
> Hi Saravana, Luca, Nuno,
>=20
> On Tue, 20 Feb 2024 16:37:05 -0800
> Saravana Kannan <saravanak@google.com> wrote:
>=20
> ...
>=20
> > >=20
> > > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > > index a9a292d6d59b..5c5f808b163e 100644
> > > --- a/drivers/of/overlay.c
> > > +++ b/drivers/of/overlay.c
> > > @@ -1202,6 +1202,12 @@ int of_overlay_remove(int *ovcs_id)
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 goto out;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
> > >=20
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /*
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * Wait for any ongoing de=
vice link removals before removing some of
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * nodes
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 device_link_wait_removal();
> > > +=C2=A0=20
> >=20
> > Nuno in his patch[1] had this "wait" happen inside
> > __of_changeset_entry_destroy(). Which seems to be necessary to not hit
> > the issue that Luca reported[2] in this patch series. Is there any
> > problem with doing that?
> >=20
> > Luca for some reason did a unlock/lock(of_mutex) in his test patch and
> > I don't think that's necessary.
>=20
> I think the unlock/lock in Luca's case and so in Nuno's case is needed.
>=20
> I do the device_link_wait_removal() wihout having the of_mutex locked.
>=20
> Now, suppose I do the device_link_wait_removal() call with the of_mutex l=
ocked.
> The following flow is allowed and a deadlock is present.
>=20
> of_overlay_remove()
> =C2=A0 lock(of_mutex)
> =C2=A0=C2=A0=C2=A0=C2=A0 device_link_wait_removal()
>=20
> And, from the workqueue jobs execution:
> =C2=A0 ...
> =C2=A0=C2=A0=C2=A0 device_put()
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 some_driver->remove()
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 of_overlay_remove() <--- The j=
ob will never end.
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 It is waiting for of_mutex.
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Deadlock
>=20

We may need some input from Saravana (and others) on this. I might be missi=
ng
something but can a put_device() lead into a driver remove callback? Driver=
 code is
not device code and put_device() leads to device_release() which will eithe=
r call the
device=C2=A0->release(), ->type->release() or the class ->dev_release(). An=
d, IMO, calling
of_overlay_remove() or something like that (like something that would lead =
to
unbinding a device from it's driver) in a device release callback would be =
at the
very least very questionable. Typically, what you see in there is of_node_p=
ut() and
things like kfree() of the device itself or any other data.

The driver remove callback should be called when unbinding the device from =
it's
drivers and devlinks should also be removed after device_unbind_cleanup() (=
i.e, after
the driver remove callback).

Having said the above, the driver core has lots of subtleties so, again, I =
can be
missing something. But at this point I'm still not seeing any deadlock...

- Nuno S=C3=A1