netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mihai Moldovan <ionic@ionic.de>
To: ath11k@lists.infradead.org, ath12k@lists.infradead.org,
	Kalle Valo <kvalo@kernel.org>, Jeff Johnson <jjohnson@kernel.org>,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konradybcio@kernel.org>,
	"David S . Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Simon Horman <horms@kernel.org>,
	linux-wireless@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	netdev@vger.kernel.org
Subject: [RFC] [PATCH v2 01/13] net: qrtr: support registering endpoint-specific data
Date: Mon, 25 Nov 2024 04:50:16 +0100	[thread overview]
Message-ID: <064e351e175e809e9a774022fa2f104b67862d9e.1732506261.git.ionic@ionic.de> (raw)
In-Reply-To: <cover.1732506261.git.ionic@ionic.de>

This adds the infrastructure for registering endpoint-specific data for
endpoint IDs.

Endpoint-specific data can be used to map an endpoint ID to a specific
endpoint backend.

Additional API is introduced as a common header in include/net to allow
other parts of the kernel to query endpoint IDs for endpoint-specific
data or make the QRTR subsystem assign a new endpoint ID for passed
endpoint-specific data. This will allow other systems to register
endpoint IDs before actual socket creation and usage, which in turn
allows proper binding to endpoint IDs from the start.

The endpoint registration function is changed to re-use endpoint IDs
that match the backend's endpoint-specific data if possible, assign a
new endpoint ID if the endpoint-specific data is not known yet, or
create a new endpoint ID without endpoint-specific data attached to it
if all else fails.

There is one gripe with this implementation: other kernel subsystems
can, theoretically, assign an unlimited number of new endpoint IDs and
thus exhaust endpoint ID space. No API is provided to delete endpoint
IDs and we also do not track which endpoint IDs are in use and which are
not, which means that even the QRTR-internal code cannot easily clean up
unused endpoint IDs. The only exception to this are endpoint IDs
attached to QRTR nodes, which will be deleted when the QRTR nodes
themselves are deleted.

This is probably a potential memory leak that we can live with.

Fixing that is rather difficult. We would either have to add some form
of refcounting, wrap the endpoint-specific data pointer into yet another
structure together with a kref and use that to free unused endpoint IDs,
or periodically clean unused endpoint IDs up in a timer (executing, say,
every 10 minutes), essentially doing garbage collection.

Garbage collection is being frowned upon, especially in the kernel, but
in this case, it really would make the most sense. Clients might
allocate endpoint IDs that are never actually used (for instance because
the client uses a wrong endpoint-specific data pointer, which is not
used by a QRTR backend), and will need to hold a reference to this for
their entire life time, which essentially defeats the concept of
cleanup of unused endpoint IDs via reference counting.

Clients can create endpoint IDs quite some time before the QRTR
subsystem uses them, but there is no way to easily tell when this will
be. The idea is that minutes as orders of magnitude would probably be a
safe value for which to regard an endpoint ID as unused.

Signed-off-by: Mihai Moldovan <ionic@ionic.de>
Depends-on: 25a7151cdc98 ("net: qrtr: ns: support multiple endpoints")
Link: https://patch.msgid.link/20241018181842.1368394-1-denkenz@gmail.com
---
 MAINTAINERS        |   1 +
 include/net/qrtr.h |  11 ++++
 net/qrtr/af_qrtr.c | 124 +++++++++++++++++++++++++++++++++++++++++++--
 net/qrtr/qrtr.h    |   5 ++
 4 files changed, 138 insertions(+), 3 deletions(-)
 create mode 100644 include/net/qrtr.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 96b9344c3524..6993067c4194 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19115,6 +19115,7 @@ QUALCOMM IPC ROUTER (QRTR) DRIVER
 M:	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
 L:	linux-arm-msm@vger.kernel.org
 S:	Maintained
+F:	include/net/qrtr.h
 F:	include/trace/events/qrtr.h
 F:	include/uapi/linux/qrtr.h
 F:	net/qrtr/
diff --git a/include/net/qrtr.h b/include/net/qrtr.h
new file mode 100644
index 000000000000..799c84eb35ad
--- /dev/null
+++ b/include/net/qrtr.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef __NET_QRTR_H
+#define __NET_QRTR_H
+
+#include <linux/types.h>
+
+int qrtr_endpoint_id_get(const void *data, u32 *id);
+int qrtr_endpoint_id_assign(void *data, u32 *id);
+int qrtr_endpoint_id_get_or_assign(void *data, u32 *id);
+
+#endif	/* __NET_QRTR_H */
diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
index cf8b5483ba2c..59227b3d49f4 100644
--- a/net/qrtr/af_qrtr.c
+++ b/net/qrtr/af_qrtr.c
@@ -11,6 +11,7 @@
 #include <linux/wait.h>
 
 #include <net/sock.h>
+#include <net/qrtr.h>
 
 #include "qrtr.h"
 
@@ -649,6 +650,114 @@ static struct sk_buff *qrtr_alloc_ctrl_packet(struct qrtr_ctrl_pkt **pkt,
 	return skb;
 }
 
+/**
+ * qrtr_endpoint_id_get() - get a registered endpoint for given data
+ * @data: endpoint-specific data to fetch ID for
+ * @id: pointer to store endpoint ID into
+ * Return: 0 on success, negative error code on failure
+ *
+ * The endpoint-specific data must not be NULL.
+ * The output parameter id must not be NULL.
+ * If no endpoint ID can be mapped to the endpoint-specific data, id will be
+ * set to 0.
+ */
+int qrtr_endpoint_id_get(const void *data, u32 *id)
+{
+	unsigned long idx = 0;
+	void *iter_data = NULL;
+
+	if (!id)
+		return -EINVAL;
+
+	if (!data)
+		return -EINVAL;
+
+	*id = 0;
+	rcu_read_lock();
+	xa_for_each_range(&qrtr_endpoints, idx, iter_data,
+			  QRTR_ENDPOINT_RANGE.min, QRTR_ENDPOINT_RANGE.max) {
+		if (iter_data == data) {
+			*id = idx;
+			break;
+		}
+	}
+	rcu_read_unlock();
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(qrtr_endpoint_id_get);
+
+/**
+ * qrtr_endpoint_id_assign() - assigns a new endpoint ID for given data
+ * @data: endpoint-specific data to assign new ID for
+ * @id: pointer to store endpoint ID into
+ * Return: 0 on success, negative error code on failure
+ *
+ * The endpoint-specific data must not be NULL.
+ * The output parameter id must not be NULL.
+ * On error, id will be set to 0.
+ */
+int qrtr_endpoint_id_assign(void *data, u32 *id)
+{
+	int rc = 0;
+
+	if (!id)
+		return -EINVAL;
+
+	if (!data)
+		return -EINVAL;
+
+	rc = xa_alloc_cyclic(&qrtr_endpoints, id, data, QRTR_ENDPOINT_RANGE,
+			     &next_endpoint_id, GFP_KERNEL);
+	if (rc)
+		*id = 0;
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(qrtr_endpoint_id_assign);
+
+/**
+ * qrtr_endpoint_id_get_or_assign() - gets or assigns endpoint ID for data
+ * @data: endpoint-specific data to assign new ID for
+ * @id: pointer to store endpoint ID into
+ * Return: positive on success, negative error code on failure
+ *
+ * The endpoint-specific data must not be NULL.
+ *
+ * If the endpoint-specific data is already registered to an endpoint ID, this
+ * ID will be assigned to id. Otherwise, this function assigns a new
+ * endpoint ID and associates it with the given endpoint-specific data.
+ *
+ * The output parameter id must not be NULL. It will either be set to the
+ * fetched or newly assigned endpoint ID on success, or set to 0 on error.
+ */
+int qrtr_endpoint_id_get_or_assign(void *data, u32 *id)
+{
+	int rc = 0;
+
+	if (!data)
+		return -EINVAL;
+
+	if (!id)
+		return -EINVAL;
+
+	rc = qrtr_endpoint_id_get(data, id);
+
+	if (rc) {
+		*id = 0;
+		return rc;
+	}
+
+	if (!*id)
+		rc = qrtr_endpoint_id_assign(data, id);
+
+	if (rc)
+		*id = 0;
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(qrtr_endpoint_id_get_or_assign);
+
 /**
  * qrtr_endpoint_register() - register a new endpoint
  * @ep: endpoint to register
@@ -670,9 +779,18 @@ int qrtr_endpoint_register(struct qrtr_endpoint *ep, unsigned int nid)
 	if (!node)
 		return -ENOMEM;
 
-	rc = xa_alloc_cyclic(&qrtr_endpoints, &endpoint_id, NULL,
-			     QRTR_ENDPOINT_RANGE, &next_endpoint_id,
-			     GFP_KERNEL);
+	rc = qrtr_endpoint_id_get_or_assign(ep->endpoint_data, &endpoint_id);
+
+	/*
+	 * The previous function fails if ep->endpoint_data is NULL, so retry.
+	 *
+	 * We're going to assign an endpoint ID without endpoint-specific data
+	 * set in this case.
+	 */
+	if (rc)
+		rc = xa_alloc_cyclic(&qrtr_endpoints, &endpoint_id, NULL,
+				     QRTR_ENDPOINT_RANGE, &next_endpoint_id,
+				     GFP_KERNEL);
 
 	if (rc < 0)
 		goto free_node;
diff --git a/net/qrtr/qrtr.h b/net/qrtr/qrtr.h
index b4f50336ae75..3509168e8a40 100644
--- a/net/qrtr/qrtr.h
+++ b/net/qrtr/qrtr.h
@@ -12,13 +12,18 @@ struct sk_buff;
 /**
  * struct qrtr_endpoint - endpoint handle
  * @xmit: Callback for outgoing packets
+ * @endpoint_data: endpoint-specific data pointer, can be NULL
  *
  * The socket buffer passed to the xmit function becomes owned by the endpoint
  * driver.  As such, when the driver is done with the buffer, it should
  * call kfree_skb() on failure, or consume_skb() on success.
+ *
+ * If endpoint_data is NULL, endpoint IDs can not be directly mapped to a
+ * specific endpoint.
  */
 struct qrtr_endpoint {
 	int (*xmit)(struct qrtr_endpoint *ep, struct sk_buff *skb);
+	void *endpoint_data;
 	/* private: not for endpoint use */
 	struct qrtr_node *node;
 	u32 id;
-- 
2.45.2


  reply	other threads:[~2024-11-25  4:00 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-11-25  3:50 [RFC] [PATCH v2 00/13] ath1{1,2}k: support multiple PCI devices in one system Mihai Moldovan
2024-11-25  3:50 ` Mihai Moldovan [this message]
2024-11-25  3:50 ` [RFC] [PATCH v2 02/13] net: qrtr: mhi: register mhi_controller as endpoint-specific data Mihai Moldovan
2024-11-25  3:50 ` [RFC] [PATCH v2 03/13] net: qrtr: smd: register rpmsg_device " Mihai Moldovan
2024-11-25  3:50 ` [RFC] [PATCH v2 04/13] net: qrtr: tun: register inode " Mihai Moldovan
2024-11-25  3:50 ` [RFC] [PATCH v2 05/13] soc: qcom: qmi_helpers: add QRTR endpoint ID to qmi_handle Mihai Moldovan
2024-11-25  3:50 ` [RFC] [PATCH v2 06/13] soc: qcom: qmi_helpers: optionally bind to QRTR endpoint ID in qmi_sock_create Mihai Moldovan
2024-11-25  3:50 ` [RFC] [PATCH v2 07/13] wifi: ath11k: add QRTR endpoint ID hif feature Mihai Moldovan
2024-11-25  3:50 ` [RFC] [PATCH v2 08/13] wifi: ath11k: stub QRTR endpoint ID fetching for AHB Mihai Moldovan
2024-11-25  3:50 ` [RFC] [PATCH v2 09/13] wifi: ath11k: implement QRTR endpoint ID fetching for PCI Mihai Moldovan
2024-11-25  3:50 ` [RFC] [PATCH v2 10/13] wifi: ath11k: bind to QRTR endpoint ID in ath11k_qmi_init_service Mihai Moldovan
2024-11-25  3:50 ` [RFC] [PATCH v2 11/13] wifi: ath12k: add QRTR endpoint ID hif feature Mihai Moldovan
2024-11-25  3:50 ` [RFC] [PATCH v2 12/13] wifi: ath12k: implement QRTR endpoint ID fetching for PCI Mihai Moldovan
2024-11-25  3:50 ` [RFC] [PATCH v2 13/13] wifi: ath12k: bind to QRTR endpoint ID in ath12k_qmi_init_service Mihai Moldovan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=064e351e175e809e9a774022fa2f104b67862d9e.1732506261.git.ionic@ionic.de \
    --to=ionic@ionic.de \
    --cc=andersson@kernel.org \
    --cc=ath11k@lists.infradead.org \
    --cc=ath12k@lists.infradead.org \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=horms@kernel.org \
    --cc=jjohnson@kernel.org \
    --cc=konradybcio@kernel.org \
    --cc=kuba@kernel.org \
    --cc=kvalo@kernel.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=manivannan.sadhasivam@linaro.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).