Re: [PATCH v3 4/4] wifi: ath12k: Connect to the QMI server belonging to the device owned by this driver

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Asymmetric node-id filtering in ath12k QMI:
  ath12k_qmi_ops_new_server()…

--

wifi: ath12k: Connect to the QMI server belonging to the device owned by this driver

This patch adds a get_qrtr_node_id() HIF callback that returns the QRTR node
id derived from the MHI controller index, and uses it in the QMI new_server
callback to skip WLFW services that live on a different node. A node id of
zero keeps the previous unfiltered behaviour.

> diff --git a/drivers/net/wireless/ath/ath12k/qmi.c b/drivers/net/wireless/ath/ath12k/qmi.c
> index 280e50a1f31d8..e1e75cc9d1441 100644
> --- a/drivers/net/wireless/ath/ath12k/qmi.c
> +++ b/drivers/net/wireless/ath/ath12k/qmi.c

[ ... ]

> @@ -3940,8 +3941,18 @@ static int ath12k_qmi_ops_new_server(struct qmi_handle *qmi_hdl,
>  	struct ath12k_qmi *qmi = container_of(qmi_hdl, struct ath12k_qmi, handle);
>  	struct ath12k_base *ab = qmi->ab;
>  	struct sockaddr_qrtr *sq = &qmi->sq;
> +	unsigned int node_id;
>  	int ret;
>  
> +	/* Identical devices advertise the same QMI services, so connect only to
> +	 * the QMI server on this device's node. A node id of 0 means the
> +	 * transport has not assigned a unique node id, so accept the server
> +	 * unfiltered.
> +	 */
> +	node_id = ath12k_hif_get_qrtr_node_id(ab);
> +	if (node_id && service->node != node_id)
> +		return 0;
> +

[Severity: High]
Should this return a negative errno instead of 0 for the foreign node?

In drivers/soc/qcom/qmi_interface.c, qmi_recv_new_server() only discards the
service record when the callback returns a negative value:

	ret = ops->new_server(qmi, svc);
	if (ret < 0)
		kfree(svc);
	else
		list_add(&svc->list_node, &qmi->lookup_results);

With the return 0 above, the other device's WLFW service stays in this
handle's lookup_results even though this device never connected to it.

The matching teardown path in the same qmi_ops table has no node id check:

drivers/net/wireless/ath/ath12k/qmi.c:ath12k_qmi_ops_del_server() {
	struct ath12k_qmi *qmi = container_of(qmi_hdl, struct ath12k_qmi, handle);
	struct ath12k_base *ab = qmi->ab;

	ath12k_dbg(ab, ATH12K_DBG_QMI, "qmi wifi fw del server\n");
	ath12k_qmi_driver_event_post(qmi, ATH12K_QMI_EVENT_SERVER_EXIT, NULL);
}

and qmi_recv_del_server() invokes it for every retained entry whose node
matches, with qmi_recv_bye() passing port == -1 to match all ports on the
dying node:

drivers/soc/qcom/qmi_interface.c:qmi_recv_del_server() {
	list_for_each_entry_safe(svc, tmp, &qmi->lookup_results, list_node) {
		if (node != -1 && svc->node != node)
			continue;
		if (port != -1 && svc->port != port)
			continue;

		if (ops->del_server)
			ops->del_server(qmi, svc);
	...
}

net/qrtr/ns.c:server_del() notifies every lookup matching service/instance
regardless of node, so device A's handle does receive DEL_SERVER or BYE for
device B's node when device B's firmware crashes or that card is unbound or
hot-removed.

Can that spuriously fault the healthy device? The event worker reacts to
SERVER_EXIT unconditionally:

drivers/net/wireless/ath/ath12k/qmi.c:ath12k_qmi_driver_event_work() {
	...
		case ATH12K_QMI_EVENT_SERVER_EXIT:
			set_bit(ATH12K_FLAG_CRASH_FLUSH, &ab->dev_flags);
	...
}

ATH12K_FLAG_CRASH_FLUSH makes CE sends, WMI sends, data TX
(ath12k_wifi7_dp_tx()) and mgmt TX return -ESHUTDOWN, and it makes
ath12k_core_stop() skip ath12k_dp_reoq_lut_addr_reset() and
ath12k_qmi_firmware_stop() on a device whose firmware is still running:

drivers/net/wireless/ath/ath12k/core.c:ath12k_core_stop() {
	if (!test_bit(ATH12K_FLAG_CRASH_FLUSH, &ab->dev_flags)) {
		ath12k_dp_reoq_lut_addr_reset(ath12k_ab_to_dp(ab));
		ath12k_qmi_firmware_stop(ab);
	}
	...
}

The flag is only cleared on a later FW_READY event or on core reconfigure,
so the untouched device can stay in this state indefinitely.

Would returning a negative errno here (so the foreign service is never
tracked), and/or adding the same service->node check to
ath12k_qmi_ops_del_server(), cover the teardown direction as well?

There is a second effect from the retained entries: each filtered
NEW_SERVER adds another node of the foreign device into lookup_results,
so a foreign device in a firmware restart loop accumulates duplicate
entries that each later produce their own SERVER_EXIT post.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-qrtr-multi-ep-v3-0-c00831e88957%40oss.qualcomm.com