Re: [PATCH v2 3/4] wifi: ath11k: 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 · 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 fix: ath11k_qmi_ops_new_server() returns 0 (success)
  when it decides to skip a WLFW server that belongs to the *other*…
- [Low] Concern that the commit fixes a real user-visible multi-device
  initialization bug (identical ath11k PCIe cards connecting to each…

--

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

This adds a get_qrtr_node_id() HIF callback returning the QRTR node id
derived from the MHI controller index, and uses it in the QMI new_server
callback so a device only connects to the WLFW server running on its own
node. A node id of zero keeps the previous unfiltered behaviour.

[Severity: Low]
This isn't a bug, but the commit fixes a user-visible misbehaviour (two
identical ath11k PCIe cards connecting to each other's WLFW firmware) and
carries no Fixes: trailer.

The fix depends on qrtr_host_node_id()/QRTR_NODE_HOST_BASE in
include/net/qrtr.h and on the host-assigned node id plus the
NEW_SERVER/DEL_SERVER node rewrite from the preceding net/qrtr patches, so
a Fixes: tag pointing at the original ath11k commit would invite stable
tooling to pick this up without those dependencies.

Would it help to say so explicitly in the commit message, for example
"depends on the preceding net/qrtr changes; not suitable for stable on its
own", instead of leaving the tag out silently?

> diff --git a/drivers/net/wireless/ath/ath11k/qmi.c b/drivers/net/wireless/ath/ath11k/qmi.c
> index 7dc07339b9579..dadf78f30b897 100644
> --- a/drivers/net/wireless/ath/ath11k/qmi.c
> +++ b/drivers/net/wireless/ath/ath11k/qmi.c
> @@ -3170,8 +3170,18 @@ static int ath11k_qmi_ops_new_server(struct qmi_handle *qmi_hdl,
>  	struct ath11k_qmi *qmi = container_of(qmi_hdl, struct ath11k_qmi, handle);
>  	struct ath11k_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 = ath11k_hif_get_qrtr_node_id(ab);
> +	if (node_id && service->node != node_id)
> +		return 0;
> +

[Severity: High]
Should this return a negative errno rather than 0 when the server belongs
to the other device's node?

The QMI core only drops the service record when ->new_server() returns a
negative value:

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

So returning 0 here keeps the foreign node's WLFW service on this handle's
lookup_results list, even though we deliberately did not connect to it.

When that foreign server later goes away (sibling firmware crash, unbind of
the other card, or its MHI endpoint unregistering), net/qrtr/ns.c
server_del()/ctrl_cmd_bye() call lookup_notify() for every lookup matching
the service/instance, which reaches this device's handle:

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 (ops->del_server)
			ops->del_server(qmi, svc);
}

ath11k_qmi_ops_del_server() does no node filtering and posts
ATH11K_QMI_EVENT_SERVER_EXIT unconditionally, which in
ath11k_qmi_driver_event_work() does:

		case ATH11K_QMI_EVENT_SERVER_EXIT:
			set_bit(ATH11K_FLAG_CRASH_FLUSH, &ab->dev_flags);
			set_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags);

			if (!ab->is_reset)
				ath11k_core_pre_reconfigure_recovery(ab);

Can this drive crash-flush and recovery on the healthy device whose own
firmware never went down?  Since the new filter also rejects the foreign
node's re-announcement, no matching SERVER_ARRIVE follows, so does that
adapter stay stuck with ATH11K_FLAG_CRASH_FLUSH and ATH11K_FLAG_RECOVERY
set until the driver is reloaded?

Would returning a negative errno here (so the core frees the record), and/or
adding the same node id check to ath11k_qmi_ops_del_server(), cover the
del_server side of the cross-device confusion as well?

>  	sq->sq_family = AF_QIPCRTR;
>  	sq->sq_node = service->node;
>  	sq->sq_port = service->port;

[ ... ]

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