Received-SPF: pass (google.com: domain of linux-kernel+bounces-77900-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <6e6870b37cdd3204bbf54ebce3401b3af13e9131.camel@gmail.com>
Subject: Re: [PATCH 1/2] driver core: Introduce device_link_wait_removal()
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: Saravana Kannan <saravanak@google.com>
Cc: Herve Codina <herve.codina@bootlin.com>, 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: Fri, 23 Feb 2024 09:13:21 +0100
In-Reply-To: <CAGETcx9h4=k9XW+jZCw9zcVZruNZLPDQDt_sNZYXc05eQ2_uWQ@mail.gmail.com>
References: <20231130174126.688486-1-herve.codina@bootlin.com>
	 <20231130174126.688486-2-herve.codina@bootlin.com>
	 <CAGETcx9uP86EHyKJNifBMd23oCsA+KpMa+e36wJEEnHDve+Avg@mail.gmail.com>
	 <c4e1092298244c288212e9f6cdbf7c26d0e9d83b.camel@gmail.com>
	 <CAGETcx9h4=k9XW+jZCw9zcVZruNZLPDQDt_sNZYXc05eQ2_uWQ@mail.gmail.com>
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 Thu, 2024-02-22 at 17:08 -0800, Saravana Kannan wrote:
> On Tue, Feb 20, 2024 at 10:56=E2=80=AFPM Nuno S=C3=A1 <noname.nuno@gmail.=
com> wrote:
> >=20
> > On Tue, 2024-02-20 at 16:31 -0800, Saravana Kannan wrote:
> > > On Thu, Nov 30, 2023 at 9:41=E2=80=AFAM Herve Codina <herve.codina@bo=
otlin.com>
> > > wrote:
> > > >=20
> > > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > introduces a workqueue to release the consumer and supplier devices=
 used
> > > > in the devlink.
> > > > In the job queued, devices are release and in turn, when all the
> > > > references to these devices are dropped, the release function of th=
e
> > > > device itself is called.
> > > >=20
> > > > Nothing is present to provide some synchronisation with this workqu=
eue
> > > > in order to ensure that all ongoing releasing operations are done a=
nd
> > > > so, some other operations can be started safely.
> > > >=20
> > > > For instance, in the following sequence:
> > > > =C2=A0 1) of_platform_depopulate()
> > > > =C2=A0 2) of_overlay_remove()
> > > >=20
> > > > During the step 1, devices are released and related devlinks are re=
moved
> > > > (jobs pushed in the workqueue).
> > > > During the step 2, OF nodes are destroyed but, without any
> > > > synchronisation with devlink removal jobs, of_overlay_remove() can =
raise
> > > > warnings related to missing of_node_put():
> > > > =C2=A0 ERROR: memory leak, expected refcount 1 instead of 2
> > > >=20
> > > > Indeed, the missing of_node_put() call is going to be done, too lat=
e,
> > > > from the workqueue job execution.
> > > >=20
> > > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > > operations waiting for the end of devlink removals (i.e. end of
> > > > workqueue jobs).
> > > > Also, as a flushing operation is done on the workqueue, the workque=
ue
> > > > used is moved from a system-wide workqueue to a local one.
> > >=20
> > > Thanks for the bug report and fix. Sorry again about the delay in
> > > reviewing the changes.
> > >=20
> > > Please add Fixes tag for 80dd33cf72d1.
> > >=20
> > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > > ---
> > > > =C2=A0drivers/base/core.c=C2=A0=C2=A0=C2=A0 | 26 ++++++++++++++++++=
+++++---
> > > > =C2=A0include/linux/device.h |=C2=A0 1 +
> > > > =C2=A02 files changed, 24 insertions(+), 3 deletions(-)
> > > >=20
> > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > index ac026187ac6a..2e102a77758c 100644
> > > > --- a/drivers/base/core.c
> > > > +++ b/drivers/base/core.c
> > > > @@ -44,6 +44,7 @@ static bool fw_devlink_is_permissive(void);
> > > > =C2=A0static void __fw_devlink_link_to_consumers(struct device *dev=
);
> > > > =C2=A0static bool fw_devlink_drv_reg_done;
> > > > =C2=A0static bool fw_devlink_best_effort;
> > > > +static struct workqueue_struct *fw_devlink_wq;
> > > >=20
> > > > =C2=A0/**
> > > > =C2=A0 * __fwnode_link_add - Create a link between two fwnode_handl=
es.
> > > > @@ -530,12 +531,26 @@ static void devlink_dev_release(struct device
> > > > *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=C2=A0=C2=A0=C2=A0 * It may take a wh=
ile to complete this work because of the SRCU
> > > > =C2=A0=C2=A0=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=C2=A0 * supplier devices get =
deleted when it runs, so put it into the
> > > > "long"
> > > > -=C2=A0=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 * supplier devices get =
deleted when it runs, so put it into the
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * dedicated workqueue.
> > > > =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 queue_work(system_long_wq, &l=
ink->rm_work);
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 queue_work(fw_devlink_wq, &li=
nk->rm_work);
> > >=20
> > > This has nothing to do with fw_devlink. fw_devlink is just triggering
> > > the issue in device links. You can hit this bug without fw_devlink to=
o.
> > > So call this device_link_wq since it's consistent with device_link_* =
APIs.
> > >=20
> >=20
> > I'm not sure if I got this right in my series. I do call
> > devlink_release_queue() to
> > my queue. But on the Overlay side I use fwnode_links_flush_queue() beca=
use
> > it looked
> > more sensible from an OF point of view. And including (in OF code)
> > linux/fwnode.h
> > instead linux/device.h makes more sense to me.
> >=20
> > > > =C2=A0}
> > > >=20
> > > > +/**
> > > > + * device_link_wait_removal - Wait for ongoing devlink removal job=
s to
> > > > terminate
> > > > + */
> > > > +void device_link_wait_removal(void)
> > > > +{
> > > > +=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 * devlink removal jobs =
are queued in the dedicated work queue.
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * To be sure that all r=
emoval jobs are terminated, ensure that
> > > > any
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * scheduled work has ru=
n to completion.
> > > > +=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 drain_workqueue(fw_devlink_wq=
);
> > >=20
> > > Is there a reason this needs to be drain_workqueu() instead of
> > > flush_workqueue(). Drain is a stronger guarantee than we need in this
> > > case. All we are trying to make sure is that all the device link
> > > remove work queued so far have completed.
> > >=20
> >=20
> > Yeah, I'm also using flush_workqueue().
> >=20
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> > > > +
> > > > =C2=A0static struct class devlink_class =3D {
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .name =3D "devlink",
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .dev_groups =3D devlink_=
groups,
> > > > @@ -4085,9 +4100,14 @@ int __init devices_init(void)
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sysfs_dev_char_kobj =3D =
kobject_create_and_add("char", dev_kobj);
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!sysfs_dev_char_kobj=
)
> > > > =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 char_kobj_err;
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fw_devlink_wq =3D alloc_workq=
ueue("fw_devlink_wq", 0, 0);
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!fw_devlink_wq)
> > >=20
> > > Fix the name appropriately here too please.
> >=20
> > Hi Saravana,
> >=20
> > Oh, was not aware of this series... Please look at my first patch. It
> > already has a
> > review tag by Rafael. I think the creation of the queue makes more sens=
e to
> > be done
> > in devlink_class_init(). Moreover, Rafael complained in my first versio=
n
> > that
> > erroring out because we failed to create the queue is too harsh since
> > devlinks can
> > still work.
>=20
> I think Rafael can be convinced on this one. Firstly, if we fail to
> allocate so early, we have bigger problems.

That's true...

>=20
> > So, what we do is to schedule the work if we have a queue or too call
> > device_link_release_fn() synchronously if we don't have the queue (note=
 that
> > failing
> > to allocate the queue is very unlikely anyways).
>=20
> device links don't really work when you synchronously need to delete a
> link since it always uses SRCUs (it used to have a #ifndef CONFIG_SRCU
> locking). That's like saying a code still works when it doesn't hit a

Hmm, can you elaborate please? Why wouldn't it work if we call it synchrono=
usly?
Sure, we'll have the synchronize_srcu() call which might take some time but=
 I'm
not honestly seeing what could go wrong other than waiting?

I can also see that we can potentially hold the devlink lock for some time =
but
can that lead to any deadlock (It would actually be nice - if doable at all=
 - to
not release the refcounts with a lock hold)?
> deadlock condition.
>=20
> Let's stick with Herve's patch series since he send it first and it
> has fewer things that need to be fixed. If he ignores this thread for

Not exactly true :). If you look at my reply in the other thread (my series=
)
you'll see that I actually sent it first (as RFC - and spotted the issue wa=
y
back in May last year). About the stuff to fix, not sure if it's more. For =
now,
your major complain seems to be about synchronously calling=20
device_link_release_fn() and I did not had it in my v1. But anyways, I just=
 want
a fix for this to land as quick as possible :)=C2=A0

And I guess we also need Rafael to agree in erroring if we fail to allocate=
 the
queue as he was against it.

- Nuno S=C3=A1