Received-SPF: pass (google.com: domain of linux-kernel+bounces-78170-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: <cdf0a9facd95a2b7ee618e6130dedb9aabf4ed09.camel@gmail.com>
Subject: Re: [PATCH 1/2] driver core: Introduce device_link_wait_removal()
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: Herve Codina <herve.codina@bootlin.com>, Saravana Kannan
	 <saravanak@google.com>
Cc: 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>
Date: Fri, 23 Feb 2024 11:45:02 +0100
In-Reply-To: <20240223101115.6bf7d570@bootlin.com>
References: <20231130174126.688486-1-herve.codina@bootlin.com>
	 <20231130174126.688486-2-herve.codina@bootlin.com>
	 <CAGETcx9uP86EHyKJNifBMd23oCsA+KpMa+e36wJEEnHDve+Avg@mail.gmail.com>
	 <20240223101115.6bf7d570@bootlin.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 Fri, 2024-02-23 at 10:11 +0100, Herve Codina wrote:
> Hi Saravana,
>=20
> On Tue, 20 Feb 2024 16:31:13 -0800
> Saravana Kannan <saravanak@google.com> wrote:
>=20
> ...
>=20
> > > +void device_link_wait_removal(void)
> > > +{
> > > +=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 * devlink removal jobs ar=
e queued in the dedicated work queue.
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * To be sure that all rem=
oval jobs are terminated, ensure that
> > > any
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * scheduled work has run =
to completion.
> > > +=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 drain_workqueue(fw_devlink_wq);=
=C2=A0=20
> >=20
> > Is there a reason this needs to be drain_workqueu() instead of
> > flush_workqueue(). Drain is a stronger guarantee than we need in this
> > case. All we are trying to make sure is that all the device link
> > remove work queued so far have completed.
>=20
> I used drain_workqueue() because drain_workqueue() allows for jobs alread=
y
> present in a workqueue to re-queue a job and drain_workqueue() will wait
> also for this new job completion.
>=20
> I think flush_workqueue() doesn't wait for this chain queueing.
>=20
> In our case, my understanding was that device_link_release_fn() calls
> put_device() for the consumer and the supplier.
> If refcounts reaches zero, devlink_dev_release() can be called again
> and re-queue a job.
>=20

Looks sensible. The only doubt (that Saravana mays know better) is that I'm=
 not
sure put_device() on a supplier or consumer can actually lead to
devlink_dev_release(). AFAIU, a consumer or a supplier should not be a devi=
ce
from the devlink class. Hence, looking at device_release(), I'm not sure it=
 can
happen unless for some odd reason someone is messing with devlinks in .remo=
ve()
or .type->remove().

- Nuno S=C3=A1