RE: [External Mail] Re: [PATCH v4 1/7] net: wwan: t9xx: Add PCIe core

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

Hi Simon,

> > +static irqreturn_t mtk_pci_irq_msix(int irq, void *data)
> > +{
> > +	if (unlikely(!irq_state) ||
> > +	    unlikely(!((irq_state & GENMASK(priv->irq_cnt - 1, 0)) &
> > +		      irq_desc->msix_bits)))
> > +		return IRQ_NONE;
>
> [Severity: High]
> Does this logic silently drop hardware interrupts when fewer than 32
> vectors are allocated?

No interrupts are dropped. This is the MSI-X merged mode design.
When fewer vectors are allocated, mtk_pci_set_msix_merged() configures
the hardware to merge all interrupt sources into the available vectors.

For example, with irq_cnt=1, the hardware maps ALL sources (including
MHCCIF at hardware bit 28) to vector 0. The MSI-X status register
shows BIT(0), not BIT(28). So irq_desc[0].msix_bits = BIT(0), and
GENMASK(0,0) & BIT(0) & BIT(0) = 1 — the check passes correctly.

The GENMASK filters to allocated vector bits only, then checks if the
current handler's vector is set. This works for both full allocation
(32 vectors, 1:1 mapping) and merged mode (fewer vectors, N:1 mapping).

> > +		ret = pci_request_irq(pdev, i, mtk_pci_irq_msix, NULL,
> > +				      &irq_desc[i], irq_desc[i].name);
>
> [Severity: Medium]
> Could this trigger a -Wformat-security compiler warning?

We will change to pci_request_irq(..., "%s",
irq_desc[i].name) to avoid format string issues.

> > +	hw_bits = mtk_pci_ext_h2d_evt_hw_bits(ch);
> > +	mtk_pci_write32(mdev, rc_base + MHCCIF_RC2EP_SW_TCHNUM,
> > +			ffs(hw_bits) - 1);
>
> [Severity: Medium]
> Will this break channel index calculation on big-endian systems?

This driver targets MediaTek T9xx PCIe WWAN modems on x86/ARM64-LE
platforms only. On little-endian, cpu_to_le32() is a no-op and
LE32_TO_U32() is a simple cast — no byte-swap occurs. The Kconfig
enforces depends on PCI && ACPI, effectively restricting to LE
platforms where this hardware exists.

> [Severity: Medium]
> What happens if the driver attempts to send an unmapped channel?

All callers of mtk_pci_send_ext_evt() pass known-valid channel values
that are mapped in SET_HW_BITS. The is_power_of_2(ch) check at the
top of the function rejects invalid input. The FSM single-thread
design ensures all ext_evt sends go through a controlled path, so
an unmapped channel cannot reach ffs().

> > +	priv->ext_reg_base = priv->bar23_addr - ATR_PCIE_REG_TRSL_ADDR;
>
> [Severity: Medium]
> Is it safe to store an out-of-bounds pointer?

This is a standard pattern in PCIe MMIO drivers using Address
Translation Registers (ATR). The modem's internal register space
starts at address 0x0 in its own view, but the host accesses it
through BAR2/3 mapped at offset ATR_PCIE_REG_TRSL_ADDR (0x10000000).

Pre-computing ext_reg_base = bar23_addr - 0x10000000 allows using
modem register addresses directly: ioread32(ext_reg_base + reg_addr).
ext_reg_base is typed as void __iomem *, an opaque cookie for MMIO
access — never dereferenced as a regular pointer. The ioread32/
iowrite32 functions perform platform-specific MMIO access on it.
UBSAN does not flag __iomem arithmetic. This same pattern is used
by other kernel PCIe drivers (e.g., mtk_t7xx).

> > +static void mtk_pci_remove(struct pci_dev *pdev)
> > +{
> > +	if (mtk_pci_pldr(mdev)) { ... }
> > +	pci_clear_master(pdev);
> > +	mtk_pci_free_irq(mdev);
> > +	mtk_mhccif_exit(mdev);
>
> [Severity: High]
> Does this sequence result in the device being powered off before
> the OS interrupt handlers and workqueues are synchronized?

The device is not left powered off. mtk_pci_pldr() calls ACPI
PXP._OFF then PXP._ON — the _ON call restores power and brings
the PCIe link back up. After _ON returns, the device is powered
on and BARs are accessible.

The PLDR sequence is: (1) _OFF: power-cycle modem firmware,
(2) msleep for power drain, (3) _ON: restore power and link.
After step 3, the device is live with a fresh firmware state.

Additionally, before PLDR, mtk_pci_mask_irq() disables the MHCCIF
interrupt at hardware level, preventing new interrupts. Between
pci_clear_master and mtk_pci_free_irq, mtk_pci_dev_exit() tears
down FSM and trans_ctrl, ensuring no workqueues remain scheduled.

> > +	/* Request a slot reset. */
> > +	return PCI_ERS_RESULT_CAN_RECOVER;
>
> [Severity: Medium]
> Will returning PCI_ERS_RESULT_CAN_RECOVER actually abort the slot
> reset?

We will change to PCI_ERS_RESULT_DISCONNECT and update
the comment, since the driver does not currently support AER
recovery.

Thanks.

Jack Wu