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

On Sun, 20 Sep 2026 07:54:00 +0800 Jakub Kicinski wrote:

Thanks for the review.

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?

Will fix in v9, by dropping the precondition rather than documenting it.
The three helpers each do one write to a BAR 0/1 register, which needs
no vector, so the priv->irq_type term goes away and only the irq_id
range check stays.  priv->irq_type then has no reader left and is
removed with it.

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:

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?

Will fix in v9, by passing NULL.  Both the struct acpi_buffer and the
acpi_os_free() go away with it.

Can this tail re-enable the MSI-X source after teardown has masked it?

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.

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

Will fix in v9, by the first option: mtk_mhccif_exit() masks the source
again after cancel_work_sync(), so the teardown is the last writer of
the mask.  Not the flag, which would put a check that is only ever true
during teardown into the interrupt path.

This isn't a bug, but would a pci_resource_len() sanity check here be
worth adding?

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.

No change planned, but we would like guidance on how to record that.

On the check itself: the driver binds two fixed PCI IDs, so a device
that answers them with an undersized BAR is broken hardware rather than
an input to validate, and what can be caught generically is caught
already - pcim_iomap_region() fails on an invalid BAR index, on a BAR
the kernel cannot request, and on a zero-length BAR, and both return
values are checked.  The upstreaming guidance we were pointed at in the
other thread is explicit about this class of change, lines 119-120:

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

The difficulty is what happens next.  submitting-patches.rst lines
317-319 say:

  "Review comments or questions that do not lead to a code change
   should almost certainly bring about a comment or changelog entry so
   that the next reviewer better understands what is going on."

The changelog entry lives under the --- and disappears when the patch is
applied, so the only form that survives into the tree, and therefore the
only one that stops the next reviewer from asking again, is a code
comment.  But line 124 of the same guidance says:

  "Do not add code comments unless you have very high confidence the
   code would be misinterpreted without them."

So for a declined defensive check the two cannot both be satisfied: the
comment that keeps it from being re-raised is itself a comment we are
asked not to add.  Unless told otherwise we will take the changelog-only
route and accept that the question can come back.

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?

Probe is the side that follows the convention: it
acquires mhccif_init -> request_irq -> dev_init -> set_master and the
labels release in exactly the reverse order, while remove() is the
deliberate deviation, for the reason in the comment above it - with a
live device the callback has to go first so the work cannot be requeued
while it is cancelled.  That reason does not apply during probe unwind:
at every one of these labels either the MSI-X source has never been
unmasked, since the unmask is after the last failure that uses them, or
the vectors are already gone, so the work cannot be requeued in either
order.

The two writes in the tail are BAR writes, not operations on the freed
vectors, and the BARs stay mapped until devres runs after probe returns,
so they are harmless; mtk_pci_get_virq_id() returns -EINVAL once the
vectors are freed and mtk_pci_unregister_irq() skips synchronize_irq()
accordingly, which is correct because pci_free_irq() has already
synchronised.  The one thing that did matter here - the source left
unmasked by the tail of the work - is the re-mask above, and that fix
covers this path too.

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:

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?

Will fix in v9, by the second option: pcim_enable_device() becomes
pci_enable_device(), remove() calls pci_disable_device() before the
reset and the probe error path gets a matching label.  BAR mapping is
unaffected, pcim_iomap_region() registers its own devres.

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:

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?

FASTBOOT_RESET_DELAY_MS is not t7xx's reset delay.
It is used in one branch of t7xx_reset_device(), the branch that enters
firmware download mode:

	if (type == FLDR) {
		ret = t7xx_acpi_reset(t7xx_dev, "_RST");
	} else if (type == PLDR) {
		ret = t7xx_acpi_reset(t7xx_dev, "MRST._RST");
	} else if (type == FASTBOOT) {
		t7xx_host_event_notify(t7xx_dev, FASTBOOT_DL_NOTIFY);
		t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_DEVICE_RESET);
		msleep(FASTBOOT_RESET_DELAY_MS);
	}

The two reset branches - including the "MRST._RST" one this patch uses -
do not wait at all.  For an ordinary reset t7xx does what we do, so the
comparison does not carry over to this path.

Where t7xx waits, t9xx waits too.  The firmware download path is not in
this series; it arrives with mtk_devlink.c in the second one, and there
the reboot into download mode is waited for:

	ret = mtk_fsm_evt_submit(devlink->mdev, FSM_EVT_FB_RESET, FSM_F_DL_FB,
				 NULL, 0, EVT_MODE_BLOCKING);
	...
	timeout_jiff = msecs_to_jiffies(MTK_FB_WAIT_DL_MODE_TIMEOUT_MS);
	ret = wait_for_completion_timeout(&devlink->download_comp, timeout_jiff);

As for MRST._RST itself, it is an ACPI control method, so
acpi_evaluate_object() returns only once the AML has finished; the
platform owns that timing.  After it, remove() only calls
pci_load_and_free_saved_state(), which touches host memory - and with
the change above, devres no longer touches the device either.

On the re-bind window: probe does not treat a still-resetting device as
success.  The ATR read-modify-writes are dropped by a device that is not
answering, so nothing is mis-programmed, and mtk_pci_link_check() is
still reached before probe returns, so the bind fails with -ENOLINK and
can be retried.

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?

Will fix in v9 - the line becomes %u, without a cast.