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

[Simon Horman <horms@kernel.org>]

On Mon, Sep 21, 2026 at 02:56:44PM +0800, han.junyang@zte.com.cn wrote:
> From: Junyang Han <han.junyang@zte.com.cn>
> 
> The DingHai firmware publishes a version compatibility block and a
> RISC-V health buffer at fixed offsets within BAR 0.
> 
> After the PCI capabilities are mapped, poll the compatibility block
> until the firmware populates it (the region reads as all ones until
> then) and verify the driver/firmware version contract. Then wait for
> the RISC-V management core to set its power-on flag in the health
> buffer before the rest of the probe continues.
> 
> Firmware images predating the health buffer protocol (health version
> other than 1 and patch level below ZXDH_HPIRQ_PATCH) skip the
> readiness wait.
> 
> Signed-off-by: Junyang Han <han.junyang@zte.com.cn>
> ---
>  drivers/net/ethernet/zte/dinghai/en_pf.c | 113 +++++++++++++++++++++++
>  drivers/net/ethernet/zte/dinghai/en_pf.h |  46 +++++++++
>  2 files changed, 159 insertions(+)
> 
> diff --git a/drivers/net/ethernet/zte/dinghai/en_pf.c b/drivers/net/ethernet/zte/dinghai/en_pf.c
> index 86d437408820..7c991e0951a8 100644
> --- a/drivers/net/ethernet/zte/dinghai/en_pf.c
> +++ b/drivers/net/ethernet/zte/dinghai/en_pf.c
> @@ -6,6 +6,8 @@
> 
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include <net/devlink.h>
>  #include <linux/dma-mapping.h>
>  #include "en_pf.h"
> @@ -369,6 +371,103 @@ int zxdh_pf_modern_cfg_init(struct zxdh_core_dev *zxdh_dev)
>  	return ret;
>  }
> 
> +/* Read the firmware version block and verify the driver/firmware
> + * version contract.
> + */
> +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;
> +	u32 erased;
> +
> +	fw_compat = &pf_dev->fw_compat;
> +	compat = pf_dev->pci_ioremap_addr[0] + ZXDH_FW_COMPAT_OFFSET;
> +
> +	/* The region reads as all ones until the firmware populates it at
> +	 * the end of its boot; allow up to 200 s for a cold boot.
> +	 */
> +	readx_poll_timeout(ioread32, compat, erased, erased != 0xffffffffU,
> +			   USEC_PER_SEC,
> +			   ZXDH_FW_COMPAT_TIMEOUT_SEC * USEC_PER_SEC);

Hi,

There is an AI-generated review of this patch available at
https://sashiko.dev/#/patchset/202609211451400236_aZ55Ox3y7NW8MQnYImM%40zte.com.cn

In my view the critical point made there, which I'd appreciate you looking
into, is:

  Is the timeout error intentionally ignored here? If legacy firmware never
  populates this region, wouldn't the 200 second stall exceed the default
  udev timeout (180s), causing the worker to be killed and completely
  breaking legacy hardware support?

> +
> +	/* 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;
> +	}
> +
> +	if (fw_compat->major != ZXDH_MAJOR) {
> +		dev_err(zxdh_dev->device,
> +			"driver major %u incompatible with firmware major %u\n",
> +			ZXDH_MAJOR, fw_compat->major);
> +		return -EINVAL;
> +	}
> +
> +	if (fw_compat->fw_minor < ZXDH_FW_MINOR) {
> +		dev_err(zxdh_dev->device,
> +			"firmware fw_minor %u older than required %u\n",
> +			fw_compat->fw_minor, ZXDH_FW_MINOR);
> +		return -EINVAL;
> +	}
> +
> +	if (fw_compat->drv_minor > ZXDH_DRV_MINOR) {
> +		dev_err(zxdh_dev->device,
> +			"driver drv_minor %u older than required by firmware %u\n",
> +			ZXDH_DRV_MINOR, fw_compat->drv_minor);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

...