[PATCH v8 6/6] net: wwan: t9xx: Add AT & MBIM WWAN ports

[Wu. JackBB (GSM)]

Thanks for the review.  Seven of the eight findings lead to code changes
in v9.  The one that does not is the w_port publish order, so that answer
is the long one below.

On Sun, 20 Sep 2026 07:54:00 +0800 Jakub Kicinski wrote:
> Is this description accurate?  mtk_port_wwan_init() only calls
> mtk_port_struct_init(), mutex_init(&port->w_priv.w_lock) and picks
> w_priv.w_type; the wwan_create_port() call is in mtk_port_wwan_enable(),
> which is reached from the new FSM_STATE_READY case in
> mtk_port_mngr_fsm_state_handler().

Will fix in v9.  It is not accurate, and the message is rewritten to say
that init() only prepares the port object while enable() is what calls
wwan_create_port(), on the FSM_STATE_READY transition.

> Two other changes also seem worth a mention in the log:
>
> - the new mtk_port_enable_by_type(port_mngr, PORT_TBL_MD) hook runs
>   ports_ops[...]->enable() for every MD-table port that has
>   port->enable set, including PORT_TYPE_INTERNAL ports, so this changes
>   behaviour on a shared code path rather than only adding two ports.
>
> - two new CLDMA1 hardware queues (TXQ(5)/RXQ(5) and TXQ(2)/RXQ(2)) are
>   added to mtk_queue_info_m9xx[].

Both go into the same rewrite.  On the first one, there is a detail worth
having in the log rather than just the fact: port->enable is not a driver
constant.  mtk_port_status_update() sets it from MTK_INFO_FLD_EN in a
device message, so which ports that hook touches is chosen by the modem,
not by the table.

> Is the "all or nothing" claim in this comment true beyond the queue-full
> case?  force_send only suppresses the -EAGAIN branch in
> mtk_pcie_hif_submit_skb():
>
>         if (mtk_queue_list_is_full(trans, que) && !force_send) {
>                 ret = -EAGAIN;
>
> On the second and later iterations these paths still return early after
> previous packets of the same message were already submitted:
>
> - mtk_port_status_check() returning -ENODEV/-EBADF if a concurrent
>   close or disable clears PORT_S_OPEN/PORT_S_WR or sets PORT_S_FLUSH
> - __dev_alloc_skb(port->tx_mtu, GFP_KERNEL) returning NULL (-ENOMEM)
> - mtk_port_send_data() returning -EIO when
>   !atomic_read(&trans->available), or -EINVAL on queue lookup failure,
>   or a negative trb->status in blocking mode

Will fix in v9.  The claim is not true, and the fix is to the code rather
than to the comment; see the next answer.  One correction to the list:
mtk_port_send_data() cannot return -ETIMEDOUT either way, because the
blocking wait loops back to start_wait on timeout instead of giving up.

> Can this truncate an AT or MBIM message on the wire?  For any write
> larger than tx_mtu - sizeof(struct mtk_ccci_header) (about 3440 bytes
> with Q_MTU_3_5K), the leading packets are already submitted when this
> returns.  mtk_port_wwan_tx() forwards the errno and the core does:
>
> net/wwan/wwan_core.c:wwan_port_fops_write() {
>         ...
>         ret = wwan_port_op_tx(port, head, ...);
>         if (!ret)
>                 return count;
>
> freeskb:
>         kfree_skb(head);
>         return ret;
> }
>
> so userspace is told the whole write failed and will normally re-send
> the entire message, duplicating the prefix the modem already received.
> Would it make sense to keep going until the message is complete, or to
> build and submit the fragments only after all of them are allocated?

Will fix in v9, by the second option.  The loop splits in two: every
packet is allocated and filled onto a local list first, and only then are
they submitted, with force_send false for the first one so a full queue
still yields -EAGAIN with nothing sent.

That moves the allocation failure and the skb_copy_bits() failure ahead
of the first submit, and mtk_port_status_check() is done once before the
build instead of once per packet.  What stays reachable after the first
submit is -EIO, -EINVAL and a negative trb->status, all of which mean the
channel is going away - so the rewritten comment claims only that, and
the words "all or nothing" go.

> Can this leave a wait entry pointing into freed memory?  port->trb_wq is
> embedded in struct mtk_port, but struct mtk_port is freed while the
> userspace fd (and the poll/epoll entry registered here) is still alive.
>
> On device removal:
>
> mtk_pci_dev_exit() -> mtk_port_mngr_exit() -> mtk_port_tbl_destroy() {
>         ...
>         ports_ops[port->info.type]->disable(port);
>         ...
>         mtk_port_free_or_backup(port_mngr, port, s_list);
> }
>
> mtk_port_wwan_disable() calls wwan_remove_port(), which forces stop()
> even with an open fd:
>
> net/wwan/wwan_core.c:wwan_remove_port() {
>         mutex_lock(&port->ops_lock);
>         if (port->start_count) {
>                 port->ops->stop(port);
>                 port->start_count = 0;
>         }
>         port->ops = NULL;
>         ...
> }
>
> stop() is mtk_port_wwan_close(), so mtk_port_common_close() clears
> PORT_S_OPEN and mtk_port_put_locked() drops the open-time kref.  The
> stale-list guard in mtk_port_free_or_backup() is then already false:
>
>         if (port->info.type != PORT_TYPE_INTERNAL) {
>                 if (test_bit(PORT_S_OPEN, &port->status)) {
>                         list_add_tail(&port->stale_entry, &s_list->ports);
>                 ...
>                 kref_put(&port->kref, mtk_port_release);
>
> so mtk_port_release() -> kfree(port) frees the waitqueue head.  The fd
> stays valid because the core only NULLs port->ops, and nothing calls
> wake_up_pollfree() on trb_wq.  When the entry is torn down later
> (poll_freewait() -> free_poll_entry() -> remove_wait_queue(), or
> ep_remove_wait_queue() at EPOLL_CTL_DEL/close) the spin_lock and
> list_del operate on freed memory.  With epoll the dangling entry
> persists until the fd is closed.

Will fix in v9.  mtk_port_wwan_disable() calls
wake_up_pollfree(&port->trb_wq) right after wwan_remove_port() returns.
The RCU delay that wake_up_pollfree() asks its callers for lands in
patch 4/6, where struct mtk_port and mtk_port_release() are introduced:
mtk_port_release() there frees the port with kfree_rcu() instead of
kfree().

One note on the code you quote: v9 removes the stale list entirely, in
patch 4/6 - it is unreachable for the reason your own trace shows, that
disable() always clears PORT_S_OPEN before that test runs.

> Should negative returns be distinguished from "queue full" here?  The
> callee mixes a boolean with error codes on the same int:
>
> drivers/net/wwan/t9xx/pcie/mtk_trans_ctrl.c:mtk_pcie_hif_cmd_func() {
>         case HIF_CTRL_CMD_CHECK_TX_FULL:
>                 ...
>                 if (!atomic_read(&trans->available)) {
>                         ret = -EIO;
>                         break;
>                 }
>                 ...
>                 ret = mtk_queue_list_is_full(trans, que);
> }
>
> If the transport becomes unavailable before the FSM disables the port,
> mtk_port_status_check() still passes, send_cmd() returns -EIO and the
> mask stays 0, so a poll()ing writer is told the port is neither
> writable nor in error.  Would testing for > 0 as full and mapping < 0
> to EPOLLERR be more accurate?

Will fix in v9 - the return is latched into a local, negative maps to
EPOLLERR | EPOLLHUP the way the mtk_port_status_check() line above it
already does, and only zero sets EPOLLOUT | EPOLLWRNORM.  The callee's
signature is left alone; it is patch 3/6 code with other callers, and
this is the caller misreading it.

> This isn't a bug today, but the state bits and the w_port pointer are
> published in the opposite order from how the consumers read them.
> wwan_create_port() registers the char device, so userspace can open the
> node and set PORT_S_OPEN while w_priv.w_port is still NULL; an RX frame
> arriving in that window is dropped with -ENXIO by mtk_port_wwan_recv().
> mtk_port_wwan_disable() also relies on PORT_S_ENABLE alone as proof
> that w_port is valid and passes it to wwan_remove_port() unchecked,
> which holds only because both enable and disable run from the single
> FSM kthread (mtk_fsm_evt_handler() -> mtk_fsm_switch_state() ->
> mtk_ctrl_fsm_state_listener()).  Would it be more robust to store
> w_port before setting PORT_S_ENABLE/PORT_S_WR, and to NULL-check
> w_port in the disable path?

No fix planned, and the comment above the set_bit() pair gets one more
sentence so the next reader does not have to work this out again.

On the order: w_port is the return value of wwan_create_port(), and the
bits have to be set before that call because the node becomes openable
inside it - device_add() publishes it and ModemManager opens immediately
- and mtk_port_common_open() rejects a port without PORT_S_ENABLE.  That
ordering is itself the v7 fix for exactly that race.  So there is no
order in which the pointer precedes the bits; only one in which the
pointer is stored earlier relative to the call, and there is no such
point.

The window you describe is real, and its only effect is the -ENXIO drop.
Reaching it needs userspace to open the node and the modem to answer
within the few instructions between wwan_create_port() returning and the
store, and AT and MBIM are solicited: the response follows a write that
follows the open.

We did consider holding w_lock across wwan_create_port(), which would
close the window - mtk_port_wwan_recv() would block until the pointer is
stored, and nothing on the open path takes w_lock, so it does not
deadlock.  We are not proposing it, because it puts a driver mutex around
a core registration call that does device_add(), cdev_add() and a uevent,
which creates a lock-ordering edge against wwan core internals for no
behavioural gain.  Saying so here rather than leaving it implied.

On the NULL check: it cannot fail.  mtk_port_wwan_disable() returns early
unless test_and_clear_bit(PORT_S_ENABLE) succeeds, PORT_S_ENABLE is set
only on the path that goes on to store w_port, and it is cleared again on
the single failure path between them.  Your reasoning about the FSM
kthread is right as far as it goes, but the removal path gives a stronger
guarantee than serialisation: mtk_pci_dev_exit() calls mtk_fsm_stop(),
which is kthread_stop(), before mtk_trans_ctrl_exit() -> mtk_ctrl_exit()
-> mtk_port_mngr_exit() -> mtk_port_tbl_destroy() -> disable().  The FSM
thread is not just serialised with the last disable(), it is already
gone.  A check whose only possible outcome is "not NULL" would suggest a
second caller exists.

> Should this clear be followed by a wake_up_all(&port->trb_wq)?
> PORT_S_WR is part of the blocking TX wait condition:
>
> drivers/net/wwan/t9xx/mtk_port.c:mtk_port_send_data() {
>         ret = wait_event_interruptible_timeout(port->trb_wq,
>                                                trb->status <= 0 ||
>                                                test_bit(PORT_S_FLUSH, &port->status) ||
>                                                !test_bit(PORT_S_WR, &port->status),
>                                                MTK_DFLT_TRB_TIMEOUT);
> }
>
> mtk_port_common_close() wakes both queues when it sets PORT_S_FLUSH,
> but here the flag change is silent.  If mtk_port_ch_disable() fails
> early (for example its __dev_alloc_skb() returns NULL) and no TRB
> completion wakes the queue, the sleeper only re-evaluates after
> MTK_DFLT_TRB_TIMEOUT, while wwan_remove_port() below waits on the same
> port->ops_lock the writer holds via wwan_port_op_tx().

Will fix in v9 - wake_up_all(&port->trb_wq) after the clear.  It is
slightly worse than you describe: on timeout the wait goes back to
start_wait rather than returning, so the sleeper re-evaluates every
MTK_DFLT_TRB_TIMEOUT and never gives up on its own.

The same clear is in mtk_port_internal_disable(), which patch 4/6
introduces, so the same wake goes there.  The clear on the
wwan_create_port() failure path needs no wake - the node was never
created, so there is no writer.

> Can a frame be queued after the core has purged the rx queue, and then
> be read by the next opener?  PORT_S_OPEN is tested outside w_lock here,
> and the close path clears it without taking w_lock:
>
> mtk_port_wwan_close() -> mtk_port_common_close() {
>         clear_bit(PORT_S_OPEN, &port->status);
>         ...
> }
>
> net/wwan/wwan_core.c:wwan_port_op_stop() {
>         if (!port->start_count) {
>                 if (port->ops)
>                         port->ops->stop(port);
>                 skb_queue_purge(&port->rxq);
>         }
> }
>
> The CLDMA rx context never takes ops_lock, so this interleaving looks
> possible:
>
>   rx: test_bit(PORT_S_OPEN) -> true
>   close: mtk_port_common_close() clears PORT_S_OPEN
>   close: wwan_port_op_stop() does skb_queue_purge(&port->rxq)
>   rx: mutex_lock(&w_lock), w_port still non-NULL, wwan_port_rx()
>
> The skb then stays on w_port->rxq, and mtk_port_common_open() only
> purges the driver-side list:
>
>         skb_queue_purge(&port->rx_skb_list);
>         set_bit(PORT_S_OPEN, &port->status);
>
> so the next opener of the AT or MBIM node reads a response belonging to
> the previous session.  Would checking PORT_S_OPEN under w_lock (and
> clearing it under the same lock) close this?

Will fix in v9, and yes to both halves.  mtk_port_wwan_recv() moves the
PORT_S_OPEN test inside the w_lock section it already takes, and
mtk_port_wwan_close() takes w_lock around mtk_port_common_close() so the
clear happens under it - that keeps mtk_port_common_close() itself
unchanged, which matters because the internal ports share it and have no
w_lock.

Then any receive that saw the bit set has finished its wwan_port_rx()
before the clear, so before stop() returns, so before the core purges.
One ordering becomes load bearing with this: mtk_port_wwan_disable() must
keep dropping w_lock before it calls wwan_remove_port(), which takes
ops_lock and calls stop().  It does today; it will be in the change
description so it is not tidied away later.