RE: [External Mail] Re: [PATCH v10 0/6] net: wwan: t9xx: Add MediaTek T9XX WWAN driver

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

On Wed, 21 Oct 2026 10:08 +0800, Jakub Kicinski <kuba@kernel.org> wrote:
> On Wed, 14 Oct 2026 10:50:58 +0800 Jack Wu via B4 Relay wrote:
> > T9XX is the PCIe host device driver for MediaTek's
> > t900 modem. The driver uses the WWAN framework
> > infrastructure to create the following control ports
> > and network interfaces for data transactions.
> > * /dev/wwan0at0 - Interface that supports AT commands.
> > * /dev/wwan0mbim0 - Interface conforming to the MBIM
> >   protocol.
> > * wwan0-X - Primary network interface for IP traffic.
>
> AFAIU AI finds so many bugs in patch 3 it overflows the output token
> budget. Please see the review for other patches here:
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014-t9xx_driver_v1-v10-3-4d0dde3613f5@compal.com
> You gotta feed patch 3 into some models yourself and try to bring
> the bug count to a sane baseline before we can proceed :(

Will do.  We are setting up a local instance of sashiko
(https://github.com/sashiko-dev/sashiko) and will run patch 3 through it
ourselves, fix what it finds and repeat until the count is down to a sane
baseline;

Patch 2 came back with no findings.  The answers to the findings on
patches 1, 4, 5 and 6 follow, patch by patch.

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

netdev-ai wrote:
> This isn't a bug, but nothing seems to call mtk_pci_ack_dev_state(),
> including in the later patches of the series.
>
> The other helpers that have no caller yet, such as
> mtk_pci_get_dev_state() and the ext_evt helpers, get callers in
> mtk_fsm.c in "net: wwan: t9xx: Add FSM thread". This one only has its
> definition here and its prototype in mtk_pci.h.
>
> Could it be dropped until something needs it?

Will fix in v11 - the function and its prototype are deleted.  Confirmed
it has no call site at any commit of the series, while
mtk_pci_get_dev_state() does gain one in patch 5.  The register define
stays, since the read side still uses it.

> The comment in struct mtk_pci_priv says irq_cb_lock protects both
> irq_cb_list[] and irq_cb_data[]. Here, though, irq_cb_data[] is cleared
> after the lock is dropped. By that point mtk_pci_register_irq() already
> sees the slot as free.
>
> What happens if mtk_pci_register_irq() runs for the same irq_id in that
> window? Its data pointer would be overwritten with NULL. The handler
> would then call the new callback with NULL data, and for example
> mtk_mhccif_irq_cb() dereferences mdev->hw_priv.
>
> In the series as posted, register and unregister for the same irq_id
> look serialized. The MHCCIF IRQ is handled in probe and remove. The
> CLDMA IRQs are handled by the FSM kthread, or in teardown after
> kthread_stop(). So this doesn't look reachable today.
>
> Would it be better to take irq_cb_lock again for the irq_cb_data[]
> clear after synchronize_irq()? Or should the comment say that callers
> must serialize?

Will fix in v11 with your second option: the comment changes to say that
the lock covers irq_cb_list[] and the register-side irq_cb_data[] store,
that the teardown clear is deliberately after synchronize_irq() and
outside the lock because a handler past its READ_ONCE() still needs the
data, and that callers must not register and unregister one irq_id
concurrently.

Taking the lock again would not close the window, which is why I am not
doing it as well: the register call and the late clear are two separate
critical sections, so a register that lands between them still has its
data pointer cleared afterwards, exactly as you describe.  The lock would
only make the clobber look protected.  What excludes the interleaving is
the caller serialization you identified, so that is what the comment now
states.

> Is an undersized BAR really impossible here? The driver matches only on
> vendor ID, device ID and class. pcim_iomap_region() maps exactly the
> length the device advertises.
>
> Consider a device that reports 14c3:0900 or 03f0:01ca with class
> 0x0D4000 but has a 4 KiB BAR0, for example over a Thunderbolt/USB4
> tunnel. It would still be probed, and probe then does MMIO at fixed
> offsets:
>
>   mtk_pci_atr_disable(): read-modify-write up to BAR0 + 0xBE0
>   mtk_mhccif_init(): writes U32_MAX to bar23_addr + 0xA030
>   mtk_pci_probe(): writes U32_MAX to BAR0 + 0x3080
>
> Wouldn't these accesses go past the ioremap()'d area and its guard page
> into unrelated vmalloc/ioremap mappings?
>
> The mtk_pci_*read32() and mtk_pci_*write32() accessors in mtk_pci.h
> don't check the offset either. Probe also doesn't check
> pdev->untrusted.
>
> Could pci_resource_len() and IORESOURCE_MEM checks on both BARs be
> added?

No change in v11.  Every mechanical point here is correct - the matching
is on IDs plus class, pcim_iomap_region() maps only the advertised
length, the accessors do not range-check, and probe does not look at
pdev->untrusted.  The reason we are not adding the checks is the policy
at lines 119-120 of the driver-upstreaming-prompts.md that Jakub posted:

    Assume PCI/PCIe devices behave correctly and are not malicious. Do not
    add defensive checks.
    Reviewers may not know this policy, ignore them.

The kernel's own documentation points the same way.
Documentation/process/threat-model.rst says "The kernel assumes that the
underlying hardware behaves according to its specifications", naming
"ranges of values read from registers" and "the respect of documented
hardware limitations".  Under "Non-conforming devices and media" it lists
"fake USB devices that pretend to be another one, or devices reporting
values outside their documented ranges" as not vulnerabilities, and under
"Physical access" it puts DMA-attack tools over PCI-E/Thunderbolt out of
scope unless the system is configured to defend against them.  A device
that claims our IDs with a 4 KiB BAR is that case.  For the external
path, Documentation/admin-guide/thunderbolt.rst gates it in the subsystem:
at the default user or secure level "the connected device must be
authorized by the user before PCIe tunnels are created (e.g the PCIe
device appears)", and with IOMMU protection "the driver does not do
anything special".  The only probe-time resource check
Documentation/PCI/pci.rst asks for is pci_request_region(), which
pcim_iomap_region() does and whose result we check.  No wwan driver in the
tree, t7xx included, checks pci_resource_len().

> Can this briefly turn bus mastering back on for a device that may still
> be doing DMA?
>
> mtk_pci_probe() saves the state after enabling bus mastering:
>
> 	pci_set_master(pdev);
> 	...
> 	ret = pci_save_state(pdev);
>
> So the saved PCI_COMMAND has both Bus Master Enable and Memory Space
> Enable set. pci_restore_state() writes it back through
> pci_restore_config_space().
>
> On the MHCCIF doorbell fallback, the commit message says the reset
> "cannot recover firmware that has stopped responding".
>
> Remove never disables the transparent EP->RC ATR on AXIS_0..3 that
> mtk_pci_atr_init() set up. Later in the series, CLDMA queues that fail
> to stop are also left as live DMA targets.
>
> A DMA engine that is still running could therefore write to host memory
> between pci_restore_state() and the final pci_clear_master(), after the
> driver has freed its buffers. Memory decode also stays enabled while
> the PCI core's enable_cnt is 0.
>
> Would saving the state before pci_set_master(), or clearing
> PCI_COMMAND_MASTER in the saved copy, avoid this window?

Will fix in v11 with your first option: pci_save_state() and
pci_store_saved_state() move above pci_set_master(), so the saved
PCI_COMMAND no longer carries Bus Master Enable and the restore in
remove() cannot re-enable it.  The compensating pci_clear_master() at the
end of remove() goes away with it.  Nothing else is lost by moving the
save, as the MSI-X vectors are allocated before either point.

One qualification rather than a flat yes: on the PLDR path the endpoint
is already back at power-on defaults before the restore runs, so nothing
is armed to drive the window.  It is the MHCCIF doorbell fallback, where
the device is not hard reset, that your scenario applies to - so the fix
is worth making but the common path was not exposed.

Your two side observations are both right and both separate from this
fix: remove() never calls mtk_pci_atr_disable(), and patch 3 can leave a
queue that failed to stop.  Neither is needed to close this window, since
with Bus Master Enable clear the device cannot master the bus either way.


> The commit message says:
>
>   Removal resets the modem, because the next driver load has to
>   redo the device handshake and status sync and cannot do that
>   against firmware left running by the previous load.
>
> Doesn't the same apply across kexec or a warm reboot?
>
> There is no .shutdown callback. On kexec, pci_device_shutdown() calls
> only drv->shutdown and then pci_clear_master(). So neither MRST._RST
> nor the DEV_EVT_H2D_DEVICE_RESET doorbell is sent.
>
> mtk_pci_probe() doesn't reset the device either. It only does:
>
> 	if (pdev->state_saved)
> 		pci_restore_state(pdev);
>
> Through the end of the series, mtk_pci_pldr() and
> mtk_pci_send_ext_evt() are called only from mtk_pci_remove().
>
> mtk_mhccif_init() also says BOOT_FLOW_SYNC is a one-shot notification
> that the device never sends again. The previous kernel has already
> acked it.
>
> So could a kexec'd kernel, or a reboot where the platform doesn't
> assert PERST# or cut power, probe a modem whose firmware is still
> running? That modem would then fail the boot handshake until a cold
> power cycle.
>
> Would a .shutdown handler that does the same reset, or a reset step in
> probe, cover this case?

Will fix in v11 - the driver gets a .shutdown that runs the same teardown
and reset as remove(), by calling it.

You are right, and this round I looked at it from the device's side
rather than deferring it.  On a notebook the normal reboot and poweroff
paths reset the M.2 slot through the platform reset or remove its power,
so the modem raises the boot notification again; kexec is the path that
does neither.  kernel_kexec() still goes through device_shutdown(), so a
.shutdown is called there, and with none the firmware is left running with
BOOT_FLOW_SYNC already consumed - exactly the hang you describe.

Calling remove() rather than writing a reset-only shutdown is deliberate:
remove() already ends with the reset, and its ordering ("no MMIO and no
MSI-X teardown may follow it") only holds if the state machine and the
CLDMA work are stopped first, which a reset-only shutdown would skip.
I did not take the reset-in-probe route because telling a fresh device
whose notification is still latched from one whose notification was
consumed by an earlier kernel needs device-state encodings we would be
guessing at.  kexec_in_progress is not exported to modules, so the reset
also runs on ordinary reboot and poweroff; that costs one extra reset at
shutdown and nothing else.

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

netdev-ai wrote:
> Is this description accurate for this commit on its own?
>
> As far as I can tell, nothing calls ->enable() yet. The same goes for
> mtk_port_internal_open(), mtk_port_internal_close(),
> mtk_port_internal_write(), mtk_port_internal_recv_register() and
> mtk_port_status_update(). mtk_pcie_hif_init() has no caller either, so
> no TRB reaches mtk_ctrl_trb_handler().
>
> Later patches in the series connect these:
>
> - mtk_ctrl_fsm_state_listener() calls mtk_port_mngr_fsm_state_handler()
>   and mtk_port_mngr_fsm_state_handler_late(), which call ->enable().
> - mtk_fsm.c uses the internal port functions.
> - query_rtft_action[] calls mtk_port_status_update().
>
> The previous patch, "Add control DMA interface", already says the FSM
> listener comes with "Add FSM thread".
>
> Could the commit message say that these paths only become live with the
> later FSM patch?

Will fix in v11 - the commit message will say that the port-state, RX
dispatch and TRB kref paths only become live with the FSM patch, the same
way the previous patch already says it for the FSM listener.

> The TRB keeps a raw pointer to the port without taking a port reference:
>
>     trb->priv = port;
>
> This is set in mtk_port_trb_init(). The CLDMA RX queue's rxq->arg also
> holds a raw port pointer.
>
> mtk_port_tbl_destroy() calls ->disable() and then mtk_port_free(). That
> ends in kfree_rcu() through mtk_port_release(), and nothing checks for
> outstanding TRBs or an armed RX queue first. Can a late TRB completion or
> RX dispatch then use the freed port?
>
> At this commit no TRB is processed, because mtk_pcie_hif_init() has no
> caller. Later in the series, mtk_trans_ctrl_exit() calls
> mtk_pcie_hif_exit() (mtk_cldma_quiesce(), mtk_ctrl_trb_srv_exit(),
> mtk_cldma_exit()) before mtk_ctrl_exit()->mtk_port_mngr_exit(). That
> order seems to be what keeps this safe.
>
> Is that teardown order meant to be the lifetime rule for ports?

Yes, that teardown order is the lifetime rule, and you have traced it
correctly: mtk_pcie_hif_exit() quiesces CLDMA, joins the trb services and
frees the CLDMA state before mtk_ctrl_exit() reaches the port manager, so
no TRB or armed RX queue outlives a port.

No code change.  Taking a port reference for trb->priv and rxq->arg would
add a kref round trip to the RX hot path to cover a sequence the ordering
already excludes.  I am also deliberately not writing this down as a
comment in this patch: the guarantee is established by mtk_pcie_hif_exit(),
which the FSM patch adds, and a comment here describing it would be
describing code that does not exist yet at this commit.  The ordering goes
into this patch's commit message instead, with the item above.

> The sequence number is rolled back only when the synchronous
> add_header/status_check/submit step fails. What happens if the TRB is
> queued and then fails later in mtk_ctrl_trb_handler()?
>
> Say tx_burst_cnt is 0 and mtk_cldma_submit_tx() returns an error other
> than -EAGAIN. Examples are a DMA mapping failure from
> mtk_cldma_txbuf_set(), or -EINVAL. The handler then does:
>
>     skb_unlink(skb, skb_list);
>     trb->status = err;
>     trb->trb_complete(skb);
>
> port->tx_seq is still advanced, so the next packet goes out as N+1 even
> though N never left the host. A blocking writer gets the error from
> trb->status:
>
>     else if (mtk_port_trb_done(trb))
>         ret = trb->status ? trb->status : len;
>
> but by then it is past the rollback branch above.
>
> Would this leave a gap in the AST-asserted sequence that the modem
> checks?

Will fix in v11 - the sequence number is assigned when the trb thread
hands the packet to CLDMA, and only advanced if that succeeds, so a packet
that fails after it was queued takes no number and the next one reuses it.

Rolling back from the completion path would not have worked, since later
packets may already carry N+1; numbering at the head of the queue avoids
that.  The rollback in mtk_port_send_data() and trb->seq_taken go away.

> mtk_port_ch_enable() now resets rx_seq to a -1 sentinel:
>
>     port->tx_seq = 0;
>     port->rx_seq = -1;
>
> This check still skips when port->rx_seq is 0, as if 0 meant "nothing
> received yet".
>
> After any packet with seq 0, mtk_port_rx_dispatch() stores
> port->rx_seq = 0. That happens for the first packet after each enable,
> and again each time the 15-bit sequence wraps from 0x7fff to 0.
>
> Doesn't the packet after a seq 0 packet then skip the order check, so a
> gap such as 0 -> 5 goes unnoticed? Can the port->rx_seq test be dropped
> now that -1 is the reset value?

Will fix in v11, together with the next item, by rewriting
mtk_port_check_rx_seq() the way t7xx_port_next_rx_seq_num() does it.  The
"nothing received yet" case becomes an explicit test against the reset
sentinel instead of a test of port->rx_seq for zero, which removes the
packet-after-seq-0 hole you describe, including on every 15-bit wrap.

> When a gap is found, rx_seq is resynced and -EPROTO is returned.
> mtk_port_rx_dispatch() then frees this packet:
>
>     ret = mtk_port_check_rx_seq(port, ccci_h);
>     if (unlikely(ret))
>         goto drop_data;
>
> Isn't this packet a valid, newer message? Its only problem is that an
> earlier one was lost.
>
> mtk_cldma_rx_done_work() drops packets on purpose: in non-BD mode a
> failed buffer reload recycles the old buffer and drops the packet, and
> len_err drops oversized packets. Each of those drops would also throw
> away the next good message on the control, AT or MBIM channel. The t7xx
> driver only prints a ratelimited warning and still delivers the packet.
>
> There are two smaller points. First, this dev_warn() runs in the RX
> worker without a ratelimit, so a modem that keeps sending bad sequence
> numbers can flood the log.
>
> Second, mtk_port_internal_enable() treats -EBUSY as success. If the
> modem does not restart numbering at 0 on ENABLE, the first packet after
> enable is dropped, because (N - 0xffff) & 0x7fff == 1 only when N == 0.
>
> Would a ratelimited warning that still delivers the packet fit better
> here?

Will fix in v11 - yes, and the same way t7xx does: the check is skipped
only when AST is clear or nothing has been received since reset, a
mismatch is reported with dev_warn_ratelimited(), the receiver resyncs to
the packet it just got, and the packet is always delivered.
mtk_port_rx_dispatch() no longer drops on it.

You are right that the old behaviour turned one lost packet into two, and
the point about where losses come from is what makes it matter: the CLDMA
RX worker drops deliberately on a failed reload in non-BD mode and on
len_err, and each of those used to cost the next good message on the
channel as well.

The -EBUSY case is closed by the same change: a modem that does not restart
numbering at 0 on ENABLE now gets its first packet delivered with one
ratelimited warning instead of losing it.  Whether -EBUSY should count as
success is left as it is.

> If ->recv() returns -EAGAIN, the skb is freed here but -EAGAIN still goes
> back to mtk_cldma_rx_done_work(). That function calls rx_done() again
> with the same skb:
>
>     ret = rxq->rx_done(rx_skb, rxq->arg, atomic_read(&rxq->need_exit) ? true : false);
>     if (ret == -EAGAIN)
>         usleep_range(1000, 2000);
>     } while (ret == -EAGAIN);
>
> Wouldn't that be a use-after-free of rx_skb? The force_recv argument is
> also never used.
>
> No receiver in the series returns -EAGAIN today:
>
> - mtk_fsm_md_ctrl_msg_handler() and mtk_fsm_sap_ctrl_msg_handler()
>   always return 0.
> - mtk_port_wwan_recv() returns 0 or -ENXIO.
> - mtk_port_internal_recv() returns -ENXIO or whatever recv_cb returns.
>
> So this looks latent for now. Should mtk_port_rx_dispatch() keep the skb
> on -EAGAIN, or change the return code it passes up?

Will fix in v11 - mtk_port_rx_dispatch() will stop freeing the skb and
then returning -EAGAIN as a retry request.

> Three updates to tx_seq run without a lock: this increment, the tx_seq--
> rollback in mtk_port_send_data(), and the reset in mtk_port_ch_enable().
> Must callers of mtk_port_send_data() serialize writers per port?
>
> That seems to hold for now:
>
> - Internal writes only come from the FSM kthread.
> - WWAN writes go through wwan_port_op_tx() under the WWAN core's
>   ops_lock.
> - Completions never touch tx_seq.
> - mtk_port_ch_enable() runs before the port is opened.
>
> Could this requirement be written down near mtk_port_send_data()?

Will fix in v11, but the comment states a different rule: after the
tx_seq change above, writers no longer touch tx_seq.  It is stamped and
advanced only at hand-off by the one thread serving the port's TX queue,
and the comment on tx_seq says so; the rollback is gone.

Your reasoning for why the old code was safe is correct.

> msg->version, head_pattern, tail_pattern and port_cnt are all read
> before anything checks data_len >= sizeof(*msg). Can a short
> port-enumeration message from the device cause an out-of-bounds read of
> up to 11 bytes here?
>
> A later patch in this series adds this check before the header fields
> are read:
>
>     if (data_len < sizeof(*msg))
>         return -EPROTO;
>
> The function also has no caller at this commit. Could that check move
> into this patch instead?

Will fix in v11 - a data_len < sizeof(*msg) check before the header is read,
in this patch.

> By this point PORT_S_ENABLE is already cleared. A failure from
> mtk_port_ch_disable() is only logged; it could be -ENOMEM from
> __dev_alloc_skb(), -ETIMEDOUT or -EBUSY. mtk_port_tbl_destroy() then
> frees the port anyway.
>
> In that case, can the CLDMA RX queue still be armed, with rxq->arg
> pointing at the freed port?
>
> At this commit no queue can be opened at all. Later in the series an
> armed queue seems to be torn down before the port is freed on both
> paths:
>
> - On FSM_STATE_OFF, mtk_ctrl_fsm_state_listener() disables the ports
>   and then calls mtk_pcie_hif_exit() while the ports are still alive.
> - On unbind, mtk_trans_ctrl_exit() calls mtk_pcie_hif_exit() before
>   mtk_ctrl_exit().
>
> Is this path meant to rely on mtk_pcie_hif_exit()?

Yes, it relies on mtk_pcie_hif_exit(), on both paths, and your trace of
them is correct.

No change.  The return value is already taken and logged, which was the
change from an earlier round; acting on it instead would mean abandoning
teardown and leaving the port and its queue alive indefinitely, which is
worse than logging.  As with the raw pointer question above, the ordering
guarantee comes from the FSM patch, so it is stated in this patch's commit
message rather than as a comment describing code this commit does not have.

> This reads priv->recv_cb and priv->arg without a lock.
> mtk_port_internal_recv_register() stores them with no ordering:
>
>     priv->arg = arg;
>     priv->recv_cb = cb;
>
> mtk_port_common_close() clears PORT_S_OPEN, but it does not unregister
> the callback or wait for an RX callback already in progress. Could a
> client that frees arg after close still be called back on freed memory?
> Could it see a cb and arg that don't match?
>
> At the end of the series the only internal user is
> mtk_fsm_ctrl_ch_start(), and it always registers the same handler with
> arg = fsm. fsm is freed in mtk_fsm_exit(), after mtk_pcie_hif_exit() has
> drained the CLDMA RX workqueue, so this looks latent. Should the API
> spell out these rules, or enforce them, for future users?

Will fix in v11 - the contract will be spelled out at
mtk_port_internal_recv_register(): the callback and its argument must stay
valid until the receive path has been drained, and a client must not free
the argument on the strength of close() alone.

> The PORT_S_OPEN test and the set_bit() are separate steps with no lock
> between them. Two other functions follow the same pattern:
>
> - mtk_port_internal_close() tests PORT_S_OPEN, then calls
>   mtk_port_common_close() and mtk_port_put_locked().
> - mtk_port_internal_enable() tests PORT_S_ENABLE, then calls
>   mtk_port_ch_enable().
>
> With concurrent callers, couldn't a port be opened twice, closed twice
> (two kref_put() calls), or enabled twice?
>
> Later in the series the callers seem to be serialized:
>
> - Internal open and close run only on the FSM kthread, or after
>   kthread_stop().
> - Enable and disable run under fsm->notifier_lock or during serialized
>   teardown.
> - WWAN open and close are serialized by the WWAN core's ops_lock and the
>   driver's w_lock.
>
> Would test_and_set_bit() be a simple way to make this hold anyway?

Will fix in v11 - test_and_set_bit() for open and enable,
test_and_clear_bit() for close.

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

netdev-ai wrote:
> When the device marks a feature RTFT_TYPE_MUST_SUPPORT in its
> feature_query, this copies the device's type/version byte back unchanged
> with data_len = 0. Nothing checks whether this driver implements that
> feature ID or version.
>
> Can HS3 tell the firmware that the host supports mandatory features it
> has no handler for?
>
> For comparison, t7xx_prepare_device_rt_data() in the t7xx driver skips
> MUST_BE_SUPPORTED features instead of affirming them. Without the
> protocol specification it is hard to say what the firmware does with
> this answer, so this may be harmless.

This issue is related to the firmware behavior of the modem;
we are currently inquiring about this with MTK.

> This callback runs from the mhccif work under priv->mhccif_lock. The FSM
> kthread changes the same channel masks without that lock, in
> mtk_fsm_enter_off_state():
>
> 	mtk_pci_mask_ext_evt(mdev, DEV_EVT_D2H_BOOT_FLOW_SYNC);
> 	for (hs_id = 0; hs_id < HS_ID_MAX; hs_id++)
> 		mtk_pci_mask_ext_evt(mdev, fsm->hs_info[hs_id].mhccif_ch);
>
> Suppose DEV_RM masks these while this handler is running. Can the
> unconditional unmask at the exit label, or the HS channel unmask in
> mtk_fsm_idle_evt_handler(), undo the OFF masking?
>
> The hs_err path in mtk_fsm_startup_act() has a related problem:
>
> hs_err:
> 	for (int hs_id = 0; hs_id < HS_ID_MAX; hs_id++)
> 		mtk_pci_unmask_ext_evt(mdev, fsm->hs_info[hs_id].mhccif_ch);
>
> This also unmasks the other handshake's channel while its HS1 event may
> still be queued. That seems to break the "exactly one side owns the
> channel" rule described in mtk_fsm_hs1_handler(). Could it lead to a
> duplicate HS1 event that re-runs the BOOTUP notifiers and sends HS1
> again?
>
> Also, fsm->hs_done_flag is written by mtk_fsm_idle_evt_handler() from
> the mhccif work. The FSM thread reads it twice with plain accesses.

Will fix in v11: the hs_err path will unmask only the handshake channels
that startup actually masked rather than all of them, and
fsm->hs_done_flag will be read once into a local instead of twice with
plain accesses.

> Is this comment accurate? The commit message makes the same claim:
>
>   "dma_pool_destroy() releases the backing pages even when blocks are
>   still outstanding"
>
> mm/dmapool.c:dma_pool_destroy() does this when blocks are still active:
>
> 	if (pool->nr_active) {
> 		dev_err(pool->dev, "%s %s busy\n", __func__, pool->name);
> 		busy = true;
> 	}
>
> 	list_for_each_entry_safe(page, tmp, &pool->page_list, page_list) {
> 		if (!busy)
> 			dma_free_coherent(pool->dev, pool->allocation,
> 					  page->vaddr, page->dma);
>
> So a busy pool's coherent pages are never returned to the allocator.
>
> Skipping dma_pool_destroy() therefore protects nothing. It leaks the
> struct dma_pool objects and leaves them on dev->dma_pools, where they
> stay visible in the pools sysfs attribute after unbind and across
> rebind.
>
> Could the comment and commit message be corrected, and dma_pool_destroy()
> be called unconditionally?

Will fix in v11: dma_pool_destroy() will be called unconditionally and the
ring_leaked branch around it goes away, along with the comment and the
commit message paragraph that claimed the opposite.

> Does is_stopping actually keep the queue from being restarted here?
>
> mtk_cldma_start_xfer(), for any queue on this drv_info, calls
> mtk_cldma_hw_recovery() before it takes ring_lock and checks
> is_stopping:
>
> 	if (unlikely(!val)) {
> 		ret = mtk_cldma_hw_recovery(drv_info, qno);
> 		...
> 	}
> 	spin_lock(&txq->ring_lock);
> 	if (unlikely(txq->is_stopping)) {
>
> mtk_cldma_hw_recovery() calls mtk_cldma_rearm_queues(). Stop timeouts in
> mtk_cldma_close() -> mtk_cldma_txq_free()/mtk_cldma_rxq_free() call it
> too. mtk_cldma_rearm_queues() looks only at tx_started and HWO, never at
> is_stopping:
>
> 	if (READ_ONCE(txq->tx_started) &&
> 	    !(req->gpd->tx_gpd.gpd_flags & CLDMA_GPD_FLAG_HWO)) {
> 		...
> 	} else {
> 		mtk_cldma_setup_start_addr(drv_info, DIR_TX, i,
> 					   req->gpd_dma_addr);
> 	}
> 	...
> 	if (READ_ONCE(txq->tx_started))
> 		mtk_cldma_start_queue(drv_info, DIR_TX, i);
>
> Say the trb thread reaches this after the successful stop above but
> before tx_started is cleared. The engine is then restarted on this
> queue. mtk_cldma_txq_flush() then clears HWO, calls dma_unmap_single()
> on the pending buffers and completes their skbs. Can the device DMA
> from those unmapped buffers?
>
> If the stop above fails, is_stopping and tx_started both stay set. A
> later rearm would then restart a queue that the submit path keeps
> rejecting.

Will fix in v11: mtk_cldma_rearm_queues() will test is_stopping per queue
and skip any queue that is stopping, so the flag governs rearm as well as
submit.

Your reading of the order is right - start_xfer() calls hw_recovery(), and
through it rearm_queues(), before it takes ring_lock and tests
is_stopping, and rearm_queues() looks only at tx_started and HWO.  So the
guard runs after the action it is meant to guard.

> This recovery depends on a single rx_done_work run reaching the restart.
> mtk_cldma_rx_done_work() has goto out exits inside its GPD loop:
> mtk_cldma_reload_rx_skb() failing on an empty slot, a BD-mode reload
> failure, and dma_mapping_error() when recycling the buffer. These exits
> skip the need_restart handling:
>
> 	if (mtk_cldma_reload_rx_skb(mdev, rxq, req))
> 		goto out;

Will fix, but in patch 3 rather than patch 5: the in-loop exits will go
through the need_restart handling instead of past it.

need_restart is introduced by "net: wwan: t9xx: Add control DMA interface",
and the three exits and the post-loop block are identical in that commit
and in both commits after it, so the defect is already there when the
function first appears and has to be fixed by amending that patch.

> Can CLDMA work be queued again after mtk_cldma_quiesce() has flushed
> drv_info->wq?
>
> The trb service thread keeps running until mtk_ctrl_trb_srv_exit() stops
> it, and it keeps draining TRBs that are already queued. Two of those
> paths call queue_work(drv_info->wq, &txq->tx_done_work) through
> mtk_cldma_rearm_queues():
>
>   mtk_cldma_tx()->mtk_cldma_start_xfer()->mtk_cldma_hw_recovery()
>     ->mtk_cldma_rearm_queues()
>
>   mtk_cldma_close()->mtk_cldma_txq_free() (stop timeout)
>     ->mtk_cldma_rearm_queues()
>
> mtk_cldma_rearm_queues() does not check drv_info->quiesced. Through
> mtk_cldma_drv_init() it also writes int_mask = 0 again.
>
> mtk_cldma_tx_done_work() now loads srv once at entry. Then
> mtk_ctrl_trb_srv_exit() does:
>
> 	kthread_stop(srv->trb_thread);
> 	mtk_ctrl_chs_flush(srv);
> 	...
> 	kfree(srv);
> 	trans->trb_srv[i] = NULL;
>
> A re-queued tx_done_work can load srv before the NULL store and then run:
>
> 	if (was_starved && srv)
> 		wake_up(&srv->trb_waitq);
>
> after kfree(srv). Is that a use-after-free? The workqueue is only drained
> later, in mtk_cldma_dev_exit().
>
> Separately, mtk_cldma_drv_quiesce() uses flush_workqueue() rather than
> drain_workqueue(). Would an rx_done_work queued by err_work during the
> flush be missed?
>
> The commit message says "only then does it join the trb service threads
> and let mtk_cldma_exit() free what they dereference". With the paths
> above, that guarantee does not seem to hold.

Will fix in v11, at the producer: mtk_cldma_rearm_queues() will bail out
when the device is quiesced, and mtk_cldma_drv_quiesce() will use
drain_workqueue() instead of flush_workqueue().

Both of your questions are yes.  The window is that quiesce flushes the
workqueue while the trb service thread is still running, so a TRB it drains
afterwards reaches start_xfer, hw_recovery and rearm_queues, which queues
tx_done_work without consulting quiesced; trb_srv_exit() then frees srv,
and the work wakes a pointer it sampled while it was still non-NULL.  The
`&& srv` test cannot help for that reason.  And flush_workqueue() waits
only for work already queued, so err_work queueing rx_done_work during the
flush is missed - drain_workqueue() is the right call, though not
sufficient on its own, since nothing stops the trb thread queueing more
afterwards.  Hence both halves.

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

netdev-ai wrote:
> Can blocking writes grow the HIF TX skb_list without bound here?
>
> In blocking mode, force_send starts out true. That means even the first
> packet skips the depth check in mtk_pcie_hif_submit_skb():
>
> 	if (mtk_queue_list_is_full(trans, que) && !force_send) {
> 		ret = -EAGAIN;
> 		goto unlock;
> 	}
>
> This doesn't match the comment above MTK_PORT_MAX_TX_PKT. It also doesn't
> match the commit message, which says "every packet after the first is
> submitted with force_send, which bypasses the depth check".
>
> In blocking mode, the only throttle left is the
> wait_event_interruptible_timeout() in mtk_port_send_data(). If a signal is
> pending, it returns -ERESTARTSYS right away, and that gets mapped to
> -EINTR.
>
> The loop above treats -EINTR as accepted and keeps going. So all the
> remaining packets (up to MTK_PORT_MAX_TX_PKT) are force-submitted without
> waiting, and the write reports success.
>
> Blocking mode never calls mtk_port_wwan_tx_pause(), so wwan_wait_tx() never
> blocks the next write() either. A writer that keeps getting signals could
> add up to 16 skbs of about 3.5K each per write() call. For example, another
> thread could call tgkill() in a loop, or a fast itimer with a handler could
> be running.
>
> When mtk_cldma_submit_tx() returns -EAGAIN for a full GPD ring, the TX
> dispatch in the TRB service leaves the skb on skb_list. If the modem drains
> slowly or stalls, does anything stop that list from growing until memory
> runs out? These skbs come from __dev_alloc_skb(GFP_KERNEL) and are not
> charged to a memcg.
>
> Non-blocking writes look bounded, because their first packet is depth
> checked.
>
> Would one of these work instead?
>
> - depth check the first packet in blocking mode too
> - stop submitting the rest of the message once -EINTR is seen
> - add a hard cap in the HIF that applies even with force_send

Will fix in v11, with the first of those plus a wait: the first packet gets
depth checked again, and in blocking mode the write waits for HIF queue
space before it starts submitting instead of bypassing the check.  The
comment and the commit message then match the code as they are written.

Thanks,
Jack

pw-bot: cr