Re: [PATCH v8 4/6] net: wwan: t9xx: Add control port

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: wwan: t9xx: Add control port

This adds the t9xx control port layer: a port manager that creates and
destroys ports, keeps them in a global "stale list" across driver
re-creation, and moves data between port I/O and CLDMA via TRBs. It also
adds the internal port operations (open, close, write, recv_register) and
wires the port configuration into the control plane init path.

A few questions about the new code below.

[ ... ]

[ ... ]

[Severity: Medium]

mtk_port_restore_from_stale_list() reports failure through ERR_PTR(), but
mtk_port_tbl_create() discards the result. Should this be checked with
IS_ERR(), keeping in mind the success return is NULL rather than a port?

When mtk_port_tbl_add() -> radix_tree_insert() fails (-ENOMEM for a tree
node, or -EEXIST), the helper re-queues the current port, drops the extra
kref and returns early, so the remaining stale ports are never restored:
they keep PORT_S_ON_STALE_LIST set and port->port_mngr == NULL.

Since the error is swallowed, the loop below then finds nothing for those
rx_ch values:

	if (!mtk_port_search_by_id(port_mngr, dflt_port->rx_ch)) {
		port = mtk_port_alloc_and_add(port_mngr, dflt_port);

and creates a second port object for the same channel, while
mtk_port_mngr_init() still returns 0.

Does the user still holding the stale handle then end up with a port whose
port_mngr is NULL and PORT_S_ENABLE clear, so every mtk_port_send_data()
on it fails in mtk_port_status_check() with -ENODEV?

[Severity: Medium]

radix_tree_deref_slot() is rcu_dereference(*slot), and this loop runs in
plain process context (driver removal and the mtk_port_mngr_init() error
path) with no rcu_read_lock() held. With CONFIG_PROVE_RCU=y, does this
produce a "suspicious rcu_dereference_check() usage" splat on every
teardown?

Wrapping the loop in rcu_read_lock() would not work either, since
ports_ops[]->disable() sleeps: mtk_port_ch_disable() does a GFP_KERNEL
skb allocation and then wait_event_timeout() for up to
MTK_DFLT_TRB_TIMEOUT.

More generally, the synchronisation the radix-tree API asks callers to
provide looks incomplete here. The mutators run under port_mngr_grp_mtx
from mtk_port_restore_from_stale_list() and mtk_port_free_or_backup(), but
with no lock at all from mtk_port_tbl_create() -> mtk_port_alloc_and_add()
-> mtk_port_tbl_add(); the readers mtk_port_search_by_id() and
mtk_port_search_by_name() hold neither the mutex nor rcu_read_lock(), and
mtk_port_release() frees a tree-published object with a plain kfree():

	ports_ops[port->info.type]->exit(port);
	kfree(port);

Would a single writer lock covering all mutators (including the port_cnt
updates) plus RCU-deferred freeing be more appropriate?

[ ... ]

[Severity: Low]

The ida_alloc_range() return value is stored in *dev_id without being
tested, and mtk_port_stale_list_init() still reports success. On -ENOMEM,
or -ENOSPC once MTK_DFLT_MAX_DEV_CNT (10) devices are bound, does an errno
end up in port_mngr->dev_id while mtk_port_mngr_init() returns 0?

That value is later passed to ida_free() unconditionally in
mtk_port_stale_list_exit() below, while the other two ida_free() call
sites in this file guard it, in mtk_port_release():

	list_empty(&s_list->ports) && s_list->dev_id >= 0) {
		ida_free(&ccci_dev_ids, s_list->dev_id);

and in mtk_port_stale_list_grp_cleanup():

	if (s_list->dev_id >= 0)
		ida_free(&ccci_dev_ids, s_list->dev_id);

The bogus free itself is harmless, since ida_free() starts with
"if ((int)id < 0) return;", so this is about the unchecked allocation and
the inconsistency with the guarded call sites.

[ ... ]

[ ... ]

[Severity: Low]

This isn't a bug, but PORT_F_ALLOW_DROP has no reader anywhere. It is set
on every entry of port_cfg_m9xx[], yet the only runtime consumer of
port->info.flags is mtk_port_internal_write():

	return mtk_port_send_data(port, skb,
				  !!(port->info.flags & PORT_F_BLOCKING),
				  !!(port->info.flags &
				     (PORT_F_BLOCKING | PORT_F_FORCE_SEND)));

Ports with and without the flag behave identically. Could it be dropped
until the code that honours it is added?

+	PORT_F_FORCE_SEND = BIT(6),
+};

[ ... ]

[ ... ]

[Severity: High]

mtk_port_internal_disable() clears PORT_S_ENABLE and then discards the
return value of mtk_port_ch_disable(), which can fail with -ENOMEM (skb
allocation), -EIO (mtk_pcie_hif_submit_skb() whenever
atomic_read(&trans->available) == 0) or -ETIMEDOUT after
MTK_DFLT_TRB_TIMEOUT. Can this leave the software state claiming the
channel is down while trans->usr_cnt[hif_id][txqno] still counts a user?

The only decrement is in mtk_ch_status_check():

	case TRB_CMD_DISABLE:
		if (trans->usr_cnt[que->hif_id][que->txqno] > 0) {
			trans->usr_cnt[que->hif_id][que->txqno]--;
			if (!trans->usr_cnt[que->hif_id][que->txqno])
				break;
		}
		trb->status = -EBUSY;

and mtk_ctrl_ch_flush() / mtk_ctrl_chs_flush() complete queued
ENABLE/DISABLE TRBs with

	trb->status = -EIO;
	trb->trb_complete(skb);

without passing through mtk_ch_status_check() at all.

For a modem that stops responding during teardown:

FSM_STATE_OFF
  mtk_port_disable() -> mtk_port_ch_disable() returns -ETIMEDOUT, the
  DISABLE skb is still queued
  mtk_port_internal_disable() ignores it and clears PORT_S_ENABLE
ops->exit() -> mtk_pcie_hif_exit()
  mtk_cldma_exit() frees drv_info->txq[]/rxq[]
  mtk_ctrl_trb_srv_exit() -> kthread_stop() -> mtk_ctrl_chs_flush()
  completes the queued DISABLE with -EIO, usr_cnt stays at 1

On the next bootup the ENABLE raises usr_cnt to 2, so
mtk_ch_status_check() takes the "already open" branch,
mtk_cldma_check_ch_cfg() finds drv_info->txq/rxq == NULL and returns
-EINVAL, and mtk_port_ch_enable() fails. The compensating
mtk_port_ch_disable() in mtk_port_internal_enable() then only takes
usr_cnt from 2 back to 1, never to 0.

Does that make the control channel unusable for the rest of the probe, so
the MD/SAP handshake never completes and the modem never reaches
FSM_STATE_READY, with every later boot cycle repeating it?

Related question on the same function: is -EBUSY safe to treat as a
successful enable without distinguishing where it came from?
mtk_cldma_open() rolls the usr_cnt reference back at its out: label when
it returns -EBUSY, while mtk_ch_status_check() keeps it.

[ ... ]

These are the two descriptors that request the PORT_F_ALLOW_DROP behaviour
mentioned above.

[ ... ]