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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 4 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The modem is reset only on the way down, in mtk_pci_remove()
  and in mtk_pci_shutdown() (which calls mtk_pci_remove()).
- [Medium] In mtk_pci_remove(), which every shutdown also reaches through
  mtk_pci_shutdown(), the MHCCIF doorbell fallback runs when…
- [Medium] mtk_pci_bar_init() maps BAR0/1 and BAR2/3 with
  pcim_iomap_region(), which maps exactly the BAR length the device…
- [Medium] mtk_pci_request_irq() allocates exactly 32 MSI-X vectors.
- [Low] mtk_pci_bar_init() passes mdev->dev_str as the @name argument to
  both pcim_iomap_region() calls.
- [Low] mtk_pci_remove() writes U32_MAX to
  REG_IMASK_HOST_MSIX_CLR_GRP0_0, then calls mtk_pci_free_irq(), and…

--

net: wwan: t9xx: Add PCIe core

This adds the PCIe core of the MediaTek T9xx 5G WWAN modem driver. It
covers probe and remove, MSI-X setup, the MHCCIF event interface and the
register access helpers. Remove and shutdown reset the modem through the
ACPI MRST._RST method, with an MHCCIF reset doorbell as the fallback.

> diff --git a/drivers/net/wwan/t9xx/pcie/mtk_pci.c b/drivers/net/wwan/t9xx/pcie/mtk_pci.c
> new file mode 100644
> index 0000000000000..902bcbe1512e9
> --- /dev/null
> +++ b/drivers/net/wwan/t9xx/pcie/mtk_pci.c
> @@ -0,0 +1,1044 @@

[ ... ]

> +static int mtk_pci_bar_init(struct mtk_md_dev *mdev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(mdev->dev);
> +	struct mtk_pci_priv *priv = mdev->hw_priv;
> +
> +	/* Fixed offsets used on these mappings (MSI-X registers up to
> +	 * 0x3103 on BAR0/1, the ATR-biased MHCCIF window on BAR2/3) are
> +	 * guaranteed by the only two devices this driver binds (0x0900,
> +	 * 0x01CA), whose BAR sizes are fixed by hardware.  A device that
> +	 * misreports them is out of scope rather than impossible, so no
> +	 * pci_resource_len() checks.
> +	 */
> +	priv->mac_reg_base = pcim_iomap_region(pdev, MTK_BAR_0_1_IDX,
> +					       mdev->dev_str);

[Severity: Medium]
pcim_iomap_region() maps exactly the BAR length the device reports. Binding
only needs a vendor/device/class match. So what happens with a device that
spoofs 14c3:0900 or 03f0:01ca and reports small BARs, for example a
hot-plugged Thunderbolt/USB4 device? It would get these fixed-offset
accesses:

  mtk_pci_probe() writes U32_MAX at BAR0 + 0x3080
  mtk_pci_irq_msix() reads BAR0 + 0x0F00 and BAR0 + 0x3100
  mtk_mhccif_init() writes U32_MAX at bar23_addr + 0xA030 (via ext_reg_base)

The comment treats such a device as out of scope. Still, can these writes
land past the ioremap area and its guard page, in neighbouring ioremap or
vmalloc mappings?

Two pci_resource_len() checks (BAR0 >= 0x3104, BAR2 >= 0xA044) look like
enough to rule this out.

> +	if (IS_ERR(priv->mac_reg_base)) {
> +		dev_err(mdev->dev, "Failed to map BAR0/1\n");
> +		return PTR_ERR(priv->mac_reg_base);
> +	}
> +
> +	priv->bar23_addr = pcim_iomap_region(pdev, MTK_BAR_2_3_IDX,
> +					     mdev->dev_str);

[Severity: Low]
Is mdev->dev_str the intended name argument for both pcim_iomap_region()
calls? The kernel-doc in drivers/pci/devres.c describes @name as "Name of
the driver requesting the resource". It becomes the resource name in
/proc/iomem and in resource conflict messages.

dev_str is a copy of pci_name(pdev), so the BARs get labelled with the PCI
address (e.g. 0000:01:00.0) instead of mtk_t9xx. t7xx passes DRIVER_NAME
to the same API. Would KBUILD_MODNAME work here?

[ ... ]

> +static int mtk_pci_request_irq(struct mtk_md_dev *mdev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(mdev->dev);
> +	int ret;
> +
> +	ret = pci_alloc_irq_vectors(pdev, MTK_IRQ_CNT_MAX,
> +				    MTK_IRQ_CNT_MAX, PCI_IRQ_MSIX);
> +	if (ret < 0) {
> +		dev_err(mdev->dev,
> +			"Unable to alloc %d MSI-X vectors: ret=%d\n",
> +			MTK_IRQ_CNT_MAX, ret);
> +		return ret;
> +	}
> +
> +	ret = mtk_pci_request_irq_msix(mdev);
> +	if (ret)
> +		pci_free_irq_vectors(pdev);
> +
> +	return ret;
> +}

[Severity: Medium]
Should the device's MSI-X vector configuration be programmed here? The
handler only takes status bit i on vector i:

mtk_pci_irq_msix() {
    ...
	irq_state &= irq_desc->msix_bits; /* scope to this vector */
	if (unlikely(!irq_state))
		return IRQ_NONE;
    ...
}

Nothing writes the MAC register that tells the device how many host
vectors exist.

The register map matches t7xx: ISTATUS at 0x0F00, IMASK SET/CLR at
0x3000/0x3080, and the status dummy register at 0x0D1C. t7xx writes
T7XX_PCIE_CFG_MSIX (0x03ec) with ffs(irq_count) * 2 - 1 after allocating
its vectors:

t7xx_setup_msix()
  t7xx_pcie_set_mac_msix_cfg(t7xx_dev, EXT_INT_NUM)

It writes it again in t7xx_pcie_interrupt_reinit().

Here the 1:1 layout depends on the register's reset or leftover value. If
that value merges sources onto fewer vectors, would the MHCCIF source
(bit 28) land on a vector whose handler drops it and returns IRQ_NONE?

That would leave the boot handshake unserviced. The spurious IRQ detector
could also end up disabling the line.

[ ... ]

> +static void mtk_pci_remove(struct pci_dev *pdev)
> +{
> +	struct mtk_md_dev *mdev = pci_get_drvdata(pdev);
> +	struct mtk_pci_priv *priv = mdev->hw_priv;
> +	struct device *dev = &pdev->dev;
> +	int i, ret;
> +
> +	/* Silence every source before tearing anything down. */
> +	mtk_pci_mac_write32(priv, REG_IMASK_HOST_MSIX_CLR_GRP0_0, U32_MAX);
> +
> +	/* Free the vectors first: once no handler can run, the unmask at the
> +	 * tail of mtk_mhccif_isr_work() cannot re-open the source behind
> +	 * mtk_mhccif_exit()'s back.
> +	 */
> +	mtk_pci_free_irq(mdev);
> +	mtk_mhccif_exit(mdev);

[Severity: Low]
Does this ordering give the guarantee the comment describes?
cancel_work_sync() only runs later, inside mtk_mhccif_exit(). So a
mhccif_work queued before the mask-all write can still run after
mtk_pci_free_irq(), and it ends with:

mtk_mhccif_isr_work() {
    ...
	mtk_pci_unmask_irq(mdev, priv->mhccif_irq_id);
}

This writes BIT(28) to REG_IMASK_HOST_MSIX_SET_GRP0_0. By then
pci_free_irq_vectors()->pci_msix_shutdown() has cleared
PCI_MSIX_FLAGS_ENABLE and called pci_intx_for_msi(dev, 1). The source is
unmasked with INTx enabled until mtk_mhccif_exit() masks it again.

There is a second window between the mask-all write and
mtk_pci_free_irq(). A pending work can unmask the source there, take a new
MSI-X interrupt and requeue itself.

The end state is masked. Would calling mtk_mhccif_exit() before
mtk_pci_free_irq() close both windows? While the vectors still exist,
mtk_pci_unregister_irq() gets a valid virq for synchronize_irq(), and
cancel_work_sync() plus the final mask finish before the vectors are
freed.

> +	pci_clear_master(pdev);
> +	pci_disable_device(pdev);
> +
> +	/* Reset last: the endpoint comes back at power-on defaults, so no MMIO
> +	 * and no MSI-X teardown may follow it.
> +	 */
> +	ret = mtk_pci_pldr(mdev);
> +	if (!ret) {
> +		/* MRST._RST may return before the link has retrained, and a
> +		 * config write the device does not take is simply lost.
> +		 */
> +		for (i = 0; i < MTK_PLDR_READY_POLLS && !mtk_pci_link_check(mdev); i++)
> +			msleep(MTK_PLDR_READY_POLL_MS);
> +		if (!mtk_pci_link_check(mdev))
> +			dev_warn(dev, "device not back %d ms after reset, its config space may not be restored\n",
> +				 MTK_PLDR_READY_POLLS * MTK_PLDR_READY_POLL_MS);
> +	} else if (!mtk_pci_link_check(mdev)) {
> +		dev_warn(dev, "PLDR failed (%d) and link is down, modem not reset\n", ret);
> +	} else {
> +		dev_warn(dev, "PLDR failed (%d), trying MHCCIF reset\n", ret);
> +		if (mtk_pci_send_ext_evt(mdev, DEV_EVT_H2D_DEVICE_RESET)) {
> +			dev_err(dev, "MHCCIF reset failed\n");
> +		} else {
> +			/* Unlike MRST._RST the doorbell returns before the device
> +			 * has reset, so let it settle before the restore below.
> +			 */
> +			msleep(MTK_MHCCIF_RESET_DELAY_MS);
> +		}
> +	}

[Severity: Medium]
The PLDR branch polls mtk_pci_link_check() because "a config write the
device does not take is simply lost", and it warns if the device has not
come back. The doorbell branch only sleeps a fixed 2000 ms and then goes
on to the restore.

If the doorbell reset takes the endpoint off the link and it has not
retrained within 2 s, are the pci_restore_state() writes below silently
dropped?

pci_load_and_free_saved_state() has already freed priv->saved_state and
set it to NULL, and pci_restore_state() clears dev->state_saved. Nothing
is left to retry with. Would the next bind then see BARs the device does
not decode, with nothing in the log?

Probe allows platforms without MRST._RST, and there every remove and
shutdown takes this branch. Could it use the same link poll and warning
as the PLDR branch?

> +
> +	/* Without this a runtime-PM suspend after unbind would snapshot the
> +	 * reset state over our copy, and whoever binds next finds BARs the
> +	 * device does not decode.
> +	 */
> +	pci_load_and_free_saved_state(pdev, &priv->saved_state);
> +	pci_restore_state(pdev);
> +}
> +
> +/* A kexec reboot neither resets nor powers down the slot, and the boot
> + * notification the next kernel waits for is one-shot, so reset the modem
> + * on the way down the same way remove() does.
> + */
> +static void mtk_pci_shutdown(struct pci_dev *pdev)
> +{
> +	mtk_pci_remove(pdev);
> +}

[Severity: Medium]
The commit message says the shutdown reset is needed because "a kexec
reboot gets no platform reset and the next kernel would otherwise find
that firmware still running". Is crash kexec (kdump, kexec -p) covered?
That path is:

__crash_kexec()
  machine_crash_shutdown()
  machine_kexec()

It never calls device_shutdown(), so mtk_pci_shutdown() does not run. The
capture kernel then probes a modem whose firmware is still running.

By then the crashed kernel's mtk_mhccif_isr_work() has already acked
BOOT_FLOW_SYNC, and the comment in mtk_mhccif_init() says:

	 * Do NOT ack here: EP2RC status bits are latched one-shot
	 * notifications (e.g. BOOT_FLOW_SYNC raised before the driver
	 * loads) that the device never re-sends.

mtk_pci_probe() neither resets the modem nor checks whether firmware is
already running. Would the boot flow wait for a BOOT_FLOW_SYNC that never
comes? Later in the series, mtk_fsm_dev_add_act() moves to FSM_STATE_ON,
unmasks BOOT_FLOW_SYNC and waits.

The same state also seems reachable after mtk_pci_remove() if the reset
does not take effect. That covers a down link, or no MRST._RST combined
with firmware that ignores the doorbell. The commit message says the
doorbell "cannot recover firmware that has stopped responding - the case
the reset exists for".

Resetting the modem, or detecting running firmware, in probe would cover
these cases. It would also remove the 1-2 s wait that every reboot and
power-off now has, which still does not help kdump.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105-t9xx_driver_v1-v11-0-2084aa512864%40compal.com