Re: [PATCH net-next v4 1/3] dinghai: add firmware version check and RISC-V readiness polling

[han.junyang]

Hi Simon,
thanks for the reviews.

Replying to the AI review :
> [Severity: High]
> Does this code verify the BAR length before calculating offsets into BAR 0?
> 
> The driver computes fixed offsets into BAR 0 (ZXDH_FW_COMPAT_OFFSET at 0x5400,
> and later ZXDH_RISCV_HB_OFFSET at 0x5300) without validating that the reported
> BAR length is large enough.
> 
> If a malformed PCI device, emulated device, or corrupt PCI configuration space
> presents a BAR smaller than 0x5400 plus the size of the structure, can this lead
> to an out-of-bounds MMIO read and page fault during probe?

These offsets are part of the device interface, not something
negotiated at runtime: BAR 0 is mapped in full at probe time
(zxdh_pf_pci_init() ioremaps pci_resource_len(pdev, 0)), and
ZXDH_FW_COMPAT_OFFSET / ZXDH_RISCV_HB_OFFSET are fixed regions
of the hardware spec that our own firmware implements. A BAR
too small for them can only come from a device misimplementing
its own spec, which is not a condition this driver defends
against.

> [Severity: High]
> Is there a performance regression here where this loop forces an unconditional
> 20-second delay on legacy firmware?
> 
> If the driver probes legacy hardware that predates the compatibility region,
> the region permanently contains the erased pattern (0xFF). Since legacy
> firmware never writes to this region, zxdh_pf_fw_compat_ready() will not see
> the fields lose their 0xFF values, meaning the loop waits for the entire
> timeout before proceeding.
> 
> Could this delay severely degrade system boot or module load times?

This wait is the legacy-firmware detection itself, not a
regression: legacy firmware keeps the erased pattern in the
region, which is indistinguishable from slow firmware until the
timeout expires. The assumption is logged with dev_info(), and
the 20 s stay well within the udev settle timeout.

> [Severity: Low]
> Are we passing 1,000,000 microseconds to usleep_range() in this macro?
> 
> The readx_poll_timeout macro expands to use usleep_range() for its sleep
> intervals. Passing USEC_PER_SEC exceeds the recommended 20ms maximum for
> usleep_range(), exhausting hrtimers unnecessarily.
> 
> Would it be better to use a smaller polling interval (e.g., 20ms) or a macro
> that utilizes msleep() to avoid this timer regression?

The quoted 20 ms cap comes from the removed timers-howto.rst;
neither its replacement nor the current usleep_range() docs
state such a limit. readx_poll_timeout() documents sleep_us as
the maximum interval between reads and sleeps
(sleep_us >> 2) + 1 .. sleep_us, so each iteration here sleeps
250 ms..1 s — matching the seconds-scale nature of the wait.
Same for the riscv_power_on wait below.

> [Severity: Medium]
> Could this logic incorrectly return success for a dead or physically removed
> PCI device?
> 
> If a PCI device experiences a bus error or is disconnected, reading its MMIO
> registers typically returns an all-ones value (0xFF). Here, an all-ones read
> causes module_id to equal 255.
> 
> Since the driver treats this as legacy firmware, it skips the compatibility
> check. Later, in zxdh_pf_wait_riscv_ready(), a health_version of 255 is also
> allowed to skip synchronization, leading to a successful probe.
> 
> Does the driver need a check to distinguish between actual legacy firmware and
> a broken PCIe link?

It does not get that far. With the link down, configuration
space reads also return 0xff, so the vendor-capability scan in
zxdh_pf_modern_cfg_init() — which runs before
zxdh_pf_fw_compat_check() — finds no common config and probe
exits with -ENODEV ("missing capabilities, leaving for legacy
driver"). If the device dies later in probe instead, the
all-0xff reads are treated as legacy firmware as described, and
the next real hardware interaction — pci_alloc_irq_vectors() in
zxdh_pf_irq_table_init() — fails, unwinding through the normal
error paths with the dev_info() above in the log. So probe does
not succeed against a dead device.

> [Severity: Low]
> This is another instance of passing 1,000,000 microseconds to usleep_range()
> via readx_poll_timeout. Should this use a smaller sleep interval or an
> msleep()-backed polling macro?

Answered above.