Received-SPF: pass (google.com: domain of linux-kernel+bounces-46733-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <9a4b4dd60c35dd4e00b97e19fced0e79a86f6eef.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 16:49:35 +0100
In-Reply-To: <CAJZ5v0hW60++hhKG=yBE7+rB9qt2m+tO_WAf=YFhNU2-y3VHFw@mail.gmail.com>
References: 
	<20240123-fix-device-links-overlays-v1-1-9e4f6acaab6c@analog.com>
	 <dcb1b6dbc2172dd66bfdcc0c8135e0d98f1c22dd.camel@gmail.com>
	 <CAJZ5v0gAK9CChRPSx7Lu=BrGQo22q4swpvvN3__wFw68NfqKPA@mail.gmail.com>
	 <25d3cfd74b26eb6a4aa07f1da93ccf4815b0b1c6.camel@gmail.com>
	 <CAJZ5v0hX4Yv7UVng=O4tZyb_O7D2EcymdEDdSUrVDPk6h51VjA@mail.gmail.com>
	 <8682d7f7ee1a60902b1f3e5529a4adbaf4846aa0.camel@gmail.com>
	 <CAJZ5v0hW60++hhKG=yBE7+rB9qt2m+tO_WAf=YFhNU2-y3VHFw@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 16:10 +0100, Rafael J. Wysocki wrote:
> On Wed, Jan 31, 2024 at 3:52=E2=80=AFPM Nuno S=C3=A1 <noname.nuno@gmail.c=
om> wrote:
> >=20
> > On Wed, 2024-01-31 at 15:28 +0100, Rafael J. Wysocki wrote:
> > > On Wed, Jan 31, 2024 at 3:18=E2=80=AFPM Nuno S=C3=A1 <noname.nuno@gma=
il.com> wrote:
> > > >=20
> > > > 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.com> 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, th=
e
> > > > > > > possible
> > > > > > > release of an of_node is also asynchronous. If these nodes we=
re
> > > > > > > added
> > > > > > > through overlays we have a problem because this does not resp=
ect
> > > > > > > the
> > > > > > > devicetree overlays assumptions that when a changeset is
> > > > > > > being removed in __of_changeset_entry_destroy(), it must hold=
 the
> > > > > > > last
> > > > > > > reference to that node. Due to the async nature of device lin=
ks
> > > > > > > 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() ins=
tead
> > > > > > > of
> > > > > > > queueing it. Yes, it might take some significant time for
> > > > > > > device_link_release_fn() to complete because of synchronize_s=
rcu()
> > > > > > > but
> > > > > > > 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
> > > > > > > devicetree
> > > > > > > folks [1]. It got rejected because it was not really fixing t=
he
> > > > > > > root
> > > > > > > cause of the issue (which I do agree). Please see the link wh=
ere 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") intro=
duced
> > > > > > > queue_work() as we could be releasing the last device referen=
ce
> > > > > > > and
> > > > > > > hence
> > > > > > > sleeping which is against SRCU callback requirements. However=
,
> > > > > > > that
> > > > > > > same
> > > > > > > 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 a=
ny
> > > > > > > reason to not be able to run device_link_release_fn()
> > > > > > > synchronously if
> > > > > > > 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 synchronizati=
on
> > > > > > > point
> > > > > > > somewhere.
> > > > > > >=20
> > > > > > > Anyways, I would like to have some feedback on how acceptable
> > > > > > > would
> > > > > > > this
> > > > > > > be or what else could I do so we can have a "clean" dt overla=
y
> > > > > > > removal.
> > > > > > >=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
> > > > > > > when
> > > > > > > an
> > > > > > > overlay is being removed (maybe we could even have an explici=
t
> > > > > > > flag
> > > > > > > for
> > > > > > > it?) and only directly call device_link_release_fn() in that =
case.
> > > > > > >=20
> > > > > > > [1]:
> > > > > > > https://lore.kernel.org/linux-devicetree/20230511151047.17798=
41-1-nuno.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_nod=
e, OF_DETACHED))
> > > > > > > +=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_nod=
e, OF_OVERLAY))
> > > > > > > +=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 c=
ontainer_of(work, struct
> > > > > > > device_link,
> > > > > > > rm_work);
> > > > > > > @@ -532,8 +544,19 @@ static void devlink_dev_release(struct d=
evice
> > > > > > > *dev)
> > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * synchronization in dev=
ice_link_release_fn() and if the
> > > > > > > consumer
> > > > > > > or
> > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * supplier devices get d=
eleted when 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 supp=
lier, consumer nodes is being
> > > > > > > removed
> > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * through overlay removal, th=
e expectation in
> > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * __of_changeset_entry_destro=
y() is for the node 'kref' to
> > > > > > > be 1
> > > > > > > which
> > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * cannot be guaranteed with t=
he async nature of
> > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * device_link_release_fn(). H=
ence, 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->r=
m_work);
> > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0 if (device_node_overlay_removal(lin=
k->consumer) ||
> > > > > > > +=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
> > > > > > > ---
>=20
> [cut]
>=20
> > > No, IMV devlink_dev_release() needs to be called via
> > > device_link_put_kref(), but it may run device_link_release_fn()
> > > directly if the link is marked in a special way or something like
> > > this.
> >=20
> > Sorry, I'm not totally getting this. I'm directly calling
> > device_link_release_fn() from=C2=A0 devlink_dev_release(). We should on=
ly get
> > into
> > devlink_dev_release() after all the references are dropped right (being=
 it
> > the
> > release callback for the link class)?
>=20
> OK, I got confused somehow, sorry.
>=20
> It should work.
>=20
> I kind of don't like adding OF-specific code to the driver core, but
> if this is fine with Greg, it can be done.=C2=A0 It should depend on

Not perfect but I'm not seeing any other way. We need to somehow see if the=
 node
is part of an OVERLAY and AFAIK, the only way is looking at the node flags.=
 I'll
wait on Greg's feedback.

> CONFIG_OF_OVERLAY, though.

I guess that should be already indirectly implied. I mean if CONFIG_OF_OVER=
LAY
is not set, I guess there's not way for
of_node_check_flag(dev->of_node, OF_OVERLAY)) return true. But yeah, I can =
bail
out right away if IS_ENABLED(CONFIG_OF_OVERLAY) is not set.

> I would like a comment to be added to device_link_release_fn() to
> explain why the overlay case needs synchronous execution in there.

I do have the following comment before checking device_node_overlay_removal=
():


"* However, if any of the supplier, consumer nodes is being removed
 * through overlay removal, the expectation in
 * __of_changeset_entry_destroy() is for the node 'kref' to be 1 which
 * cannot be guaranteed with the async nature of
 * device_link_release_fn(). Hence, do it synchronously for the overlay
 * case."

I can elaborate more if you prefer...

>=20
> > device_node_overlay_removal() is my way to see if the link "is marked i=
n a
> > special way" as you put it. This checks if one of the supplier/consumer=
 is
> > marked as an OVERLAY and if it's being removed (I think that OF_DETACHE=
D
> > tells
> > us that but some feedback from DT guys will be helpful).
> >=20
> > Alternatively, I could just check the OF_OVERLAY flag when the link is =
being
> > created and have a new variable in struct device_link to flag the
> > synchronous
> > release. Disadvantage is that in this way even a sysfs unbind or module
> > unload
> > (without necessarily removing the overly) would lead to a synchronous
> > release
> > which can actually make sense (now that I think about it). Because if
> > someone
> > does some crazy thing like "echo device > unbind" and then removes the
> > overlay
> > we could still hit the overly removal path before device_link_release_f=
n()
> > completed.
>=20
> This sounds more complicated than the current patch.

Agreed...

- Nuno S=C3=A1