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

[Bjorn Andersson]

On Fri, Jul 31, 2026 at 03:47:29PM +0530, Pranav Mahesh Phansalkar wrote:
> From: Chris Lew <christopher.lew@xxxxxxxxxxxxxxxx>
> 
> Hello message is currently sent only by the name server in response to
> a hello message received from a remote. When two endpoints operate in a
> slave model, neither sends the initial hello, stalling further
> communication.
> 
> Transfer ownership of the HELLO handshake to the core layer. On
> endpoint registration, schedule a work item that sends a HELLO packet
> to the new endpoint once the name server is bound. An atomic hello_sent
> flag prevents the name server from sending duplicate HELLOs once the
> core has successfully sent one. If the initial send fails, the work is
> rescheduled to retry.

Why would something queue more than a single HELLO?

If I read the code correctly, qrtr_endpoint_register() will schedule the
sending of a HELLO and then as we receive the first incoming HELLO we
forward that to the `ns` which will try to send another HELLO, which we
per our flags silently discard.

But why not throw out say_hello()?

Then qrtr_endpoint_register() would schedule the HELLO and
qrtr_node_enqueue() can reschedule the say_hello work for every HELLO
send that fails - no state left.

> 
> Prevent duplicate HELLO packets from reaching the name server by
> tracking received HELLOs with a hello_rcvd flag. Without this, each
> duplicate HELLO would trigger announce_servers() in the NS, causing
> unnecessary re-advertisement of all locally registered services to the
> remote and redundant lookup notifications to observing clients.
> 

Why sweep the problem under the rug? If the peer is misbehaving and send
multiple HELLO messages, wouldn't it be useful to put something in the
log?

If the concern is that ns will call announce_servers() multiple times,
wouldn't the right place to solve that be in the NS?

> Co-developed-by: Pranav Mahesh Phansalkar <pranav.phansalkar@xxxxxxxxxxxxxxxx>
> Signed-off-by: Pranav Mahesh Phansalkar <pranav.phansalkar@xxxxxxxxxxxxxxxx>
> Co-developed-by: Deepak Kumar Singh <deepak.singh@xxxxxxxxxxxxxxxx>
> Signed-off-by: Deepak Kumar Singh <deepak.singh@xxxxxxxxxxxxxxxx>
> Signed-off-by: Chris Lew <christopher.lew@xxxxxxxxxxxxxxxx>

This isn't right, Pranav. You're the sender of this email your name
should be last here, because you handled the patch last.

> ---
> To: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> To: David S. Miller <davem@xxxxxxxxxxxxx>
> To: Eric Dumazet <edumazet@xxxxxxxxxx>
> To: Jakub Kicinski <kuba@xxxxxxxxxx>
> To: Paolo Abeni <pabeni@xxxxxxxxxx>
> To: Simon Horman <horms@xxxxxxxxxx>
> Cc: linux-arm-msm@xxxxxxxxxxxxxxx
> Cc: netdev@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx

Not sure what you did to b4, but this looks wrong.

> ---
>  net/qrtr/af_qrtr.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 78 insertions(+), 1 deletion(-)
> 
> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
> index d02ef9a74c3ca..03234bd1fe54f 100644
> --- a/net/qrtr/af_qrtr.c
> +++ b/net/qrtr/af_qrtr.c
> @@ -9,6 +9,7 @@
>  #include <linux/termios.h>	/* For TIOCINQ/OUTQ */
>  #include <linux/spinlock.h>
>  #include <linux/wait.h>
> +#include <linux/workqueue.h>
>  
>  #include <net/sock.h>
>  
> @@ -120,8 +121,11 @@ 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 sent to endpoint
> + * @hello_rcvd: hello packet received from endpoint
>   * @rx_queue: receive queue
>   * @item: list item for broadcast list
> + * @say_hello: scheduled work for initiating hello
>   */
>  struct qrtr_node {
>  	struct mutex ep_lock;
> @@ -132,8 +136,12 @@ struct qrtr_node {
>  	struct xarray qrtr_tx_flow;
>  	struct mutex qrtr_tx_lock; /* for qrtr_tx_flow */
>  
> +	atomic_t hello_sent;
> +	atomic_t hello_rcvd;
> +
>  	struct sk_buff_head rx_queue;
>  	struct list_head item;
> +	struct work_struct say_hello;
>  };
>  
>  /**
> @@ -187,6 +195,8 @@ static void __qrtr_node_release(struct kref *kref)
>  	list_del(&node->item);
>  	mutex_unlock(&qrtr_node_lock);
>  
> +	cancel_work_sync(&node->say_hello);
> +
>  	skb_queue_purge(&node->rx_queue);
>  
>  	/* Free tx flow counters */
> @@ -341,6 +351,12 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
>  	size_t len = skb->len;
>  	int rc, confirm_rx;
>  
> +	if (type == QRTR_TYPE_HELLO &&
> +	    atomic_cmpxchg(&node->hello_sent, 0, 1) != 0) {
> +		kfree_skb(skb);
> +		return 0;
> +	}
> +
>  	confirm_rx = qrtr_tx_wait(node, to->sq_node, to->sq_port, type);
>  	if (confirm_rx < 0) {
>  		kfree_skb(skb);
> @@ -379,6 +395,11 @@ 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 && type == QRTR_TYPE_HELLO) {
> +		atomic_set(&node->hello_sent, 0);
> +		schedule_work(&node->say_hello);
> +	}
> +
>  	return rc;
>  }
>  
> @@ -527,6 +548,13 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
>  		if (!ipc)
>  			goto err;
>  
> +		if (cb->type == QRTR_TYPE_HELLO &&
> +		    atomic_cmpxchg(&node->hello_rcvd, 0, 1) != 0) {
> +			qrtr_port_put(ipc);
> +			kfree_skb(skb);
> +			return 0;
> +		}
> +
>  		if (sock_queue_rcv_skb(&ipc->sk, skb)) {
>  			qrtr_port_put(ipc);
>  			goto err;
> @@ -570,6 +598,35 @@ 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(work, struct qrtr_node, say_hello);
> +
> +	/* NS must be bound before we can send */
> +	ctrl = qrtr_port_lookup(QRTR_PORT_CTRL);
> +	if (!ctrl)
> +		return;
> +
> +	skb = qrtr_alloc_ctrl_packet(&pkt, GFP_KERNEL);
> +	if (!skb) {
> +		qrtr_port_put(ctrl);
> +		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
> @@ -595,6 +652,10 @@ int qrtr_endpoint_register(struct qrtr_endpoint *ep, unsigned int nid)
>  	node->nid = QRTR_EP_NID_AUTO;
>  	node->ep = ep;
>  
> +	atomic_set(&node->hello_sent, 0);
> +	atomic_set(&node->hello_rcvd, 0);
> +	INIT_WORK(&node->say_hello, qrtr_hello_work);
> +
>  	xa_init(&node->qrtr_tx_flow);
>  	mutex_init(&node->qrtr_tx_lock);
>  
> @@ -605,6 +666,9 @@ 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_work(&node->say_hello);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(qrtr_endpoint_register);
> @@ -796,8 +860,21 @@ static int __qrtr_bind(struct socket *sock,
>  	sock_reset_flag(sk, SOCK_ZAPPED);
>  
>  	/* Notify all open ports about the new controller */
> -	if (port == QRTR_PORT_CTRL)
> +	if (port == QRTR_PORT_CTRL) {
> +		struct qrtr_node *node;
> +
> +		/* Reset HELLO state on all nodes so the handshake can be
> +		 * re-established with the new NS instance.
> +		 */
> +		mutex_lock(&qrtr_node_lock);
> +		list_for_each_entry(node, &qrtr_all_nodes, item) {
> +			atomic_set(&node->hello_sent, 0);
> +			atomic_set(&node->hello_rcvd, 0);
> +		}
> +		mutex_unlock(&qrtr_node_lock);

This code existed to handle the case when the userspace qrtr-ns
restarted, but we will always register the in-kernel NS so that use case
doesn't exist anymore.

Do you really need to reset these things?

Regards,
Bjorn

> +
>  		qrtr_reset_ports();
> +	}
>  
>  	return 0;
>  }
> 
> ---
> base-commit: 502d801f0ab03e4f32f9a33d203154ce84887921
> change-id: 20260623-qrtr-hello-on-ep-register-7064b66ccd70
> 
> Best regards,
> -- 
> Pranav Mahesh Phansalkar <pranav.phansalkar@xxxxxxxxxxxxxxxx>
>