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

[Wu. JackBB (GSM)]

Thanks for the review.  All five findings lead to code changes in v9,
all of them in this patch, which is where the code is introduced.

Two of them are answered by a deletion rather than by a check: looking
at the paths you point at, the stale list and the device-id ida cannot
run at all in this series, so v9 removes them instead of hardening
them.  Details under the first and fourth answers.

One answer below is long: the usr_cnt sequence, where we are changing
the code you pointed at but deliberately not changing the behaviour you
expected to change.  That one needs the full walk-through.

On Sun, 20 Sep 2026 07:54:00 +0800 Jakub Kicinski wrote:
> 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?

Will fix in v9 by deleting the helper, together with the rest of the
stale list.  Nothing in this series can put a port on that list:
mtk_port_free_or_backup() backs a port up only if it is not
PORT_TYPE_INTERNAL and still has PORT_S_OPEN set, and its only caller
runs ->disable() on every port first - which for the wwan ports patch
6/6 adds is wwan_remove_port() forcing stop() and clearing that bit.
The list is therefore always empty and the restore always a no-op.

So v9 removes mtk_port_restore_from_stale_list(), struct mtk_stale_list,
PORT_S_ON_STALE_LIST, the backup branch of mtk_port_free_or_backup(),
mtk_port_stale_list_{create,destroy,search,init,exit,grp_cleanup}(), the
port fields that only served them (stale_entry, dev_str, minor), and the
->reset port op, whose only call site is inside the restore.

> 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.

That goes with it: mtk_port_tbl_create() loses both the restore call and
its s_list parameter, so the loop allocates each entry of the cfg table
exactly once, and the only error it can report is
mtk_port_alloc_and_add()'s, which it already propagates.

> 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?

Will fix in v9.  Yes, and you are right that rcu_read_lock() is not the
way out.  The file already has the right accessor -
MTK_PORT_SEARCH_FROM_RADIX_TREE(), which uses rcu_dereference_raw() and
is what the other two iterators in this file use; mtk_port_tbl_destroy()
will use it too, along with the radix_tree_is_internal_node() guard it
is also missing.

> 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?

Will do both in v9: a struct mutex in mtk_port_mngr, taken inside
mtk_port_tbl_add() and mtk_port_tbl_del() so it covers the radix-tree
operation and the port_cnt update together; and a struct rcu_head in
struct mtk_port so mtk_port_release() can use kfree_rcu().  The kfree()
on the mtk_port_alloc_and_add() error path stays plain, since that port
never reached the tree.

> 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?

Yes.  In v9 the allocation is removed rather than checked: dev_id is
written once and read once, to hand it straight back to ida_free(), so
it names nothing in this series.  ccci_dev_ids, MTK_DFLT_MAX_DEV_CNT and
both dev_id fields are parked on the stale list and go with it.

That is also why nothing is observed today - ida_free() ignores negative
ids, so the errno never reached anything that looked at it.

> 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():

Will fix in v9 by removing it.  The flag and both of its users are
introduced by this patch, so the whole removal is here; the entries
patch 6/6 adds are simply written without it.

> 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?

There are two things here, and we are changing one of them.

The discarded return value: will fix in v9.
mtk_port_internal_disable() captures it and dev_warn()s on failure.

The behaviour: no change.  PORT_S_ENABLE is still cleared before the
call, and a failure still does not abort the teardown.  Both are
deliberate, and the rest of this answer is why - your sequence below is
exactly the case that would make them wrong, so it needs answering step
by step.

To the question as asked: within one power cycle, yes, usr_cnt can be
left counting a user.  Across cycles, no.

> 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.

Every step before this one is accurate - including that the flush path
completes the queued DISABLE without going through
mtk_ch_status_check(), so usr_cnt is left at 1 by that teardown.  What
does not follow is the next ENABLE raising it to 2: the count starts
each bring-up at zero.  mtk_pcie_hif_init() in mtk_trans_ctrl.c:

        for (i = 0; i < NR_CLDMA; i++) {
                for (j = 0; j < HW_QUE_NUM; j++) {
                        skb_queue_head_init(&trans->trans_list[i].skb_list[j]);
                        trans->trans_list[i].tx_burst_cnt[j] = 0;
                        /* usr_cnt tracks the queues rebuilt by mtk_cldma_init()
                         * below, so it must be reset with them. ...
                         */
                        trans->usr_cnt[i][j] = 0;
                }
        }
        ret = mtk_cldma_init(trans);

and that is ops->init, run on every FSM_STATE_ON.  The count and the
queues it counts are created and destroyed together, so a residue from a
torn-down cycle cannot reach the next one.  The comment is there because
this lifetime mismatch did escape us once, in an earlier version; the
reset is the fix for it.

The port-layer half of the state is reset just as unconditionally, and
that is what makes the discarded return value the right behaviour rather
than an oversight.  On FSM_STATE_OFF the port handler runs before the
HIF handler, and mtk_port_disable() walks every port through
ports_ops[]->disable().  Because mtk_port_internal_disable() clears
PORT_S_ENABLE with test_and_clear_bit() before the call that may fail,
every port enters the next cycle with the bit clear and re-issues its
ENABLE.  Honouring the return value - bailing before the clear, or
restoring the bit on failure - is what would produce the permanent
failure you describe: mtk_port_internal_enable() returns early when
PORT_S_ENABLE is set, so a port that kept the bit would never reopen its
channel on a device whose usr_cnt had meanwhile been reset to zero.

Within a single cycle the three failures differ: -EIO means submit_skb()
refused because trans->available is already 0, i.e. teardown is under
way; -ETIMEDOUT means the TRB is still queued and the kthread decrements
when it reaches it; only -ENOMEM leaves nothing to decrement, and then a
re-enable in the same cycle takes the count 0->1->2 against a queue that
really is open, mtk_ch_status_check() returns -EBUSY, and the port works.
The count is one too high until the next FSM_STATE_ON and nothing else
reads it.  The flush cannot unbalance it the other way: the increment
happens in mtk_ch_status_check(), which the flush path skips, so a
flushed ENABLE never counted.

> 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?

No, for the reason above: the next FSM_STATE_ON zeroes usr_cnt before
mtk_cldma_init() rebuilds the queues, so the handshake starts from a
clean count.

> 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.

Yes, it is safe, and no change is planned for this part.  The two do not
disagree; they describe different states, and both keep the same
invariant - usr_cnt equals the number of enables that a later disable
will match.

mtk_ch_status_check() increments, finds the count already above one,
confirms through mtk_cldma_check_ch_cfg() that the queues exist, and
returns -EBUSY keeping the reference.  The caller really is an
additional user of an open queue and its later DISABLE will decrement
it, so treating that -EBUSY as a successful enable is correct.

mtk_cldma_open() is only reached when status_check saw the count go 0->1,
that is, when this caller is supposed to be the first user and
drv_info->txq/rxq nevertheless already exist.  There the count and the
hardware disagree and nothing was opened, so rolling back at out: is
what keeps the invariant.

The -EINVAL branch of status_check keeps its increment, which is
balanced as well - by the compensating mtk_port_ch_disable() in
mtk_port_internal_enable() that the comment there describes.

One more disable path has the same discarded return: the one in
mtk_port_wwan_disable().  That function is added by patch 6/6, so the
identical dev_warn() goes there rather than here.  The compensating
mtk_port_ch_disable() calls on the enable paths keep ignoring their
return, since there the enable is already failing and it is that error
that gets propagated.