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

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

Thanks for the review.

On Sun, 4 Oct 2026 17:12:00 +0800 netdev-bot+sashiko@kernel.org wrote:

> Should this also depend on PCI_MSI?
>
> mtk_pci_request_irq() asks for exactly 32 MSI-X vectors and has no INTx
> fallback:
[...]
> With CONFIG_PCI_MSI=n, the pci_alloc_irq_vectors_affinity() stub in
> include/linux/pci.h returns -ENOSPC for any request other than a single
> INTx vector.

Will fix in v10 - "depends on PCI && ACPI" becomes "depends on PCI_MSI
&& ACPI".  PCI_MSI depends on PCI, so the PCI dependency is kept.

> This may be fine. The mask in mtk_pci_mask_irq() is a posted iowrite32(),
> and it is not read back before synchronize_irq().
[...]
> The one remaining way for the source to be re-enabled is the unmask in
> mtk_mhccif_isr_work(). That is covered in the comment on mtk_pci_remove()
> below.

Your reading is correct.  On PCIe a read cannot pass a previously posted
write to the same function, so the two reads at the top of
mtk_pci_irq_msix() flush the mask before the handler examines a single
bit.  A read-back inside mtk_pci_mask_irq() would add an MMIO round trip
to the interrupt path to re-establish that.

The unmask you point at is the teardown ordering item below, not a
posted-write problem, and the reorder there removes it.

> This isn't a bug, but MTK_PCI_VENDOR_ID (0x14C3) in mtk_pci.h duplicates
> PCI_VENDOR_ID_MEDIATEK from include/linux/pci_ids.h. Could the table use
> PCI_VENDOR_ID_MEDIATEK, as t7xx does?

Will fix in v10 - the first table entry uses PCI_VENDOR_ID_MEDIATEK and
the MTK_PCI_VENDOR_ID define is deleted.

> Is CEI_PCI_VENDOR_ID the right name for 0x03F0? Other in-tree drivers
> treat that vendor ID as HP.
[...]
> The commit message also mentions only the T900 device. It does not say
> that the driver binds this second vendor/device ID, which becomes visible
> through MODULE_DEVICE_TABLE and module autoloading.

Will fix in v10, following t7xx: the define goes away and the entry
becomes the literal 0x03f0 with a product comment.  "CEI" names the ODM,
not the holder of 0x03F0.  The commit message gains a sentence saying the
driver binds the T900 and its HP-branded variant.

> Should this be pci_bus_address(pdev, MTK_BAR_2_3_IDX)?
>
> pci_resource_start() returns the CPU physical address of the BAR.
[...]
> Some hosts have a bridge window with a non-zero CPU-to-bus offset (see
> pcibios_resource_to_bus()). On those hosts, would the window fail to
> match BAR2 accesses?

Will fix in v10 - cfg.src_addr = pci_bus_address(pdev,
MTK_BAR_2_3_IDX).  The offset is zero on the x86 laptops this modem ships
in, so both failure modes you describe are latent rather than observed.

> Is 0x3080 the right upper bound? mtk_pci_reg.h defines
> REG_IMASK_HOST_MSIX_GRP0_0 as 0x3100, and mtk_pci_irq_msix() reads it on
> every interrupt:
[...]
> That makes the highest BAR0 offset accessed 0x3103.

Will fix in v10 - the bound in that comment becomes 0x3103.  0x3080 is
REG_IMASK_HOST_MSIX_CLR_GRP0_0, the write side; the comment should have
named its read-side pair, as you say.  The comment itself stays, for the
reason below.

> Can a device with smaller BARs cause out-of-bounds MMIO here?
>
> pcim_iomap_region() maps only the BAR length the device reports, and
> nothing checks pci_resource_len().
[...]
> Could both BAR lengths be checked before they are used?

AI is wrong.  The guidance we were given for this series says, at
lines 119-120 of driver-upstreaming-prompts.md:

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

A device presenting our IDs with a 4 KiB BAR2 is not a conforming device,
so it is the malicious case that rule puts out of scope.

This was declined in v9 on the same grounds, and the comment in
mtk_pci_bar_init() is the "mark the condition as impossible" marker that
was asked for in place of a check.  It stays; only its number was wrong,
which is the item above.

> Can the MHCCIF vector fire again while mtk_mhccif_exit() runs, or after
> it returns?
>
> mtk_mhccif_isr_work() always ends by unmasking the source:
>
>         mtk_pci_clear_irq(mdev, priv->mhccif_irq_id);
>         mtk_pci_unmask_irq(mdev, priv->mhccif_irq_id);
[...]
> The probe error path calls mtk_pci_free_irq() before mtk_mhccif_exit().
> Would using that order in remove avoid this?
>
> The ordering is unchanged at the end of the series, where
> mtk_pci_dev_exit() is added in front of it.

Will fix in v10, with the order you suggest: remove() calls
mtk_pci_free_irq() before mtk_mhccif_exit(); mtk_pci_free_irq() also
clears priv->irq_cnt; and mtk_pci_unregister_irq() publishes the NULL
callback before synchronize_irq() and clears the data after it, so no
handler can pair a live callback with NULL data.

One correction: the second scenario - work queued after
cancel_work_sync() has returned - is not reachable.
mtk_pci_unregister_irq() clears irq_cb_list[28] before cancel_work_sync()
is called and nothing re-publishes it, so every handler in that window
takes the dev_err_ratelimited() branch and never reaches queue_work().
There is no use-after-free after remove() returns.  The NULL-data
scenario is real, and it is what the changes above close.

> Does the commit message match this code? It says:
>
>   There is no software reset that returns the device from running
>   firmware to a state the next probe can boot, ... so mtk_pci_remove()
>   evaluates MRST._RST on the device's own ACPI node.
>
> This code also has a fallback. When mtk_pci_pldr() fails and the link is
> still up, remove rings the MHCCIF DEV_EVT_H2D_DEVICE_RESET doorbell,
> which is a software reset.
[...]
> If the doorbell cannot restore a bootable state, isn't rebind broken on
> every platform without MRST._RST?
>
> If the doorbell can restore it, is the commit message's reason for doing
> the ACPI reset in remove accurate?
>
> Probe also never checks that MRST._RST exists.

Will fix in v10 - the commit message, not the code.  The accurate
statement is not that no software reset exists but that none is reliable:
MRST._RST works whatever the firmware is doing, while
DEV_EVT_H2D_DEVICE_RESET is a request to that firmware and so is useless
exactly when it is wedged.

Your three questions, all covered by the rewritten paragraph: rebind
degrades to the doorbell without MRST._RST rather than breaking; the
preference for the ACPI reset holds once stated as reliability rather
than existence; and probe deliberately does not require MRST._RST, since
a missing method only makes removal best effort.  The three
"Unsupported, ..." dev_err() calls become dev_dbg() so that an unbind on
such a platform stops logging errors.

> Does pci_load_and_free_saved_state() restore anything in hardware here?
>
> According to the comment above, the reset brings the endpoint back at
> power-on defaults, so its BARs and command register are cleared.
> pci_load_and_free_saved_state() only reloads pdev->saved_config_space and
> writes nothing to the device.
[...]
> A different driver bound afterwards (for example vfio-pci through
> driver_override) would see BARs the device does not decode, because only
> this driver's probe restores them.
>
> With the MHCCIF fallback, the reset is only a doorbell with no delay and
> no readiness wait. Could the device reset in the middle of the next probe
> and wipe the ATR and MSI-X mask setup?

Will fix in v10 - pci_restore_state(pdev) after
pci_load_and_free_saved_state(), and an msleep() on the asynchronous
doorbell path to let the device settle before it, as t7xx does on its own
asynchronous path.  It restores nothing today and all three consequences
follow.  The comment above the reset is corrected with it: no MMIO may
follow, but the config restore deliberately does.

> Is a .shutdown callback needed here?
>
> On reboot or kexec, remove is not called, so the modem stays in running
> firmware.
[...]
> Without IOMMU translation, could a DMA engine left armed by the previous
> kernel (such as CLDMA with stale descriptor addresses) resume transfers
> into host memory the new kernel owns?
>
> The probe error paths also never reset the device. t7xx provides
> t7xx_pci_shutdown() for this.

It is needed, but not in this series.  .shutdown is a power-management
callback: in our tree it is one line into the same suspend path the
dev_pm_ops entries use - pause the FSM, suspend the PM entities, ring
EXT_EVT_H2D_PCIE_PM_SUSPEND_REQ and wait for the modem to acknowledge.
This series has no dev_pm_ops and none of that machinery, so a bespoke
reset-based .shutdown added here would be deleted again when the PM
series lands.  It is registered there, next to .driver.pm.

On the DMA question: an armed engine is stopped by clearing bus
mastering, and pci_device_shutdown() already does that on kexec
(drivers/pci/pci-driver.c), so stale CLDMA descriptors cannot resume on
their own.  What .shutdown adds is leaving the modem in a state the next
kernel's probe can hand-shake with, which is the PM problem above.

pw-bot: cr