[PATCH v7 0/6] net: wwan: t9xx: Add MediaTek T9XX WWAN driver

[Wu. JackBB (GSM)]

On Fri, 28 Aug 2026 21:27:00 +0000 Jakub Kicinski <kuba at kernel.org> wrote:

> On Fri, 28 Aug 2026 17:17:17 +0800 Jack Wu via B4 Relay wrote:
> > Subject: [PATCH v7 0/6] net: wwan: t9xx: Add MediaTek T9XX WWAN driver
>
> net-next is closed. Please a week from now when it's open.
> Before you repost please fix the bugs pointed out by AI:
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260828-t9xx_driver_v1-v7-0-bf8f6074d88a%40compal.com
> This link will only reveal the results next week per the embargo message, wait
> for that to happen of course.

All findings from the AI ​​review are addressed below,  and our responses.
Because there are many projects, I try to minimize the number of lines in each question.

Subject: Re: [PATCH v7 1/6] net: wwan: t9xx: Add PCIe core

netdev-ai wrote:
> Can a hardirq observe a valid callback together with an already-cleared data
> pointer here?  mtk_pci_unregister_irq() clears the two fields as separate
> stores, and mtk_pci_irq_handler() loads them as separate loads:
> ...
> Would masking plus synchronize_irq() inside
> mtk_pci_unregister_irq(), before either field is cleared, close this?

Will fix in v8.  Will mask the source and call synchronize_irq() inside
mtk_pci_unregister_irq() before either field is cleared.  The smp_wmb()
between the two stores then goes away, and the redundant mask +
synchronize_irq() in mtk_cldma_dev_exit() is removed in patch 3, which
introduced it.

> Separately, the "already been register" test in mtk_pci_register_irq() reads
> irq_cb_list[] with no lock held.  Can two registrations for the same irq_id
> both pass that test and overwrite each other's data pointer?

Will fix in v8.  Will add a spinlock in struct mtk_pci_priv covering
both arrays, held across the test and the stores in register and across
the clearing in unregister.  The register-side smp_wmb() stays, since
the hardirq reader does not take the lock.

Not reachable today - both registration sites are serialised - but the
test-and-set is not atomic and what makes it safe lives in a different
patch.

> The two translators map three D2H channels and one H2D channel, while enum
> mtk_pci_evt_d2h and enum mtk_pci_evt_h2d in mtk_pci.h declare roughly
> fourteen and eleven channels, and mtk_pci_reg.h already defines the matching
> hardware bits (for example MHCCIF_EP2RC_EVT_PCIE_PM_SUSPEND_ACK).
> ...
> Should the unmapped enum values be dropped until they get a translation, or
> should the mask/unmask/clear helpers report the unmapped case the way
> mtk_pci_send_ext_evt() does?

Will fix in v8, by the first option.  enum mtk_pci_evt_d2h and enum
mtk_pci_evt_h2d will be deleted from mtk_pci.h in their entirety and the
"PM" claim removed from the commit message.

Dropping rather than reporting because the EXT_EVT_* constants have no
user across the whole series; the four channels actually used are in
enum mtk_dev_evt_{d2h,h2d} and all four are translated, so once the dead
enums are gone an -EINVAL return would be unreachable code.

> Both evaluations fail with the same -EFAULT, so a caller cannot tell "device
> untouched" from "device left powered off".  mtk_pci_remove() treats both the
> same and falls back to mtk_pci_reset(mdev, RESET_MHCCIF), which writes the
> device's MHCCIF doorbell via mtk_pci_write32() with no presence check ...

Will fix in v8.  mtk_pci_pldr() will return -EIO when PXP._OFF failed
(device untouched) and -ENXIO when PXP._ON failed (device powered off),
documented in its kernel-doc.  remove() will take the MHCCIF fallback
only when mtk_pci_link_check() passes, and will check and log its return
value.

> Does anything reach RESET_FLDR?  Through the end of the series
> mtk_pci_reset() is only called from mtk_pci_remove(), with RESET_MHCCIF, so
> mtk_pci_fldr() and its ACPI _RST handling have no caller.  Could that be
> added along with its first user?

Will fix in v8.  Will delete mtk_pci_fldr() and its _RST block, and with
the only call site using one reset type the dispatch layer goes too:
enum mtk_reset_type, mtk_pci_dev_reset() and mtk_pci_reset().  remove()
will call mtk_pci_send_ext_evt() directly.  FLDR returns with the error
recovery that needs it.

> Does this leave the MHCCIF device-side status latched?  The work reads
> MHCCIF_EP2RC_SW_INT_STS, dispatches callbacks, and then clears and unmasks
> only the level 1 MSI-X bit.  mtk_pci_clear_ext_evt() is the only writer of
> MHCCIF_EP2RC_SW_INT_ACK and it has no caller in this patch ...

Will fix in v8.  mtk_mhccif_isr_work() will acknowledge every asserted
unmasked bit before dispatching, as t7xx does, then read back before
clearing level 1.  This needs a new raw-bits ack helper, since
mtk_pci_clear_ext_evt() takes channels and cannot express a bit that has
no channel.

> mtk_mhccif_init() also neither clears pending EP2RC status nor programs
> MHCCIF_EP2RC_SW_INT_EAP_MASK to a known value before probe unmasks the level
> 1 vector.  Is the mask register left in whatever state the firmware chose?

Will fix in v8.  mtk_mhccif_init() will write U32_MAX to EAP_MASK_SET
and acknowledge U32_MAX, so the block is in a known state before probe
unmasks the vector; consumers unmask the channels they need.

> Is bar[] used anywhere?  The six values are never read afterwards and the
> PCIBIOS status from pci_read_config_dword() is discarded, while the rest of
> the function works from pcim_iomap_table().  Can this loop be dropped?

Will fix in v8.  Will drop the loop, the bar[] array, the loop variable
and MTK_PCI_BAR_NUM, which has no other user.

> drivers/pci/devres.c documents both pcim_iomap_regions() and
> pcim_iomap_table() as deprecated, with "Do not use it in new code", and
> directs new code to pcim_iomap_region().  Only BAR 0/1 and BAR 2/3 are
> needed here - would two pcim_iomap_region() calls do instead?

Will fix in v8.  Will rewrite mtk_pci_bar_init() around two
pcim_iomap_region() calls with IS_ERR() checks, and delete
MTK_REQUESTED_BARS with its last user.  The NULL tests go with them.

> irq_state has already been narrowed to this vector's single bit by the
> irq_desc->msix_bits line above, and irq_desc[i].msix_bits is BIT(i).  For
> vector 30 that makes "irq_state & ~BIT(30)" equal to 0, so this write masks
> nothing ...
> If the device ever asserts source 30, does that leave it asserted and
> unmasked while the handler claims it, so it re-enters repeatedly with a
> dev_err() each time?  What is the BIT(30) exemption meant to cover ...

Will fix in v8.  Will drop the BIT(30) exemption outright, and have
mtk_pci_irq_handler() clear the source in the no-callback branch and use
dev_err_ratelimited().

On what it was meant to cover: it is our own v7 regression.  In v6
irq_state was still the whole status word at that point, so "& ~BIT(30)"
did exempt source 30 while masking the others; the v7 change that scoped
irq_state to irq_desc->msix_bits made it a no-op and we failed to remove
it.  We have no record of what source 30 was for and no supported device
maps it, which is why it is dropped rather than reinstated.

> Does this masking ever run?  mtk_dev_cfg_0900 is the config for both entries
> of t9xx_pci_table and sets only .flag = MTK_CFG_PM_SW_IRQ, so
> MTK_CFG_IRQ_DFLT_MASK is never set and the write is skipped on every
> supported device.
> ...
> Should mtk_dev_cfg_0900 set MTK_CFG_IRQ_DFLT_MASK, or should the flag and
> this conditional go away?

Will fix in v8, by the second option: the write becomes unconditional
and moves directly in front of mtk_pci_request_irq().
MTK_CFG_IRQ_DFLT_MASK, MTK_CFG_PM_SW_IRQ, MTK_CFG_DISABLE_AP_DRM, the
now valueless "flag" member, istatus_host_ctrl_addr and
REG_ISTATUS_HOST_CTRL_NEW are all deleted.

Unconditional rather than setting the flag because masking every source
before the handlers are installed is not a per-device quirk.

> pci_save_state() can return -ENOMEM when a capability save buffer was never
> allocated, since pci_allocate_cap_save_buffers() returns void and only logs
> its failures, and it sets pdev->state_saved before saving capability state.
> ...
> Should the pci_save_state() error be propagated, so probe does not succeed
> with a partial snapshot that pci_load_and_free_saved_state() later feeds
> back?

Will fix in v8.  Will check the pci_save_state() return value and fail
probe with it.  The store-failure errno also changes from -EFAULT to
-ENOMEM, which is the only reason pci_store_saved_state() returns NULL.

> Is the device quiesced at this point?  Only the MHCCIF vector is masked here
> and the other 31 MSI-X handlers are still installed.
> ...
> Should the reset be done after the interrupt resources are released, or
> should removal not reset the device at all?

Will fix in v8, by the first option.  remove() will mask all 32 sources,
then run mtk_mhccif_exit(), mtk_pci_free_irq(), pci_clear_master(), and
only then the reset, followed by pci_load_and_free_saved_state().  The
separate mask + cancel_work_sync() at the top becomes redundant and is
removed, and patch 5's hunk that inserts mtk_pci_dev_exit() after the
PLDR is respun so it is the first statement of remove().

> mtk_pci_pldr() takes its ACPI handle from pci_upstream_bridge(), so every
> unbind or rmmod evaluates PXP._OFF on the bridge, sleeps 500 ms, then
> evaluates PXP._ON.
> ...
> Could it mention that removal power-cycles the slot and adds half
> a second to unbind, and is the unconditional power cycle on every remove
> intended?

Will fix the commit message in v8: it will state that removal evaluates
PXP._OFF on the upstream bridge, waits 500 ms and evaluates PXP._ON,
that unbind therefore takes at least half a second, that this induces a
link down/up on the port, and that the driver assumes the modem is the
only endpoint behind that port.  No code change.

The power cycle is intended and unconditional on purpose: the T9xx has
no software reset that returns the modem from running firmware to a
state the next probe can hand firmware to again, so leaving it running
across unbind means the next bind finds it mid-session and the boot
handshake never completes.  PXP is the platform-provided handle for that
rail.

On the shared-resource premise: this driver targets laptops, where the
modem sits in the single M.2 WWAN slot and PXP is scoped to that slot,
so nothing else is behind that port.  A platform that shared the rail
would need its own enablement and its own validation, which is not
something we can do from here.

> cfg.type is set here and to ATR_AXI2PCI in the loop below, but
> mtk_pci_setup_atr() never reads cfg->type - it programs the registers from
> transparent, size, src_addr, trsl_addr, port, table, trsl_param and trsl_id
> only.  enum mtk_atr_type and the type member of struct mtk_atr_cfg exist
> solely to feed this ignored field; should they be dropped, or used?

Will fix in v8, by dropping them: enum mtk_atr_type, the type member of
struct mtk_atr_cfg and both assignments are deleted.  The direction of a
window is already determined by the source port and the translation
target, so there is nothing for the field to be used for.


Subject: Re: [PATCH v7 2/6] net: wwan: t9xx: Add control plane transaction layer

netdev-ai wrote:
> This isn't a bug, but should mtk_ctrl_init() and mtk_ctrl_exit() use
> EXPORT_SYMBOL_GPL() instead?
> ...
> Both symbols look like a private interface between the two halves of one
> driver (mtk_t9xx.ko and mtk_t9xx_pcie.ko) rather than a general kernel
> API, so the plain EXPORT_SYMBOL() also makes them available to non-GPL
> modules.

Will fix in v8.  Both become EXPORT_SYMBOL_GPL().  Your reading of the
interface is right - the only cross-module callers are in
pcie/mtk_trans_ctrl.c, and mtk_t9xx_pcie.ko is MODULE_LICENSE("GPL")
too, so nothing else changes.

> The same pattern shows up for the exports added in the later patches of
> the series (mtk_fsm_* and mtk_port_trb_free), and the two exports here are
> still EXPORT_SYMBOL() at the end of the series, so a follow-up conversion
> would be needed there as well.

Will fix in v8, in each patch that adds the export rather than in a
follow-up: two here, mtk_port_trb_free in patch 4, and the mtk_fsm_*
ones in patch 5.  Two are deleted instead of converted -
mtk_fsm_notifier_register() and mtk_fsm_notifier_unregister() have no
cross-module caller.  That leaves seven EXPORT_SYMBOL_GPL() and no plain
EXPORT_SYMBOL() in the driver.

One thing we considered and did not do: EXPORT_SYMBOL_NS_GPL() with a
driver namespace plus MODULE_IMPORT_NS() in the PCIe module.  Happy to
do that instead if you prefer it.


Subject: Re: [PATCH v7 3/6] net: wwan: t9xx: Add control DMA interface

netdev-ai wrote:
> Should the device-written length be clamped against the allowance the
> driver actually advertised for this BD rather than against frag_size?
> ...
> Separately, min_t(u16, ...) truncates the u32 frag_size to 16 bits, which
> makes the clamp a no-op for any future frag_size at or above 64 KiB.

Will fix in v8, all three.  Each BD will be clamped against its own
data_allow_len, the running total bounded against req->mtu with the
packet dropped rather than over-filled, and the comparison becomes
min_t(u32, ...) after le16_to_cpu().

> Can the TX XFER_DONE interrupt end up permanently masked here?  The
> interrupts are unmasked before drv_info->txq[txq->txqno] is published, and
> mtk_cldma_stop_queue() does not clear L2TISAR0, so a leftover status bit
> asserts as soon as the unmask happens ...

Will fix in v8.  Will reorder txq_alloc() to publish the queue first,
then program the start address, then unmask.

> The publishing store is also a plain assignment, so an IRQ-context reader
> on another CPU can observe txq before ring_lock, req_pool and the
> work_struct are visible.  mtk_cldma_rxq_alloc() does the opposite and
> publishes drv_info->rxq[] before starting and unmasking - would the same
> ordering (with smp_store_release()) work here?

Will fix in v8.  Both drv_info->txq[] and drv_info->rxq[] become
smp_store_release() with smp_load_acquire() in the ISR; rxq[] is
included because it has the same plain-store problem even though its
ordering is already right.

> When the stop times out, is it safe to continue and release the ring?
> mtk_cldma_stop_queue() returns -ETIMEDOUT only when the UL/SO STATUS
> active bit is still set after the poll ...
> Should a stop timeout escalate to cldma_drv_reset() or the FSM reset path
> before descriptor memory goes back to the pool?

Will fix in v8, by the first.  A live-link stop timeout will escalate to
cldma_drv_reset() and re-poll once; if the queue still will not stop the
ring is leaked with a dev_err() rather than returning a live DMA target
to the pool.  This gives cldma_drv_reset() its first caller.

> Also, mtk_cldma_queue_status() returns LINK_ERROR_VAL unmasked and the
> poll condition treats an all-ones read as "stopped" - is that intended?

Will fix in v8.  mtk_cldma_stop_queue() will split the two outcomes:
-ENODEV for the all-ones read, which frees as it does today, and
-ETIMEDOUT for a live link that will not stop, which takes the reset
path above.

Treating an all-ones read as "stopped" is intended and stays - a gone
device is certainly not doing DMA, and blocking teardown for ten times
the timeout on a surprise-removed card is worse than proceeding.
Returning the same -ETIMEDOUT for both is the part that is wrong: a
surprise removal and a genuine stop failure are indistinguishable to the
caller, and as you note just above, both currently lead to the same
unsafe free.

> This isn't a functional bug, but rxq->rxqno is printed before it is
> assigned.  rxq comes from kzalloc_obj(), so the message always says
> "rxq0" no matter which queue failed.  Would rxq->que->rxqno be the right
> value to print here?

Will fix in v8, by moving "rxq->rxqno = rxq->que->rxqno;" above the
validation, which also makes the function read in the order it executes.

While checking that we noticed the radix_tree_lookup() result on the
line above is dereferenced without a NULL check, as are the ones in
txq_alloc() and in mtk_cldma_open()/_tx()/_close().  All five get a
check in v8.

> Can this double-free the RX head SKB in BD mode?  mtk_cldma_alloc_rx_bd()
> aliases req->skb with bd_dsc_pool[0].skb and leaves both pointers set ...
> Should rxq_free() follow that pattern?  The queue table at the end of the
> series uses equal MTU and fragment sizes, so nr_bds is 0 and this stays
> latent for now.

Will fix in v8, by following rxq_alloc_cancel() as you suggest: each
bd_dsc->skb freed once and req->skb never separately.

Your latency analysis is right; the BD path is dead on every shipping
queue, so this and the length clamp above stay latent.

> Can the address programmed here be stale?  free_idx is sampled under
> ring_lock, but the lock is dropped before cldma_setup_start_addr() runs,
> and mtk_cldma_tx_done_work() advances free_idx under the same lock ...
> Should the read and the register write be in the same critical section?

Will fix in v8.  ring_lock will be held across the read,
cldma_setup_start_addr() and cldma_start_queue(); both are MMIO writes,
not sleeping calls, so extending the critical section is safe.

> txq->tx_started is also written with plain stores here and in
> txq_alloc()/txq_free() but read with READ_ONCE() below - is the mixture
> intentional?

Not intentional - the READ_ONCE() was added without the matching stores.
All three stores become WRITE_ONCE() in v8.

> Is the recovery scope right in this branch?  A zero UL start-address
> register indicates the whole CLDMA instance lost its configuration, and
> cldma_drv_init() reprograms IP-global registers ...
> Should this re-arm every configured queue of the instance, or escalate to
> the reset/FSM path?

Will fix in v8, by the first.  After cldma_drv_init() the branch will
walk every allocated txq and rxq of that drv_info and re-arm each, log a
dev_err() on entry, and return the failure to the caller instead of
reporting a transfer as submitted.

> Does this make probe fail on every supported device at this commit?
>
> mtk_queue_info_m9xx[] is empty, so queue_info_num is 0, and
> mtk_trans_ctrl_init() rejects that unconditionally ...
> Given that mtk_pci_probe() now calls mtk_pci_dev_init() ->
> mtk_trans_ctrl_init(), is the "not yet reachable from probe" part still
> accurate?  The queue table is only populated by the later "Add control
> port" patch, so a bisection landing on this commit sees a dead driver.
> Could the probe-time wiring move to that later patch, or the table be
> populated here?

Will fix in v8, by the second: the two control queues (CONTROL and
SAP_CONTROL) are added to mtk_queue_info_m9xx[] in this patch, so probe
brings up the CLDMA instance and its control queues here.  Patch 4 then
adds only the port-layer wiring, and patch 6 adds UART2 and MBIM as
before.  The commit message paragraph is rewritten accordingly, and
verified by building patch 3 alone and confirming the bind on 0x01CA.

Populating the table rather than deferring the wiring, because the
control queues are the queues this patch's code exists to drive; moving
the wiring to patch 4 would leave patch 3 introducing a CLDMA layer that
nothing instantiates.

> Should struct mtk_ctrl_info.srv_cfg be declared with the real type?  It is
> declared as int **, but this initializer supplies a
> const int [NR_CLDMA][HW_QUE_NUM], and the cast also discards const.
> ...
> Would const int (*)[HW_QUE_NUM] or a fixed-size array member be clearer?

Will fix in v8.  srv_cfg is redeclared as const int (*)[HW_QUE_NUM] and
the cast goes away with it, so the const is preserved end to end.

> Should this use skb_queue_len_lockless()?  qlen is read here without the
> queue lock while producers update it under skb_list->lock in
> mtk_pcie_hif_submit_skb().

Will fix in v8.  Changed to skb_queue_len_lockless(), which also
documents the intent at the call site.

> Is the reasoning in this comment right?  It says the producer's list lock
> supplies the acquire/release ordering for ->cb visibility, but the
> consumer never acquires that lock - it uses skb_peek(), skb_queue_is_last()
> and skb_peek_next() unlocked and only takes the lock inside skb_unlink().
> ...
> Should the traversal hold skb_list->lock?

Will fix in v8.  The traversal will take skb_list->lock - the loop body
is a non-sleeping MMIO submit path, so the lock can be held throughout -
and the comment is replaced with one that states the invariant that
actually holds.

The reasoning is wrong: it conflates single-consumer with
single-accessor.  One consumer rules out concurrent dequeues, not
concurrent enqueues, and the head insert on the TRB_CMD_DISABLE path
changes what skb_peek() returned while the consumer is still using it.

> Can the burst be stranded if this kick fails?  The return value is
> discarded, but by this point mtk_cldma_submit_tx() already armed the GPDs
> and the SKBs are unlinked from skb_list, and the kick can fail ...
> Nothing then starts the transfer and nothing calls trb->trb_complete(), so
> the ring budget stays consumed and blocking writers in
> mtk_port_send_data() keep waiting.

Will fix in v8.  mtk_cldma_tx() will check the mtk_cldma_start_xfer()
return, complete the TRB with the error status and log the failure, so
the budget is released and the blocking writer wakes with a real errno
instead of a timeout.

> Can submit_lock be locked before it is initialized?  trans comes from a
> single devm_kzalloc() in mtk_trans_ctrl_init() and lives for the whole
> device lifetime, while mutex_init() runs only after the two failure paths
> above.
> ...
> Would initializing submit_lock and available once in
> mtk_trans_ctrl_init(), next to the devm_kzalloc(), avoid both?

Will fix in v8, exactly as you suggest: both are initialised once next
to the devm_kzalloc(), before any path that can return early and outside
anything ops->init re-runs.  Yes to both halves - and the re-init of a
held mutex is the nastier one, since it silently destroys the lock state
rather than being a benign no-op.

> Related: at this commit mtk_ctrl_trb_srv_exit() does
> kthread_stop(srv->trb_thread) with no NULL check on srv, although all
> trans->trb_srv[] slots are NULL after a failed init.  A NULL check
> appears later in the series.

Will fix in v8 by moving the "if (!srv) continue;" guard out of patch 5
and into patch 3, where the code it guards is introduced.  Patch 5 keeps
the rest of that hunk.

Worth flagging that "later in the series" is the defect rather than a
late fix: patch 3 introduces both the service array and the unwind that
dereferences it, so patches 3 and 4 sit in the tree with a NULL deref on
their own error path.

> Is this teardown order safe?  mtk_ctrl_trb_srv_exit() kfree()s each srv
> and sets trans->trb_srv[i] = NULL, but the CLDMA interrupt is still
> registered and unmasked at that point ...
> Would unregistering/masking the CLDMA interrupt and freeing the queues
> first, then destroying the TRB services, close this?

Will fix in v8, as you suggest: mtk_cldma_exit() first, then
mtk_ctrl_trb_srv_exit().

> Does the atomic_read() actually keep que alive here?  The lookup and the
> dereference in mtk_queue_list_is_full() run without submit_lock and
> without rcu_read_lock(), while mtk_pcie_hif_exit() clears available under
> the mutex, releases it, and only then frees the entries ...
> Would holding submit_lock across the check, lookup and use - as
> mtk_pcie_hif_submit_skb() does - be the simpler fix?

Will fix in v8, as you suggest.  submit_lock will be held across the
check, lookup and use, and also across the clear-and-free in
mtk_pcie_hif_exit() so the free cannot begin while a lookup is in
flight.  The atomic_read() stays as a cheap early reject before the
lock.  The other send_cmd() commands are audited for the same pattern in
the same patch.

Your note about reachability sets its priority: an unprivileged poll(2)
on an open WWAN port, concurrent with device removal or an FSM
transition, is enough to reach it.

> This isn't a functional bug, but a few things look off here.
>
> The label is named err_free_cfg yet frees nothing, and it hardcodes
> -ENOMEM, so a configuration-lookup failure is reported as an allocation
> failure and the real err from mtk_ctrl_init() is discarded.
>
> mtk_trans_get_ctrl_info() is also called with NULL for a cfg parameter it
> never uses.

Will fix in v8.  The label is renamed to what it does and the real err
is returned.

On the NULL argument: at patch 3 you are right, but patch 4 both uses
the parameter and passes a real cfg.  v8 drops the parameter here and
reintroduces it in patch 4 with its first real caller, so no commit
carries an argument its own code does not use.

> Several members and macros added by this patch have no reader here or at
> the end of the series: struct mtk_ctrl_trans.tx_mtu_cfg and rx_mtu_cfg,
> struct mtk_ctrl_blk.cfg, struct mtk_ctrl_info.ctrl_cfg, plus
> DMA_POOL_NAME_LEN, CLDMA_RETRY_DELAY_MS, cldma_drv_ops_null,
> MTK_DFLT_PORT_NAME_LEN, MTU_RSV_ROOM and HIF_ID().

Will fix in v8, with three corrections - I checked each by grep over the
applied series.

Deleted outright: tx_mtu_cfg, rx_mtu_cfg, CLDMA_RETRY_DELAY_MS,
cldma_drv_ops_null, MTU_RSV_ROOM, HIF_ID(), and the duplicate
MTK_DFLT_PORT_NAME_LEN in pcie/mtk_trans_ctrl.c (the mtk_port.h one is
the one referenced).

Not dead: DMA_POOL_NAME_LEN has four users and stays as is.

Dead only at patch 3: struct mtk_ctrl_info.ctrl_cfg is assigned and read
back by patch 4, so the declaration moves into patch 4 rather than being
deleted.

Dead but write-only: struct mtk_ctrl_blk.cfg is declared in patch 3 and
assigned in patch 4, never read.  Removing it means removing the patch-4
assignment too, and v8 does both.

> One more note on the TX buffer mapping, which came up during review but
> does not look like a bug: mtk_cldma_txbuf_set() maps only
> skb_headlen(skb) in non-BD mode while the descriptor length and
> req->data_len are set from skb->len.
> ...
> An explicit invariant check or a WARN would make that contract easier to
> keep, but nothing is currently broken.

Agreed, and v8 adds the check: a WARN_ON_ONCE(skb_is_nonlinear(skb))
returning -EINVAL at the top of the non-BD path.  The mapping itself is
unchanged - for a genuinely non-linear SKB the fragments are not
contiguous, so mapping skb->len instead would map the wrong memory; the
correct handling is to refuse it.

Two notes on distribution.  Each fix goes in the patch that introduced
the defect, so no commit in v8 carries a bug a later commit repairs: the
trb_srv_exit() NULL guard moves from patch 5 into patch 3, and the
ctrl_cfg declaration and the get_ctrl_info() cfg parameter move from
patch 3 into patch 4.  Only the probe-failure item touches a commit
message.


Subject: Re: [PATCH v7 5/6] net: wwan: t9xx: Add FSM thread

netdev-ai wrote:
> Can a failing HIF init still leave the FSM in FSM_STATE_ON?  The notifier
> callback returns void, and mtk_fsm_switch_state() has already committed
> fsm->state = to_state and emitted the uevent before the post notifiers are
> called, so this dev_err() is the only trace of the failure.
> ...
> Should probe fail, or the transition be rejected, when the data path was
> never created?

Will fix in v8, but by neither of those two.  The HIF init failure will
be recorded on the FSM and the subsequent progression to FSM_STATE_READY
refused, so the device stays bound, the FSM stops where it failed, and
both the state and the uevent stream show it.

Not failing probe, because probe can only cover one of the two sites you
list.  mtk_pcie_hif_init() runs on the DEV_ADD transition, still within
probe, but mtk_cldma_dev_init() runs on the BOOTUP transition, which the
device triggers with D2H_BOOT_FLOW_SYNC long after probe has returned.
Recording the failure and refusing READY covers both with one mechanism.
What is wrong today is that we announce READY and create WWAN ports on
top of a transport that does not exist.

Not rejecting from the notifier, because the void return is deliberate:
listeners are informational, and giving them a veto means defining what
a partial rollback across several listeners means.

> Should these two cases also consult rtft_info_cfg?
>
> mtk_fsm_hs_info_init_by_hsid() declares QUERY_RTFT_ID_MD_PORT_ENUM and
> QUERY_RTFT_ID_SAP_PORT_ENUM as RTFT_TYPE_MUST_SUPPORT on the host side.
> ...
> Is a device that rejects a host-mandatory feature meant
> to be accepted here?

It is not, and will fix in v8: both cases return -EPROTO when the host
declared MUST_SUPPORT for that feature, so such a device no longer
reaches HS3.  RTFT_TYPE_NOT_EXIST is 0, so only the two PORT_ENUM ids
can trigger it and the change cannot reject anything we accept today.

> Can this free the skb twice?  The caller treats a negative return as "the
> callback did not take the skb":
> ...
> The remaining paths in both handlers return 0 after freeing, so should
> these paths return 0 as well?

Will fix in v8 - three sites, all returning 0 after the free so the
handler owns the skb on every path.  Note the scatter-gather path in
mtk_port_rx_dispatch() falls through to the same drop_data label, so it
double-frees too.

> Does this shared epilogue clear rt_data for an skb that a queued event
> still owns?
> ...
> mtk_fsm_sap_ctrl_msg_handler() returns 0 from its duplicate path without
> touching rt_data.  Should the MD handler do the same?

Will fix in v8.  The MD handler's duplicate and default paths return 0
without touching rt_data, matching the SAP handler, and both dereference
sites get a NULL check regardless.

> What happens when this submit fails?  mtk_fsm_evt_submit() returns
> FSM_EVT_RET_FAIL when its GFP_ATOMIC kzalloc fails, and the caller has
> already latched the device state:
> ...
> so a repeated boot-flow notification carrying the same dev_state is dropped
> and nothing retriggers the transition.

Will fix in v8.  The result is checked, last_dev_state rolled back on
failure and the handshake channels left masked, so the device's retry is
not filtered out by state we latched for an event that never existed.

> mtk_fsm_hs1_handler() has the same shape - it ignores the submit result and
> then consumes the hardware notification anyway ...

Same fix: on a failed submit the channel is left unmasked and uncleared
so the device can re-notify, and the failure is logged.

> Is the handshake trigger also lost when an HS1 event is processed while the
> FSM is still FSM_STATE_ON?  mtk_fsm_startup_act() takes this path:
> ...
> and never reaches mtk_fsm_ctrl_ch_start() or mtk_fsm_send_hs1_msg().

It is, and will fix in v8: that path switches to BOOTUP and then falls
into the handshake instead of returning.  This is the more likely of the
two in practice - not an allocation-failure corner but the ordinary race
between the DEV_ADD that moves ON to BOOTUP and an HS1 from a device
that booted quickly.

> What serializes hs_info->rt_data between this code and the receive path?
> ...
> While the kthread is inside this branch for skb1, the rx path can clear
> rt_data and publish skb2 together with a second event; the
> dev_kfree_skb(hs_info->rt_data) above then frees skb2 while its event is
> still queued.

Nothing serializes them.  Will fix in v8 by handing ownership to the
event rather than locking the field: the rx path attaches the skb to the
event it submits, and the kthread consumes and frees the skb it was
given without reading hs_info->rt_data at all.  hs_info->rt_data
survives only as the duplicate-HS2 guard.

That is a bigger change than a lock, deliberately - this and the
epilogue bug above are two symptoms of one missing ownership rule, and
locking the field would leave the rule unstated.

> mtk_fsm_parse_hs2_msg() also caches the data pointer once but re-reads the
> length on every bounds check:
> ...
> Can a concurrently published larger rt_data_len make that loop walk past
> skb1's linear data and hand those bytes to mtk_port_status_update()?

It can, and the same ownership change covers it: the length comes from
the skb the kthread owns, so pointer and length can no longer disagree.
This is the part that makes the race an out-of-bounds read rather than
only a lifetime bug.

> Does the exit label leak the kthread created above?
>
> Both of these error paths return without kthread_stop(), and mdev->fsm is
> only published on success, so mtk_fsm_exit() cannot stop it either.

It does.  Will fix in v8 with a proper unwind ladder so both paths
kthread_stop() the handler before returning.

> Related asymmetry: mtk_fsm_hs_info_init() returns on the first
> mtk_dev_register_dev_evt() failure without unregistering the channels its
> earlier iterations registered, and this error path only unregisters
> DEV_EVT_D2H_BOOT_FLOW_SYNC, never calling mtk_fsm_hs_info_exit().

Also correct.  Will fix in v8: that path calls mtk_fsm_hs_info_exit(),
and mtk_fsm_hs_info_init() unwinds its own loop so its contract is
all-or-nothing.

> Should mtk_fsm_exit() also call mtk_fsm_ctrl_ch_stop()?
> ...
> Calling mtk_fsm_ctrl_ch_stop() here after
> the handler is stopped looks idempotent - would that work?

It is idempotent - every access is guarded by "if (hs_info->ctrl_port)"
and the pointer is NULLed - and v8 calls it there.  On its own it would
not be enough, since the port table is already gone by then; it works
because of the ordering change at the end of this section.

> Is the 0x0900 entry missing from this table?  mtk_pci_probe() assigns
> mdev->hw_ver = pdev->device, and t9xx_pci_table binds both ids:
> ...
> On a 0x0900 device mtk_cldma_get_drv_info() leaves drv_ops and hw_regs
> NULL, so mtk_cldma_dev_init() aborts with -EIO before any DMA pool,
> workqueue or MSI-X registration exists ...

It is.  Will fix in v8 by adding the {0x0900, ...} entry to
cldma_drv_info_tbl[].  This is the most serious item in the round:
0x0900 is the MediaTek-branded ID and the primary device, 0x01CA the
Compal variant, so the table covers the derivative and omits the
original.

Two tables keyed on the same value can diverge, so for v8 every
hw_ver-keyed table in the series is audited for both IDs.  The two IDs
are the same silicon, T900, built by two manufacturers; they differ only
in the ID, so an entry missing from one table is a table bug rather than
a hardware difference.

> Does the QUEUE_ERROR condition stay latched and masked after this?
>
> mtk_cldma_get_intr_status() masks every bit it observed, but clears only
> the XFER_DONE bits:
> ...
> After the first error, is
> that queue's error reporting disarmed for the rest of its life with a stale
> status bit set?

It is.  Will fix in v8 by acknowledging QUEUE_ERROR through
cldma_clr_intr_status() and re-unmasking, so reporting stays armed for
the life of the queue.

> Should the offending queue also be stopped or reset rather than only
> logged?

Yes.  Will fix in v8: the offending queue is stopped and the error
reported upward, sharing the escalation path being added in patch 3 for
the stop-timeout case.

> Can mtk_cldma_isr() be entered with a NULL drv_info because the callback is
> unregistered before the source is masked and before synchronize_irq()?
> ...
> The barriers order the two stores but do not make the pair atomic, so an
> interrupt that already latched cb can call mtk_cldma_isr(irq, NULL), which
> immediately does drv_info->drv_ops->cldma_get_intr_status(...).

It can.  Fixed on the patch-1 side: mtk_pci_unregister_irq() masks the
source and calls synchronize_irq() before clearing either field, so no
handler can be in flight when the pair is torn down.

> Separately, mtk_cldma_isr() ends with:
> ...
> so a handler still running when the mask above is issued re-arms the vector
> afterwards, leaving an enabled source with no registered callback.  Would
> mask, then synchronize_irq(), then unregister be the safer order?

It would, and that is the v8 order.  The re-arm then happens before
synchronize_irq() returns, so the mask that matters is the one issued
after the handler has finished.

> Can the device still own descriptors in these pools when they are
> destroyed?
> ...
> If the engine ignored the stop and still holds
> HWO descriptors, does dma_pool_destroy() hand that coherent memory back
> while the start/current address registers still point at it?

It does.  Will fix in v8: mtk_cldma_dev_exit() quiesces the instance
before destroying the pools - mask the IP-level interrupt and reset the
block via cldma_drv_reset() - and leaks a pool whose ring patch 3 had to
leak.  The stop-timeout escalation itself goes in patch 3, where
txq_free()/rxq_free() are introduced.

> Is it safe to proceed into mtk_trans_ctrl_exit() after DEV_RM failed or
> timed out?  At that point the event may still be queued, or executing in
> the FSM kthread, and the event gate is only closed later, inside
> mtk_fsm_exit().

It is not safe, and the timeout is reachable - mtk_port_disable() waits
per port, and a device that keeps sending handshake notifications keeps
STARTUP events ahead of DEV_RM.  v8 removes the situation rather than
the symptom, by the reordering at the end of this section.

> Two consequences seem to follow.  First, the fallback in
> mtk_trans_ctrl_exit() reads the flag outside submit_lock:
> ...
> Can the remove path and the FSM thread run it concurrently and double free
> the cldma_dev, or call kthread_stop() twice on the trb threads?

Both, yes.  Will fix in v8 by making mtk_pcie_hif_exit() idempotent and
taking submit_lock across the availability test and the teardown, with
mtk_cldma_exit() taking and clearing trans->dev under the same lock so
the check-then-free is atomic.

> Second, mtk_ctrl_exit() sets mdev->ctrl_blk = NULL and mtk_port_mngr_exit()
> frees the port tables before mtk_fsm_exit() stops the thread, so a queued
> STARTUP event can still reach mtk_fsm_ctrl_ch_start() ->
> mtk_port_internal_open() and dereference mdev->ctrl_blk.  Should the gate be
> closed and the in-flight event joined before the transport plane is
> dismantled?

It should, and that is the v8 fix for this whole group:
mtk_pci_dev_exit() closes the gate and stops the FSM kthread before
mtk_trans_ctrl_exit() runs, so no event can be executing while the
transport plane is dismantled.  The DEV_RM submit stays, but its failure
no longer means "proceed with a live FSM".  Resulting order:

	close gate -> stop FSM thread -> ctrl_ch_stop -> trans_ctrl_exit
	-> fsm_exit

Two notes on distribution.  Each fix goes in the patch that introduced
the defect: the "if (!srv) continue;" guard this patch currently adds to
mtk_ctrl_trb_srv_exit() moves back into patch 3, and the stop-timeout
escalation lands in patch 3 rather than here.  Separately, per the
patch-2 discussion, the four mtk_fsm_* exports in this file become
EXPORT_SYMBOL_GPL() and the mtk_fsm_notifier_register/unregister exports
are deleted.  No commit message in the series changes for any of this.


Subject: Re: [PATCH v7 6/6] net: wwan: t9xx: Add AT & MBIM WWAN ports

netdev-ai wrote:
> Is the from_user_space == true branch reachable?  Both new callbacks,
> mtk_port_wwan_write() and mtk_port_wwan_write_blocking(), pass false, so
> the copy_from_user() arm and the union user_buf.ubuf member added in
> mtk_port_io.h have no user in this commit.
> ...
> Similarly, is the scatter gather helper below reachable?

Neither is reachable.  Will fix in v8 by deleting union user_buf, its
.ubuf member and mtk_port_copy_data_from() outright, which also retires
the -EINVAL/-EFAULT question.  The scatter-gather helper goes too - see
the next answer.

> Can this condition ever be true with the queue table this commit adds?
> Every entry in mtk_queue_info_m9xx[] uses Q_MTU_3_5K for the MTU and
> Q_FRAG_3_5K for the fragment size ...
> Is this intended as groundwork for a later user?  The
> commit message doesn't mention it.

Not groundwork - dead code.  Will fix in v8 by removing
mtk_port_common_write_frag_skb() and the gate that guards it.  tx_mtu is
3456 and tx_frag_size 3584 for every port on every queue, so the
condition is constant false.

The same dead shape exists on the CLDMA side, at pcie/mtk_cldma.c:654,

	if (txq->que->tx_mtu > tx_frag_size && tx_frag_size)
		txq->nr_bds = ...

so the multi-BD TX path is never armed either.  That code belongs to
patch 3 and you did not raise it, so we have left it alone rather than
change patch 3 under a patch 6 comment.  Happy to remove it too if you
prefer.

> Can this read past the end of the head skb?  The transmit skb handed to
> the driver is not linear once the user write exceeds caps.frag_len.
> wwan_port_fops_write() builds a frag_list chain and calls the tx op once
> with the head:
> ...
> Would skb_copy_bits() or an explicit walk over the frag_list be the right
> fix here?

Yes, and will fix in v8 with skb_copy_bits(): mtk_port_common_write()
takes the source skb rather than a raw pointer, and each per-packet copy
becomes an skb_copy_bits() into the skb_put() region, which walks the
frag_list and returns -EFAULT if the range exceeds src->len.

skb_copy_bits() rather than an open-coded walk because the open-coded
version would also have to handle paged frags, which the core does not
produce today but is not forbidden from producing.

The arithmetic is 62080 bytes for a 65536-byte write: the second packet
already runs 3424 bytes past the head's linear area and packets 3
through 20 are entirely out of bounds.

> Does this turn a partial transmit into a reported total failure?
>
> mtk_port_common_write() submits one packet per iteration and returns the
> bytes already accepted when a later iteration fails:
> ...
> wwan_port_fops_write() then frees the skb and reports the error to
> userspace, so a retry from userspace re-sends the prefix that was already
> handed to the transport.  Is that intended?

Not intended.  Will fix in v8 by making a write all-or-nothing:
continuation packets are submitted with force_send set, so only the
first packet can fail with -EAGAIN and at that point nothing has been
sent.  mtk_port_common_write() returns 0 or -errno, and both
"ret != skb->len" checks are gone.

One thing you did not ask about, found while checking this.  These two
callbacks steer mtk_port_send_data() by mutating shared state:

	port->info.flags &= ~PORT_F_BLOCKING;	/* mtk_port_wwan_write */
	port->info.flags |= PORT_F_BLOCKING;	/* ..._write_blocking */

and wwan_port_fops_write() does not hold port->data_lock across
wwan_port_op_tx() - data_lock covers the read path and TCFLSH only.  So
two threads writing the same node race on that field and an O_NONBLOCK
writer can clear a blocking writer's flag mid-transfer.  Since the fix
above has to plumb the mode through anyway, mtk_port_send_data() takes
explicit blocking and force_send arguments in v8 and the flag mutation
is deleted.  That is a patch 4 signature change; its patch 4 caller
passes the previously derived values, so patch 4 still builds and
behaves identically on its own.

> The same two questions apply to the blocking transmit callback, which is
> the one used for writes without O_NONBLOCK.

Agreed.  The two callbacks collapse into one implementation taking a
bool in v8, so both fixes above exist in exactly one place - the
duplication is how they came to differ in the first instance.

> Can these two abort paths leave the CLDMA channel open with no software
> owner?  Both return with PORT_S_ENABLE still clear after issuing a
> disable.
> ...
> Note that mtk_port_internal_enable() deliberately does not call
> mtk_port_ch_disable() after a failed enable.  Is the asymmetric teardown
> here intentional?

Not intentional - the internal path is the one missing an unwind, not
the careful one, and it has the same -ETIMEDOUT exposure.  Two changes
in v8: mtk_pcie_hif_submit_skb() queues a DISABLE ahead of TX trbs but
behind any pending ENABLE for the same queue (patch 3), and
mtk_port_internal_enable() gets the same unwind these two paths have
(patch 4).

The head insertion itself stays - a disable does need to overtake a TX
backlog; what it must not overtake is a pending ENABLE, because those
two do not commute.

> This isn't a bug, but the ordering here is a little confusing to read.
> mtk_port_wwan_enable() sets PORT_S_ENABLE and PORT_S_WR before
> wwan_create_port() and publishes w_priv.w_port only afterwards, while this
> function reads w_priv.w_port outside w_lock.
> ...
> Would it be
> clearer to set the state bits and assign w_priv.w_port after
> wwan_create_port() succeeds?

Will fix in v8: the w_priv.w_port read in mtk_port_wwan_disable() moves
inside w_lock, and a comment above the set_bit() pair records why the
ordering is required.  The ordering itself stays as it is.

Your serialization analysis is correct.  The bits cannot move after
wwan_create_port() because that call registers the character device, so
userspace can open the node the moment it returns, and
mtk_port_common_open() rejects an open when PORT_S_ENABLE is clear.
Setting the bits afterwards would introduce a real window in which a
legitimate open(2) fails with ENODEV.  So the order is required rather
than accidental, and nothing in the file says so - which is the actual
defect your question exposes.

> Should this also check that the port has been opened?  The only condition
> tested is that w_priv.w_port is non-NULL, which becomes true at
> FSM_STATE_READY, long before (and possibly without) userspace ever opening
> the AT or MBIM character device.
> ...
> Both ports are
> configured PORT_F_ALLOW_DROP, but no path here actually drops.

It should.  Will fix in v8: mtk_port_wwan_recv() tests PORT_S_OPEN and
returns -ENXIO, which mtk_port_rx_dispatch() already turns into a real
free, so nothing else is needed to make the drop happen.

One deliberate difference from t7xx: the message is
dev_dbg_ratelimited() rather than dev_err_ratelimited().  An unopened
port receiving unsolicited traffic is normal for this modem and would
log on every boot; the drop is a policy decision here, not a fault.

--
Jack Wu