Received-SPF: pass (google.com: domain of linux-kernel+bounces-91747-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
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>
In-Reply-To: <20240304174933.7ad023f9@bootlin.com>
From: Saravana Kannan <saravanak@google.com>
Date: Mon, 4 Mar 2024 22:47:09 -0800
Message-ID: <CAGETcx-tmyJA30GtdU_dO9tWFoK+rO5tm-On4tPR7oQotnMkqQ@mail.gmail.com>
Subject: Re: [PATCH v3 2/2] of: overlay: Synchronize of_overlay_remove() with
 the devlink removals
To: Herve Codina <herve.codina@bootlin.com>
Cc: Rob Herring <robh@kernel.org>, =?UTF-8?B?TnVubyBTw6E=?= <noname.nuno@gmail.com>, 
	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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Mar 4, 2024 at 8:49=E2=80=AFAM Herve Codina <herve.codina@bootlin.c=
om> wrote:
>
> Hi Rob,
>
> On Mon, 4 Mar 2024 09:22:02 -0600
> Rob Herring <robh@kernel.org> wrote:
>
> ...
>
> > > > @@ -853,6 +854,14 @@ static void free_overlay_changeset(struct
> > > > overlay_changeset *ovcs)
> > > >  {
> > > >   int i;
> > > >
> > > > + /*
> > > > +  * Wait for any ongoing device link removals before removing some=
 of
> > > > +  * nodes. Drop the global lock while waiting
> > > > +  */
> > > > + mutex_unlock(&of_mutex);
> > > > + device_link_wait_removal();
> > > > + mutex_lock(&of_mutex);
> > >
> > > I'm still not convinced we need to drop the lock. What happens if som=
eone else
> > > grabs the lock while we are in device_link_wait_removal()? Can we gua=
rantee that
> > > we can't screw things badly?
> >
> > It is also just ugly because it's the callers of
> > free_overlay_changeset() that hold the lock and now we're releasing it
> > behind their back.
> >
> > 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?

Rob,

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.

[1] - https://lore.kernel.org/all/20231220181627.341e8789@booty/

> >
>
> Indeed, having device_link_wait_removal() is not needed when applying the
> overlay fails.
>
> 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.
>
> This lead to the following sequence:
> --- 8< ---
> int of_overlay_remove(int *ovcs_id)
> {
>         ...
>         mutex_lock(&of_mutex);
>         ...
>
>         ret =3D __of_changeset_revert_notify(&ovcs->cset);
>         ...
>
>         ret_tmp =3D overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE);
>         ...
>
>         mutex_unlock(&of_mutex);
>         device_link_wait_removal();
>         mutex_lock(&of_mutex);
>
>         free_overlay_changeset(ovcs);
>         ...
>         mutex_unlock(&of_mutex);
>         ...
> }
> --- 8< ---
>
> In this sequence, the question is:
> Do we need to release the mutex lock while device_link_wait_removal() is
> called ?

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.

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.

Please CC me on the next series. And I'm glad folks convinced you to
use flush_workqueue(). As I said in the older series, I think
drain_workqueue() will actually break device links.

-Saravana


-Saravana