[PATCH net] Revert "net: qrtr: Send HELLO message on endpoint register"

This reverts commit 544d85de4dc22c01badfd8cefa59829ce35c4858.

It causes suspend and resume problems for numerous people, including
maintainers and developers of ath wifi drivers. A fix was proposed[1],
but did not get any traction, so go for a revert instead.

Cc: Chris Lew <[email protected]>
Cc: Deepak Kumar Singh <[email protected]>
Cc: Pranav Mahesh Phansalkar <[email protected]>
Reported-by: Daniel J Blueman <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/ [1]
Reported-by: Takashi Iwai <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/
Reported-by: Vlastimil Babka <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/
Reported-by: Jeff Johnson <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Thorsten Leemhuis <[email protected]>
---

I'm not affected by this, but due to all those reporters mentioned above
it seems it would be good to get this fixed for 7.3-rc4, as Linus' rule
of thumb wrt to fixing regressions is "generally be 'within a week',
preferably before the next rc". Which is why I currently plan to ask
Linus to pull this directly. If this is a bad idea, just tell me.

The patch from Daniel linked above[1] could have fixed the problem, but
none of the developers of the culprit or the maintainers for this area
commented on it within a week, despite some "please fix this soon" and
"Tested-by" replies from various maintainers of other subsystems. So
let's go for a revert instead, the original change can later easily be
reapplied in improved form. The change reverted looks like something
that maybe shouldn't have been merged after -rc1 anyway, but this is not
my area of expertise, so I might be totally wrong there, as mentioned
earlier.

Ciao, Thorsten
---
 net/qrtr/af_qrtr.c | 66 ++--------------------------------------------
 net/qrtr/ns.c      | 35 +++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 65 deletions(-)

diff --git net/qrtr/af_qrtr.c net/qrtr/af_qrtr.c
index 78347c937af76b..a30fa56e6aa31c 100644
--- net/qrtr/af_qrtr.c
+++ net/qrtr/af_qrtr.c
@@ -9,7 +9,6 @@
 #include <linux/termios.h>	/* For TIOCINQ/OUTQ */
 #include <linux/spinlock.h>
 #include <linux/wait.h>
-#include <linux/workqueue.h>
 
 #include <net/sock.h>
 
@@ -121,10 +120,8 @@ static DEFINE_XARRAY_ALLOC(qrtr_ports);
  * @nid: node id
  * @qrtr_tx_flow: xarray of qrtr_tx_flow, keyed by node << 32 | port
  * @qrtr_tx_lock: lock for qrtr_tx_flow inserts
- * @hello_sent: hello packet send successful
  * @rx_queue: receive queue
  * @item: list item for broadcast list
- * @say_hello: delayed work for sending hello packet
  */
 struct qrtr_node {
 	struct mutex ep_lock;
@@ -135,11 +132,8 @@ struct qrtr_node {
 	struct xarray qrtr_tx_flow;
 	struct mutex qrtr_tx_lock; /* for qrtr_tx_flow */
 
-	bool hello_sent;
-
 	struct sk_buff_head rx_queue;
 	struct list_head item;
-	struct delayed_work say_hello;
 };
 
 /**
@@ -193,8 +187,6 @@ static void __qrtr_node_release(struct kref *kref)
 	list_del(&node->item);
 	mutex_unlock(&qrtr_node_lock);
 
-	cancel_delayed_work_sync(&node->say_hello);
-
 	skb_queue_purge(&node->rx_queue);
 
 	/* Free tx flow counters */
@@ -349,14 +341,6 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 	size_t len = skb->len;
 	int rc, confirm_rx;
 
-	mutex_lock(&node->ep_lock);
-	if (!node->hello_sent && type != QRTR_TYPE_HELLO) {
-		mutex_unlock(&node->ep_lock);
-		kfree_skb(skb);
-		return -EAGAIN;
-	}
-	mutex_unlock(&node->ep_lock);
-
 	confirm_rx = qrtr_tx_wait(node, to->sq_node, to->sq_port, type);
 	if (confirm_rx < 0) {
 		kfree_skb(skb);
@@ -369,7 +353,7 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 	hdr->src_node_id = cpu_to_le32(from->sq_node);
 	hdr->src_port_id = cpu_to_le32(from->sq_port);
 	if (to->sq_port == QRTR_PORT_CTRL) {
-		hdr->dst_node_id = cpu_to_le32(READ_ONCE(node->nid));
+		hdr->dst_node_id = cpu_to_le32(node->nid);
 		hdr->dst_port_id = cpu_to_le32(QRTR_PORT_CTRL);
 	} else {
 		hdr->dst_node_id = cpu_to_le32(to->sq_node);
@@ -388,8 +372,6 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 			rc = node->ep->xmit(node->ep, skb);
 		else
 			kfree_skb(skb);
-		if (!rc && type == QRTR_TYPE_HELLO)
-			node->hello_sent = true;
 		mutex_unlock(&node->ep_lock);
 	}
 	/* Need to ensure that a subsequent message carries the otherwise lost
@@ -397,9 +379,6 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 	if (rc && confirm_rx)
 		qrtr_tx_flow_failed(node, to->sq_node, to->sq_port);
 
-	if (rc == -EAGAIN && type == QRTR_TYPE_HELLO)
-		schedule_delayed_work(&node->say_hello, msecs_to_jiffies(100));
-
 	return rc;
 }
 
@@ -437,7 +416,7 @@ static void qrtr_node_assign(struct qrtr_node *node, unsigned int nid)
 	spin_lock_irqsave(&qrtr_nodes_lock, flags);
 	radix_tree_insert(&qrtr_nodes, nid, node);
 	if (node->nid == QRTR_EP_NID_AUTO)
-		WRITE_ONCE(node->nid, nid);
+		node->nid = nid;
 	spin_unlock_irqrestore(&qrtr_nodes_lock, flags);
 }
 
@@ -591,38 +570,6 @@ static struct sk_buff *qrtr_alloc_ctrl_packet(struct qrtr_ctrl_pkt **pkt,
 	return skb;
 }
 
-static void qrtr_hello_work(struct work_struct *work)
-{
-	struct sockaddr_qrtr from = {AF_QIPCRTR, 0, QRTR_PORT_CTRL};
-	struct sockaddr_qrtr to = {AF_QIPCRTR, 0, QRTR_PORT_CTRL};
-	struct qrtr_ctrl_pkt *pkt;
-	struct qrtr_node *node;
-	struct qrtr_sock *ctrl;
-	struct sk_buff *skb;
-
-	node = container_of(to_delayed_work(work), struct qrtr_node, say_hello);
-
-	/* NS must be bound before we can send; retry with backoff if not ready */
-	ctrl = qrtr_port_lookup(QRTR_PORT_CTRL);
-	if (!ctrl) {
-		schedule_delayed_work(&node->say_hello, msecs_to_jiffies(100));
-		return;
-	}
-
-	skb = qrtr_alloc_ctrl_packet(&pkt, GFP_KERNEL);
-	if (!skb) {
-		qrtr_port_put(ctrl);
-		schedule_delayed_work(&node->say_hello, msecs_to_jiffies(100));
-		return;
-	}
-
-	pkt->cmd = cpu_to_le32(QRTR_TYPE_HELLO);
-	from.sq_node = qrtr_local_nid;
-	to.sq_node = node->nid;
-	qrtr_node_enqueue(node, skb, QRTR_TYPE_HELLO, &from, &to);
-	qrtr_port_put(ctrl);
-}
-
 /**
  * qrtr_endpoint_register() - register a new endpoint
  * @ep: endpoint to register
@@ -648,9 +595,6 @@ int qrtr_endpoint_register(struct qrtr_endpoint *ep, unsigned int nid)
 	node->nid = QRTR_EP_NID_AUTO;
 	node->ep = ep;
 
-	node->hello_sent = false;
-	INIT_DELAYED_WORK(&node->say_hello, qrtr_hello_work);
-
 	xa_init(&node->qrtr_tx_flow);
 	mutex_init(&node->qrtr_tx_lock);
 
@@ -661,9 +605,6 @@ int qrtr_endpoint_register(struct qrtr_endpoint *ep, unsigned int nid)
 	mutex_unlock(&qrtr_node_lock);
 	ep->node = node;
 
-	/* Initiate HELLO handshake from the core layer */
-	schedule_delayed_work(&node->say_hello, 0);
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(qrtr_endpoint_register);
@@ -938,9 +879,6 @@ static int qrtr_bcast_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 
 	mutex_lock(&qrtr_node_lock);
 	list_for_each_entry(node, &qrtr_all_nodes, item) {
-		/* Skip nodes with no assigned node ID yet. */
-		if (READ_ONCE(node->nid) == QRTR_EP_NID_AUTO)
-			continue;
 		skbn = pskb_copy(skb, GFP_KERNEL);
 		if (!skbn)
 			break;
diff --git net/qrtr/ns.c net/qrtr/ns.c
index bcb090ee79d494..c5e7e01db24984 100644
--- net/qrtr/ns.c
+++ net/qrtr/ns.c
@@ -212,7 +212,6 @@ static void lookup_notify(struct sockaddr_qrtr *to, struct qrtr_server *srv,
 		pr_err("failed to send lookup notification\n");
 }
 
-/* Announce the list of servers registered on the local node */
 static int announce_servers(struct sockaddr_qrtr *sq)
 {
 	struct qrtr_server *srv;
@@ -327,8 +326,38 @@ static int server_del(struct qrtr_node *node, unsigned int port, bool bcast)
 	return 0;
 }
 
+static int say_hello(struct sockaddr_qrtr *dest)
+{
+	struct qrtr_ctrl_pkt pkt;
+	struct msghdr msg = { };
+	struct kvec iv;
+	int ret;
+
+	iv.iov_base = &pkt;
+	iv.iov_len = sizeof(pkt);
+
+	memset(&pkt, 0, sizeof(pkt));
+	pkt.cmd = cpu_to_le32(QRTR_TYPE_HELLO);
+
+	msg.msg_name = (struct sockaddr *)dest;
+	msg.msg_namelen = sizeof(*dest);
+
+	ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
+	if (ret < 0)
+		pr_err("failed to send hello msg\n");
+
+	return ret;
+}
+
+/* Announce the list of servers registered on the local node */
 static int ctrl_cmd_hello(struct sockaddr_qrtr *sq)
 {
+	int ret;
+
+	ret = say_hello(sq);
+	if (ret < 0)
+		return ret;
+
 	return announce_servers(sq);
 }
 
@@ -745,6 +774,10 @@ int qrtr_ns_init(void)
 	qrtr_ns.bcast_sq.sq_node = QRTR_NODE_BCAST;
 	qrtr_ns.bcast_sq.sq_port = QRTR_PORT_CTRL;
 
+	ret = say_hello(&qrtr_ns.bcast_sq);
+	if (ret < 0)
+		goto err_wq;
+
 	/* As the qrtr ns socket owner and creator is the same module, we have
 	 * to decrease the qrtr module reference count to guarantee that it
 	 * remains zero after the ns socket is created, otherwise, executing

base-commit: 5dd1818b15d98d4a20806cd00b1b40320b06004f
-- 
2.55.0