[PATCH] net/ice: support for more flexible loading of DDP package

[PATCH] net/ice: support for more flexible loading of DDP package

[Zhichao Zeng]

The "Dynamic Device Personalization" package is loaded at initialization
time by the driver, but the specific package file loaded depends upon
what package file is found first by searching through a hard-coded list
of firmware paths.

To enable greater control over the package loading, this commit two ways
to support custom DDP packages:
1. Add device option to choose a specific DDP package file to load.
   For example:
   -a 80:00.0,ddp_pkg_file=/path/to/ice-version.pkg
2. Read firmware search path from
   "/sys/module/firmware_class/parameters/path" like the kernel behavior.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
---
 doc/guides/nics/ice.rst      | 12 +++++++
 drivers/net/ice/ice_ethdev.c | 61 ++++++++++++++++++++++++++++++++++++
 drivers/net/ice/ice_ethdev.h |  2 ++
 3 files changed, 75 insertions(+)

diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index ae975d19ad..0484fafbc1 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -108,6 +108,18 @@ Runtime Configuration
 
     -a 80:00.0,default-mac-disable=1
 
+- ``DDP Package File``
+
+  Rather than have the driver search for the DDP package to load,
+  or to override what package is used,
+  the ``ddp_pkg_file`` option can be used to provide the path to a specific package file.
+  For example::
+
+    -a 80:00.0,ddp_pkg_file=/path/to/ice-version.pkg
+
+  There is also support for customizing the firmware search path, will read the search path
+  from "/sys/module/firmware_class/parameters/path" and try to load DDP package.
+
 - ``Protocol extraction for per queue``
 
   Configure the RX queues to do protocol extraction into mbuf for protocol
diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 304f959b7e..bd78c14000 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -36,6 +36,7 @@
 #define ICE_ONE_PPS_OUT_ARG       "pps_out"
 #define ICE_RX_LOW_LATENCY_ARG    "rx_low_latency"
 #define ICE_MBUF_CHECK_ARG       "mbuf_check"
+#define ICE_DDP_FILENAME          "ddp_pkg_file"
 
 #define ICE_CYCLECOUNTER_MASK  0xffffffffffffffffULL
 
@@ -52,6 +53,7 @@ static const char * const ice_valid_args[] = {
 	ICE_RX_LOW_LATENCY_ARG,
 	ICE_DEFAULT_MAC_DISABLE,
 	ICE_MBUF_CHECK_ARG,
+	ICE_DDP_FILENAME,
 	NULL
 };
 
@@ -692,6 +694,18 @@ handle_field_name_arg(__rte_unused const char *key, const char *value,
 	return 0;
 }
 
+static int
+handle_ddp_filename_arg(__rte_unused const char *key, const char *value, void *name_args)
+{
+	const char **filename = name_args;
+	if (strlen(value) >= ICE_MAX_PKG_FILENAME_SIZE) {
+		PMD_DRV_LOG(ERR, "The DDP package filename is too long : '%s'", value);
+		return -1;
+	}
+	*filename = strdup(value);
+	return 0;
+}
+
 static void
 ice_check_proto_xtr_support(struct ice_hw *hw)
 {
@@ -1873,6 +1887,22 @@ ice_load_pkg_type(struct ice_hw *hw)
 	return package_type;
 }
 
+static int ice_read_customized_path(char *pkg_file)
+{
+	char buf[ICE_MAX_PKG_FILENAME_SIZE];
+	FILE *fp = fopen(ICE_PKG_FILE_CUSTOMIZED_PATH, "r");
+	if (fp == NULL) {
+		PMD_INIT_LOG(ERR, "Failed to read CUSTOMIZED_PATH");
+		return -EIO;
+	}
+	if (fscanf(fp, "%s\n", buf) > 0)
+		strncpy(pkg_file, buf, ICE_MAX_PKG_FILENAME_SIZE);
+	else
+		return -EIO;
+
+	return 0;
+}
+
 int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 {
 	struct ice_hw *hw = &adapter->hw;
@@ -1882,12 +1912,28 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 	size_t bufsz;
 	int err;
 
+	if (adapter->devargs.ddp_filename != NULL) {
+		strlcpy(pkg_file, adapter->devargs.ddp_filename, sizeof(pkg_file));
+		if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0) {
+			goto load_fw;
+		} else {
+			PMD_INIT_LOG(ERR, "Cannot load DDP file: %s\n", pkg_file);
+			return -1;
+		}
+	}
+
 	if (!use_dsn)
 		goto no_dsn;
 
 	memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
 	snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
 		"ice-%016" PRIx64 ".pkg", dsn);
+
+	ice_read_customized_path(pkg_file);
+	strcat(pkg_file, opt_ddp_filename);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
+		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
 		ICE_MAX_PKG_FILENAME_SIZE);
 	strcat(pkg_file, opt_ddp_filename);
@@ -1901,6 +1947,10 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 		goto load_fw;
 
 no_dsn:
+	ice_read_customized_path(pkg_file);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
+		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
 	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
 		goto load_fw;
@@ -2217,6 +2267,14 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
 	ret = rte_kvargs_process(kvlist, ICE_RX_LOW_LATENCY_ARG,
 				 &parse_bool, &ad->devargs.rx_low_latency);
 
+	if (ret)
+		goto bail;
+
+	ret = rte_kvargs_process(kvlist, ICE_DDP_FILENAME,
+				 &handle_ddp_filename_arg, &ad->devargs.ddp_filename);
+	if (ret)
+		goto bail;
+
 bail:
 	rte_kvargs_free(kvlist);
 	return ret;
@@ -2689,6 +2747,8 @@ ice_dev_close(struct rte_eth_dev *dev)
 	ice_free_hw_tbls(hw);
 	rte_free(hw->port_info);
 	hw->port_info = NULL;
+	free((void *)(uintptr_t)ad->devargs.ddp_filename);
+	ad->devargs.ddp_filename = NULL;
 	ice_shutdown_all_ctrlq(hw, true);
 	rte_free(pf->proto_xtr);
 	pf->proto_xtr = NULL;
@@ -6981,6 +7041,7 @@ RTE_PMD_REGISTER_PARAM_STRING(net_ice,
 			      ICE_PROTO_XTR_ARG "=[queue:]<vlan|ipv4|ipv6|ipv6_flow|tcp|ip_offset>"
 			      ICE_SAFE_MODE_SUPPORT_ARG "=<0|1>"
 			      ICE_DEFAULT_MAC_DISABLE "=<0|1>"
+				  ICE_DDP_FILENAME "=</path/to/file>"
 			      ICE_RX_LOW_LATENCY_ARG "=<0|1>");
 
 RTE_LOG_REGISTER_SUFFIX(ice_logtype_init, init, NOTICE);
diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
index 3ea9f37dc8..2781362d04 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
@@ -568,6 +569,7 @@ struct ice_devargs {
 	/* Name of the field. */
 	char xtr_field_name[RTE_MBUF_DYN_NAMESIZE];
 	uint64_t mbuf_check;
+	const char *ddp_filename;
 };
 
 /**
-- 
2.34.1

Re: [PATCH] net/ice: support for more flexible loading of DDP package

[Bruce Richardson]

On Wed, Aug 28, 2024 at 11:53:35AM +0800, Zhichao Zeng wrote:
> The "Dynamic Device Personalization" package is loaded at initialization
> time by the driver, but the specific package file loaded depends upon
> what package file is found first by searching through a hard-coded list
> of firmware paths.
> 
> To enable greater control over the package loading, this commit two ways
> to support custom DDP packages:
> 1. Add device option to choose a specific DDP package file to load.
>    For example:
>    -a 80:00.0,ddp_pkg_file=/path/to/ice-version.pkg
> 2. Read firmware search path from
>    "/sys/module/firmware_class/parameters/path" like the kernel behavior.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>

Hi Zhichao,

since there are two different methods being supported for picking a DDP
package this patch would be better split into two, one for each method
added.

The support for #1 above is already on-list as a standalone patch[1], so you
really only need to do a new patch for #2 above. However, I'm ok for you to
take my patch and include it in a 2-patch set for this if you prefer, since
both patches will be related to choosing a DDP file. I'll leave it up to
you whether v2 is a single patch for the search path, or a 2-patch set
including [1].

Regards,
/Bruce

[1] https://patches.dpdk.org/project/dpdk/patch/20240812152815.1132697-5-bruce.richardson@intel.com/

> ---
>  doc/guides/nics/ice.rst      | 12 +++++++
>  drivers/net/ice/ice_ethdev.c | 61 ++++++++++++++++++++++++++++++++++++
>  drivers/net/ice/ice_ethdev.h |  2 ++
>  3 files changed, 75 insertions(+)
> 
> diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
> index ae975d19ad..0484fafbc1 100644
> --- a/doc/guides/nics/ice.rst
> +++ b/doc/guides/nics/ice.rst
> @@ -108,6 +108,18 @@ Runtime Configuration
>  
>      -a 80:00.0,default-mac-disable=1
>  
> +- ``DDP Package File``
> +
> +  Rather than have the driver search for the DDP package to load,
> +  or to override what package is used,
> +  the ``ddp_pkg_file`` option can be used to provide the path to a specific package file.
> +  For example::
> +
> +    -a 80:00.0,ddp_pkg_file=/path/to/ice-version.pkg
> +
> +  There is also support for customizing the firmware search path, will read the search path
> +  from "/sys/module/firmware_class/parameters/path" and try to load DDP package.
> +
>  - ``Protocol extraction for per queue``
>  
>    Configure the RX queues to do protocol extraction into mbuf for protocol
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 304f959b7e..bd78c14000 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -36,6 +36,7 @@
>  #define ICE_ONE_PPS_OUT_ARG       "pps_out"
>  #define ICE_RX_LOW_LATENCY_ARG    "rx_low_latency"
>  #define ICE_MBUF_CHECK_ARG       "mbuf_check"
> +#define ICE_DDP_FILENAME          "ddp_pkg_file"
>  
>  #define ICE_CYCLECOUNTER_MASK  0xffffffffffffffffULL
>  
> @@ -52,6 +53,7 @@ static const char * const ice_valid_args[] = {
>  	ICE_RX_LOW_LATENCY_ARG,
>  	ICE_DEFAULT_MAC_DISABLE,
>  	ICE_MBUF_CHECK_ARG,
> +	ICE_DDP_FILENAME,
>  	NULL
>  };
>  
> @@ -692,6 +694,18 @@ handle_field_name_arg(__rte_unused const char *key, const char *value,
>  	return 0;
>  }
>  
> +static int
> +handle_ddp_filename_arg(__rte_unused const char *key, const char *value, void *name_args)
> +{
> +	const char **filename = name_args;
> +	if (strlen(value) >= ICE_MAX_PKG_FILENAME_SIZE) {
> +		PMD_DRV_LOG(ERR, "The DDP package filename is too long : '%s'", value);
> +		return -1;
> +	}
> +	*filename = strdup(value);
> +	return 0;
> +}
> +
>  static void
>  ice_check_proto_xtr_support(struct ice_hw *hw)
>  {
> @@ -1873,6 +1887,22 @@ ice_load_pkg_type(struct ice_hw *hw)
>  	return package_type;
>  }
>  
> +static int ice_read_customized_path(char *pkg_file)
> +{
> +	char buf[ICE_MAX_PKG_FILENAME_SIZE];
> +	FILE *fp = fopen(ICE_PKG_FILE_CUSTOMIZED_PATH, "r");
> +	if (fp == NULL) {
> +		PMD_INIT_LOG(ERR, "Failed to read CUSTOMIZED_PATH");
> +		return -EIO;
> +	}
> +	if (fscanf(fp, "%s\n", buf) > 0)
> +		strncpy(pkg_file, buf, ICE_MAX_PKG_FILENAME_SIZE);
> +	else
> +		return -EIO;
> +
> +	return 0;
> +}
> +
>  int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
>  {
>  	struct ice_hw *hw = &adapter->hw;
> @@ -1882,12 +1912,28 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
>  	size_t bufsz;
>  	int err;
>  
> +	if (adapter->devargs.ddp_filename != NULL) {
> +		strlcpy(pkg_file, adapter->devargs.ddp_filename, sizeof(pkg_file));
> +		if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0) {
> +			goto load_fw;
> +		} else {
> +			PMD_INIT_LOG(ERR, "Cannot load DDP file: %s\n", pkg_file);
> +			return -1;
> +		}
> +	}
> +
>  	if (!use_dsn)
>  		goto no_dsn;
>  
>  	memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
>  	snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
>  		"ice-%016" PRIx64 ".pkg", dsn);
> +
> +	ice_read_customized_path(pkg_file);
> +	strcat(pkg_file, opt_ddp_filename);
> +	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
> +		goto load_fw;
> +
>  	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
>  		ICE_MAX_PKG_FILENAME_SIZE);
>  	strcat(pkg_file, opt_ddp_filename);
> @@ -1901,6 +1947,10 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
>  		goto load_fw;
>  
>  no_dsn:
> +	ice_read_customized_path(pkg_file);
> +	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
> +		goto load_fw;
> +
>  	strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
>  	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
>  		goto load_fw;
> @@ -2217,6 +2267,14 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
>  	ret = rte_kvargs_process(kvlist, ICE_RX_LOW_LATENCY_ARG,
>  				 &parse_bool, &ad->devargs.rx_low_latency);
>  
> +	if (ret)
> +		goto bail;
> +
> +	ret = rte_kvargs_process(kvlist, ICE_DDP_FILENAME,
> +				 &handle_ddp_filename_arg, &ad->devargs.ddp_filename);
> +	if (ret)
> +		goto bail;
> +
>  bail:
>  	rte_kvargs_free(kvlist);
>  	return ret;
> @@ -2689,6 +2747,8 @@ ice_dev_close(struct rte_eth_dev *dev)
>  	ice_free_hw_tbls(hw);
>  	rte_free(hw->port_info);
>  	hw->port_info = NULL;
> +	free((void *)(uintptr_t)ad->devargs.ddp_filename);
> +	ad->devargs.ddp_filename = NULL;
>  	ice_shutdown_all_ctrlq(hw, true);
>  	rte_free(pf->proto_xtr);
>  	pf->proto_xtr = NULL;
> @@ -6981,6 +7041,7 @@ RTE_PMD_REGISTER_PARAM_STRING(net_ice,
>  			      ICE_PROTO_XTR_ARG "=[queue:]<vlan|ipv4|ipv6|ipv6_flow|tcp|ip_offset>"
>  			      ICE_SAFE_MODE_SUPPORT_ARG "=<0|1>"
>  			      ICE_DEFAULT_MAC_DISABLE "=<0|1>"
> +				  ICE_DDP_FILENAME "=</path/to/file>"
>  			      ICE_RX_LOW_LATENCY_ARG "=<0|1>");
>  
>  RTE_LOG_REGISTER_SUFFIX(ice_logtype_init, init, NOTICE);
> diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
> index 3ea9f37dc8..2781362d04 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
> @@ -568,6 +569,7 @@ struct ice_devargs {
>  	/* Name of the field. */
>  	char xtr_field_name[RTE_MBUF_DYN_NAMESIZE];
>  	uint64_t mbuf_check;
> +	const char *ddp_filename;
>  };
>  
>  /**
> -- 
> 2.34.1
> 

RE: [PATCH] net/ice: support for more flexible loading of DDP package

[Zeng, ZhichaoX]

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Wednesday, August 28, 2024 3:55 PM
> To: Zeng, ZhichaoX <zhichaox.zeng@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] net/ice: support for more flexible loading of DDP package
> 
> On Wed, Aug 28, 2024 at 11:53:35AM +0800, Zhichao Zeng wrote:
> > The "Dynamic Device Personalization" package is loaded at
> > initialization time by the driver, but the specific package file
> > loaded depends upon what package file is found first by searching
> > through a hard-coded list of firmware paths.
> >
> > To enable greater control over the package loading, this commit two
> > ways to support custom DDP packages:
> > 1. Add device option to choose a specific DDP package file to load.
> >    For example:
> >    -a 80:00.0,ddp_pkg_file=/path/to/ice-version.pkg
> > 2. Read firmware search path from
> >    "/sys/module/firmware_class/parameters/path" like the kernel behavior.
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
> 
> Hi Zhichao,
> 
> since there are two different methods being supported for picking a DDP
> package this patch would be better split into two, one for each method added.
> 
> The support for #1 above is already on-list as a standalone patch[1], so you
> really only need to do a new patch for #2 above. However, I'm ok for you to
> take my patch and include it in a 2-patch set for this if you prefer, since both
> patches will be related to choosing a DDP file. I'll leave it up to you whether v2
> is a single patch for the search path, or a 2-patch set including [1].
> 
> Regards,
> /Bruce
> 
> [1]
> https://patches.dpdk.org/project/dpdk/patch/20240812152815.1132697-
> 5-bruce.richardson@intel.com/
> 
Hi Bruce,

Thanks for your comments, sorry I didn't check the patchwork and didn't notice that #1 had been submitted, I'll rework the patch for #2 separately, thanks.

Regards
Zhichao
> > ---
> >  doc/guides/nics/ice.rst      | 12 +++++++
> >  drivers/net/ice/ice_ethdev.c | 61
> > ++++++++++++++++++++++++++++++++++++
> >  drivers/net/ice/ice_ethdev.h |  2 ++
> >  3 files changed, 75 insertions(+)
> >
<Snip>

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

[Zhichao Zeng]

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.

Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>

---
v2: separate the patch and rewrite the log
---
 doc/guides/nics/ice.rst      |  5 +++++
 drivers/net/ice/ice_ethdev.c | 27 +++++++++++++++++++++++++++
 drivers/net/ice/ice_ethdev.h |  1 +
 3 files changed, 33 insertions(+)

diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index ae975d19ad..741cd42cb7 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -108,6 +108,11 @@ Runtime Configuration
 
     -a 80:00.0,default-mac-disable=1
 
+- ``DDP Package File``
+
+  Support for customizing the firmware search path, will read the search path
+  from "/sys/module/firmware_class/parameters/path" and try to load DDP package.
+
 - ``Protocol extraction for per queue``
 
   Configure the RX queues to do protocol extraction into mbuf for protocol
diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 304f959b7e..fc0954ff34 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -1873,6 +1873,22 @@ ice_load_pkg_type(struct ice_hw *hw)
 	return package_type;
 }
 
+static int ice_read_customized_path(char *pkg_file)
+{
+	char buf[ICE_MAX_PKG_FILENAME_SIZE];
+	FILE *fp = fopen(ICE_PKG_FILE_CUSTOMIZED_PATH, "r");
+	if (fp == NULL) {
+		PMD_INIT_LOG(ERR, "Failed to read CUSTOMIZED_PATH");
+		return -EIO;
+	}
+	if (fscanf(fp, "%s\n", buf) > 0)
+		strncpy(pkg_file, buf, ICE_MAX_PKG_FILENAME_SIZE);
+	else
+		return -EIO;
+
+	return 0;
+}
+
 int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 {
 	struct ice_hw *hw = &adapter->hw;
@@ -1888,6 +1904,12 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 	memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
 	snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
 		"ice-%016" PRIx64 ".pkg", dsn);
+
+	ice_read_customized_path(pkg_file);
+	strcat(pkg_file, opt_ddp_filename);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
+		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
 		ICE_MAX_PKG_FILENAME_SIZE);
 	strcat(pkg_file, opt_ddp_filename);
@@ -1901,6 +1923,10 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 		goto load_fw;
 
 no_dsn:
+	ice_read_customized_path(pkg_file);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
+		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
 	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
 		goto load_fw;
@@ -6981,6 +7007,7 @@ RTE_PMD_REGISTER_PARAM_STRING(net_ice,
 			      ICE_PROTO_XTR_ARG "=[queue:]<vlan|ipv4|ipv6|ipv6_flow|tcp|ip_offset>"
 			      ICE_SAFE_MODE_SUPPORT_ARG "=<0|1>"
 			      ICE_DEFAULT_MAC_DISABLE "=<0|1>"
+				  ICE_DDP_FILENAME "=</path/to/file>"
 			      ICE_RX_LOW_LATENCY_ARG "=<0|1>");
 
 RTE_LOG_REGISTER_SUFFIX(ice_logtype_init, init, NOTICE);
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

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

[Zhichao Zeng]

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.

Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>

---
v2: separate the patch and rewrite the log
---
 doc/guides/nics/ice.rst      |  5 +++++
 drivers/net/ice/ice_ethdev.c | 26 ++++++++++++++++++++++++++
 drivers/net/ice/ice_ethdev.h |  1 +
 3 files changed, 32 insertions(+)

diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index ae975d19ad..741cd42cb7 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -108,6 +108,11 @@ Runtime Configuration
 
     -a 80:00.0,default-mac-disable=1
 
+- ``DDP Package File``
+
+  Support for customizing the firmware search path, will read the search path
+  from "/sys/module/firmware_class/parameters/path" and try to load DDP package.
+
 - ``Protocol extraction for per queue``
 
   Configure the RX queues to do protocol extraction into mbuf for protocol
diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 304f959b7e..5dfb3d9c21 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -1873,6 +1873,22 @@ ice_load_pkg_type(struct ice_hw *hw)
 	return package_type;
 }
 
+static int ice_read_customized_path(char *pkg_file)
+{
+	char buf[ICE_MAX_PKG_FILENAME_SIZE];
+	FILE *fp = fopen(ICE_PKG_FILE_CUSTOMIZED_PATH, "r");
+	if (fp == NULL) {
+		PMD_INIT_LOG(ERR, "Failed to read CUSTOMIZED_PATH");
+		return -EIO;
+	}
+	if (fscanf(fp, "%s\n", buf) > 0)
+		strncpy(pkg_file, buf, ICE_MAX_PKG_FILENAME_SIZE);
+	else
+		return -EIO;
+
+	return 0;
+}
+
 int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 {
 	struct ice_hw *hw = &adapter->hw;
@@ -1888,6 +1904,12 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 	memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
 	snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
 		"ice-%016" PRIx64 ".pkg", dsn);
+
+	ice_read_customized_path(pkg_file);
+	strcat(pkg_file, opt_ddp_filename);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
+		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
 		ICE_MAX_PKG_FILENAME_SIZE);
 	strcat(pkg_file, opt_ddp_filename);
@@ -1901,6 +1923,10 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 		goto load_fw;
 
 no_dsn:
+	ice_read_customized_path(pkg_file);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
+		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
 	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
 		goto load_fw;
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

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

[Bruce Richardson]

On Thu, Aug 29, 2024 at 11:41:58AM +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.
> 
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
> 

See review comments inline below.

Thanks,
/Bruce

> ---
> v2: separate the patch and rewrite the log
> ---
>  doc/guides/nics/ice.rst      |  5 +++++
>  drivers/net/ice/ice_ethdev.c | 26 ++++++++++++++++++++++++++
>  drivers/net/ice/ice_ethdev.h |  1 +
>  3 files changed, 32 insertions(+)
> 
> diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
> index ae975d19ad..741cd42cb7 100644
> --- a/doc/guides/nics/ice.rst
> +++ b/doc/guides/nics/ice.rst
> @@ -108,6 +108,11 @@ Runtime Configuration
>  
>      -a 80:00.0,default-mac-disable=1
>  
> +- ``DDP Package File``
> +
> +  Support for customizing the firmware search path, will read the search path
> +  from "/sys/module/firmware_class/parameters/path" and try to load DDP package.
> +

This is no longer a configuration parameter, so this section can be
removed from the doc.

There is a section in the ice docs for "Limitations or Known Issues" where
the search paths for DDP package files are given. That could do with an
update in this patch. Also, the details of DDP searching probably should go
in its own section rather than having it as a limitation - move it further
up the doc to be more accessible to users.

>  - ``Protocol extraction for per queue``
>  
>    Configure the RX queues to do protocol extraction into mbuf for protocol
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 304f959b7e..5dfb3d9c21 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -1873,6 +1873,22 @@ ice_load_pkg_type(struct ice_hw *hw)
>  	return package_type;
>  }
>  
> +static int ice_read_customized_path(char *pkg_file)
> +{
> +	char buf[ICE_MAX_PKG_FILENAME_SIZE];
> +	FILE *fp = fopen(ICE_PKG_FILE_CUSTOMIZED_PATH, "r");
> +	if (fp == NULL) {
> +		PMD_INIT_LOG(ERR, "Failed to read CUSTOMIZED_PATH");

Is this actually an error? Will older kernels not be missing this path, in
which case it's not really a problem at all? I think any message should be
at a much lower logging level to avoid unnecessary warnings to users.

> +		return -EIO;
> +	}
> +	if (fscanf(fp, "%s\n", buf) > 0)

This looks unsafe to me, you are not specifying a maximum string length in
the call to fscanf.
For strings, just use fgets, or alternatively just use regular "open" and
"read" system calls.

> +		strncpy(pkg_file, buf, ICE_MAX_PKG_FILENAME_SIZE);

strncpy is insecure in many cases (such as here!) and should not be used.
This will not null-terminate the string at all. For string copies use safe
functions like "strlcpy".

However, there is no need to do a copy here at all. Remove the variable
"buf" and instead read the value directly into "pkg_file". Using a
temporary variable has no benefit.

NOTE: you also need to change the function signature to pass in the length
of pkg_file variable. When passing a string field to be completed, you
always need to pass in the length value too.

> +	else
> +		return -EIO;
> +
> +	return 0;
> +}
> +
>  int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
>  {
>  	struct ice_hw *hw = &adapter->hw;
> @@ -1888,6 +1904,12 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
>  	memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
>  	snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
>  		"ice-%016" PRIx64 ".pkg", dsn);
> +
> +	ice_read_customized_path(pkg_file);
> +	strcat(pkg_file, opt_ddp_filename);

This is also at risk of buffer overflows. Use strlcat and check for errors
that the paths are not too long.

> +	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
> +		goto load_fw;
> +
>  	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
>  		ICE_MAX_PKG_FILENAME_SIZE);
>  	strcat(pkg_file, opt_ddp_filename);
> @@ -1901,6 +1923,10 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
>  		goto load_fw;
>  
>  no_dsn:
> +	ice_read_customized_path(pkg_file);

Minor nit, but why read sysfs twice using system calls. Use a local
variable to save the value from the first read rather than repeating it.

> +	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
> +		goto load_fw;
> +
>  	strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
>  	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
>  		goto load_fw;
> 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
> 

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

[Zhichao Zeng]

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>

---
v3: update doc, fix code error
v2: separate the patch and rewrite the log
---
 doc/guides/nics/ice.rst      | 64 ++++++++++++++++++++----------------
 drivers/net/ice/ice_ethdev.c | 41 +++++++++++++++++++++++
 drivers/net/ice/ice_ethdev.h |  1 +
 3 files changed, 77 insertions(+), 29 deletions(-)

diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index ae975d19ad..af026d2d60 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -80,6 +80,41 @@ 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 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), 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 support customized DDP search path like kernel behaviour, driver will
+read DDP path from '/sys/module/firmware_class/parameters/path' as CUSTOMIZED_PATH.
+During initialization, the driver searches in the following paths in order:
+CUSTOMIZED_PATH(mentioned above), /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``.
+
+   .. Note::
+
+      Windows support: The DDP package is not supported on Windows so,
+      loading of the package is disabled on Windows.
+
 Configuration
 -------------
 
@@ -487,32 +522,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..31beb935ce 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -1873,21 +1873,57 @@ 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 (pkg_file[strlen(pkg_file) - 2] == '/') {
+		pkg_file[strlen(pkg_file) - 1] = '\0';
+	} else {
+		pkg_file[strlen(pkg_file) - 1] = '/';
+		pkg_file[strlen(pkg_file)] = '\0';
+	}
+
+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;
 
+	ice_read_customized_path(customized_path, ICE_MAX_PKG_FILENAME_SIZE);
+
 	if (!use_dsn)
 		goto no_dsn;
 
 	memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
 	snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
 		"ice-%016" PRIx64 ".pkg", dsn);
+
+	strlcpy(pkg_file, customized_path, ICE_MAX_PKG_FILENAME_SIZE);
+	strlcat(pkg_file, opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
+		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
 		ICE_MAX_PKG_FILENAME_SIZE);
 	strcat(pkg_file, opt_ddp_filename);
@@ -1901,6 +1937,11 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 		goto load_fw;
 
 no_dsn:
+	strlcpy(pkg_file, customized_path, ICE_MAX_PKG_FILENAME_SIZE);
+	strlcat(pkg_file, "ice.pkg", ICE_MAX_PKG_FILENAME_SIZE);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
+		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
 	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
 		goto load_fw;
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

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

[Zhichao Zeng]

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>

---
v3: update doc, fix code error
v2: separate the patch and rewrite the log
---
 doc/guides/nics/ice.rst      | 64 ++++++++++++++++++++----------------
 drivers/net/ice/ice_ethdev.c | 41 +++++++++++++++++++++++
 drivers/net/ice/ice_ethdev.h |  1 +
 3 files changed, 77 insertions(+), 29 deletions(-)

diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index ae975d19ad..daf1e89cf6 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -80,6 +80,41 @@ 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 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), 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 support customized DDP search path, driver will read 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(mentioned above), /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``.
+
+   .. Note::
+
+      Windows support: The DDP package is not supported on Windows so,
+      loading of the package is disabled on Windows.
+
 Configuration
 -------------
 
@@ -487,32 +522,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..31beb935ce 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -1873,21 +1873,57 @@ 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 (pkg_file[strlen(pkg_file) - 2] == '/') {
+		pkg_file[strlen(pkg_file) - 1] = '\0';
+	} else {
+		pkg_file[strlen(pkg_file) - 1] = '/';
+		pkg_file[strlen(pkg_file)] = '\0';
+	}
+
+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;
 
+	ice_read_customized_path(customized_path, ICE_MAX_PKG_FILENAME_SIZE);
+
 	if (!use_dsn)
 		goto no_dsn;
 
 	memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
 	snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
 		"ice-%016" PRIx64 ".pkg", dsn);
+
+	strlcpy(pkg_file, customized_path, ICE_MAX_PKG_FILENAME_SIZE);
+	strlcat(pkg_file, opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
+		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
 		ICE_MAX_PKG_FILENAME_SIZE);
 	strcat(pkg_file, opt_ddp_filename);
@@ -1901,6 +1937,11 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 		goto load_fw;
 
 no_dsn:
+	strlcpy(pkg_file, customized_path, ICE_MAX_PKG_FILENAME_SIZE);
+	strlcat(pkg_file, "ice.pkg", ICE_MAX_PKG_FILENAME_SIZE);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
+		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
 	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
 		goto load_fw;
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

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

[Zhichao Zeng]

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>

---
v4: fix CI error
v3: update doc, fix code error
v2: separate the patch and rewrite the log
---
 doc/guides/nics/ice.rst      | 63 +++++++++++++++++++-----------------
 drivers/net/ice/ice_ethdev.c | 44 +++++++++++++++++++++++++
 drivers/net/ice/ice_ethdev.h |  1 +
 3 files changed, 79 insertions(+), 29 deletions(-)

diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index ae975d19ad..7f347684e7 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -80,6 +80,40 @@ 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 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), 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 support customized DDP search path, driver will read 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(mentioned above), /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``.
+
+   .. Note::
+
+      Windows support: The DDP package is not supported on Windows so,
+      loading of the package is disabled on Windows.
+
 Configuration
 -------------
 
@@ -487,32 +521,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..13b3d79a3d 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -1873,21 +1873,60 @@ 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;
+
+	if (pkg_file[strlen(pkg_file) - 2] == '/') {
+		pkg_file[strlen(pkg_file) - 1] = '\0';
+	} else {
+		pkg_file[strlen(pkg_file) - 1] = '/';
+		pkg_file[strlen(pkg_file)] = '\0';
+	}
+
+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;
 
+	ice_read_customized_path(customized_path, ICE_MAX_PKG_FILENAME_SIZE);
+
 	if (!use_dsn)
 		goto no_dsn;
 
 	memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
 	snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
 		"ice-%016" PRIx64 ".pkg", dsn);
+
+	strlcpy(pkg_file, customized_path, ICE_MAX_PKG_FILENAME_SIZE);
+	strlcat(pkg_file, opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
+		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
 		ICE_MAX_PKG_FILENAME_SIZE);
 	strcat(pkg_file, opt_ddp_filename);
@@ -1901,6 +1940,11 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 		goto load_fw;
 
 no_dsn:
+	strlcpy(pkg_file, customized_path, ICE_MAX_PKG_FILENAME_SIZE);
+	strlcat(pkg_file, "ice.pkg", ICE_MAX_PKG_FILENAME_SIZE);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
+		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
 	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
 		goto load_fw;
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

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

[Bruce Richardson]

On Fri, Sep 13, 2024 at 02:15:37PM +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>
> 
> ---
> v4: fix CI error
> v3: update doc, fix code error
> v2: separate the patch and rewrite the log
> ---
>  doc/guides/nics/ice.rst      | 63 +++++++++++++++++++-----------------
>  drivers/net/ice/ice_ethdev.c | 44 +++++++++++++++++++++++++
>  drivers/net/ice/ice_ethdev.h |  1 +
>  3 files changed, 79 insertions(+), 29 deletions(-)
> 
> diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
> index ae975d19ad..7f347684e7 100644
> --- a/doc/guides/nics/ice.rst
> +++ b/doc/guides/nics/ice.rst
> @@ -80,6 +80,40 @@ 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 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.

Hi Zhichao,

I realise that this text above is not written by you, but when moving this
text we can update it a little.

* in second sentence, drop the word "a" before "limited functionality".
* I feel we are missing a few words in the second sentence to clarify the
  need for DDP support. For example, after the word "but" we could clarify
  with "for normal use" or "for DPDK use" or some similar phrase.

> +
> +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,

For the filenames: ice.pkg and ice-xxxxxx.pkg, put them in `` quotes to
make them monospaced/literal text. Same for filenames below.

> +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 support customized DDP search path, driver will read search path

"support" -> "supports using a"
", driver" -> ". The driver"
"search path" -> "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(mentioned above), /lib/firmware/updates/intel/ice/ddp and

you can drop "(mentioned above)".

> +/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``.

The behaviour is not exactly clear here. The text seems to imply (at least
ot me) that the search order is to check each directory first for the
serial-number package and then for ice.pkg, before moving on to the next
directory. However, the actual search pattern in the code  is to check each
directory in turn for a serial number package, and then scan each directory
in turn for an ice.pkg file. Consider the case of having two files:

$CUSTOMIZED_PATH/ice.pkg
/lib/firmware/intel/ice/ddp/ice-xxxxxxx.pkg

As described in the doc text above, I would expect the first of these two
packages to be used, since it's in the customized path, but in fact the
second will be used by the code since it has a serial number on it so is
found in the initial scan for a numbered-package.

> +
> +   .. Note::
> +
> +      Windows support: The DDP package is not supported on Windows so,
> +      loading of the package is disabled on Windows.

I notice that the comma "," is in the wrong place in
the original text - it should come before the "so" not after it - but
overall the whole line can just be shortened to:
	"Windows support: DDP packages are not supported on Windows"

<snip for brevity>

Thanks,
/Bruce

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

[Zhichao Zeng]

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';
+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) {
+		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;
+
 	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

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

[Bruce Richardson]

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
> 

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

[Zeng, ZhichaoX]

Hi Bruce, thanks for your reply.

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Thursday, September 19, 2024 3:50 PM
> To: Zeng, ZhichaoX <zhichaox.zeng@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v5] net/ice: support customized search path for DDP
> package
> 
> 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
> > ---
<snip>
> > 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.

The string read by fgets ends with "\n", my intention was to remove the newline.

> > +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.

Will use the return value instead of string length and check the logic in kernel.

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

The kernel searches for DDP packages in the CUSTOMIZED_PATH first, then the default path.

Regards
Zhichao

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

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

[Bruce Richardson]

On Fri, Sep 20, 2024 at 09:34:55AM +0100, Zeng, ZhichaoX wrote:
> Hi Bruce, thanks for your reply.
> 
> > -----Original Message-----
> > From: Richardson, Bruce <bruce.richardson@intel.com>
> > Sent: Thursday, September 19, 2024 3:50 PM
> > To: Zeng, ZhichaoX <zhichaox.zeng@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [PATCH v5] net/ice: support customized search path for DDP
> > package
> > 
> > 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
> > > ---
> <snip>
> > > 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.
> 
> The string read by fgets ends with "\n", my intention was to remove the newline.
> 

Ah, apologies, my bad. That makes sense now. However, I think it would be
clearer for future readers if this was more explicitly called out. At
minimum use a comment, but even better is an explicit check for a '\n'
before you strip it.

> > > +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.
> 
> Will use the return value instead of string length and check the logic in kernel.
> 
> > 
> > > +		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?
> > 
> 
> The kernel searches for DDP packages in the CUSTOMIZED_PATH first, then the default path.
> 
Thanks for confirming.

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

[Zhichao Zeng]

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>

---
v6: use open() instead of fopen()
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      | 60 +++++++++++++++++++-----------------
 drivers/net/ice/ice_ethdev.c | 47 ++++++++++++++++++++++++++--
 drivers/net/ice/ice_ethdev.h |  1 +
 3 files changed, 76 insertions(+), 32 deletions(-)

diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index ae975d19ad..a6104db4d4 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -80,6 +80,37 @@ 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.
+
+   .. Note::
+
+      Windows support: DDP packages are not supported on Windows.
+
 Configuration
 -------------
 
@@ -487,32 +518,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..1584b3251c 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -6,6 +6,7 @@
 #include <ethdev_pci.h>
 
 #include <ctype.h>
+#include <fcntl.h>
 #include <stdio.h>
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -1873,21 +1874,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)
+{
+	int fp, n;
+
+	fp = open(ICE_PKG_FILE_CUSTOMIZED_PATH, O_RDONLY);
+	if (fp < 0) {
+		PMD_INIT_LOG(INFO, "Failed to read CUSTOMIZED_PATH");
+		return -EIO;
+	}
+
+	n = read(fp, pkg_file, buff_len - 1);
+	if (n == 0) {
+		close(fp);
+		return -EIO;
+	}
+
+	if (pkg_file[n - 1] == '\n')
+		n--;
+
+	pkg_file[n] = '\0';
+
+	close(fp);
+	return 0;
+}
+
 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;
-
 	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 (ice_read_customized_path(customized_path,
+		ICE_MAX_PKG_FILENAME_SIZE) == 0) {
+		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;
+
 	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

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

[Bruce Richardson]

On Fri, Sep 20, 2024 at 06:05:59PM +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>
> 
> ---
> v6: use open() instead of fopen()
> 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      | 60 +++++++++++++++++++-----------------
>  drivers/net/ice/ice_ethdev.c | 47 ++++++++++++++++++++++++++--
>  drivers/net/ice/ice_ethdev.h |  1 +
>  3 files changed, 76 insertions(+), 32 deletions(-)
> 
> diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
> index ae975d19ad..a6104db4d4 100644
> --- a/doc/guides/nics/ice.rst
> +++ b/doc/guides/nics/ice.rst
> @@ -80,6 +80,37 @@ 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.
> +
Minor nit - when writing documentation, rather than wrapping the lines at a
fixed column width, it is better to wrap on punctuation and to always start
a sentence on a new line. This reduces the diffs when editing documentation
in future.

In this case, I'll just adjust the line-wrapping on apply, so no need for a
new revision of the patch.

/Bruce

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

[Bruce Richardson]

On Tue, Oct 01, 2024 at 02:54:49PM +0100, Bruce Richardson wrote:
> On Fri, Sep 20, 2024 at 06:05:59PM +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>
> > 
> > ---
> > v6: use open() instead of fopen()
> > 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      | 60 +++++++++++++++++++-----------------
> >  drivers/net/ice/ice_ethdev.c | 47 ++++++++++++++++++++++++++--
> >  drivers/net/ice/ice_ethdev.h |  1 +
> >  3 files changed, 76 insertions(+), 32 deletions(-)
> > 
> > diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
> > index ae975d19ad..a6104db4d4 100644
> > --- a/doc/guides/nics/ice.rst
> > +++ b/doc/guides/nics/ice.rst
> > @@ -80,6 +80,37 @@ 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.
> > +
> Minor nit - when writing documentation, rather than wrapping the lines at a
> fixed column width, it is better to wrap on punctuation and to always start
> a sentence on a new line. This reduces the diffs when editing documentation
> in future.
> 
> In this case, I'll just adjust the line-wrapping on apply, so no need for a
> new revision of the patch.
> 

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Patch applied to dpdk-next-net-intel.
Thanks,
/Bruce

[PATCH] net/ice: support for more flexible loading of DDP package

[Zhichao Zeng]

The "Dynamic Device Personalization" package is loaded at initialization
time by the driver, but the specific package file loaded depends upon
what package file is found first by searching through a hard-coded list
of firmware paths.

To enable greater control over the package loading, this commit two ways
to support custom DDP packages:
1. Add device option to choose a specific DDP package file to load.
   For example:
   -a 80:00.0,ddp_pkg_file=/path/to/ice-version.pkg
2. Read firmware search path from
   "/sys/module/firmware_class/parameters/path" like the kernel behavior.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
---
 doc/guides/nics/ice.rst      | 12 ++++++++
 drivers/net/ice/ice_ethdev.c | 59 ++++++++++++++++++++++++++++++++++++
 drivers/net/ice/ice_ethdev.h |  2 ++
 3 files changed, 73 insertions(+)

diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index ae975d19ad..0484fafbc1 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -108,6 +108,18 @@ Runtime Configuration
 
     -a 80:00.0,default-mac-disable=1
 
+- ``DDP Package File``
+
+  Rather than have the driver search for the DDP package to load,
+  or to override what package is used,
+  the ``ddp_pkg_file`` option can be used to provide the path to a specific package file.
+  For example::
+
+    -a 80:00.0,ddp_pkg_file=/path/to/ice-version.pkg
+
+  There is also support for customizing the firmware search path, will read the search path
+  from "/sys/module/firmware_class/parameters/path" and try to load DDP package.
+
 - ``Protocol extraction for per queue``
 
   Configure the RX queues to do protocol extraction into mbuf for protocol
diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 304f959b7e..a1b542c1af 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -36,6 +36,7 @@
 #define ICE_ONE_PPS_OUT_ARG       "pps_out"
 #define ICE_RX_LOW_LATENCY_ARG    "rx_low_latency"
 #define ICE_MBUF_CHECK_ARG       "mbuf_check"
+#define ICE_DDP_FILENAME          "ddp_pkg_file"
 
 #define ICE_CYCLECOUNTER_MASK  0xffffffffffffffffULL
 
@@ -52,6 +53,7 @@ static const char * const ice_valid_args[] = {
 	ICE_RX_LOW_LATENCY_ARG,
 	ICE_DEFAULT_MAC_DISABLE,
 	ICE_MBUF_CHECK_ARG,
+	ICE_DDP_FILENAME,
 	NULL
 };
 
@@ -692,6 +694,18 @@ handle_field_name_arg(__rte_unused const char *key, const char *value,
 	return 0;
 }
 
+static int
+handle_ddp_filename_arg(__rte_unused const char *key, const char *value, void *name_args)
+{
+	const char **filename = name_args;
+	if (strlen(value) >= ICE_MAX_PKG_FILENAME_SIZE) {
+		PMD_DRV_LOG(ERR, "The DDP package filename is too long : '%s'", value);
+		return -1;
+	}
+	*filename = strdup(value);
+	return 0;
+}
+
 static void
 ice_check_proto_xtr_support(struct ice_hw *hw)
 {
@@ -1873,6 +1887,20 @@ ice_load_pkg_type(struct ice_hw *hw)
 	return package_type;
 }
 
+static int ice_read_customized_path(char *pkg_file)
+{
+	char buf[ICE_MAX_PKG_FILENAME_SIZE];
+	FILE *fp = fopen(ICE_PKG_FILE_CUSTOMIZED_PATH, "r");
+	if (fp == NULL) {
+		PMD_INIT_LOG(ERR, "Failed to read CUSTOMIZED_PATH");
+		return -EIO;
+	}
+	fscanf(fp, "%s\n", buf);
+	strncpy(pkg_file, buf, ICE_MAX_PKG_FILENAME_SIZE);
+
+	return 0;
+}
+
 int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 {
 	struct ice_hw *hw = &adapter->hw;
@@ -1882,12 +1910,28 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 	size_t bufsz;
 	int err;
 
+	if (adapter->devargs.ddp_filename != NULL) {
+		strlcpy(pkg_file, adapter->devargs.ddp_filename, sizeof(pkg_file));
+		if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0) {
+			goto load_fw;
+		} else {
+			PMD_INIT_LOG(ERR, "Cannot load DDP file: %s\n", pkg_file);
+			return -1;
+		}
+	}
+
 	if (!use_dsn)
 		goto no_dsn;
 
 	memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
 	snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
 		"ice-%016" PRIx64 ".pkg", dsn);
+
+	ice_read_customized_path(pkg_file);
+	strcat(pkg_file, opt_ddp_filename);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
+		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
 		ICE_MAX_PKG_FILENAME_SIZE);
 	strcat(pkg_file, opt_ddp_filename);
@@ -1901,6 +1945,10 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 		goto load_fw;
 
 no_dsn:
+	ice_read_customized_path(pkg_file);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
+		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
 	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
 		goto load_fw;
@@ -2217,6 +2265,14 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
 	ret = rte_kvargs_process(kvlist, ICE_RX_LOW_LATENCY_ARG,
 				 &parse_bool, &ad->devargs.rx_low_latency);
 
+	if (ret)
+		goto bail;
+
+	ret = rte_kvargs_process(kvlist, ICE_DDP_FILENAME,
+				 &handle_ddp_filename_arg, &ad->devargs.ddp_filename);
+	if (ret)
+		goto bail;
+
 bail:
 	rte_kvargs_free(kvlist);
 	return ret;
@@ -2689,6 +2745,8 @@ ice_dev_close(struct rte_eth_dev *dev)
 	ice_free_hw_tbls(hw);
 	rte_free(hw->port_info);
 	hw->port_info = NULL;
+	free((void *)(uintptr_t)ad->devargs.ddp_filename);
+	ad->devargs.ddp_filename = NULL;
 	ice_shutdown_all_ctrlq(hw, true);
 	rte_free(pf->proto_xtr);
 	pf->proto_xtr = NULL;
@@ -6981,6 +7039,7 @@ RTE_PMD_REGISTER_PARAM_STRING(net_ice,
 			      ICE_PROTO_XTR_ARG "=[queue:]<vlan|ipv4|ipv6|ipv6_flow|tcp|ip_offset>"
 			      ICE_SAFE_MODE_SUPPORT_ARG "=<0|1>"
 			      ICE_DEFAULT_MAC_DISABLE "=<0|1>"
+				  ICE_DDP_FILENAME "=</path/to/file>"
 			      ICE_RX_LOW_LATENCY_ARG "=<0|1>");
 
 RTE_LOG_REGISTER_SUFFIX(ice_logtype_init, init, NOTICE);
diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
index 3ea9f37dc8..2781362d04 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
@@ -568,6 +569,7 @@ struct ice_devargs {
 	/* Name of the field. */
 	char xtr_field_name[RTE_MBUF_DYN_NAMESIZE];
 	uint64_t mbuf_check;
+	const char *ddp_filename;
 };
 
 /**
-- 
2.34.1