Received-SPF: pass (google.com: domain of linux-kernel+bounces-46583-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: <25d3cfd74b26eb6a4aa07f1da93ccf4815b0b1c6.camel@gmail.com>
Subject: Re: [PATCH RESEND RFC] driver: core: don't queue device links
 removal for dt overlays
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: nuno.sa@analog.com, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
 Frank Rowand <frowand.list@gmail.com>, Rob Herring <robh+dt@kernel.org>,
 linux-kernel@vger.kernel.org,  Saravana Kannan <saravanak@google.com>
Date: Wed, 31 Jan 2024 15:21:11 +0100
In-Reply-To: <CAJZ5v0gAK9CChRPSx7Lu=BrGQo22q4swpvvN3__wFw68NfqKPA@mail.gmail.com>
References: 
	<20240123-fix-device-links-overlays-v1-1-9e4f6acaab6c@analog.com>
	 <dcb1b6dbc2172dd66bfdcc0c8135e0d98f1c22dd.camel@gmail.com>
	 <CAJZ5v0gAK9CChRPSx7Lu=BrGQo22q4swpvvN3__wFw68NfqKPA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.3 
Precedence: bulk
MIME-Version: 1.0

On Wed, 2024-01-31 at 14:30 +0100, Rafael J. Wysocki wrote:
> On Wed, Jan 31, 2024 at 1:20=E2=80=AFPM Nuno S=C3=A1 <noname.nuno@gmail.c=
om> wrote:
> >=20
> > On Tue, 2024-01-23 at 16:40 +0100, Nuno Sa via B4 Relay wrote:
> > > From: Nuno Sa <nuno.sa@analog.com>
> > >=20
> > > For device links, releasing the supplier/consumer devices references
> > > happens asynchronously in device_link_release_fn(). Hence, the possib=
le
> > > release of an of_node is also asynchronous. If these nodes were added
> > > through overlays we have a problem because this does not respect the
> > > devicetree overlays assumptions that when a changeset is
> > > being removed in __of_changeset_entry_destroy(), it must hold the las=
t
> > > reference to that node. Due to the async nature of device links that
> > > cannot be guaranteed.
> > >=20
> > > Given the above, in case one of the link consumer/supplier is part of
> > > an overlay node we call directly device_link_release_fn() instead of
> > > queueing it. Yes, it might take some significant time for
> > > device_link_release_fn() to complete because of synchronize_srcu() bu=
t
> > > we would need to, anyways, wait for all OF references to be released =
if
> > > we want to respect overlays assumptions.
> > >=20
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > ---
> > > This RFC is a follow up of a previous one that I sent to the devicetr=
ee
> > > folks [1]. It got rejected because it was not really fixing the root
> > > cause of the issue (which I do agree). Please see the link where I
> > > fully explain what the issue is.
> > >=20
> > > I did also some git blaming and did saw that commit
> > > 80dd33cf72d1 ("drivers: base: Fix device link removal") introduced
> > > queue_work() as we could be releasing the last device reference and h=
ence
> > > sleeping which is against SRCU callback requirements. However, that s=
ame
> > > commit is now making use of synchronize_srcu() which may take
> > > significant time (and I think that's the reason for the work item?).
> > >=20
> > > However, given the dt overlays requirements, I'm not seeing any
> > > reason to not be able to run device_link_release_fn() synchronously i=
f we
> > > detect an OVERLAY node is being released. I mean, even if we come up
> > > (and I did some experiments in this regard) with some async mechanism=
 to
> > > release the OF nodes refcounts, we still need a synchronization point
> > > somewhere.
> > >=20
> > > Anyways, I would like to have some feedback on how acceptable would t=
his
> > > be or what else could I do so we can have a "clean" dt overlay remova=
l.
> > >=20
> > > I'm also including dt folks so they can give some comments on the new
> > > device_node_overlay_removal() function. My goal is to try to detect w=
hen
> > > an
> > > overlay is being removed (maybe we could even have an explicit flag f=
or
> > > it?) and only directly call device_link_release_fn() in that case.
> > >=20
> > > [1]:
> > > https://lore.kernel.org/linux-devicetree/20230511151047.1779841-1-nun=
o.sa@analog.com/
> > > ---
> > > =C2=A0drivers/base/core.c | 25 ++++++++++++++++++++++++-
> > > =C2=A01 file changed, 24 insertions(+), 1 deletion(-)
> > >=20
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index 14d46af40f9a..31ea001f6142 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -497,6 +497,18 @@ static struct attribute *devlink_attrs[] =3D {
> > > =C2=A0};
> > > =C2=A0ATTRIBUTE_GROUPS(devlink);
> > >=20
> > > +static bool device_node_overlay_removal(struct device *dev)
> > > +{
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 if (!dev_of_node(dev))
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 return false;
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 if (!of_node_check_flag(dev->of_node, OF_DE=
TACHED))
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 return false;
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 if (!of_node_check_flag(dev->of_node, OF_OV=
ERLAY))
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 return false;
> > > +
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 return true;
> > > +}
> > > +
> > > =C2=A0static void device_link_release_fn(struct work_struct *work)
> > > =C2=A0{
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct device_link *link =3D container=
_of(work, struct device_link,
> > > rm_work);
> > > @@ -532,8 +544,19 @@ static void devlink_dev_release(struct device *d=
ev)
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * synchronization in device_link=
_release_fn() and if the consumer
> > > or
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * supplier devices get deleted w=
hen it runs, so put it into the
> > > "long"
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * workqueue.
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 *
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * However, if any of the supplier, co=
nsumer nodes is being removed
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * through overlay removal, the expect=
ation in
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * __of_changeset_entry_destroy() is f=
or the node 'kref' to be 1
> > > which
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * cannot be guaranteed with the async=
 nature of
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * device_link_release_fn(). Hence, do=
 it synchronously for the
> > > overlay
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * case.
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */
> > > -=C2=A0=C2=A0=C2=A0=C2=A0 queue_work(system_long_wq, &link->rm_work);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 if (device_node_overlay_removal(link->consu=
mer) ||
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 device_node_overlay=
_removal(link->supplier))
> > > +=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_release_fn(&link->rm_work);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 else
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 queue_work(system_long_wq, &link->rm_work);
> > > =C2=A0}
> > >=20
> > > =C2=A0static struct class devlink_class =3D {
> > >=20
> > > ---
> > > base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
> > > change-id: 20240123-fix-device-links-overlays-5422e033a09b
> > > --
> > >=20
> > > Thanks!
> > > - Nuno S=C3=A1
> > >=20
> >=20
> > Hi Rafael,
> >=20
> > Would be nice to have your feedback on this one or if this is a complet=
e
> > nack...
> > I think calling device_link_release_fn() synchronously is ok but I migh=
t be
> > completely wrong.
>=20
> Well, it sounds like you are expecting me to confirm that what you are
> doing makes sense, but I cannot do that, because I am not sufficiently
> familiar with DT overlays.
>=20

I'm trying to understand if there's no hidden issue by calling it synchrono=
usly.
(don't think there is but this is rather core stuff :)).

From the DT guys, it would be helpful to get feedback on the new
device_node_overlay_removal() helper I'm introducing. The goal is to just d=
o the
sync release in case we detect a node being removed as a result of an overl=
ay
removal.

> You first need to convince yourself that you are not completely wrong.

I mean, the problem is definitely real and if you see the link I pasted in =
the
cover, this will all lead to big splats.

>=20
> > +Cc Saravan as he should also be very familiar with device_links and se=
e if
> > the
> > above fairly simple solution is sane.
> >=20
> > I also don't want to be pushy as I know you guys are all very busy but =
it's
> > (i
> > think) the third time I resend the patch :)
>=20
> Sorry about that, I haven't realized that my input is requisite.
>=20

Yeah, get_mantainers gives me you and Greg but I think you're the main dev =
on
dev_links right?

> So the patch not only calls device_link_release_fn() synchronously,
> but it also calls this function directly and I, personally, wouldn't
> do at least the latter.
>=20

So you mean adding something like adding a new

device_link_release(struct device_link *link) helper
and either call it synchronously from devlink_dev_release() or asynchronous=
ly
from device_link_release_fn()?

I can drop the RFC and send a patch with the above...

- Nuno S=C3=A1