Re: [dpdk-dev] [PATCH v3] net/ice: support device-specific DDP package loading

["Xu, Ting" <ting.xu@intel.com>]

Hi, Xiaolong,

As for the pkg_file, we first determine the pkg file path according to the search order. And then it will be given to the original pkg loading function, ice_load_pkg, to check if the pkg is available. If not, an error is returned. 
I do not check the access because it will be checked in the next step, so I  want to reduce some operations. 

Thanks!
------------------
Best Regards,
Xu Ting


-----Original Message-----
From: Ye, Xiaolong 
Sent: Tuesday, September 3, 2019 5:44 PM
To: Xu, Ting <ting.xu@intel.com>
Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
Subject: Re: [dpdk-dev] [PATCH v3] net/ice: support device-specific DDP package loading

Hi, Ting

On 09/03, Ting Xu wrote:
>This patch adds the feature that supports loading DDP package according 
>to the device serial number. Prior to loading the default DDP package 
>(ice.pkg), the driver will check for the presence of a device-specific 
>DDP package with the name containing 64-bit PCIe Device Serial Number 
>(ice-xxxxxxxxxxxxxxxx.pkg) during initialization. Users can use "lspci 
>-vs" to get the device serial number.
>The pkg search path are /lib/firmware/updates/intel/ice/ddp/
>and /lib/firmware/intel/ice/ddp/. If the package exists, the driver 
>will download it to the device instead of the default one. The loaded 
>package type (OS default and COMMS) will be stored in 
>ice_adapter->active_pkg_type. The package version is stored in 
>ice_hw->active_pkg_ver.
>
>Signed-off-by: Ting Xu <ting.xu@intel.com>
>
>-------------------------------------------------------------------
>
>v3: try to fix compile error; modify to change the pkg loading order.
>
>v2: modify codes according to the comments.
>---

You should put your change log after '---' marker, then it will be ignored when applying by `git am`, other separator like '----------------------...' 
you used will still show in the commit log.

> drivers/net/ice/ice_ethdev.c | 138 ++++++++++++++++++++++++++++++++++-
> drivers/net/ice/ice_ethdev.h |   8 ++
> 2 files changed, 144 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/ice/ice_ethdev.c 
>b/drivers/net/ice/ice_ethdev.c index 686d6f00f..922b71ac6 100644
>--- a/drivers/net/ice/ice_ethdev.c
>+++ b/drivers/net/ice/ice_ethdev.c
>@@ -28,7 +28,16 @@ static const char * const ice_valid_args[] = {  };
> 
> #define ICE_DFLT_OUTER_TAG_TYPE ICE_AQ_VSI_OUTER_TAG_VLAN_9100 
>-#define ICE_DFLT_PKG_FILE "/lib/firmware/intel/ice/ddp/ice.pkg"
>+
>+/* DDP package search path */
>+#define ICE_PKG_FILE_DEFAULT "/lib/firmware/intel/ice/ddp/ice.pkg"
>+#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_OS_DEFAULT_PKG_NAME		"ICE OS Default Package"
>+#define ICE_COMMS_PKG_NAME			"ICE COMMS Package"
>+#define ICE_MAX_PKG_FILENAME_SIZE   256
> 
> int ice_logtype_init;
> int ice_logtype_driver;
>@@ -1265,15 +1274,137 @@ ice_pf_setup(struct ice_pf *pf)
> 	return 0;
> }
> 
>+/* PCIe configuration space setting */
>+#define PCI_CFG_SPACE_SIZE          256
>+#define PCI_CFG_SPACE_EXP_SIZE      4096
>+#define PCI_EXT_CAP_ID(header)      (int)((header) & 0x0000ffff)
>+#define PCI_EXT_CAP_NEXT(header)    (((header) >> 20) & 0xffc)
>+#define PCI_EXT_CAP_ID_DSN          0x03
>+
>+static int
>+ice_pci_find_next_ext_capability(struct rte_pci_device *dev, int cap) 
>+{
>+	uint32_t header;
>+	int ttl;
>+	int pos = PCI_CFG_SPACE_SIZE;
>+
>+	/* minimum 8 bytes per capability */
>+	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
>+
>+	if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
>+		PMD_INIT_LOG(ERR, "ice error reading extended capabilities\n");
>+		return -1;
>+	}
>+
>+	/*
>+	 * If we have no capabilities, this is indicated by cap ID,
>+	 * cap version and next pointer all being 0.
>+	 */
>+	if (header == 0)
>+		return 0;
>+
>+	while (ttl-- > 0) {
>+		if (PCI_EXT_CAP_ID(header) == cap)
>+			return pos;
>+
>+		pos = PCI_EXT_CAP_NEXT(header);
>+
>+		if (pos < PCI_CFG_SPACE_SIZE)
>+			break;
>+
>+		if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
>+			PMD_INIT_LOG(ERR, "ice error reading extended capabilities\n");
>+			return -1;
>+		}
>+	}
>+
>+	return 0;
>+}
>+
>+/* Extract device serial number from PCIe Configuration Space and
>+ * determine the pkg file path according to the DSN.
>+ */
>+static int
>+ice_pkg_file_search_path(struct rte_pci_device *pci_dev, char 
>+*pkg_file) {
>+	int pos;
>+	char opt_ddp_filename[ICE_MAX_PKG_FILENAME_SIZE];
>+	uint32_t dword;
>+	uint32_t dsn_low, dsn_high;
>+
>+	pos = ice_pci_find_next_ext_capability(pci_dev, PCI_EXT_CAP_ID_DSN);
>+
>+	if (pos) {
>+		rte_pci_read_config(pci_dev, &dword, 4, pos + 4);
>+		dsn_low = dword;
>+		rte_pci_read_config(pci_dev, &dword, 4, pos + 8);
>+		dsn_high = dword;
>+		snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
>+			 "ice-%08x%08x.pkg", dsn_high, dsn_low);
>+	} else {
>+		PMD_INIT_LOG(INFO, "Failed to read device serial number\n");
>+		strncpy(pkg_file, ICE_PKG_FILE_DEFAULT,
>+			ICE_MAX_PKG_FILENAME_SIZE);
>+
>+		return 0;
>+	}
>+
>+	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
>+		ICE_MAX_PKG_FILENAME_SIZE);
>+	if (!access(strcat(pkg_file, opt_ddp_filename), 0))
>+		return 0;
>+
>+	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_DEFAULT,
>+		ICE_MAX_PKG_FILENAME_SIZE);
>+	if (!access(strcat(pkg_file, opt_ddp_filename), 0))
>+		return 0;
>+
>+	strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
>+	if (!access(pkg_file, 0))
>+		return 0;
>+
>+	strncpy(pkg_file, ICE_PKG_FILE_DEFAULT, ICE_MAX_PKG_FILENAME_SIZE);

Do we need to check access for the pkg_file here too? What if the file doesn't exist or user doesn't have access to it?

Thanks,
Xiaolong

>+
>+	return 0;
>+}
>+
>+static enum ice_pkg_type
>+ice_get_pkg_type(struct ice_hw *hw)
>+{
>+	enum ice_pkg_type package_type;
>+
>+	/* store the activated package type (OS default or Comms) */
>+	if (!strncmp((char *)hw->active_pkg_name, ICE_OS_DEFAULT_PKG_NAME,
>+		ICE_PKG_NAME_SIZE))
>+		package_type = ICE_PKG_TYPE_OS_DEFAULT;
>+	else if (!strncmp((char *)hw->active_pkg_name, ICE_COMMS_PKG_NAME,
>+		ICE_PKG_NAME_SIZE))
>+		package_type = ICE_PKG_TYPE_COMMS;
>+	else
>+		package_type = ICE_PKG_TYPE_UNKNOWN;
>+
>+	PMD_INIT_LOG(INFO, "Active package is: %d.%d.%d.%d, %s\n",
>+		hw->active_pkg_ver.major, hw->active_pkg_ver.minor,
>+		hw->active_pkg_ver.update, hw->active_pkg_ver.draft,
>+		hw->active_pkg_name);
>+
>+	return package_type;
>+}
>+
> static int ice_load_pkg(struct rte_eth_dev *dev)  {
> 	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>-	const char *pkg_file = ICE_DFLT_PKG_FILE;
>+	char pkg_file[ICE_MAX_PKG_FILENAME_SIZE];
> 	int err;
> 	uint8_t *buf;
> 	int buf_len;
> 	FILE *file;
> 	struct stat fstat;
>+	struct rte_pci_device *pci_dev = RTE_DEV_TO_PCI(dev->device);
>+	struct ice_adapter *ad =
>+		ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
>+
>+	ice_pkg_file_search_path(pci_dev, pkg_file);
> 
> 	file = fopen(pkg_file, "rb");
> 	if (!file)  {
>@@ -1313,6 +1444,9 @@ static int ice_load_pkg(struct rte_eth_dev *dev)
> 		PMD_INIT_LOG(ERR, "ice_copy_and_init_hw failed: %d\n", err);
> 		goto fail_exit;
> 	}
>+
>+	ad->active_pkg_type = ice_get_pkg_type(hw);
>+
> 	err = ice_init_hw_tbls(hw);
> 	if (err) {
> 		PMD_INIT_LOG(ERR, "ice_init_hw_tbls failed: %d\n", err); diff --git 
>a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h index 
>5063960a8..d1d07641d 100644
>--- a/drivers/net/ice/ice_ethdev.h
>+++ b/drivers/net/ice/ice_ethdev.h
>@@ -124,6 +124,13 @@
> #define ICE_ETH_OVERHEAD \
> 	(RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN + ICE_VLAN_TAG_SIZE * 2)
> 
>+/* DDP package type */
>+enum ice_pkg_type {
>+	ICE_PKG_TYPE_UNKNOWN,
>+	ICE_PKG_TYPE_OS_DEFAULT,
>+	ICE_PKG_TYPE_COMMS,
>+};
>+
> struct ice_adapter;
> 
> /**
>@@ -296,6 +303,7 @@ struct ice_adapter {
> 	uint32_t ptype_tbl[ICE_MAX_PKT_TYPE] __rte_cache_min_aligned;
> 	bool is_safe_mode;
> 	struct ice_devargs devargs;
>+	enum ice_pkg_type active_pkg_type; /* loaded ddp package type */
> };
> 
> struct ice_vsi_vlan_pvid_info {
>--
>2.17.1
>