Received-SPF: pass (google.com: domain of linux-kernel+bounces-93258-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::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>
 <CAGETcx-tmyJA30GtdU_dO9tWFoK+rO5tm-On4tPR7oQotnMkqQ@mail.gmail.com>
 <2f497783da939f13d8c8faeab931cac0ef9c98eb.camel@gmail.com>
 <20240305112708.56869e4c@bootlin.com> <c2be9a4cbbe6946e2f98a69d915c22bc52c9f1d9.camel@gmail.com>
In-Reply-To: <c2be9a4cbbe6946e2f98a69d915c22bc52c9f1d9.camel@gmail.com>
From: Saravana Kannan <saravanak@google.com>
Date: Tue, 5 Mar 2024 18:59:47 -0800
Message-ID: <CAGETcx9W81JNeqKFJvG_ydGMdBML9SvNMsmoxnCuyKgf2vN89w@mail.gmail.com>
Subject: Re: [PATCH v3 2/2] of: overlay: Synchronize of_overlay_remove() with
 the devlink removals
To: =?UTF-8?B?TnVubyBTw6E=?= <noname.nuno@gmail.com>
Cc: Herve Codina <herve.codina@bootlin.com>, 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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Mar 5, 2024 at 2:43=E2=80=AFAM Nuno S=C3=A1 <noname.nuno@gmail.com>=
 wrote:
>
> On Tue, 2024-03-05 at 11:27 +0100, Herve Codina wrote:
> > Hi Nuno, Saravana, Rob,
> >
> > On Tue, 05 Mar 2024 08:36:45 +0100
> > Nuno S=C3=A1 <noname.nuno@gmail.com> wrote:
> >
> > > 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@b=
ootlin.com>
> > > > 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(str=
uct
> > > > > > > > overlay_changeset *ovcs)
> > > > > > > >  {
> > > > > > > >   int i;
> > > > > > > >
> > > > > > > > + /*
> > > > > > > > +  * Wait for any ongoing device link removals before remov=
ing
> > > > > > > > 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 happen=
s if
> > > > > > > someone else
> > > > > > > grabs the lock while we are in device_link_wait_removal()? Ca=
n we
> > > > > > > guarantee 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 relea=
sing it
> > > > > > behind their back.
> > > > > >
> > > > > > As device_link_wait_removal() is called before we touch anythin=
g,
> > > > > > 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/
> > > >
> > >
> > > 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 cont=
radict
> > > myself a bit because this is just theory but if we look at pci_stop_d=
ev(),
> > > 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 q=
ueue in
> > > free_overlay_changeset() - as the queue won't be flushed at all and w=
e could
> > > 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?
> >
> > Instead of having the wait called in __of_changeset_entry_destroy() and=
 so
> > called in a loop. I could move this call in the __of_changeset_entry_de=
stroy()
> > caller (without any of_mutex lock drop).
> >
>
> Oh, good catch! At this point all the devlinks removals (related to the
> changeset) should have been queued so yes, we should only need to flush o=
nce.
>
> > So this will look like this:
> > --- 8< ---
> > void of_changeset_destroy(struct of_changeset *ocs)
> > {
> >       struct of_changeset_entry *ce, *cen;
> >
> >       device_link_wait_removal();
> >
> >       list_for_each_entry_safe_reverse(ce, cen, &ocs->entries, node)
> >               __of_changeset_entry_destroy(ce);
> > }
> > --- 8< ---
> >
> > I already tested on my system and it works correctly with
> > device_link_wait_removal() only called from of_changeset_destroy()
> > as proposed.
> >
> > Saravana, Nuno, Rob does it seems ok for you ?

Looks good to me.

-Saravana

> >
>
> It looks good to me...
>
> - Nuno S=C3=A1
> >
>