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

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

Thanks for the review. 

> 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.

Will fix in v12: mtk_pci_bar_init() checks both BAR lengths before
mapping them and fails the probe with -ENODEV, with the bounds you
suggest: the end of the highest register this patch uses in each BAR.
The control DMA patch raises the BAR2/3 bound when it adds the CLDMA
registers.  The comment that said there are no such checks goes away.

> 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?

Will fix in v12: both pcim_iomap_region() calls pass KBUILD_MODNAME.

> 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.

No functional change; v12 adds a two-line comment before
pci_alloc_irq_vectors() saying that 32 vectors map 1:1 at the MAC's
power-on default and the register is left alone.

That 1:1 layout is the reset default of the T9xx MAC, not a leftover,
and nothing in this driver ever writes a merged value; remove() goes
through MRST._RST, which returns the MAC to that default.  t7xx writes
the register because it uses EXT_INT_NUM (8) vectors, which is not its
default; writing ffs(32) * 2 - 1 here would program a value this
hardware has never been run with, for a state the driver cannot create.

> 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.

Will fix in v12 by rewording the comment; the order stays.  The tail
unmask can still set the bit, but with the vectors freed it has nothing
to fire on before mtk_mhccif_exit() cancels the work and masks it again.
The current order came from the v10 review; the reverse order lets that
unmask hit a live vector after the callback is cleared, which trades a
harmless state for an "Unhandled irq_id" error.  The window before
mtk_pci_free_irq() is benign, since a requeued work is cancelled by
cancel_work_sync() after its callbacks are unregistered under
mhccif_lock, and the INTx point does not apply: the MAC gates INTx
through a separate enable register that this driver never sets.

> 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?

No functional change; v12 rewords the comment at the msleep() to say why
this branch does not poll.

The doorbell is asynchronous: when it returns the link is still up, so a
link poll right after it passes before the reset has even started and
proves nothing.  Whether the reset takes the link down at all is up to
the firmware, so a "down, then back" poll would either wait out its
timeout on every remove or need device behaviour the host cannot
observe.  The fixed 2 s is the settle t7xx uses after its own
asynchronous reset doorbell (FASTBOOT_RESET_DELAY_MS). The MRST._RST
branch, which shipping platforms take, keeps its poll and warning.

> 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.

Will fix in v12: the commit message will say that crash kexec skips
.shutdown, so kdump is not covered and the modem stays unusable in the
capture kernel until the next platform reset.  As in the v11 thread, a
kdump initramfs does not load this driver in practice, and the machine
reboots normally after the dump.

> 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".

No functional change; the commit message will add that probe does not
reset the modem or look for running firmware, together with the next
point.  In both cases a reset in probe would have the same tools and
fail the same way: with the link down the device is unreachable, and
without MRST._RST the only reset is the same doorbell the firmware has
just ignored.  The commit message already says removal is degraded
there, and remove() logs both cases.

> 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.

No change, as discussed in the v11 thread.  Detecting running firmware
in probe needs device-state encodings that are only interpreted by the
handshake later in the series, so probe would be guessing; an
unconditional reset there adds a modem reboot to every normal boot and
still only has the doorbell on platforms without MRST._RST. The 1-2 s at
shutdown is the cost of not being able to tell kexec apart
(kexec_in_progress is not exported to modules), and the commit message
states it.

Thanks,
Jack

pw-bot: cr