Re: [PATCH v5] net/ice: support customized search path for DDP package

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Sep 19, 2024 at 11:29:34AM +0800, Zhichao Zeng wrote:
> This patch adds support for customizing firmware search path for
> DDP package like the kernel behavior, it will read the search path
> from "/sys/module/firmware_class/parameters/path", and try to load
> DDP package.
> 
> Also, updates documentation for loading the DDP package in ice.rst.
> 
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
> 
> ---
> v5: update documentation, fix code logic
> v4: fix CI error
> v3: update doc, fix code error
> v2: separate the patch and rewrite the log
> ---
>  doc/guides/nics/ice.rst      | 61 +++++++++++++++++++-----------------
>  drivers/net/ice/ice_ethdev.c | 44 ++++++++++++++++++++++++--
>  drivers/net/ice/ice_ethdev.h |  1 +
>  3 files changed, 75 insertions(+), 31 deletions(-)
> 
> diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
> index ae975d19ad..1494f70415 100644
> --- a/doc/guides/nics/ice.rst
> +++ b/doc/guides/nics/ice.rst
> @@ -80,6 +80,38 @@ are listed in the Tested Platforms section of the Release Notes for each release
>     |    24.03  |     1.13.7    |      1.3.35     |  1.3.45   |    1.3.13    |    4.4    |
>     +-----------+---------------+-----------------+-----------+--------------+-----------+
>  
> +Dynamic Device Personalization (DDP) package loading
> +----------------------------------------------------
> +
> +The Intel E810 requires a programmable pipeline package be downloaded
> +by the driver to support normal operations. The E810 has limited
> +functionality built in to allow PXE boot and other use cases, but for DPDK use
> +the driver must download a package file during the driver initialization
> +stage.
> +
> +The default DDP package file name is ``ice.pkg``. For a specific NIC, the
> +DDP package supposed to be loaded can have a filename: ``ice-xxxxxx.pkg``,
> +where 'xxxxxx' is the 64-bit PCIe Device Serial Number of the NIC. For
> +example, if the NIC's device serial number is 00-CC-BB-FF-FF-AA-05-68,
> +the device-specific DDP package filename is ``ice-00ccbbffffaa0568.pkg``
> +(in hex and all low case), please review README from
> +`Intel® Ethernet 800 Series Dynamic Device Personalization (DDP) for Telecommunication (Comms) Package
> +<https://www.intel.com/content/www/us/en/download/19660/intel-ethernet-800-series-dynamic-device-personalization-ddp-for-telecommunication-comms-package.html>`_
> +for more information. A symbolic link to the DDP package file is also ok.
> +The same package file is used by both the kernel driver and the ICE PMD.
> +
> +ICE PMD supports using a customized DDP search path. The driver will read
> +the search path from '/sys/module/firmware_class/parameters/path'
> +as 'CUSTOMIZED_PATH'.
> +During initialization, the driver searches in the following paths in order:
> +'CUSTOMIZED_PATH', '/lib/firmware/updates/intel/ice/ddp' and '/lib/firmware/intel/ice/ddp'.
> +The device-specific DDP package has a higher loading priority than default DDP package.
> +The type of loaded package is stored in ``ice_adapter->active_pkg_type``.
> +
> +   .. Note::
> +
> +      Windows support: DDP packages are not supported on Windows.
> +
>  Configuration
>  -------------
>  
> @@ -487,32 +519,3 @@ Usage::
>  
>  In "brief" mode, all scheduling nodes in the tree are displayed.
>  In "detail" mode, each node's configuration parameters are also displayed.
> -
> -Limitations or Known issues
> ----------------------------
> -
> -The Intel E810 requires a programmable pipeline package be downloaded
> -by the driver to support normal operations. The E810 has a limited
> -functionality built in to allow PXE boot and other use cases, but the
> -driver must download a package file during the driver initialization
> -stage.
> -
> -The default DDP package file name is ice.pkg. For a specific NIC, the
> -DDP package supposed to be loaded can have a filename: ice-xxxxxx.pkg,
> -where 'xxxxxx' is the 64-bit PCIe Device Serial Number of the NIC. For
> -example, if the NIC's device serial number is 00-CC-BB-FF-FF-AA-05-68,
> -the device-specific DDP package filename is ice-00ccbbffffaa0568.pkg
> -(in hex and all low case). During initialization, the driver searches
> -in the following paths in order: /lib/firmware/updates/intel/ice/ddp
> -and /lib/firmware/intel/ice/ddp. The corresponding device-specific DDP
> -package will be downloaded first if the file exists. If not, then the
> -driver tries to load the default package. The type of loaded package
> -is stored in ``ice_adapter->active_pkg_type``.
> -
> -A symbolic link to the DDP package file is also ok. The same package
> -file is used by both the kernel driver and the DPDK PMD.
> -
> -   .. Note::
> -
> -      Windows support: The DDP package is not supported on Windows so,
> -      loading of the package is disabled on Windows.
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 304f959b7e..f0389cc4ee 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -1873,21 +1873,61 @@ ice_load_pkg_type(struct ice_hw *hw)
>  	return package_type;
>  }
>  
> +static int ice_read_customized_path(char *pkg_file, uint16_t buff_len)
> +{
> +	FILE *fp = fopen(ICE_PKG_FILE_CUSTOMIZED_PATH, "r");
> +	int ret = 0;
> +
> +	if (fp == NULL) {
> +		PMD_INIT_LOG(INFO, "Failed to read CUSTOMIZED_PATH");
> +		return -EIO;
> +	}
> +
> +	if (fgets(pkg_file, buff_len, fp) == NULL) {
> +		ret = -EIO;
> +		goto exit;
> +	}
> +
> +	if (strlen(pkg_file) <= 1)
> +		goto exit;
> +
> +	pkg_file[strlen(pkg_file) - 1] = '\0';

This line is unnecessary and, if it were necessary, also incorrect:
* fgets already guarantees to null-terminate
* if it didn't null terminate, then strlen wouldn't even work so this
  statement would be completely broken.

Also, as this line is unnecessary, the lines above it, and the label below
are unnecessary too.

> +exit:
> +	fclose(fp);
> +	return ret;
> +}
> +
>  int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
>  {
>  	struct ice_hw *hw = &adapter->hw;
>  	char pkg_file[ICE_MAX_PKG_FILENAME_SIZE];
> +	char customized_path[ICE_MAX_PKG_FILENAME_SIZE];
>  	char opt_ddp_filename[ICE_MAX_PKG_FILENAME_SIZE];
>  	void *buf;
>  	size_t bufsz;
>  	int err;
>  
> -	if (!use_dsn)
> -		goto no_dsn;
> +	ice_read_customized_path(customized_path, ICE_MAX_PKG_FILENAME_SIZE);
>  
>  	memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
>  	snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
>  		"ice-%016" PRIx64 ".pkg", dsn);
> +
> +	if (strlen(customized_path) > 1) {

Is this really the best way to check for validity of the search path?
I would tend to ignore length and use the return value from the function
"ice_read_customized_path". Again, maybe check the logic used in the kernel
for validating the path read.

> +		if (use_dsn) {
> +			snprintf(pkg_file, RTE_DIM(pkg_file),
> +				"%s/%s", customized_path, opt_ddp_filename);
> +			if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
> +				goto load_fw;
> +		}
> +		snprintf(pkg_file, RTE_DIM(pkg_file), "%s/%s", customized_path, "ice.pkg");
> +		if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
> +			goto load_fw;
> +	}
> +
> +	if (!use_dsn)
> +		goto no_dsn;
> +

This is changing the search path logic compared to v4. Does it match what
the kernel does in finding a package?

>  	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
>  		ICE_MAX_PKG_FILENAME_SIZE);
>  	strcat(pkg_file, opt_ddp_filename);
> diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
> index 3ea9f37dc8..8b644ed700 100644
> --- a/drivers/net/ice/ice_ethdev.h
> +++ b/drivers/net/ice/ice_ethdev.h
> @@ -51,6 +51,7 @@
>  #define ICE_PKG_FILE_UPDATES "/lib/firmware/updates/intel/ice/ddp/ice.pkg"
>  #define ICE_PKG_FILE_SEARCH_PATH_DEFAULT "/lib/firmware/intel/ice/ddp/"
>  #define ICE_PKG_FILE_SEARCH_PATH_UPDATES "/lib/firmware/updates/intel/ice/ddp/"
> +#define ICE_PKG_FILE_CUSTOMIZED_PATH "/sys/module/firmware_class/parameters/path"
>  #define ICE_MAX_PKG_FILENAME_SIZE   256
>  
>  #define MAX_ACL_NORMAL_ENTRIES    256
> -- 
> 2.34.1
>