Re: [PATCH net v2] net: qrtr: announce local services again once our HELLO is sent

[Manivannan Sadhasivam <mani@kernel.org>]

On Fri, Oct 23, 2026 at 01:31:16AM +0900, Joonhoe Kim wrote:
> Since commit 544d85de4dc2 ("net: qrtr: Send HELLO message on endpoint
> register"), qrtr_node_enqueue() drops every packet but HELLO with
> -EAGAIN until our HELLO has gone out to the node. When the remote's
> HELLO arrives first, the NEW_SERVER replies from the name service are
> dropped, and so are the broadcasts for services registered before our
> HELLO goes out. Nothing announces these services again, so the remote
> never learns about them.
> 
> On Kaanapali the modem sends its HELLO before it accepts ours. With
> rmtfs started first, the modem never learns about rmtfs and its
> watchdog resets the SoC about 40 s later.
> 
> Remember that the remote has sent a HELLO. Once our HELLO has been
> sent to such a node, pass a HELLO from it to the local name service
> again, so that it announces the services it has at that point.
> 
> The flag is not cleared when qrtr_endpoint_hello() restarts the
> handshake on resume, so a HELLO that the remote sent before then is
> not lost. The cost is that the remote gets the same NEW_SERVER
> announcements twice when its HELLO arrives before ours goes out but
> the name service handles it after, and on every resume.
> 
> Fixes: 544d85de4dc2 ("net: qrtr: Send HELLO message on endpoint register")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joonhoe Kim <26rote@gmail.com>

This version looks good to me, although I left a few comments inline.

> ---
> Changes in v2:
> - Replay the remote's HELLO to the name service from the core once our
>   HELLO is out, instead of retrying in announce_servers(). Replaying
>   the announcements once hello_sent is set was suggested in the Sashiko
>   review of v1. This is also not the delayed work in the name service
>   that I mentioned to Mani: that would still miss the broadcasts
>   dropped before our HELLO. Nothing sleeps in the name service worker
>   and there is no time limit.
> - v1: https://lore.kernel.org/all/20261016124600.31-1-26rote@gmail.com/
> 
> Tested on a Lenovo Legion Tab Y700 Gen 5 (out-of-tree board support)
> with rmtfs started before the modem: the modem's HELLO is still handled
> before ours is out ("failed to announce new service"), the replayed
> HELLO follows right after, rmtfs gets the modem_fs1/2 opens, and the
> modem stays up.
> 
>  net/qrtr/af_qrtr.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
> index e7b364742..508a2dd2d 100644
> --- a/net/qrtr/af_qrtr.c
> +++ b/net/qrtr/af_qrtr.c
> @@ -122,6 +122,7 @@ static DEFINE_XARRAY_ALLOC(qrtr_ports);
>   * @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
> + * @hello_rcvd: remote has sent a hello packet
>   * @rx_queue: receive queue
>   * @item: list item for broadcast list
>   * @say_hello: delayed work for sending hello packet
> @@ -136,6 +137,7 @@ struct qrtr_node {
>  	struct mutex qrtr_tx_lock; /* for qrtr_tx_flow */
>  
>  	bool hello_sent;
> +	bool hello_rcvd;
>  
>  	struct sk_buff_head rx_queue;
>  	struct list_head item;
> @@ -541,6 +543,11 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
>  		qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
>  	}
>  
> +	if (cb->type == QRTR_TYPE_HELLO) {
> +		/* Pairs with smp_load_acquire() in qrtr_hello_work() */
> +		smp_store_release(&node->hello_rcvd, true);

smp_store_{release/acquire}() accessors are only needed if there is an ordering
requirement. But I don't see that exist with this 'hello_rcvd' bool flag.
Wouldn't {READ/WRITE}_ONCE() suffice?

> +	}
> +
>  	if (cb->type == QRTR_TYPE_RESUME_TX) {
>  		qrtr_tx_resume(node, skb);
>  	} else {
> @@ -619,7 +626,20 @@ static void qrtr_hello_work(struct work_struct *work)
>  	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);
> +	if (qrtr_node_enqueue(node, skb, QRTR_TYPE_HELLO, &from, &to))
> +		goto out;
> +
> +	/* Pairs with smp_store_release() in qrtr_endpoint_post() */
> +	if (!smp_load_acquire(&node->hello_rcvd))
> +		goto out;
> +
> +	skb = qrtr_alloc_ctrl_packet(&pkt, GFP_KERNEL);
> +	if (skb) {
> +		pkt->cmd = cpu_to_le32(QRTR_TYPE_HELLO);
> +		to.sq_node = READ_ONCE(node->nid);

There is no READ_ONCE() in the earlier read. So please justify why you added it.

> +		qrtr_local_enqueue(NULL, skb, QRTR_TYPE_HELLO, &to, &from);

What happens to the return value?

- Mani

-- 
மணிவண்ணன் சதாசிவம்