Received-SPF: pass (google.com: domain of linux-kernel+bounces-46411-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <dcb1b6dbc2172dd66bfdcc0c8135e0d98f1c22dd.camel@gmail.com>
Subject: Re: [PATCH RESEND RFC] driver: core: don't queue device links
 removal for dt overlays
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: nuno.sa@analog.com, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
	"Rafael J. Wysocki"
	 <rafael@kernel.org>, Frank Rowand <frowand.list@gmail.com>, Rob Herring
	 <robh+dt@kernel.org>
Cc: linux-kernel@vger.kernel.org, Saravana Kannan <saravanak@google.com>
Date: Wed, 31 Jan 2024 13:24:07 +0100
In-Reply-To: <20240123-fix-device-links-overlays-v1-1-9e4f6acaab6c@analog.com>
References: 
	<20240123-fix-device-links-overlays-v1-1-9e4f6acaab6c@analog.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.3 
Precedence: bulk
MIME-Version: 1.0

On Tue, 2024-01-23 at 16:40 +0100, Nuno Sa via B4 Relay wrote:
> From: Nuno Sa <nuno.sa@analog.com>
>=20
> For device links, releasing the supplier/consumer devices references
> happens asynchronously in device_link_release_fn(). Hence, the possible
> release of an of_node is also asynchronous. If these nodes were added
> through overlays we have a problem because this does not respect the
> devicetree overlays assumptions that when a changeset is
> being removed in __of_changeset_entry_destroy(), it must hold the last
> reference to that node. Due to the async nature of device links that
> cannot be guaranteed.
>=20
> Given the above, in case one of the link consumer/supplier is part of
> an overlay node we call directly device_link_release_fn() instead of
> queueing it. Yes, it might take some significant time for
> device_link_release_fn() to complete because of synchronize_srcu() but
> we would need to, anyways, wait for all OF references to be released if
> we want to respect overlays assumptions.
>=20
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
> This RFC is a follow up of a previous one that I sent to the devicetree
> folks [1]. It got rejected because it was not really fixing the root
> cause of the issue (which I do agree). Please see the link where I
> fully explain what the issue is.
>=20
> I did also some git blaming and did saw that commit
> 80dd33cf72d1 ("drivers: base: Fix device link removal") introduced
> queue_work() as we could be releasing the last device reference and hence
> sleeping which is against SRCU callback requirements. However, that same
> commit is now making use of synchronize_srcu() which may take
> significant time (and I think that's the reason for the work item?).
>=20
> However, given the dt overlays requirements, I'm not seeing any
> reason to not be able to run device_link_release_fn() synchronously if we
> detect an OVERLAY node is being released. I mean, even if we come up
> (and I did some experiments in this regard) with some async mechanism to
> release the OF nodes refcounts, we still need a synchronization point
> somewhere.
>=20
> Anyways, I would like to have some feedback on how acceptable would this
> be or what else could I do so we can have a "clean" dt overlay removal.
>=20
> I'm also including dt folks so they can give some comments on the new
> device_node_overlay_removal() function. My goal is to try to detect when =
an
> overlay is being removed (maybe we could even have an explicit flag for
> it?) and only directly call device_link_release_fn() in that case.
>=20
> [1]:
> https://lore.kernel.org/linux-devicetree/20230511151047.1779841-1-nuno.sa=
@analog.com/
> ---
> =C2=A0drivers/base/core.c | 25 ++++++++++++++++++++++++-
> =C2=A01 file changed, 24 insertions(+), 1 deletion(-)
>=20
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 14d46af40f9a..31ea001f6142 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -497,6 +497,18 @@ static struct attribute *devlink_attrs[] =3D {
> =C2=A0};
> =C2=A0ATTRIBUTE_GROUPS(devlink);
> =C2=A0
> +static bool device_node_overlay_removal(struct device *dev)
> +{
> +	if (!dev_of_node(dev))
> +		return false;
> +	if (!of_node_check_flag(dev->of_node, OF_DETACHED))
> +		return false;
> +	if (!of_node_check_flag(dev->of_node, OF_OVERLAY))
> +		return false;
> +
> +	return true;
> +}
> +
> =C2=A0static void device_link_release_fn(struct work_struct *work)
> =C2=A0{
> =C2=A0	struct device_link *link =3D container_of(work, struct device_link=
,
> rm_work);
> @@ -532,8 +544,19 @@ static void devlink_dev_release(struct device *dev)
> =C2=A0	 * synchronization in device_link_release_fn() and if the consumer=
 or
> =C2=A0	 * supplier devices get deleted when it runs, so put it into the
> "long"
> =C2=A0	 * workqueue.
> +	 *
> +	 * However, if any of the supplier, consumer nodes is being removed
> +	 * through overlay removal, the expectation in
> +	 * __of_changeset_entry_destroy() is for the node 'kref' to be 1
> which
> +	 * cannot be guaranteed with the async nature of
> +	 * device_link_release_fn(). Hence, do it synchronously for the
> overlay
> +	 * case.
> =C2=A0	 */
> -	queue_work(system_long_wq, &link->rm_work);
> +	if (device_node_overlay_removal(link->consumer) ||
> +	=C2=A0=C2=A0=C2=A0 device_node_overlay_removal(link->supplier))
> +		device_link_release_fn(&link->rm_work);
> +	else
> +		queue_work(system_long_wq, &link->rm_work);
> =C2=A0}
> =C2=A0
> =C2=A0static struct class devlink_class =3D {
>=20
> ---
> base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
> change-id: 20240123-fix-device-links-overlays-5422e033a09b
> --
>=20
> Thanks!
> - Nuno S=C3=A1
>=20

Hi Rafael,

Would be nice to have your feedback on this one or if this is a complete na=
ck...
I think calling device_link_release_fn() synchronously is ok but I might be
completely wrong.

+Cc Saravan as he should also be very familiar with device_links and see if=
 the
above fairly simple solution is sane.

I also don't want to be pushy as I know you guys are all very busy but it's=
 (i
think) the third time I resend the patch :)

- Nuno S=C3=A1