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

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/202609282024273579VUWteKdnnbvWlyjERLQ-@zte.com.cn?part=1
---
dinghai: add firmware version check and RISC-V readiness polling

The commit adds firmware version checks and a polling mechanism for RISC-V
readiness on DingHai devices. It maps a compatibility block and a health
buffer in BAR 0 to verify driver and firmware compatibility and wait for
power-on status.

> diff --git a/drivers/net/ethernet/zte/dinghai/en_pf.c b/drivers/net/ethernet/zte/dinghai/en_pf.c
> --- a/drivers/net/ethernet/zte/dinghai/en_pf.c
> +++ b/drivers/net/ethernet/zte/dinghai/en_pf.c
[ ... ]
> @@ -369,6 +371,120 @@ int zxdh_pf_modern_cfg_init(struct zxdh_core_dev *zxdh_dev)
[ ... ]
> +static int zxdh_pf_fw_compat_check(struct zxdh_core_dev *zxdh_dev)
> +{
> +	struct zxdh_pf_dev *pf_dev = zxdh_dev->priv;
> +	struct zxdh_fw_compat __iomem *compat;
> +	struct zxdh_fw_compat *fw_compat;
> +	bool ready;
> +	int err;
> +
> +	fw_compat = &pf_dev->fw_compat;
> +	compat = pf_dev->pci_ioremap_addr[0] + ZXDH_FW_COMPAT_OFFSET;

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

> +
> +	/* The firmware publishes the block within 10 s of boot; wait twice
> +	 * that to leave margin.
> +	 */
> +	err = readx_poll_timeout(zxdh_pf_fw_compat_ready, compat, ready,
> +				 ready, USEC_PER_SEC,
> +				 ZXDH_FW_COMPAT_TIMEOUT_SEC * USEC_PER_SEC);

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

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

> +	if (err)
> +		dev_info(zxdh_dev->device,
> +			 "compat region not populated, assuming legacy firmware\n");
> +
> +	/* Firmware predating the compatibility region keeps the erased
> +	 * pattern, which fails the module id check below and defers the
> +	 * decision to the readiness wait.
> +	 */
> +	fw_compat->module_id = ioread8(&compat->module_id);
> +	fw_compat->major = ioread8(&compat->major);
> +	fw_compat->fw_minor = ioread8(&compat->fw_minor);
> +	fw_compat->drv_minor = ioread8(&compat->drv_minor);
> +	fw_compat->patch = ioread16(&compat->patch);
> +
> +	if (fw_compat->module_id != ZXDH_MODULE_ID) {
> +		dev_info(zxdh_dev->device,
> +			 "unknown module id %u, skip fw compat check\n",
> +			 fw_compat->module_id);
> +		/* Unknown firmware is treated as predating the HPIRQ
> +		 * patch, so that the readiness wait is skipped.
> +		 */
> +		fw_compat->patch = 0;
> +		return 0;
> +	}

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

[ ... ]
> +static int zxdh_pf_wait_riscv_ready(struct zxdh_core_dev *zxdh_dev)
> +{
[ ... ]
> +	err = readx_poll_timeout(ioread8, &hb->riscv_power_on, power_on,
> +				 power_on == 1, USEC_PER_SEC,
> +				 ZXDH_RISCV_READY_TIMEOUT_SEC * USEC_PER_SEC);

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