Received-SPF: pass (google.com: domain of linux-kernel+bounces-91816-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <2f497783da939f13d8c8faeab931cac0ef9c98eb.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: Saravana Kannan <saravanak@google.com>, Herve Codina
	 <herve.codina@bootlin.com>
Cc: Rob Herring <robh@kernel.org>, 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: Tue, 05 Mar 2024 08:36:45 +0100
In-Reply-To: <CAGETcx-tmyJA30GtdU_dO9tWFoK+rO5tm-On4tPR7oQotnMkqQ@mail.gmail.com>
References: <20240229105204.720717-1-herve.codina@bootlin.com>
	 <20240229105204.720717-3-herve.codina@bootlin.com>
	 <acb69aa8c1a4c4e9849123ef538b9646a71507a0.camel@gmail.com>
	 <20240304152202.GA222088-robh@kernel.org>
	 <20240304174933.7ad023f9@bootlin.com>
	 <CAGETcx-tmyJA30GtdU_dO9tWFoK+rO5tm-On4tPR7oQotnMkqQ@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 Mon, 2024-03-04 at 22:47 -0800, Saravana Kannan wrote:
> On Mon, Mar 4, 2024 at 8:49=E2=80=AFAM Herve Codina <herve.codina@bootlin=
com> wrote:
> >=20
> > Hi Rob,
> >=20
> > On Mon, 4 Mar 2024 09:22:02 -0600
> > Rob Herring <robh@kernel.org> wrote:
> >=20
> > ...
> >=20
> > > > > @@ -853,6 +854,14 @@ static void free_overlay_changeset(struct
> > > > > overlay_changeset *ovcs)
> > > > > =C2=A0{
> > > > > =C2=A0 int i;
> > > > >=20
> > > > > + /*
> > > > > +=C2=A0 * Wait for any ongoing device link removals before removi=
ng some of
> > > > > +=C2=A0 * nodes. Drop the global lock while waiting
> > > > > +=C2=A0 */
> > > > > + 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
> > > > someone else
> > > > grabs the lock while we are in device_link_wait_removal()? Can we
> > > > guarantee that
> > > > we can't screw things badly?
> > >=20
> > > It is also just ugly because it's the callers of
> > > free_overlay_changeset() that hold the lock and now we're releasing i=
t
> > > behind their back.
> > >=20
> > > As device_link_wait_removal() is called before we touch anything, can=
't
> > > it be called before we take the lock? And do we need to call it if
> > > applying the overlay fails?
>=20
> Rob,
>=20
> This[1] scenario Luca reported seems like a reason for the
> device_link_wait_removal() to be where Herve put it. That example
> seems reasonable.
>=20
> [1] - https://lore.kernel.org/all/20231220181627.341e8789@booty/
>=20

I'm still not totally convinced about that. Why not putting the check right
before checking the kref in __of_changeset_entry_destroy(). I'll contradict
myself a bit because this is just theory but if we look at pci_stop_dev(), =
which
AFAIU, could be reached from a sysfs write(), we have:

device_release_driver(&dev->dev);
..
of_pci_remove_node(dev);
	of_changeset_revert(np->data);
	of_changeset_destroy(np->data);

So looking at the above we would hit the same issue if we flush the queue i=
n
free_overlay_changeset() - as the queue won't be flushed at all and we coul=
d
have devlink removal due to device_release_driver(). Right?

Again, completely theoretical but seems like a reasonable one plus I'm not
understanding the push against having the flush in
__of_changeset_entry_destroy(). Conceptually, it looks the best place to me=
 but
I may be missing some issue in doing it there?

> > >=20
> >=20
> > Indeed, having device_link_wait_removal() is not needed when applying t=
he
> > overlay fails.
> >=20
> > I can call device_link_wait_removal() from the caller of_overlay_remove=
()
> > but not before the lock is taken.
> > We need to call it between __of_changeset_revert_notify() and
> > free_overlay_changeset() and so, the lock is taken.
> >=20
> > This lead to the following sequence:
> > --- 8< ---
> > 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 mutex_lock(&of_mutex);
> > =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 ret =3D __of_changeset_rever=
t_notify(&ovcs->cset);
> > =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 ret_tmp =3D overlay_notify(o=
vcs, OF_OVERLAY_POST_REMOVE);
> > =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 mutex_unlock(&of_mutex);
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 device_link_wait_removal();
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mutex_lock(&of_mutex);
> >=20
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 free_overlay_changeset(ovcs)=
;
> > =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 mutex_unlock(&of_mutex);
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ...
> > }
> > --- 8< ---
> >=20
> > In this sequence, the question is:
> > Do we need to release the mutex lock while device_link_wait_removal() i=
s
> > called ?
>=20
> In general I hate these kinds of sequences that release a lock and
> then grab it again quickly. It's not always a bug, but my personal
> take on that is 90% of these introduce a bug.
>=20
> Drop the unlock/lock and we'll deal a deadlock if we actually hit one.
> I'm also fairly certain that device_link_wait_removal() can't trigger
> something else that can cause an OF overlay change while we are in the
> middle of one. And like Rob said, I'm not sure this unlock/lock is a
> good solution for that anyway.

Totally agree. Unless we really see a deadlock this is a very bad idea (IMH=
O).
Even on the PCI code, it seems to me that we're never destroying a changese=
t
from a device/kobj_type release callback. That would be super weird right?

- Nuno S=C3=A1
>=20