RE: [External Mail] [PATCH v2 1/7] net: wwan: t9xx: Add PCIe core

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

Hi Jakub,

Addressing sashiko AI code review comments for this patch, as
requested by you in the patch 3/7 review:
https://patchwork.kernel.org/project/netdevbpf/patch/20260610-t9xx_driver_v1-v2-3-c65addf23b3f@compal.com/#27006088

Q1: Does this code perform an incorrect double byte-swap on big-endian
architectures? The hardware bits are manually swapped using cpu_to_le32()
and cast back to u32. This value is later passed to mtk_pci_write32(),
which utilizes iowrite32(). Since iowrite32() internally handles
host-to-little-endian conversion, swapping the value beforehand will cause
a double swap on big-endian platforms.

  This driver targets MediaTek T9xx PCIe WWAN modems, which exist
  exclusively on x86 platforms (little-endian). On little-endian,
  cpu_to_le32() is a no-op and LE32_TO_U32() is a simple cast, so
  no byte-swap occurs. The hardware register layout assumes LE host
  ordering. While the pattern is technically incorrect for big-endian,
  this hardware is not available on BE platforms and the Kconfig
  dependency (depends on PCI) combined with the device's PCIe-only
  nature effectively restricts this to x86/ARM64-LE.

Q2: Does this call to ffs() yield the wrong channel index on big-endian
systems? hw_bits has already been endian-swapped in
mtk_pci_ext_h2d_evt_hw_bits(). Using ffs() on an endian-swapped value
produces a completely incorrect bit index.

  Same as Q1 — on little-endian platforms, no swap occurs, so ffs()
  operates on the correct value. Additionally, all callers of
  mtk_pci_send_ext_evt() pass known-valid channel values through the
  is_power_of_2(ch) check, and the SET_HW_BITS mapping covers all
  valid channels.

Q3: Does clearing the top-level interrupt status at the end of the handler
without a subsequent read loop lead to permanently lost hardware events?
If a new hardware event triggers during the worker's execution, clearing
the write-1-to-clear (W1C) interrupt status after the hardware event
statuses were read at the start will discard the newly asserted events.

  No events are lost. mtk_pci_clear_irq() clears the MSI-X interrupt
  status register (BIT(irq_id) in REG_MSIX_ISTATUS_HOST_GRP0_0), NOT
  the MHCCIF event registers. The MHCCIF events are level-triggered
  from the modem's EP2RC registers — the modem sets event bits and
  they remain set until the modem clears them or the host acknowledges.

  The flow is:
    1. Hardware: modem sets EP2RC event bits -> MSI-X interrupt fires
    2. ISR: masks MSI-X bit, schedules work
    3. Worker: reads EP2RC event status, dispatches callbacks
    4. Worker: clears MSI-X status (BIT(irq_id)), unmasks MSI-X

  If a new MHCCIF event arrives during step 3, the EP2RC register
  gets the new bit set. The MSI-X clear in step 4 only clears the
  MSI-X pending bit, not the MHCCIF source. After unmask, the MHCCIF
  event source re-asserts the MSI-X interrupt because the EP2RC bits
  are still set, triggering a new ISR -> new work -> new read of the
  updated EP2RC register.

Q4: Does this code incorrectly attempt to unmap a bitmask instead of a
single BAR index? MTK_REQUESTED_BARS is defined as a bitmask, but
pcim_iounmap_region() expects a specific BAR index. Furthermore, is
this manual unmapping redundant since the memory was mapped with
pcim_iomap_regions() and will automatically be cleaned up by devres?

  Valid on both points. Fixed in v3 by removing mtk_pci_bar_exit()
  entirely. pcim_iomap_regions() is devres-managed so manual unmap
  is redundant.

Q5: Does this logic mistakenly drop hardware interrupts when fewer than 32
MSI-X vectors are allocated? If fewer than 32 vectors are allocated
(for example, irq_cnt = 1), GENMASK(0, 0) evaluates to BIT(0). A
hardware interrupt source like MHCCIF (bit 28) will result in
BIT(28) & BIT(0) == 0. Could this lead to a complete loss of critical
device events?

  No issue. This is the MSI-X merged mode design. When fewer vectors
  are allocated than interrupt sources, mtk_pci_set_msix_merged()
  configures the hardware to merge multiple interrupt sources into
  the available vectors. With irq_cnt=1, the hardware maps ALL
  interrupt sources to vector 0. The GENMASK filters the irq_state
  to only the allocated vector bits, then checks if the current
  handler's vector bit is set. This correctly handles both full
  allocation (32 vectors, 1:1 mapping) and merged mode (fewer
  vectors, N:1 mapping).

Q6: Will this manual pci_disable_device() call trigger an atomic underflow?
Since the device was initialized using pcim_enable_device(), devres will
automatically disable the device during cleanup. Manually calling
pci_disable_device() drops the enable_cnt to zero. When devres executes
its cleanup, it will call pci_disable_device() again, leading to an
underflow and a kernel warning.

  Valid. Fixed in v3 by removing pci_disable_device() from both the
  probe error path and mtk_pci_remove(). pcim_enable_device() handles
  cleanup via devres.

Q7: Does performing a PCIe Link Down Reset (PLDR) before freeing IRQs cause
PCIe Unsupported Request (UR) faults? mtk_pci_pldr() power-cycles the
device and clears its BARs. Directly after this, mtk_pci_free_irq() is
called, which attempts to access the device's MSI-X MMIO table.

  No UR faults. mtk_pci_pldr() calls ACPI PXP._OFF then PXP._ON.
  The _ON call restores power to the device, bringing the PCIe link
  back up with BARs accessible. pci_free_irq_vectors() writes to PCI
  config space (MSI-X capability enable bit) which is always
  accessible while the device is on the bus. The MSI-X table MMIO
  access for masking individual entries goes through the
  re-established BAR mapping.

Q8: Will this manual pci_disable_device() call in mtk_pci_remove() trigger
an atomic underflow for the same reason as in mtk_pci_probe()?
Is a call to mtk_pci_dev_exit() missing from the remove path?

  Two issues, both valid and fixed in v3:
  1. pci_disable_device() underflow: same as Q6, removed.
  2. Missing mtk_pci_dev_exit(): added in patch 3/7 v3 (where
     mtk_pci_dev_init is introduced).

Thanks.


================================================================================================================================================================
This message may contain information which is private, privileged or confidential of Compal Electronics, Inc. If you are not the intended recipient of this message, please notify the sender and destroy/delete the message. Any review, retransmission, dissemination or other use of, or taking of any action in reliance upon this information, by persons or entities other than the intended recipient is prohibited.
================================================================================================================================================================