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

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: wwan: t9xx: Add PCIe core

This adds the T9xx PCIe layer: probe/remove, MSI-X setup and teardown,
the MHCCIF doorbell channels used for the boot handshake, and the modem
register/interrupt helpers.  Removal resets the endpoint by evaluating
MRST._RST on the device's ACPI node, falling back to the MHCCIF reset
doorbell.

A few questions on the interrupt teardown ordering, the reset path and a
couple of smaller items below.

[ ... ]

[ ... ]

[Severity: Low]
Should the three helpers agree on what makes an irq_id valid?

mtk_pci_mask_irq(), mtk_pci_unmask_irq() and mtk_pci_clear_irq() bound
irq_id by MTK_IRQ_CNT_MAX and additionally require
priv->irq_type == PCI_IRQ_MSIX, while mtk_pci_get_virq_id() bounds the
same value by priv->irq_cnt.  mtk_pci_unregister_irq() has to satisfy
both.

On the probe failure path that jumps to free_mhccif, either
pci_alloc_irq_vectors() or mtk_pci_request_irq_msix() failed, so
priv->irq_type is 0 and priv->irq_cnt is 0.  mtk_mhccif_exit() then
calls mtk_pci_unregister_irq() -> mtk_pci_mask_irq(), which logs

	dev_err(mdev->dev, "Failed to mask irq: input irq_id=%d\n", irq_id);

for irq_id 28 during otherwise correct cleanup, and
mtk_pci_get_virq_id() rejects the id the other helpers consider in
range.  Would it be clearer to document the irq_type precondition and
have the mask/unmask/clear helpers return quietly (or use dev_dbg) when
no vectors are allocated?

[ ... ]

[ ... ]

[ ... ]

[Severity: Low]
Can this error return leak buffer.pointer?

The result object is never used here, but ACPI_ALLOCATE_BUFFER is
requested, and acpi_evaluate_object() can fail with the caller buffer
already allocated:

drivers/acpi/acpica/nsxfeval.c:acpi_evaluate_object() {
	...
	status = acpi_ut_initialize_buffer(return_buffer, buffer_space_needed);
	if (ACPI_FAILURE(status)) {
		...
	} else {
		status = acpi_ut_copy_iobject_to_eobject(info->return_object, return_buffer);
	}
	...
}

If acpi_ut_copy_iobject_to_eobject() fails after the buffer was
allocated (for example AE_NO_MEMORY while building a package), the
-EIO return here skips acpi_os_free().  Since the returned object is
unused, would passing NULL as the return buffer be simpler?

[ ... ]

+static void mtk_mhccif_isr_work(struct work_struct *work)
+{

[ ... ]

[Severity: Medium]
Can this tail re-enable the MSI-X source after teardown has masked it?

Nothing appears to serialize the worker against removal:

CPU0                              CPU1
mtk_mhccif_isr_work()
  ... past spin_unlock_bh()
                                  mtk_pci_remove()
                                    write REG_IMASK_HOST_MSIX_CLR_GRP0_0 = U32_MAX
                                    mtk_mhccif_exit()
                                      mtk_pci_unregister_irq()
                                        mtk_pci_mask_irq()
                                        synchronize_irq(virq)
  mtk_pci_clear_irq()
  mtk_pci_unmask_irq()
    write REG_IMASK_HOST_MSIX_SET_GRP0_0 = BIT(28)
                                      cancel_work_sync()

synchronize_irq() only waits for the hardirq handler, not for the
already-running work item, and priv->irq_type is still PCI_IRQ_MSIX at
that point so the unmask write goes through.  cancel_work_sync() then
returns with the source enabled, and the rest of mtk_pci_remove()
(pci_free_irq(), pci_free_irq_vectors(), the reset) runs without the
"Silence every source before tearing anything down" property it
assumes.  A later assertion in that window reaches
mtk_pci_irq_handler() with no registered callback and only produces
dev_err_ratelimited("Unhandled irq_id=28").

Would a re-mask after cancel_work_sync(), or a teardown flag checked
before the clear/unmask pair, close this?

[ ... ]

[Severity: Low]
This isn't a bug, but would a pci_resource_len() sanity check here be
worth adding?

Both mappings are used with fixed offsets afterwards, for example
REG_IMASK_HOST_MSIX_CLR_GRP0_0 (0x3080) on BAR0/1 in mtk_pci_probe(),
and mhccif_rc_base_addr 0x1000A000 relative to the biased
ext_reg_base (bar23_addr + 0xA030) in mtk_mhccif_init().  The
mtk_pci_mac_write32() and mtk_pci_write32() accessors in mtk_pci.h add
those offsets without any range enforcement, so a device reporting
undersized BARs would be written outside the ioremap mapping.  The
sibling t7xx driver has the same pattern, so this is hardening rather
than a defect.

[ ... ]

[ ... ]

+static int mtk_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{

[ ... ]

[ ... ]

[Severity: Medium]
On the clear_master path, is the ordering here inverted with respect to
mtk_pci_remove()?

mtk_pci_free_irq() runs before mtk_mhccif_exit(), so an MHCCIF work
item queued by mtk_mhccif_irq_cb() just before the failure can still
be running when pci_free_irq() and pci_free_irq_vectors() complete.
Its tail then does

	mtk_pci_clear_irq(mdev, priv->mhccif_irq_id);
	mtk_pci_unmask_irq(mdev, priv->mhccif_irq_id);

touching the interrupt mask registers after the IRQ action and the
MSI-X vectors are gone.  Should this path call mtk_mhccif_exit() (which
unregisters the callback and cancels the work) before
mtk_pci_free_irq(), the way mtk_pci_remove() does?

[Severity: Medium]
Can the "no MMIO, config write or MSI-X teardown may follow it"
invariant hold while probe uses pcim_enable_device()?

pcim_enable_device() installs a devres action that the driver core runs
in devres_release_all(), after remove() returns:

drivers/pci/devres.c:pcim_enable_device() {
	...
	devm_add_action(&pdev->dev, pcim_disable_device, pdev);
	...
}

and that action reaches do_pci_disable_device(), which does a
PCI_COMMAND config read and, if the read shows PCI_COMMAND_MASTER, a
config write.  On an endpoint that was just reset the read can return
0xFFFF (which has that bit set), so a config write of 0xFFFB is then
attempted on a possibly link-down device.  Would moving the reset out
of the tail of remove(), or using the non-managed pci_enable_device()
with an explicit disable before the reset, keep the stated ordering?

A second question on the same path: neither reset waits for
completion.  mtk_pci_send_ext_evt() only performs two posted BAR
writes (MHCCIF_RC2EP_SW_BSY and MHCCIF_RC2EP_SW_TCHNUM) with no
readback, and there is no delay or presence poll after either reset
before remove() returns.  An immediate re-bind then runs
mtk_pci_probe() -> atr_init() -> mtk_pci_atr_disable(), which
read-modify-writes the BAR0 ATR registers:

	val = mtk_pci_mac_read32(priv, REG_ATR_PCIE_WIN0_T0_SRC_ADDR_LSB + offset);
	val = val & (~BIT(0));
	mtk_pci_mac_write32(priv, REG_ATR_PCIE_WIN0_T0_SRC_ADDR_LSB + offset, val);

On a still-resetting device the reads come back as all ones and the
writes are dropped, yet probe treats this as success; mtk_pci_link_check()
is only consulted later, after MSI-X setup.  For comparison, t7xx sleeps
FASTBOOT_RESET_DELAY_MS after its equivalent doorbell in
t7xx_modem_ops.c.  Should this path wait for the endpoint to settle?

[Severity: Low]
This isn't a bug, but pci_channel_state_t is declared in
include/linux/pci.h as

	typedef unsigned int __bitwise pci_channel_state_t;

so %d degrades the sparse-restricted unsigned type to signed int, which
make C=1 reports as "restricted pci_channel_state_t degrades to
integer".  Would %u with an explicit cast be preferable here?

[ ... ]