Re: [PATCH v6 1/7] net: wwan: t9xx: Add PCIe core

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

On Mon, 17 Aug 2026 at 22:35, Jakub Kicinski <kuba@kernel.org> wrote:
> [Severity: Medium]
> Is the RGU part accurate for this patch? Searching for rgu (case
> insensitive) under drivers/net/wwan/t9xx/ returns nothing, either at this
...
> Could the RGU wording be dropped or moved to the patch that actually adds
> it?

Will fix in v7. The RGU bullet and the "Reset Generation Unit (RGU)"
mention in the intro sentence will be removed from the commit message.
There is no RGU code in this series.

> [Severity: Low]
> What uses devlink here? Grepping for devlink under drivers/net/wwan/t9xx/
> finds no match at this commit, and none at the tip of the series either.
...
> Can it be dropped until a devlink user
> appears?

Will fix in v7. The `select NET_DEVLINK` will be removed from Kconfig.

> [Severity: Medium]
> Should ATR_EN be written last in mtk_pci_setup_atr()? The SRC_ADDR_LSB write
> sets the enable bit while TRSL_ADDR_MSB/LSB and TRSL_PARAM still hold their
> previous values, so the table is live with a stale translation target for
...
> Would writing TRSL_ADDR and TRSL_PARAM first, then SRC_ADDR_LSB with
> ATR_EN, followed by a read back, be safer here too?

Will fix in v7. Will reorder the writes so that TRSL_ADDR and
TRSL_PARAM are programmed before SRC_ADDR_LSB (which carries ATR_EN),
and add a read-back after the enable write.

> [Severity: Medium]
> Does anything restore REG_PCIE_CFG_MSIX? This is the only write to that
> register, and it happens only when irq_cnt != MTK_IRQ_CNT_MAX in
> mtk_pci_request_irq_msix().
...
> so can a rebind end up with 32
> independent per-vector descriptors while the device is still in merged
> routing mode?

Will fix in v7. Confirmed with vendor that T9xx does not require
merged mode. The entire merged-mode path (including
mtk_pci_set_msix_merged and REG_PCIE_CFG_MSIX writes) will be
removed — see the merged-mode comment below for details.

> [Severity: Medium]
> Can a hardirq observe a half published callback pair here? The callback
> pointer is stored before its data pointer, with no lock, no WRITE_ONCE and
> no barrier, while mtk_pci_irq_handler() reads and dereferences both from
...
> Would publishing data before the callback pointer, and taking a lock over
> both arrays, be preferable?

Will fix in v7. Will swap the store order (data first, then
callback with WRITE_ONCE), and add READ_ONCE + smp_rmb in
the handler.

> [Severity: Medium]
> What happens when ch is a declared channel that has no hardware mapping?
...
> so 0xFFFFFFFF is written to the channel number register and 0 (success) is
> returned to the caller.
...
> mapped D2H channels), so the bogus write is not reachable today. Should
> mtk_pci_send_ext_evt() return an error when hw_bits is 0, and should the
> enums be trimmed to the channels that are actually translated?

Will fix in v7. Will add a `if (!hw_bits) return -EINVAL;` guard
after the translation call.

> [Severity: Low]
> Is it intended that consumer callbacks run in atomic context under
> mhccif_lock? A callback that sleeps is not allowed here, and a callback that
...
> Neither touches mhccif_cb_list. Could the callback contract be documented,
> or the callbacks invoked outside the lock?

The callbacks are intentionally lightweight — they only perform
MMIO reads/writes and call mtk_fsm_evt_submit() which detects
atomic context via in_softirq() and uses GFP_ATOMIC accordingly.
No callback in the series modifies the callback list or sleeps.

The spinlock protects the list traversal against concurrent
register/unregister from other contexts, not against the
callbacks themselves. Since no callback touches mhccif_cb_list,
list_for_each_entry (without _safe) is safe here.

Will add a comment documenting the callback contract: callbacks
must not sleep or modify the callback list.

> [Severity: Low]
> Should mtk_mhccif_exit() also drain priv->mhccif_cb_list? The nodes are
...
> mtk_fsm_exit() and mtk_fsm_hs_info_exit(). Would freeing any remaining
> nodes in mtk_mhccif_exit() make the teardown self contained?

Will fix in v7. Will add a list_for_each_entry_safe drain loop
in mtk_mhccif_exit() to free any remaining callback nodes.

> [Severity: Medium]
> Should these two reads be validated for the device-gone case? When the
> endpoint is unreachable (surprise removal, FLDR/PLDR in progress, link
...
> 'stat == U32_MAX && !mtk_pci_link_check(mdev)' - should the MSI-X handler do
> the same?

Will fix in v7. Will add a `if (unlikely(irq_state == U32_MAX &&
irq_enable == U32_MAX)) return IRQ_NONE;` check at the top of
mtk_pci_irq_msix().

> [Severity: High]
> Can the same consumer callback run concurrently on two CPUs here? The gate
...
> With DPMAIF (bit 24) and CLDMA0 (bit 27) pending at the same time, two
> MSI-X vectors fire on two CPUs, both latch the same irq_state (the status
> read and the mask write are not atomic and nothing serialises the handlers),
> and both dispatch all pending callbacks.
...
> can still be running mtk_cldma_isr(irq_id, drv_info) after the free. Would
> passing 'irq_state & irq_desc->msix_bits' to mtk_pci_irq_handler(), or
> registering one handler per vector as t7xx_request_irq() does, avoid this?

Will fix in v7. Resolved together with the merged-mode issue below.

> [Severity: Medium]
> Is there a window where a handler runs before irq_cnt and irq_type are
> published?
...
> Could irq_cnt and irq_type be set before the
> first pci_request_irq() call?

Will fix in v7. Resolved together with the merged-mode issue below.

> [Severity: High]
> Does the dispatch path work at all when fewer than 32 vectors are obtained?
...
> the descriptors keep msix_bits = BIT(i) and mtk_pci_irq_handler() indexes
> priv->irq_cb_list[] by the raw device bit number.
...
> mapping. Would requiring an exact vector count, as t7xx_setup_msix() does
> with pci_alloc_irq_vectors(pdev, EXT_INT_NUM, EXT_INT_NUM, PCI_IRQ_MSIX), be
> the simpler contract?

Will fix in v7. Confirmed with the vendor: T9xx does not require
merged mode. The T9xx and T7xx share the same PCIe MAC architecture,
differing only in the number of interrupt sources.

Will adopt the t7xx model:
  - Require exactly MTK_IRQ_CNT_MAX vectors (min == max)
  - Remove mtk_pci_set_msix_merged() and all rounding logic
  - Set irq_cnt/irq_type before pci_request_irq loop (fixes the
    irq_cnt timing issue above)
  - Scope dispatch to this vector's bits:
    irq_state &= irq_desc->msix_bits (fixes the concurrent
    dispatch issue above)
  - Remove the irq_id % irq_cnt in mtk_pci_get_virq_id()

This resolves the REG_PCIE_CFG_MSIX restore concern, the
concurrent dispatch, and the irq_cnt/irq_type timing issue
together as one coherent rewrite.

> [Severity: Medium]
> Can mhccif_work still be queued or running when the device is reset here?
...
> Would mask, then free/quiesce the IRQ, then cancel_work_sync(), then reset
> be the right order?

Will fix in v7. Will add cancel_work_sync(&priv->mhccif_work)
between mask_irq and mtk_pci_pldr() in mtk_pci_remove().

Thanks.

--
Jack