Re: [PATCH v2 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 2 potential
issue(s) to consider.

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

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] Incomplete node-id filtering in ath12k_qmi_ops_new_server(): the
  foreign device's WLFW server is "skipped" by returning 0, which the…
- [Low] Concern as filed: the patch lacks a Fixes: tag and silently
  depends on a QRTR change that registers MHI endpoints with…

Pre-existing issues:
- [Medium] Pre-existing resource leak (unrelated to this patch's change):
  ath12k_qmi_init_service()…

--

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

This adds a get_qrtr_node_id() HIF callback that returns the QRTR node id
derived from the MHI controller index, and uses it in
ath12k_qmi_ops_new_server() so a device only connects to the WLFW server
running on its own node. A node id of zero keeps the previous behaviour for
transports that do not assign one.

> diff --git a/drivers/net/wireless/ath/ath12k/pci.c b/drivers/net/wireless/ath/ath12k/pci.c
> index 6441927b53822..2b1b122bbb3ba 100644
> --- a/drivers/net/wireless/ath/ath12k/pci.c
> +++ b/drivers/net/wireless/ath/ath12k/pci.c

[ ... ]

> @@ -1491,6 +1492,13 @@ static int ath12k_pci_panic_handler(struct ath12k_base *ab)
>  	return NOTIFY_OK;
>  }
>  
> +static int ath12k_pci_get_qrtr_node_id(struct ath12k_base *ab)
> +{
> +	struct ath12k_pci *ab_pci = ath12k_pci_priv(ab);
> +
> +	return qrtr_host_node_id(ab_pci->mhi_ctrl->index);
> +}

[Severity: Low]
This isn't a bug, but could the commit message name the prerequisite change
this depends on?

On PCIe this helper always returns a non-zero id
(QRTR_NODE_HOST_BASE + index), so the new filter in
ath12k_qmi_ops_new_server() is unconditionally active, and it only matches
because net/qrtr/mhi.c now registers endpoints with the same id:

net/qrtr/mhi.c:qcom_mhi_qrtr_probe() {
	rc = qrtr_endpoint_register(&qdev->ep,
				    qrtr_host_node_id(mhi_dev->mhi_cntrl->index));
}

That registration comes from "net: qrtr: Assign unique node id for MHI
endpoints" in this same series, and the message only says "But now, QRTR
provides each MHI endpoint a unique node id". Without the QRTR patch, this
helper returns an id that matches no advertised server and every WLFW
server would be skipped, so spelling out the ordering requirement (and that
this must not be backported alone) would help future backporters.

[ ... ]

> diff --git a/drivers/net/wireless/ath/ath12k/qmi.c b/drivers/net/wireless/ath/ath12k/qmi.c
> index 280e50a1f31d8..22b8cbbbf4f45 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;
> +	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?

The QMI core only discards the service when new_server() returns negative:

drivers/soc/qcom/qmi_interface.c:qmi_recv_new_server() {
	ret = ops->new_server(qmi, svc);
	if (ret < 0)
		kfree(svc);
	else
		list_add(&svc->list_node, &qmi->lookup_results);
}

With "return 0", the other device's server stays recorded in
qmi->lookup_results on this device's handle, even though this device never
connected to it.

The teardown half of the lifecycle has no node id check. When the peer
device's firmware dies or the peer is unbound, net/qrtr/ns.c:server_del()
notifies every lookup matching service and instance, with no node
filtering:

net/qrtr/ns.c:server_del() {
	list_for_each(li, &qrtr_ns.lookups) {
		lookup = container_of(li, struct qrtr_lookup, li);
		if (lookup->service && lookup->service != srv->service)
			continue;
		if (lookup->instance && lookup->instance != srv->instance)
			continue;

		lookup_notify(&lookup->sq, srv, false);
	}
}

qmi_recv_del_server() then finds the retained foreign entry (also reached
with node/port == -1 from qmi_handle_net_reset() and qmi_handle_release())
and calls into the driver:

drivers/net/wireless/ath/ath12k/qmi.c:ath12k_qmi_ops_del_server() {
	ath12k_dbg(ab, ATH12K_DBG_QMI, "qmi wifi fw del server\n");
	ath12k_qmi_driver_event_post(qmi, ATH12K_QMI_EVENT_SERVER_EXIT, NULL);
}

so this healthy device gets a SERVER_EXIT with no matching ARRIVE:

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);
}

Can that latch the healthy device into the crash-flush state permanently?
ATH12K_FLAG_CRASH_FLUSH makes ath12k_dp_tx() and management TX return
-ESHUTDOWN, and ath12k_wmi_cmd_send()/ath12k_ce_send() fail the same way:

drivers/net/wireless/ath/ath12k/wifi7/dp_tx.c:ath12k_dp_tx() {
	if (test_bit(ATH12K_FLAG_CRASH_FLUSH, &ab->dev_flags))
		return -ESHUTDOWN;
}

The bit is only cleared on this device's own FW_READY or core start, and
neither happens because this device's firmware never crashed.

Would returning a negative errno (so the core never records the foreign
service) plus the same node id check in ath12k_qmi_ops_del_server() cover
both halves?

[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at the QMI
handle lifecycle in the same file: does ath12k_qmi_init_service() leak the
QMI handle on its two failure paths after qmi_handle_init() succeeded?

drivers/net/wireless/ath/ath12k/qmi.c:ath12k_qmi_init_service() {
	ab->qmi.event_wq = alloc_ordered_workqueue("ath12k_qmi_driver_event", 0);
	if (!ab->qmi.event_wq) {
		ath12k_err(ab, "failed to allocate workqueue\n");
		return -EFAULT;
	}
	...
	if (ret < 0) {
		ath12k_warn(ab, "failed to add qmi lookup\n");
		destroy_workqueue(ab->qmi.event_wq);
		return ret;
	}
}

Neither path calls qmi_handle_release(), so qmi->recv_buf, the
"qmi_msg_handler" workqueue and the kernel QRTR socket created by
qmi_handle_init() stay allocated.

ath12k_core_soc_create() returns before its err_qmi_deinit label, and the
unwind loop in ath12k_core_hw_group_create() skips the device that just
failed:

drivers/net/wireless/ath/ath12k/core.c:ath12k_core_hw_group_create() {
destroy:
	for (i--; i >= 0; i--) {
		ab = ag->ab[i];
		...
		ath12k_core_soc_destroy(ab);
}

Probe then reaches ath12k_core_free() which kfree()s ab, while the leaked
socket still holds sk->sk_user_data pointing at &ab->qmi:

drivers/soc/qcom/qmi_interface.c:qmi_sock_create() {
	sock->sk->sk_user_data = qmi;
	sock->sk->sk_data_ready = qmi_data_ready;
}

Both paths require an allocation failure at probe time, so this is a cold
path only.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-qrtr-multi-ep-v2-0-27dd80d841a0%40oss.qualcomm.com