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

["Wu. JackBB (GSM)" <JackBB_Wu@compal.com>]

Thanks for the review.

On Sun, 4 Oct 2026 17:12:00 +0800 netdev-bot+sashiko@kernel.org wrote:

> This isn't a bug, but the err_free_mem label frees nothing, because
> ctrl_blk comes from devm_kzalloc(). Also, mdev->ctrl_blk still points at
> ctrl_blk after the failure. Could this just return err, or clear
> mdev->ctrl_blk and use a label name that matches what it does?
[...]
> mtk_port_mngr_init() and mtk_port_tbl_create() only read the table (via
> memcpy() in mtk_port_alloc_and_add()). Could they take a
> const struct mtk_port_cfg * so the cast isn't needed?

Will fix in v10, both halves: the label goes, the failure path clears
mdev->ctrl_blk and returns err directly, and the cfg pointer is const
from struct mtk_port_layer_cfg down through mtk_port_mngr_init(),
mtk_port_tbl_create() and mtk_port_alloc_and_add(), so the cast
disappears.

> This isn't a bug, but port_mngr_grp_mtx, ports_ops[] (in mtk_port_io.c)
> and struct port_ops are all global and have no driver prefix. With
> CONFIG_MTK_T9XX=y they become kernel-wide symbols. The single mutex is
> also shared by every device instance.
[...]
> Could they get an mtk_ prefix, or be made static where possible?

Will fix in v10: struct port_ops becomes struct mtk_port_ops,
port_mngr_grp_mtx becomes mtk_port_mngr_grp_mtx, and ports_ops[] becomes
a static table in mtk_port_io.c reached through an accessor, which is
what the extern in mtk_port.h was there for.

> Can this return MTK_TRB_HEADER_ADDED as a byte count?
[...]
> In that case PORT_S_FLUSH is clear and the TX TRB has not completed yet.
> The code then returns trb->status, 712173, as if that many bytes had
> been written.
[...]
> Later in the series this can happen for a blocking WWAN write during
> mtk_port_wwan_disable(). Should the !PORT_S_WR case return an error
> instead?

Will fix in v10, and yes to the last question.  The header marker moves
out of trb->status into a new trb->hdr_added, so trb->status carries only
a status; and the !PORT_S_WR exit returns -EBADF, which is what
mtk_port_status_check() already returns when that bit is clear on entry,
so racing the disable and losing it produce the same errno.

The PORT_S_FLUSH exit keeps returning len.  That one is the modem-reset
path, where the port did accept the data, and P6's WWAN write treats a
short count as a hard error.

> Can mtk_port_ch_enable() return 0 before mtk_port_open_trb_complete()
> has filled in the port geometry?
[...]
> wait_event_timeout() checks trb->status <= 0 before it sleeps. A waiter
> that checks between those two points, or while the trb_srv thread is
> preempted inside the callback, returns 0 with port->tx_mtu still 0.
> There is also no acquire/release pairing, so on weakly ordered CPUs the
> MTU stores may not be visible yet.
[...]
> Would it be better to signal completion from
> mtk_port_open_trb_complete() after its last store? A struct completion,
> or a separate done flag using smp_store_release()/smp_load_acquire(),
> would do that.

Will fix in v10 with the done flag rather than the struct completion -
32 of skb->cb's 48 bytes are already in use, so a struct completion does
not fit, and the flag gives the acquire/release pair you ask for in the
same change.  struct trb gains a bool done, set with
smp_store_release() after the last store in all three completion
callbacks; mtk_port_ch_enable(), mtk_port_ch_disable() and
mtk_port_send_data() wait on smp_load_acquire(&trb->done) instead of
trb->status <= 0.

Both halves of your diagnosis are right, and they are two defects rather
than one: the transport publishes the status before it calls the
callback, and there is no ordering even when the two happen in the
intended order.  The flag closes both, and it is also what the previous
comment needs - with "completed" in trb->done, trb->status stops being a
three-valued field.

> If the ENABLE TRB completes with -EBUSY, does the port end up enabled
> with zero MTU and fragment sizes?
[...]
> This function treats -EBUSY as success, so it sets PORT_S_WR and
> PORT_S_ENABLE. If this port never had a successful ENABLE, tx_mtu,
> rx_mtu and the frag sizes stay 0.
[...]
> Should the geometry also be copied on -EBUSY, or should -EBUSY not
> count as a successful enable?

Will fix in v10 with the first of the two: mtk_port_open_trb_complete()
copies the geometry when the status is 0 or -EBUSY.  Both producers you
list have filled trb_open_priv with the real queue's numbers by the time
they complete, so there is nothing wrong with the values - only with the
condition that skips them.

-EBUSY stays a successful enable.  It means another user already owns the
queue, usr_cnt is balanced across the caller's ENABLE/DISABLE pair rather
than inside the check, and the port does own a share of the channel; this
is the same position as our answer to your usr_cnt comment on the CLDMA
patch.  Turning -EBUSY into a failure would make
mtk_port_internal_enable() issue a compensating DISABLE for a count it
still holds.

> The port still believes it owns the channel, so its later DISABLE would
> also complete with -EBUSY.

Agreed, and that one is a separate defect in mtk_cldma_open() rather than
here: it rolls usr_cnt back while telling the caller the channel is
already open, so the owner's DISABLE later sees one user too few.  We are
fixing it in the CLDMA patch, next to the usr_cnt answer above, so the
increment and the rollback stay in one commit.

> The commit message leaves out two changes to existing code.
[...]
> They are still empty at the end of the series. Could module_pci_driver()
> stay and the stubs be dropped?
[...]
> This makes the transport layer depend on the port layer's release
> function. Could the commit message describe this change to TRB
> ownership?

Will fix in v10, and yes to both questions - the first by deletion rather
than by documentation.  mtk_port_io_init()/mtk_port_io_exit() are the
remains of a cdev registration that is not in this series, so they go,
mtk_drv_init()/mtk_drv_exit() go with them and module_pci_driver() comes
back; that removes the hunk from the diff instead of explaining it.

The commit message gains a paragraph on the second: struct trb now
carries a kref, the port layer owns mtk_port_trb_free(), and
mtk_ctrl_trb_handler() holds a reference across the dispatch so a
completion cannot free the skb under the handler.  The kref member itself
also moves from the CLDMA patch into this one, which is the other change
that paragraph has to describe.

> Does anything keep the port returned here alive until
> mtk_port_get_locked() takes its reference?
[...]
> If the free runs between the lookup and the mutex_lock() in
> mtk_port_get_locked(), then kref_get() and mtk_port_common_open() use
> freed memory. The tree walk itself can also see nodes that a
> concurrent radix_tree_delete() is freeing.
[...]
> Could the lookup and kref_get() be done inside one port_mngr_grp_mtx
> section? Another option is rcu_read_lock() with
> kref_get_unless_zero(). Also, the rcu_dereference_raw() in
> MTK_PORT_SEARCH_FROM_RADIX_TREE hides this from lockdep.

Will fix in v10 with your first option, for mtk_port_internal_open().  A
new mtk_port_get_by_name() takes mtk_port_mngr_grp_mtx once and does the
walk, the type check and the kref_get() inside it;
mtk_port_search_by_name() gains lockdep_assert_held() so the
rcu_dereference_raw() stops hiding the requirement, and
mtk_port_get_locked() is deleted - its !port test is, as you say, a
re-test of a value the caller already has, and it is on the wrong side of
the mutex.  P6's mtk_port_wwan_open() has the same shape and gets the
same change in that patch.

> mtk_port_search_by_id() has the same problem. That includes the RX
> path in mtk_port_rx_dispatch(), which uses the port without taking any
> reference.

This half we are leaving as it is, because the race is not reachable:
the transport is quiesced before any port can be freed.  In
mtk_pci_dev_exit() the order is mtk_fsm_stop(), which parks the FSM
thread, then mtk_trans_ctrl_exit(), which is mtk_pcie_hif_exit()
followed by mtk_ctrl_exit().  mtk_pcie_hif_exit() stops the TRB kthreads,
masks the CLDMA queue interrupts, unregisters and synchronises the IRQ
and flushes the workqueues; only after all of that does mtk_ctrl_exit()
reach mtk_port_mngr_exit() -> mtk_port_tbl_destroy() -> mtk_port_free().
There is no RX work item in existence when the first kref_put() runs.
The FSM_STATE_OFF path frees no ports at all.

So mtk_port_search_by_id()'s caller cannot observe a port being freed,
and adding rcu_read_lock() plus kref_get_unless_zero() there would
protect against something the teardown order already excludes.  We would
rather not add a second lifetime mechanism that is never the one doing
the work; if the ordering ever changes, the RX path changes with it.

> Can mtk_port_add_header() hit skb_under_panic() here?
[...]
> Should this call skb_cow_head(skb, sizeof(*ccci_h)) and return an
> error on failure? If so, mtk_port_send_data() would need to stop
> ignoring the return value of mtk_port_add_header(). The port->tx_seq--
> in its error path would also have to be skipped when no sequence
> number was consumed.
[...]
> Or, if the API requires callers to reserve the headroom, could that be
> documented and checked here?

Will fix in v10 exactly as the middle paragraph describes: skb_cow_head()
with its return value checked, mtk_port_send_data() propagating
mtk_port_add_header()'s error, and the tx_seq rollback conditional on a
new trb->seq_taken.  We are not documenting a headroom contract instead
(your last question), because skb_cow_head() makes the contract
unnecessary and an unchecked documented contract is what we would be
asked about next round.

One correction to the premise, since it decides the severity rather than
the fix.  skb_under_panic() is not reachable here, in this patch or at
the end of the series.  The ccci header is 16 bytes and every skb that
reaches this skb_push() comes from __dev_alloc_skb() - NET_SKB_PAD, at
least 32 bytes - in mtk_port_ch_enable()/ch_disable() and in the FSM's
HS1/HS3 messages, or from mtk_port_common_write() in P6, which does an
explicit skb_reserve(skb, sizeof(struct mtk_ccci_header)).  None of them
is cloned, so the shared-data case does not arise either.  The internal
user that calls alloc_skb() with no reserve does not exist yet, which
your wording already allows for.

What is a live defect is the sentence you reach second: the discarded
return value.  mtk_port_add_header() already fails with -EINVAL when
trb->priv is NULL, and that error is thrown away today, so the skb would
go to the transport with no header at all.  The unconditional tx_seq--
is the other one: a failure before the FIELD_PREP decrements a counter
that was never incremented, and every later packet on that port is
reported out of order by the peer.  Both are fixed by the change above.

Thanks,
Jack