Received-SPF: pass (google.com: domain of linux-kernel+bounces-83904-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20231130174126.688486-1-herve.codina@bootlin.com>
 <20231130174126.688486-3-herve.codina@bootlin.com> <CAGETcx_zB95nyTpi-_kYW_VqnPqMEc8mS9sewSwRNVr0x=7+kA@mail.gmail.com>
 <20240227162422.76a00f11@bootlin.com> <86f2262d059db84070745e299d96dde3e6078220.camel@gmail.com>
 <20240227185402.57a3b924@bootlin.com> <9ae28df7a4770bf94358dac36fd5e0942877f147.camel@gmail.com>
In-Reply-To: <9ae28df7a4770bf94358dac36fd5e0942877f147.camel@gmail.com>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Tue, 27 Feb 2024 20:13:23 +0100
Message-ID: <CAJZ5v0jE8QfSrs1uqgU7RceGWbT12YymouAOhsb7WxKmMbGeGg@mail.gmail.com>
Subject: Re: [PATCH 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>, Saravana Kannan <saravanak@google.com>, 
	Luca Ceresoli <luca.ceresoli@bootlin.com>, Nuno Sa <nuno.sa@analog.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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Feb 27, 2024 at 8:08=E2=80=AFPM Nuno S=C3=A1 <noname.nuno@gmail.com=
> wrote:
>
> Hi Herve,
>
> On Tue, 2024-02-27 at 18:54 +0100, Herve Codina wrote:
> > Hi Nuno,
> >
> > On Tue, 27 Feb 2024 17:55:07 +0100
> > Nuno S=C3=A1 <noname.nuno@gmail.com> wrote:
> >
> > > On Tue, 2024-02-27 at 16:24 +0100, Herve Codina wrote:
> > > > Hi Saravana, Luca, Nuno,
> > > >
> > > > On Tue, 20 Feb 2024 16:37:05 -0800
> > > > Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > ...
> > > >
> > > > > >
> > > > > > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > > > > > index a9a292d6d59b..5c5f808b163e 100644
> > > > > > --- a/drivers/of/overlay.c
> > > > > > +++ b/drivers/of/overlay.c
> > > > > > @@ -1202,6 +1202,12 @@ int of_overlay_remove(int *ovcs_id)
> > > > > >                 goto out;
> > > > > >         }
> > > > > >
> > > > > > +       /*
> > > > > > +        * Wait for any ongoing device link removals before rem=
oving some
> > > > > > of
> > > > > > +        * nodes
> > > > > > +        */
> > > > > > +       device_link_wait_removal();
> > > > > > +
> > > > >
> > > > > Nuno in his patch[1] had this "wait" happen inside
> > > > > __of_changeset_entry_destroy(). Which seems to be necessary to no=
t hit
> > > > > the issue that Luca reported[2] in this patch series. Is there an=
y
> > > > > problem with doing that?
> > > > >
> > > > > Luca for some reason did a unlock/lock(of_mutex) in his test patc=
h and
> > > > > I don't think that's necessary.
> > > >
> > > > I think the unlock/lock in Luca's case and so in Nuno's case is nee=
ded.
> > > >
> > > > I do the device_link_wait_removal() wihout having the of_mutex lock=
ed.
> > > >
> > > > Now, suppose I do the device_link_wait_removal() call with the of_m=
utex locked.
> > > > The following flow is allowed and a deadlock is present.
> > > >
> > > > of_overlay_remove()
> > > >   lock(of_mutex)
> > > >      device_link_wait_removal()
> > > >
> > > > And, from the workqueue jobs execution:
> > > >   ...
> > > >     device_put()
> > > >       some_driver->remove()
> > > >         of_overlay_remove() <--- The job will never end.
> > > >                                  It is waiting for of_mutex.
> > > >                                  Deadlock
> > > >
> > >
> > > We may need some input from Saravana (and others) on this. I might be=
 missing
> > > something but can a put_device() lead into a driver remove callback? =
Driver code
> > > is
> > > not device code and put_device() leads to device_release() which will=
 either call
> > > the
> > > device ->release(), ->type->release() or the class ->dev_release(). A=
nd, IMO,
> > > calling
> > > of_overlay_remove() or something like that (like something that would=
 lead to
> > > unbinding a device from it's driver) in a device release callback wou=
ld be at the
> > > very least very questionable. Typically, what you see in there is of_=
node_put()
> > > and
> > > things like kfree() of the device itself or any other data.
> >
> > I think that calling of_overlay_remove() in a device release callback m=
akes
> > sense. The overlay is used to declare sub-nodes from the device node. I=
t
> > does not add/remove the device node itself but sub-nodes.
> >
>
> I think we are speaking about two different things... device release is n=
ot the same
> as the driver remove callback. I admit the pci case seems to be a beast o=
f it's own
> and I just spent some time (given your links) on it so I can't surely be =
sure about
> what I'm about to say... But, AFAICT, I did not saw any overlay or change=
set being
> removed from a kobj_type release callback.
>
> > The use case is the use of DT overlays to describe PCI devices.
> > https://lore.kernel.org/all/1692120000-46900-1-git-send-email-lizhi.hou=
@amd.com/
> > https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@boot=
lin.com/
> > --- 8< ---
> > The lan966x SoCs can be used in two different ways:
> >
> >  - It can run Linux by itself, on ARM64 cores included in the SoC. This
> >    use-case of the lan966x is currently being upstreamed, using a
> >    traditional Device Tree representation of the lan996x HW blocks [1]
> >    A number of drivers for the different IPs of the SoC have already
> >    been merged in upstream Linux.
> >
> >  - It can be used as a PCIe endpoint, connected to a separate platform
> >    that acts as the PCIe root complex. In this case, all the devices
> >    that are embedded on this SoC are exposed through PCIe BARs and the
> >    ARM64 cores of the SoC are not used. Since this is a PCIe card, it
> >    can be plugged on any platform, of any architecture supporting PCIe.
> > --- 8< ---
> >
> > This quite long story led to DT overlay support for PCI devices and so =
the
> > unittest I mentioned:
> >   https://elixir.bootlin.com/linux/v6.8-rc6/source/drivers/of/unittest.=
c#L3946
> >
> >
> > So, I have a PCI driver that bind to the lan966x PCI board.
> > This driver loads an overlay at probe() and unload it at remove().
> > Also, this driver can be module. A simple rmmod leads to the remove() c=
all.
> >
>
> Hmm, and I think that would not be an issue... Note that the code that ru=
ns in
> device_link_release_fn() is doing put_device() which ends ups on the kobj=
_type
> release callback and so far I could not see any evidence of such a callba=
ck being
> responsible of calling device_remove() on another device. That would be w=
eird (I
> think) since I would expect such a call to happen in a kind of unregister=
 function.
>
> > This driver is not yet upstream because I haven't yet fixed all the iss=
ues I
> > encountered that's why of now, I can point only the unittest related to=
 overlay
> > support for PCI.
> >
> > >
> > > The driver remove callback should be called when unbinding the device=
 from it's
> > > drivers and devlinks should also be removed after device_unbind_clean=
up() (i.e,
> > > after
> > > the driver remove callback).
> > >
> > > Having said the above, the driver core has lots of subtleties so, aga=
in, I can be
> > > missing something. But at this point I'm still not seeing any deadloc=
k...
> > >
> >
> > I gave a wrong example.
> > Based on Luca's sequence he gave in
> >   https://lore.kernel.org/all/20231220181627.341e8789@booty/
>
> Regarding Luca's comments, my first approach was actually to just make th=
e devlink
> removal synchronously... I'm still not sure what would be the issue of do=
ing that
> (other than potentially waiting some time for the srcu synchronization).

It would allow forward progress to be made, but it would add potential
delay for everybody, which is only really needed in the DT overlay
case.  Sounds like something to avoid to me.

> About the unlock, I'm just not sure what could happen if someone else (ot=
her than us) sneaks in
> and grabs the of_mutex while we are in the middle of removing an overlay.=
.

If that is possible at all, I'd call it a bug.