[PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver

[PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver

[Desnes Augusto Nunes do Rosario]

This patch implements and enables VDP support for the ibmvnic driver.
Moreover, it includes the implementation of suitable structs, signal
 transmission/handling and functions which allows the retrival of firmware
 information from the ibmvnic card through the ethtool command.

Signed-off-by: Desnes A. Nunes do Rosario <desnesn@linux.vnet.ibm.com>
Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 149 ++++++++++++++++++++++++++++++++++++-
 drivers/net/ethernet/ibm/ibmvnic.h |  27 ++++++-
 2 files changed, 173 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index d0cff28..693b502 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -573,6 +573,15 @@ static int reset_tx_pools(struct ibmvnic_adapter *adapter)
 	return 0;
 }
 
+static void release_vpd_data(struct ibmvnic_adapter *adapter)
+{
+	if (!adapter->vpd)
+		return;
+
+	kfree(adapter->vpd->buff);
+	kfree(adapter->vpd);
+}
+
 static void release_tx_pools(struct ibmvnic_adapter *adapter)
 {
 	struct ibmvnic_tx_pool *tx_pool;
@@ -753,6 +762,8 @@ static void release_resources(struct ibmvnic_adapter *adapter)
 {
 	int i;
 
+	release_vpd_data(adapter);
+
 	release_tx_pools(adapter);
 	release_rx_pools(adapter);
 
@@ -833,6 +844,53 @@ static int set_real_num_queues(struct net_device *netdev)
 	return rc;
 }
 
+static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
+{
+	struct device *dev = &adapter->vdev->dev;
+	union ibmvnic_crq crq;
+	dma_addr_t dma_addr;
+	int len;
+
+	if (adapter->vpd->buff)
+		len = adapter->vpd->len;
+
+	reinit_completion(&adapter->fw_done);
+	crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
+	crq.get_vpd_size.cmd = GET_VPD_SIZE;
+	ibmvnic_send_crq(adapter, &crq);
+	wait_for_completion(&adapter->fw_done);
+
+	if (!adapter->vpd->buff)
+		adapter->vpd->buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
+	else if (adapter->vpd->len != len)
+		adapter->vpd->buff =
+			krealloc(adapter->vpd->buff,
+				 adapter->vpd->len, GFP_KERNEL);
+
+	if (!adapter->vpd->buff) {
+		dev_err(dev, "Could allocate VPD buffer\n");
+		return -ENOMEM;
+	}
+
+	adapter->vpd->dma_addr =
+		dma_map_single(dev, adapter->vpd->buff, adapter->vpd->len,
+			       DMA_FROM_DEVICE);
+	if (dma_mapping_error(dev, dma_addr)) {
+		dev_err(dev, "Could not map VPD buffer\n");
+		return -ENOMEM;
+	}
+
+	reinit_completion(&adapter->fw_done);
+	crq.get_vpd.first = IBMVNIC_CRQ_CMD;
+	crq.get_vpd.cmd = GET_VPD;
+	crq.get_vpd.ioba = cpu_to_be32(adapter->vpd->dma_addr);
+	crq.get_vpd.len = cpu_to_be32((u32)adapter->vpd->len);
+	ibmvnic_send_crq(adapter, &crq);
+	wait_for_completion(&adapter->fw_done);
+
+	return 0;
+}
+
 static int init_resources(struct ibmvnic_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
@@ -850,6 +908,10 @@ static int init_resources(struct ibmvnic_adapter *adapter)
 	if (rc)
 		return rc;
 
+	adapter->vpd = kzalloc(sizeof(*adapter->vpd), GFP_KERNEL);
+	if (!adapter->vpd)
+		return -ENOMEM;
+
 	adapter->map_id = 1;
 	adapter->napi = kcalloc(adapter->req_rx_queues,
 				sizeof(struct napi_struct), GFP_KERNEL);
@@ -950,6 +1012,10 @@ static int ibmvnic_open(struct net_device *netdev)
 
 	rc = __ibmvnic_open(netdev);
 	netif_carrier_on(netdev);
+
+	/* Vital Product Data (VPD) */
+	ibmvnic_get_vpd(adapter);
+
 	mutex_unlock(&adapter->reset_lock);
 
 	return rc;
@@ -1878,11 +1944,15 @@ static int ibmvnic_get_link_ksettings(struct net_device *netdev,
 	return 0;
 }
 
-static void ibmvnic_get_drvinfo(struct net_device *dev,
+static void ibmvnic_get_drvinfo(struct net_device *netdev,
 				struct ethtool_drvinfo *info)
 {
+	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
+
 	strlcpy(info->driver, ibmvnic_driver_name, sizeof(info->driver));
 	strlcpy(info->version, IBMVNIC_DRIVER_VERSION, sizeof(info->version));
+	strlcpy(info->fw_version, adapter->fw_version,
+		sizeof(info->fw_version));
 }
 
 static u32 ibmvnic_get_msglevel(struct net_device *netdev)
@@ -3076,6 +3146,77 @@ static void send_cap_queries(struct ibmvnic_adapter *adapter)
 	ibmvnic_send_crq(adapter, &crq);
 }
 
+static void handle_vpd_size_rsp(union ibmvnic_crq *crq,
+				struct ibmvnic_adapter *adapter)
+{
+	struct device *dev = &adapter->vdev->dev;
+
+	if (crq->get_vpd_size_rsp.rc.code) {
+		dev_err(dev, "Error retrieving VPD size, rc=%x\n",
+			crq->get_vpd_size_rsp.rc.code);
+		complete(&adapter->fw_done);
+		return;
+	}
+
+	adapter->vpd->len = be64_to_cpu(crq->get_vpd_size_rsp.len);
+	complete(&adapter->fw_done);
+}
+
+static void handle_vpd_rsp(union ibmvnic_crq *crq,
+			   struct ibmvnic_adapter *adapter)
+{
+	struct device *dev = &adapter->vdev->dev;
+	unsigned char *substr = NULL, *ptr = NULL;
+	u8 fw_level_len = 0;
+
+	dma_unmap_single(dev, adapter->vpd->dma_addr, adapter->vpd->len,
+			 DMA_FROM_DEVICE);
+
+	if (crq->get_vpd_rsp.rc.code) {
+		dev_err(dev, "Error retrieving VPD from device, rc=%x\n",
+			crq->get_vpd_rsp.rc.code);
+		memset(adapter->fw_version, 0, 32);
+		goto complete;
+	}
+
+	/* get the position of the firmware version info
+	 * located after the ASCII 'RM' substring in the buffer
+	 */
+	substr = strnstr(adapter->vpd->buff, "RM", adapter->vpd->len);
+	if (!substr) {
+		dev_info(dev, "No FW level provided by VPD\n");
+		memset(adapter->fw_version, 0, 32);
+		goto complete;
+	}
+
+	/* get length of firmware level ASCII substring */
+	if ((substr + 2) < (adapter->vpd->buff + adapter->vpd->len)) {
+		fw_level_len = *(substr + 2);
+	} else {
+		dev_info(dev, "Length of FW substr extrapolated VDP buff\n");
+		memset(adapter->fw_version, 0, 32);
+		goto complete;
+	}
+
+	/* copy firmware version string from vpd into adapter */
+	if ((substr + 3 + fw_level_len) <
+	    (adapter->vpd->buff + adapter->vpd->len)) {
+		ptr = strncpy((char *)adapter->fw_version,
+			      substr + 3, fw_level_len);
+
+		if (!ptr) {
+			dev_err(dev, "Failed to isolate FW level string\n");
+			memset(adapter->fw_version, 0, 32);
+		}
+	} else {
+		dev_info(dev, "FW substr extrapolated VPD buff\n");
+		memset(adapter->fw_version, 0, 32);
+	}
+
+complete:
+	complete(&adapter->fw_done);
+}
+
 static void handle_query_ip_offload_rsp(struct ibmvnic_adapter *adapter)
 {
 	struct device *dev = &adapter->vdev->dev;
@@ -3807,6 +3948,12 @@ static void ibmvnic_handle_crq(union ibmvnic_crq *crq,
 		netdev_dbg(netdev, "Got Collect firmware trace Response\n");
 		complete(&adapter->fw_done);
 		break;
+	case GET_VPD_SIZE_RSP:
+		handle_vpd_size_rsp(crq, adapter);
+		break;
+	case GET_VPD_RSP:
+		handle_vpd_rsp(crq, adapter);
+		break;
 	default:
 		netdev_err(netdev, "Got an invalid cmd type 0x%02x\n",
 			   gen_crq->cmd);
diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
index 4670af8..d3a6959 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.h
+++ b/drivers/net/ethernet/ibm/ibmvnic.h
@@ -558,6 +558,12 @@ struct ibmvnic_multicast_ctrl {
 	struct ibmvnic_rc rc;
 } __packed __aligned(8);
 
+struct ibmvnic_get_vpd_size {
+	u8 first;
+	u8 cmd;
+	u8 reserved[14];
+} __packed __aligned(8);
+
 struct ibmvnic_get_vpd_size_rsp {
 	u8 first;
 	u8 cmd;
@@ -575,6 +581,13 @@ struct ibmvnic_get_vpd {
 	u8 reserved[4];
 } __packed __aligned(8);
 
+struct ibmvnic_get_vpd_rsp {
+	u8 first;
+	u8 cmd;
+	u8 reserved[10];
+	struct ibmvnic_rc rc;
+} __packed __aligned(8);
+
 struct ibmvnic_acl_change_indication {
 	u8 first;
 	u8 cmd;
@@ -700,10 +713,10 @@ union ibmvnic_crq {
 	struct ibmvnic_change_mac_addr change_mac_addr_rsp;
 	struct ibmvnic_multicast_ctrl multicast_ctrl;
 	struct ibmvnic_multicast_ctrl multicast_ctrl_rsp;
-	struct ibmvnic_generic_crq get_vpd_size;
+	struct ibmvnic_get_vpd_size get_vpd_size;
 	struct ibmvnic_get_vpd_size_rsp get_vpd_size_rsp;
 	struct ibmvnic_get_vpd get_vpd;
-	struct ibmvnic_generic_crq get_vpd_rsp;
+	struct ibmvnic_get_vpd_rsp get_vpd_rsp;
 	struct ibmvnic_acl_change_indication acl_change_indication;
 	struct ibmvnic_acl_query acl_query;
 	struct ibmvnic_generic_crq acl_query_rsp;
@@ -937,6 +950,12 @@ struct ibmvnic_error_buff {
 	__be32 error_id;
 };
 
+struct ibmvnic_vpd {
+	unsigned char *buff;
+	dma_addr_t dma_addr;
+	u64 len;
+};
+
 enum vnic_state {VNIC_PROBING = 1,
 		 VNIC_PROBED,
 		 VNIC_OPENING,
@@ -978,6 +997,10 @@ struct ibmvnic_adapter {
 	dma_addr_t ip_offload_ctrl_tok;
 	u32 msg_enable;
 
+	/* Vital Product Data (VPD) */
+	struct ibmvnic_vpd *vpd;
+	char fw_version[32];
+
 	/* Statistics */
 	struct ibmvnic_statistics stats;
 	dma_addr_t stats_token;
-- 
2.9.5

[PATCH] [net-next, v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver

[Desnes Augusto Nunes do Rosario]

This patch implements and enables VDP support for the ibmvnic driver.
Moreover, it includes the implementation of suitable structs, signal
 transmission/handling and functions which allows the retrival of firmware
 information from the ibmvnic card through the ethtool command.

Signed-off-by: Desnes A. Nunes do Rosario <desnesn@linux.vnet.ibm.com>
Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 149 ++++++++++++++++++++++++++++++++++++-
 drivers/net/ethernet/ibm/ibmvnic.h |  27 ++++++-
 2 files changed, 173 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index d0cff28..693b502 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -573,6 +573,15 @@ static int reset_tx_pools(struct ibmvnic_adapter *adapter)
 	return 0;
 }
 
+static void release_vpd_data(struct ibmvnic_adapter *adapter)
+{
+	if (!adapter->vpd)
+		return;
+
+	kfree(adapter->vpd->buff);
+	kfree(adapter->vpd);
+}
+
 static void release_tx_pools(struct ibmvnic_adapter *adapter)
 {
 	struct ibmvnic_tx_pool *tx_pool;
@@ -753,6 +762,8 @@ static void release_resources(struct ibmvnic_adapter *adapter)
 {
 	int i;
 
+	release_vpd_data(adapter);
+
 	release_tx_pools(adapter);
 	release_rx_pools(adapter);
 
@@ -833,6 +844,53 @@ static int set_real_num_queues(struct net_device *netdev)
 	return rc;
 }
 
+static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
+{
+	struct device *dev = &adapter->vdev->dev;
+	union ibmvnic_crq crq;
+	dma_addr_t dma_addr;
+	int len;
+
+	if (adapter->vpd->buff)
+		len = adapter->vpd->len;
+
+	reinit_completion(&adapter->fw_done);
+	crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
+	crq.get_vpd_size.cmd = GET_VPD_SIZE;
+	ibmvnic_send_crq(adapter, &crq);
+	wait_for_completion(&adapter->fw_done);
+
+	if (!adapter->vpd->buff)
+		adapter->vpd->buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
+	else if (adapter->vpd->len != len)
+		adapter->vpd->buff =
+			krealloc(adapter->vpd->buff,
+				 adapter->vpd->len, GFP_KERNEL);
+
+	if (!adapter->vpd->buff) {
+		dev_err(dev, "Could allocate VPD buffer\n");
+		return -ENOMEM;
+	}
+
+	adapter->vpd->dma_addr =
+		dma_map_single(dev, adapter->vpd->buff, adapter->vpd->len,
+			       DMA_FROM_DEVICE);
+	if (dma_mapping_error(dev, dma_addr)) {
+		dev_err(dev, "Could not map VPD buffer\n");
+		return -ENOMEM;
+	}
+
+	reinit_completion(&adapter->fw_done);
+	crq.get_vpd.first = IBMVNIC_CRQ_CMD;
+	crq.get_vpd.cmd = GET_VPD;
+	crq.get_vpd.ioba = cpu_to_be32(adapter->vpd->dma_addr);
+	crq.get_vpd.len = cpu_to_be32((u32)adapter->vpd->len);
+	ibmvnic_send_crq(adapter, &crq);
+	wait_for_completion(&adapter->fw_done);
+
+	return 0;
+}
+
 static int init_resources(struct ibmvnic_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
@@ -850,6 +908,10 @@ static int init_resources(struct ibmvnic_adapter *adapter)
 	if (rc)
 		return rc;
 
+	adapter->vpd = kzalloc(sizeof(*adapter->vpd), GFP_KERNEL);
+	if (!adapter->vpd)
+		return -ENOMEM;
+
 	adapter->map_id = 1;
 	adapter->napi = kcalloc(adapter->req_rx_queues,
 				sizeof(struct napi_struct), GFP_KERNEL);
@@ -950,6 +1012,10 @@ static int ibmvnic_open(struct net_device *netdev)
 
 	rc = __ibmvnic_open(netdev);
 	netif_carrier_on(netdev);
+
+	/* Vital Product Data (VPD) */
+	ibmvnic_get_vpd(adapter);
+
 	mutex_unlock(&adapter->reset_lock);
 
 	return rc;
@@ -1878,11 +1944,15 @@ static int ibmvnic_get_link_ksettings(struct net_device *netdev,
 	return 0;
 }
 
-static void ibmvnic_get_drvinfo(struct net_device *dev,
+static void ibmvnic_get_drvinfo(struct net_device *netdev,
 				struct ethtool_drvinfo *info)
 {
+	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
+
 	strlcpy(info->driver, ibmvnic_driver_name, sizeof(info->driver));
 	strlcpy(info->version, IBMVNIC_DRIVER_VERSION, sizeof(info->version));
+	strlcpy(info->fw_version, adapter->fw_version,
+		sizeof(info->fw_version));
 }
 
 static u32 ibmvnic_get_msglevel(struct net_device *netdev)
@@ -3076,6 +3146,77 @@ static void send_cap_queries(struct ibmvnic_adapter *adapter)
 	ibmvnic_send_crq(adapter, &crq);
 }
 
+static void handle_vpd_size_rsp(union ibmvnic_crq *crq,
+				struct ibmvnic_adapter *adapter)
+{
+	struct device *dev = &adapter->vdev->dev;
+
+	if (crq->get_vpd_size_rsp.rc.code) {
+		dev_err(dev, "Error retrieving VPD size, rc=%x\n",
+			crq->get_vpd_size_rsp.rc.code);
+		complete(&adapter->fw_done);
+		return;
+	}
+
+	adapter->vpd->len = be64_to_cpu(crq->get_vpd_size_rsp.len);
+	complete(&adapter->fw_done);
+}
+
+static void handle_vpd_rsp(union ibmvnic_crq *crq,
+			   struct ibmvnic_adapter *adapter)
+{
+	struct device *dev = &adapter->vdev->dev;
+	unsigned char *substr = NULL, *ptr = NULL;
+	u8 fw_level_len = 0;
+
+	dma_unmap_single(dev, adapter->vpd->dma_addr, adapter->vpd->len,
+			 DMA_FROM_DEVICE);
+
+	if (crq->get_vpd_rsp.rc.code) {
+		dev_err(dev, "Error retrieving VPD from device, rc=%x\n",
+			crq->get_vpd_rsp.rc.code);
+		memset(adapter->fw_version, 0, 32);
+		goto complete;
+	}
+
+	/* get the position of the firmware version info
+	 * located after the ASCII 'RM' substring in the buffer
+	 */
+	substr = strnstr(adapter->vpd->buff, "RM", adapter->vpd->len);
+	if (!substr) {
+		dev_info(dev, "No FW level provided by VPD\n");
+		memset(adapter->fw_version, 0, 32);
+		goto complete;
+	}
+
+	/* get length of firmware level ASCII substring */
+	if ((substr + 2) < (adapter->vpd->buff + adapter->vpd->len)) {
+		fw_level_len = *(substr + 2);
+	} else {
+		dev_info(dev, "Length of FW substr extrapolated VDP buff\n");
+		memset(adapter->fw_version, 0, 32);
+		goto complete;
+	}
+
+	/* copy firmware version string from vpd into adapter */
+	if ((substr + 3 + fw_level_len) <
+	    (adapter->vpd->buff + adapter->vpd->len)) {
+		ptr = strncpy((char *)adapter->fw_version,
+			      substr + 3, fw_level_len);
+
+		if (!ptr) {
+			dev_err(dev, "Failed to isolate FW level string\n");
+			memset(adapter->fw_version, 0, 32);
+		}
+	} else {
+		dev_info(dev, "FW substr extrapolated VPD buff\n");
+		memset(adapter->fw_version, 0, 32);
+	}
+
+complete:
+	complete(&adapter->fw_done);
+}
+
 static void handle_query_ip_offload_rsp(struct ibmvnic_adapter *adapter)
 {
 	struct device *dev = &adapter->vdev->dev;
@@ -3807,6 +3948,12 @@ static void ibmvnic_handle_crq(union ibmvnic_crq *crq,
 		netdev_dbg(netdev, "Got Collect firmware trace Response\n");
 		complete(&adapter->fw_done);
 		break;
+	case GET_VPD_SIZE_RSP:
+		handle_vpd_size_rsp(crq, adapter);
+		break;
+	case GET_VPD_RSP:
+		handle_vpd_rsp(crq, adapter);
+		break;
 	default:
 		netdev_err(netdev, "Got an invalid cmd type 0x%02x\n",
 			   gen_crq->cmd);
diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
index 4670af8..d3a6959 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.h
+++ b/drivers/net/ethernet/ibm/ibmvnic.h
@@ -558,6 +558,12 @@ struct ibmvnic_multicast_ctrl {
 	struct ibmvnic_rc rc;
 } __packed __aligned(8);
 
+struct ibmvnic_get_vpd_size {
+	u8 first;
+	u8 cmd;
+	u8 reserved[14];
+} __packed __aligned(8);
+
 struct ibmvnic_get_vpd_size_rsp {
 	u8 first;
 	u8 cmd;
@@ -575,6 +581,13 @@ struct ibmvnic_get_vpd {
 	u8 reserved[4];
 } __packed __aligned(8);
 
+struct ibmvnic_get_vpd_rsp {
+	u8 first;
+	u8 cmd;
+	u8 reserved[10];
+	struct ibmvnic_rc rc;
+} __packed __aligned(8);
+
 struct ibmvnic_acl_change_indication {
 	u8 first;
 	u8 cmd;
@@ -700,10 +713,10 @@ union ibmvnic_crq {
 	struct ibmvnic_change_mac_addr change_mac_addr_rsp;
 	struct ibmvnic_multicast_ctrl multicast_ctrl;
 	struct ibmvnic_multicast_ctrl multicast_ctrl_rsp;
-	struct ibmvnic_generic_crq get_vpd_size;
+	struct ibmvnic_get_vpd_size get_vpd_size;
 	struct ibmvnic_get_vpd_size_rsp get_vpd_size_rsp;
 	struct ibmvnic_get_vpd get_vpd;
-	struct ibmvnic_generic_crq get_vpd_rsp;
+	struct ibmvnic_get_vpd_rsp get_vpd_rsp;
 	struct ibmvnic_acl_change_indication acl_change_indication;
 	struct ibmvnic_acl_query acl_query;
 	struct ibmvnic_generic_crq acl_query_rsp;
@@ -937,6 +950,12 @@ struct ibmvnic_error_buff {
 	__be32 error_id;
 };
 
+struct ibmvnic_vpd {
+	unsigned char *buff;
+	dma_addr_t dma_addr;
+	u64 len;
+};
+
 enum vnic_state {VNIC_PROBING = 1,
 		 VNIC_PROBED,
 		 VNIC_OPENING,
@@ -978,6 +997,10 @@ struct ibmvnic_adapter {
 	dma_addr_t ip_offload_ctrl_tok;
 	u32 msg_enable;
 
+	/* Vital Product Data (VPD) */
+	struct ibmvnic_vpd *vpd;
+	char fw_version[32];
+
 	/* Statistics */
 	struct ibmvnic_statistics stats;
 	dma_addr_t stats_token;
-- 
2.9.5

Re: [PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver

[Tushar Dave]

On 11/09/2017 11:00 AM, Desnes Augusto Nunes do Rosario wrote:
> This patch implements and enables VDP support for the ibmvnic driver.
> Moreover, it includes the implementation of suitable structs, signal
>   transmission/handling and functions which allows the retrival of firmware
>   information from the ibmvnic card through the ethtool command.
> 
> Signed-off-by: Desnes A. Nunes do Rosario <desnesn@linux.vnet.ibm.com>
> Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
> ---
>   drivers/net/ethernet/ibm/ibmvnic.c | 149 ++++++++++++++++++++++++++++++++++++-
>   drivers/net/ethernet/ibm/ibmvnic.h |  27 ++++++-
>   2 files changed, 173 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index d0cff28..693b502 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -573,6 +573,15 @@ static int reset_tx_pools(struct ibmvnic_adapter *adapter)
>   	return 0;
>   }
>   
> +static void release_vpd_data(struct ibmvnic_adapter *adapter)
> +{
> +	if (!adapter->vpd)
> +		return;
> +
> +	kfree(adapter->vpd->buff);
> +	kfree(adapter->vpd);
> +}
> +
>   static void release_tx_pools(struct ibmvnic_adapter *adapter)
>   {
>   	struct ibmvnic_tx_pool *tx_pool;
> @@ -753,6 +762,8 @@ static void release_resources(struct ibmvnic_adapter *adapter)
>   {
>   	int i;
>   
> +	release_vpd_data(adapter);
> +
>   	release_tx_pools(adapter);
>   	release_rx_pools(adapter);
>   
> @@ -833,6 +844,53 @@ static int set_real_num_queues(struct net_device *netdev)
>   	return rc;
>   }
>   
> +static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
> +{
> +	struct device *dev = &adapter->vdev->dev;
> +	union ibmvnic_crq crq;
> +	dma_addr_t dma_addr;
> +	int len;
> +
> +	if (adapter->vpd->buff)
> +		len = adapter->vpd->len;
> +
> +	reinit_completion(&adapter->fw_done);
> +	crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
> +	crq.get_vpd_size.cmd = GET_VPD_SIZE;
> +	ibmvnic_send_crq(adapter, &crq);
> +	wait_for_completion(&adapter->fw_done);
> +
> +	if (!adapter->vpd->buff)
> +		adapter->vpd->buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
> +	else if (adapter->vpd->len != len)
> +		adapter->vpd->buff =
> +			krealloc(adapter->vpd->buff,
> +				 adapter->vpd->len, GFP_KERNEL);
> +
> +	if (!adapter->vpd->buff) {
> +		dev_err(dev, "Could allocate VPD buffer\n");
> +		return -ENOMEM;
> +	}
> +
> +	adapter->vpd->dma_addr =
> +		dma_map_single(dev, adapter->vpd->buff, adapter->vpd->len,
> +			       DMA_FROM_DEVICE);
> +	if (dma_mapping_error(dev, dma_addr)) {
> +		dev_err(dev, "Could not map VPD buffer\n");
> +		return -ENOMEM;
In this case, driver should release memory for adapter->vpd->buff.

> +	}
> +
> +	reinit_completion(&adapter->fw_done);
> +	crq.get_vpd.first = IBMVNIC_CRQ_CMD;
> +	crq.get_vpd.cmd = GET_VPD;
> +	crq.get_vpd.ioba = cpu_to_be32(adapter->vpd->dma_addr);
> +	crq.get_vpd.len = cpu_to_be32((u32)adapter->vpd->len);
> +	ibmvnic_send_crq(adapter, &crq);
> +	wait_for_completion(&adapter->fw_done);
> +
> +	return 0;
> +}
> +
>   static int init_resources(struct ibmvnic_adapter *adapter)
>   {
>   	struct net_device *netdev = adapter->netdev;
> @@ -850,6 +908,10 @@ static int init_resources(struct ibmvnic_adapter *adapter)
>   	if (rc)
>   		return rc;
>   
> +	adapter->vpd = kzalloc(sizeof(*adapter->vpd), GFP_KERNEL);
> +	if (!adapter->vpd)
> +		return -ENOMEM;
> +
>   	adapter->map_id = 1;
>   	adapter->napi = kcalloc(adapter->req_rx_queues,
>   				sizeof(struct napi_struct), GFP_KERNEL);
> @@ -950,6 +1012,10 @@ static int ibmvnic_open(struct net_device *netdev)
>   
>   	rc = __ibmvnic_open(netdev);
>   	netif_carrier_on(netdev);
> +
> +	/* Vital Product Data (VPD) */
> +	ibmvnic_get_vpd(adapter);
May want to check return value from ibmvnic_get_vpd() ?

-Tushar
> +
>   	mutex_unlock(&adapter->reset_lock);
>   
>   	return rc;
> @@ -1878,11 +1944,15 @@ static int ibmvnic_get_link_ksettings(struct net_device *netdev,
>   	return 0;
>   }
>   
> -static void ibmvnic_get_drvinfo(struct net_device *dev,
> +static void ibmvnic_get_drvinfo(struct net_device *netdev,
>   				struct ethtool_drvinfo *info)
>   {
> +	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
> +
>   	strlcpy(info->driver, ibmvnic_driver_name, sizeof(info->driver));
>   	strlcpy(info->version, IBMVNIC_DRIVER_VERSION, sizeof(info->version));
> +	strlcpy(info->fw_version, adapter->fw_version,
> +		sizeof(info->fw_version));
>   }
>   
>   static u32 ibmvnic_get_msglevel(struct net_device *netdev)
> @@ -3076,6 +3146,77 @@ static void send_cap_queries(struct ibmvnic_adapter *adapter)
>   	ibmvnic_send_crq(adapter, &crq);
>   }
>   
> +static void handle_vpd_size_rsp(union ibmvnic_crq *crq,
> +				struct ibmvnic_adapter *adapter)
> +{
> +	struct device *dev = &adapter->vdev->dev;
> +
> +	if (crq->get_vpd_size_rsp.rc.code) {
> +		dev_err(dev, "Error retrieving VPD size, rc=%x\n",
> +			crq->get_vpd_size_rsp.rc.code);
> +		complete(&adapter->fw_done);
> +		return;
> +	}
> +
> +	adapter->vpd->len = be64_to_cpu(crq->get_vpd_size_rsp.len);
> +	complete(&adapter->fw_done);
> +}
> +
> +static void handle_vpd_rsp(union ibmvnic_crq *crq,
> +			   struct ibmvnic_adapter *adapter)
> +{
> +	struct device *dev = &adapter->vdev->dev;
> +	unsigned char *substr = NULL, *ptr = NULL;
> +	u8 fw_level_len = 0;
> +
> +	dma_unmap_single(dev, adapter->vpd->dma_addr, adapter->vpd->len,
> +			 DMA_FROM_DEVICE);
> +
> +	if (crq->get_vpd_rsp.rc.code) {
> +		dev_err(dev, "Error retrieving VPD from device, rc=%x\n",
> +			crq->get_vpd_rsp.rc.code);
> +		memset(adapter->fw_version, 0, 32);
> +		goto complete;
> +	}
> +
> +	/* get the position of the firmware version info
> +	 * located after the ASCII 'RM' substring in the buffer
> +	 */
> +	substr = strnstr(adapter->vpd->buff, "RM", adapter->vpd->len);
> +	if (!substr) {
> +		dev_info(dev, "No FW level provided by VPD\n");
> +		memset(adapter->fw_version, 0, 32);
> +		goto complete;
> +	}
> +
> +	/* get length of firmware level ASCII substring */
> +	if ((substr + 2) < (adapter->vpd->buff + adapter->vpd->len)) {
> +		fw_level_len = *(substr + 2);
> +	} else {
> +		dev_info(dev, "Length of FW substr extrapolated VDP buff\n");
> +		memset(adapter->fw_version, 0, 32);
> +		goto complete;
> +	}
> +
> +	/* copy firmware version string from vpd into adapter */
> +	if ((substr + 3 + fw_level_len) <
> +	    (adapter->vpd->buff + adapter->vpd->len)) {
> +		ptr = strncpy((char *)adapter->fw_version,
> +			      substr + 3, fw_level_len);
> +
> +		if (!ptr) {
> +			dev_err(dev, "Failed to isolate FW level string\n");
> +			memset(adapter->fw_version, 0, 32);
> +		}
> +	} else {
> +		dev_info(dev, "FW substr extrapolated VPD buff\n");
> +		memset(adapter->fw_version, 0, 32);
> +	}
> +
> +complete:
> +	complete(&adapter->fw_done);
> +}
> +
>   static void handle_query_ip_offload_rsp(struct ibmvnic_adapter *adapter)
>   {
>   	struct device *dev = &adapter->vdev->dev;
> @@ -3807,6 +3948,12 @@ static void ibmvnic_handle_crq(union ibmvnic_crq *crq,
>   		netdev_dbg(netdev, "Got Collect firmware trace Response\n");
>   		complete(&adapter->fw_done);
>   		break;
> +	case GET_VPD_SIZE_RSP:
> +		handle_vpd_size_rsp(crq, adapter);
> +		break;
> +	case GET_VPD_RSP:
> +		handle_vpd_rsp(crq, adapter);
> +		break;
>   	default:
>   		netdev_err(netdev, "Got an invalid cmd type 0x%02x\n",
>   			   gen_crq->cmd);
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
> index 4670af8..d3a6959 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.h
> +++ b/drivers/net/ethernet/ibm/ibmvnic.h
> @@ -558,6 +558,12 @@ struct ibmvnic_multicast_ctrl {
>   	struct ibmvnic_rc rc;
>   } __packed __aligned(8);
>   
> +struct ibmvnic_get_vpd_size {
> +	u8 first;
> +	u8 cmd;
> +	u8 reserved[14];
> +} __packed __aligned(8);
> +
>   struct ibmvnic_get_vpd_size_rsp {
>   	u8 first;
>   	u8 cmd;
> @@ -575,6 +581,13 @@ struct ibmvnic_get_vpd {
>   	u8 reserved[4];
>   } __packed __aligned(8);
>   
> +struct ibmvnic_get_vpd_rsp {
> +	u8 first;
> +	u8 cmd;
> +	u8 reserved[10];
> +	struct ibmvnic_rc rc;
> +} __packed __aligned(8);
> +
>   struct ibmvnic_acl_change_indication {
>   	u8 first;
>   	u8 cmd;
> @@ -700,10 +713,10 @@ union ibmvnic_crq {
>   	struct ibmvnic_change_mac_addr change_mac_addr_rsp;
>   	struct ibmvnic_multicast_ctrl multicast_ctrl;
>   	struct ibmvnic_multicast_ctrl multicast_ctrl_rsp;
> -	struct ibmvnic_generic_crq get_vpd_size;
> +	struct ibmvnic_get_vpd_size get_vpd_size;
>   	struct ibmvnic_get_vpd_size_rsp get_vpd_size_rsp;
>   	struct ibmvnic_get_vpd get_vpd;
> -	struct ibmvnic_generic_crq get_vpd_rsp;
> +	struct ibmvnic_get_vpd_rsp get_vpd_rsp;
>   	struct ibmvnic_acl_change_indication acl_change_indication;
>   	struct ibmvnic_acl_query acl_query;
>   	struct ibmvnic_generic_crq acl_query_rsp;
> @@ -937,6 +950,12 @@ struct ibmvnic_error_buff {
>   	__be32 error_id;
>   };
>   
> +struct ibmvnic_vpd {
> +	unsigned char *buff;
> +	dma_addr_t dma_addr;
> +	u64 len;
> +};
> +
>   enum vnic_state {VNIC_PROBING = 1,
>   		 VNIC_PROBED,
>   		 VNIC_OPENING,
> @@ -978,6 +997,10 @@ struct ibmvnic_adapter {
>   	dma_addr_t ip_offload_ctrl_tok;
>   	u32 msg_enable;
>   
> +	/* Vital Product Data (VPD) */
> +	struct ibmvnic_vpd *vpd;
> +	char fw_version[32];
> +
>   	/* Statistics */
>   	struct ibmvnic_statistics stats;
>   	dma_addr_t stats_token;
> 

Re: [PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver

[Nathan Fontenot]

On 11/09/2017 01:00 PM, Desnes Augusto Nunes do Rosario wrote:
> This patch implements and enables VDP support for the ibmvnic driver.
> Moreover, it includes the implementation of suitable structs, signal
>  transmission/handling and functions which allows the retrival of firmware
>  information from the ibmvnic card through the ethtool command.
> 
> Signed-off-by: Desnes A. Nunes do Rosario <desnesn@linux.vnet.ibm.com>
> Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
> ---
>  drivers/net/ethernet/ibm/ibmvnic.c | 149 ++++++++++++++++++++++++++++++++++++-
>  drivers/net/ethernet/ibm/ibmvnic.h |  27 ++++++-
>  2 files changed, 173 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index d0cff28..693b502 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -573,6 +573,15 @@ static int reset_tx_pools(struct ibmvnic_adapter *adapter)
>  	return 0;
>  }
> 
> +static void release_vpd_data(struct ibmvnic_adapter *adapter)
> +{
> +	if (!adapter->vpd)
> +		return;
> +
> +	kfree(adapter->vpd->buff);
> +	kfree(adapter->vpd);
> +}
> +
>  static void release_tx_pools(struct ibmvnic_adapter *adapter)
>  {
>  	struct ibmvnic_tx_pool *tx_pool;
> @@ -753,6 +762,8 @@ static void release_resources(struct ibmvnic_adapter *adapter)
>  {
>  	int i;
> 
> +	release_vpd_data(adapter);
> +
>  	release_tx_pools(adapter);
>  	release_rx_pools(adapter);
> 
> @@ -833,6 +844,53 @@ static int set_real_num_queues(struct net_device *netdev)
>  	return rc;
>  }
> 
> +static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
> +{
> +	struct device *dev = &adapter->vdev->dev;
> +	union ibmvnic_crq crq;
> +	dma_addr_t dma_addr;
> +	int len;
> +
> +	if (adapter->vpd->buff)
> +		len = adapter->vpd->len;
> +
> +	reinit_completion(&adapter->fw_done);
> +	crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
> +	crq.get_vpd_size.cmd = GET_VPD_SIZE;
> +	ibmvnic_send_crq(adapter, &crq);
> +	wait_for_completion(&adapter->fw_done);
> +

Shouldn't there be a check for the return code when getting the
vpd size?


> +	if (!adapter->vpd->buff)
> +		adapter->vpd->buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
> +	else if (adapter->vpd->len != len)
> +		adapter->vpd->buff =
> +			krealloc(adapter->vpd->buff,
> +				 adapter->vpd->len, GFP_KERNEL);
> +
> +	if (!adapter->vpd->buff) {
> +		dev_err(dev, "Could allocate VPD buffer\n");
> +		return -ENOMEM;
> +	}
> +
> +	adapter->vpd->dma_addr =
> +		dma_map_single(dev, adapter->vpd->buff, adapter->vpd->len,
> +			       DMA_FROM_DEVICE);
> +	if (dma_mapping_error(dev, dma_addr)) {
> +		dev_err(dev, "Could not map VPD buffer\n");
> +		return -ENOMEM;
> +	}
> +
> +	reinit_completion(&adapter->fw_done);
> +	crq.get_vpd.first = IBMVNIC_CRQ_CMD;
> +	crq.get_vpd.cmd = GET_VPD;
> +	crq.get_vpd.ioba = cpu_to_be32(adapter->vpd->dma_addr);
> +	crq.get_vpd.len = cpu_to_be32((u32)adapter->vpd->len);
> +	ibmvnic_send_crq(adapter, &crq);
> +	wait_for_completion(&adapter->fw_done);
> +
> +	return 0;
> +}
> +
>  static int init_resources(struct ibmvnic_adapter *adapter)
>  {
>  	struct net_device *netdev = adapter->netdev;
> @@ -850,6 +908,10 @@ static int init_resources(struct ibmvnic_adapter *adapter)
>  	if (rc)
>  		return rc;
> 
> +	adapter->vpd = kzalloc(sizeof(*adapter->vpd), GFP_KERNEL);
> +	if (!adapter->vpd)
> +		return -ENOMEM;
> +
>  	adapter->map_id = 1;
>  	adapter->napi = kcalloc(adapter->req_rx_queues,
>  				sizeof(struct napi_struct), GFP_KERNEL);
> @@ -950,6 +1012,10 @@ static int ibmvnic_open(struct net_device *netdev)
> 
>  	rc = __ibmvnic_open(netdev);
>  	netif_carrier_on(netdev);
> +
> +	/* Vital Product Data (VPD) */
> +	ibmvnic_get_vpd(adapter);
> +
>  	mutex_unlock(&adapter->reset_lock);
> 
>  	return rc;
> @@ -1878,11 +1944,15 @@ static int ibmvnic_get_link_ksettings(struct net_device *netdev,
>  	return 0;
>  }
> 
> -static void ibmvnic_get_drvinfo(struct net_device *dev,
> +static void ibmvnic_get_drvinfo(struct net_device *netdev,
>  				struct ethtool_drvinfo *info)
>  {
> +	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
> +
>  	strlcpy(info->driver, ibmvnic_driver_name, sizeof(info->driver));
>  	strlcpy(info->version, IBMVNIC_DRIVER_VERSION, sizeof(info->version));
> +	strlcpy(info->fw_version, adapter->fw_version,
> +		sizeof(info->fw_version));
>  }
> 
>  static u32 ibmvnic_get_msglevel(struct net_device *netdev)
> @@ -3076,6 +3146,77 @@ static void send_cap_queries(struct ibmvnic_adapter *adapter)
>  	ibmvnic_send_crq(adapter, &crq);
>  }
> 
> +static void handle_vpd_size_rsp(union ibmvnic_crq *crq,
> +				struct ibmvnic_adapter *adapter)
> +{
> +	struct device *dev = &adapter->vdev->dev;
> +
> +	if (crq->get_vpd_size_rsp.rc.code) {
> +		dev_err(dev, "Error retrieving VPD size, rc=%x\n",
> +			crq->get_vpd_size_rsp.rc.code);
> +		complete(&adapter->fw_done);
> +		return;
> +	}
> +
> +	adapter->vpd->len = be64_to_cpu(crq->get_vpd_size_rsp.len);
> +	complete(&adapter->fw_done);
> +}
> +
> +static void handle_vpd_rsp(union ibmvnic_crq *crq,
> +			   struct ibmvnic_adapter *adapter)
> +{
> +	struct device *dev = &adapter->vdev->dev;
> +	unsigned char *substr = NULL, *ptr = NULL;
> +	u8 fw_level_len = 0;
> +
> +	dma_unmap_single(dev, adapter->vpd->dma_addr, adapter->vpd->len,
> +			 DMA_FROM_DEVICE);
> +
> +	if (crq->get_vpd_rsp.rc.code) {
> +		dev_err(dev, "Error retrieving VPD from device, rc=%x\n",
> +			crq->get_vpd_rsp.rc.code);
> +		memset(adapter->fw_version, 0, 32);
> +		goto complete;
> +	}
> +
> +	/* get the position of the firmware version info
> +	 * located after the ASCII 'RM' substring in the buffer
> +	 */
> +	substr = strnstr(adapter->vpd->buff, "RM", adapter->vpd->len);
> +	if (!substr) {
> +		dev_info(dev, "No FW level provided by VPD\n");
> +		memset(adapter->fw_version, 0, 32);
> +		goto complete;
> +	}
> +
> +	/* get length of firmware level ASCII substring */
> +	if ((substr + 2) < (adapter->vpd->buff + adapter->vpd->len)) {
> +		fw_level_len = *(substr + 2);
> +	} else {
> +		dev_info(dev, "Length of FW substr extrapolated VDP buff\n");
> +		memset(adapter->fw_version, 0, 32);
> +		goto complete;
> +	}
> +
> +	/* copy firmware version string from vpd into adapter */
> +	if ((substr + 3 + fw_level_len) <
> +	    (adapter->vpd->buff + adapter->vpd->len)) {
> +		ptr = strncpy((char *)adapter->fw_version,
> +			      substr + 3, fw_level_len);
> +
> +		if (!ptr) {
> +			dev_err(dev, "Failed to isolate FW level string\n");
> +			memset(adapter->fw_version, 0, 32);
> +		}
> +	} else {
> +		dev_info(dev, "FW substr extrapolated VPD buff\n");
> +		memset(adapter->fw_version, 0, 32);
> +	}
> +
> +complete:
> +	complete(&adapter->fw_done);
> +}
> +
>  static void handle_query_ip_offload_rsp(struct ibmvnic_adapter *adapter)
>  {
>  	struct device *dev = &adapter->vdev->dev;
> @@ -3807,6 +3948,12 @@ static void ibmvnic_handle_crq(union ibmvnic_crq *crq,
>  		netdev_dbg(netdev, "Got Collect firmware trace Response\n");
>  		complete(&adapter->fw_done);
>  		break;
> +	case GET_VPD_SIZE_RSP:
> +		handle_vpd_size_rsp(crq, adapter);
> +		break;
> +	case GET_VPD_RSP:
> +		handle_vpd_rsp(crq, adapter);
> +		break;
>  	default:
>  		netdev_err(netdev, "Got an invalid cmd type 0x%02x\n",
>  			   gen_crq->cmd);
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
> index 4670af8..d3a6959 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.h
> +++ b/drivers/net/ethernet/ibm/ibmvnic.h
> @@ -558,6 +558,12 @@ struct ibmvnic_multicast_ctrl {
>  	struct ibmvnic_rc rc;
>  } __packed __aligned(8);
> 
> +struct ibmvnic_get_vpd_size {
> +	u8 first;
> +	u8 cmd;
> +	u8 reserved[14];
> +} __packed __aligned(8);
> +
>  struct ibmvnic_get_vpd_size_rsp {
>  	u8 first;
>  	u8 cmd;
> @@ -575,6 +581,13 @@ struct ibmvnic_get_vpd {
>  	u8 reserved[4];
>  } __packed __aligned(8);
> 
> +struct ibmvnic_get_vpd_rsp {
> +	u8 first;
> +	u8 cmd;
> +	u8 reserved[10];
> +	struct ibmvnic_rc rc;
> +} __packed __aligned(8);
> +
>  struct ibmvnic_acl_change_indication {
>  	u8 first;
>  	u8 cmd;
> @@ -700,10 +713,10 @@ union ibmvnic_crq {
>  	struct ibmvnic_change_mac_addr change_mac_addr_rsp;
>  	struct ibmvnic_multicast_ctrl multicast_ctrl;
>  	struct ibmvnic_multicast_ctrl multicast_ctrl_rsp;
> -	struct ibmvnic_generic_crq get_vpd_size;
> +	struct ibmvnic_get_vpd_size get_vpd_size;
>  	struct ibmvnic_get_vpd_size_rsp get_vpd_size_rsp;
>  	struct ibmvnic_get_vpd get_vpd;
> -	struct ibmvnic_generic_crq get_vpd_rsp;
> +	struct ibmvnic_get_vpd_rsp get_vpd_rsp;
>  	struct ibmvnic_acl_change_indication acl_change_indication;
>  	struct ibmvnic_acl_query acl_query;
>  	struct ibmvnic_generic_crq acl_query_rsp;
> @@ -937,6 +950,12 @@ struct ibmvnic_error_buff {
>  	__be32 error_id;
>  };
> 
> +struct ibmvnic_vpd {
> +	unsigned char *buff;
> +	dma_addr_t dma_addr;
> +	u64 len;
> +};
> +
>  enum vnic_state {VNIC_PROBING = 1,
>  		 VNIC_PROBED,
>  		 VNIC_OPENING,
> @@ -978,6 +997,10 @@ struct ibmvnic_adapter {
>  	dma_addr_t ip_offload_ctrl_tok;
>  	u32 msg_enable;
> 
> +	/* Vital Product Data (VPD) */
> +	struct ibmvnic_vpd *vpd;
> +	char fw_version[32];
> +
>  	/* Statistics */
>  	struct ibmvnic_statistics stats;
>  	dma_addr_t stats_token;
> 

Re: [PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver

[Desnes Augusto Nunes do Rosário]

On 11/09/2017 06:31 PM, Nathan Fontenot wrote:
> On 11/09/2017 01:00 PM, Desnes Augusto Nunes do Rosario wrote:
>> This patch implements and enables VDP support for the ibmvnic driver.
>> Moreover, it includes the implementation of suitable structs, signal
>>   transmission/handling and functions which allows the retrival of firmware
>>   information from the ibmvnic card through the ethtool command.
>>
>> Signed-off-by: Desnes A. Nunes do Rosario <desnesn@linux.vnet.ibm.com>
>> Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
>> ---
>>   drivers/net/ethernet/ibm/ibmvnic.c | 149 ++++++++++++++++++++++++++++++++++++-
>>   drivers/net/ethernet/ibm/ibmvnic.h |  27 ++++++-
>>   2 files changed, 173 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
>> index d0cff28..693b502 100644
>> --- a/drivers/net/ethernet/ibm/ibmvnic.c
>> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>> @@ -573,6 +573,15 @@ static int reset_tx_pools(struct ibmvnic_adapter *adapter)
>>   	return 0;
>>   }
>>
>> +static void release_vpd_data(struct ibmvnic_adapter *adapter)
>> +{
>> +	if (!adapter->vpd)
>> +		return;
>> +
>> +	kfree(adapter->vpd->buff);
>> +	kfree(adapter->vpd);
>> +}
>> +
>>   static void release_tx_pools(struct ibmvnic_adapter *adapter)
>>   {
>>   	struct ibmvnic_tx_pool *tx_pool;
>> @@ -753,6 +762,8 @@ static void release_resources(struct ibmvnic_adapter *adapter)
>>   {
>>   	int i;
>>
>> +	release_vpd_data(adapter);
>> +
>>   	release_tx_pools(adapter);
>>   	release_rx_pools(adapter);
>>
>> @@ -833,6 +844,53 @@ static int set_real_num_queues(struct net_device *netdev)
>>   	return rc;
>>   }
>>
>> +static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
>> +{
>> +	struct device *dev = &adapter->vdev->dev;
>> +	union ibmvnic_crq crq;
>> +	dma_addr_t dma_addr;
>> +	int len;
>> +
>> +	if (adapter->vpd->buff)
>> +		len = adapter->vpd->len;
>> +
>> +	reinit_completion(&adapter->fw_done);
>> +	crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
>> +	crq.get_vpd_size.cmd = GET_VPD_SIZE;
>> +	ibmvnic_send_crq(adapter, &crq);
>> +	wait_for_completion(&adapter->fw_done);
>> +
> 
> Shouldn't there be a check for the return code when getting the
> vpd size?

Hello Nathan,

This check is already being performed on the handle_vpd_size_rsp() 
function down below.

In short, a GET_VPD_SIZE signal is sent here through a ibmvnic_crq union 
in ibmvnic_send_crq(), whereas handle_query_ip_offload_rsp() receives 
from the VNIC adapter a GET_VPD_SIZE_RSP containing a ibmvnic_crq union 
with the vpd size information and the rc.code. If successful, a 
&adapter->fw_done is sent and this part of the code continues; however 
if not, a dev_error() is thrown. Same logic applies to GET_VPD/GET_VPD_RSP.

What I am adding on the next version of the patch is a check if 
adapter->vpd->len is different than 0 before allocating 
adapter->vpd->buff, since that in a case of a failure, adapter->vpd->len 
will be 0.

Best Regards,

> 
> 
>> +	if (!adapter->vpd->buff)
>> +		adapter->vpd->buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
>> +	else if (adapter->vpd->len != len)
>> +		adapter->vpd->buff =
>> +			krealloc(adapter->vpd->buff,
>> +				 adapter->vpd->len, GFP_KERNEL);
>> +
>> +	if (!adapter->vpd->buff) {
>> +		dev_err(dev, "Could allocate VPD buffer\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	adapter->vpd->dma_addr =
>> +		dma_map_single(dev, adapter->vpd->buff, adapter->vpd->len,
>> +			       DMA_FROM_DEVICE);
>> +	if (dma_mapping_error(dev, dma_addr)) {
>> +		dev_err(dev, "Could not map VPD buffer\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	reinit_completion(&adapter->fw_done);
>> +	crq.get_vpd.first = IBMVNIC_CRQ_CMD;
>> +	crq.get_vpd.cmd = GET_VPD;
>> +	crq.get_vpd.ioba = cpu_to_be32(adapter->vpd->dma_addr);
>> +	crq.get_vpd.len = cpu_to_be32((u32)adapter->vpd->len);
>> +	ibmvnic_send_crq(adapter, &crq);
>> +	wait_for_completion(&adapter->fw_done);
>> +
>> +	return 0;
>> +}
>> +
>>   static int init_resources(struct ibmvnic_adapter *adapter)
>>   {
>>   	struct net_device *netdev = adapter->netdev;
>> @@ -850,6 +908,10 @@ static int init_resources(struct ibmvnic_adapter *adapter)
>>   	if (rc)
>>   		return rc;
>>
>> +	adapter->vpd = kzalloc(sizeof(*adapter->vpd), GFP_KERNEL);
>> +	if (!adapter->vpd)
>> +		return -ENOMEM;
>> +
>>   	adapter->map_id = 1;
>>   	adapter->napi = kcalloc(adapter->req_rx_queues,
>>   				sizeof(struct napi_struct), GFP_KERNEL);
>> @@ -950,6 +1012,10 @@ static int ibmvnic_open(struct net_device *netdev)
>>
>>   	rc = __ibmvnic_open(netdev);
>>   	netif_carrier_on(netdev);
>> +
>> +	/* Vital Product Data (VPD) */
>> +	ibmvnic_get_vpd(adapter);
>> +
>>   	mutex_unlock(&adapter->reset_lock);
>>
>>   	return rc;
>> @@ -1878,11 +1944,15 @@ static int ibmvnic_get_link_ksettings(struct net_device *netdev,
>>   	return 0;
>>   }
>>
>> -static void ibmvnic_get_drvinfo(struct net_device *dev,
>> +static void ibmvnic_get_drvinfo(struct net_device *netdev,
>>   				struct ethtool_drvinfo *info)
>>   {
>> +	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
>> +
>>   	strlcpy(info->driver, ibmvnic_driver_name, sizeof(info->driver));
>>   	strlcpy(info->version, IBMVNIC_DRIVER_VERSION, sizeof(info->version));
>> +	strlcpy(info->fw_version, adapter->fw_version,
>> +		sizeof(info->fw_version));
>>   }
>>
>>   static u32 ibmvnic_get_msglevel(struct net_device *netdev)
>> @@ -3076,6 +3146,77 @@ static void send_cap_queries(struct ibmvnic_adapter *adapter)
>>   	ibmvnic_send_crq(adapter, &crq);
>>   }
>>
>> +static void handle_vpd_size_rsp(union ibmvnic_crq *crq,
>> +				struct ibmvnic_adapter *adapter)
>> +{
>> +	struct device *dev = &adapter->vdev->dev;
>> +
>> +	if (crq->get_vpd_size_rsp.rc.code) {
>> +		dev_err(dev, "Error retrieving VPD size, rc=%x\n",
>> +			crq->get_vpd_size_rsp.rc.code);
>> +		complete(&adapter->fw_done);
>> +		return;
>> +	}
>> +
>> +	adapter->vpd->len = be64_to_cpu(crq->get_vpd_size_rsp.len);
>> +	complete(&adapter->fw_done);
>> +}
>> +
>> +static void handle_vpd_rsp(union ibmvnic_crq *crq,
>> +			   struct ibmvnic_adapter *adapter)
>> +{
>> +	struct device *dev = &adapter->vdev->dev;
>> +	unsigned char *substr = NULL, *ptr = NULL;
>> +	u8 fw_level_len = 0;
>> +
>> +	dma_unmap_single(dev, adapter->vpd->dma_addr, adapter->vpd->len,
>> +			 DMA_FROM_DEVICE);
>> +
>> +	if (crq->get_vpd_rsp.rc.code) {
>> +		dev_err(dev, "Error retrieving VPD from device, rc=%x\n",
>> +			crq->get_vpd_rsp.rc.code);
>> +		memset(adapter->fw_version, 0, 32);
>> +		goto complete;
>> +	}
>> +
>> +	/* get the position of the firmware version info
>> +	 * located after the ASCII 'RM' substring in the buffer
>> +	 */
>> +	substr = strnstr(adapter->vpd->buff, "RM", adapter->vpd->len);
>> +	if (!substr) {
>> +		dev_info(dev, "No FW level provided by VPD\n");
>> +		memset(adapter->fw_version, 0, 32);
>> +		goto complete;
>> +	}
>> +
>> +	/* get length of firmware level ASCII substring */
>> +	if ((substr + 2) < (adapter->vpd->buff + adapter->vpd->len)) {
>> +		fw_level_len = *(substr + 2);
>> +	} else {
>> +		dev_info(dev, "Length of FW substr extrapolated VDP buff\n");
>> +		memset(adapter->fw_version, 0, 32);
>> +		goto complete;
>> +	}
>> +
>> +	/* copy firmware version string from vpd into adapter */
>> +	if ((substr + 3 + fw_level_len) <
>> +	    (adapter->vpd->buff + adapter->vpd->len)) {
>> +		ptr = strncpy((char *)adapter->fw_version,
>> +			      substr + 3, fw_level_len);
>> +
>> +		if (!ptr) {
>> +			dev_err(dev, "Failed to isolate FW level string\n");
>> +			memset(adapter->fw_version, 0, 32);
>> +		}
>> +	} else {
>> +		dev_info(dev, "FW substr extrapolated VPD buff\n");
>> +		memset(adapter->fw_version, 0, 32);
>> +	}
>> +
>> +complete:
>> +	complete(&adapter->fw_done);
>> +}
>> +
>>   static void handle_query_ip_offload_rsp(struct ibmvnic_adapter *adapter)
>>   {
>>   	struct device *dev = &adapter->vdev->dev;
>> @@ -3807,6 +3948,12 @@ static void ibmvnic_handle_crq(union ibmvnic_crq *crq,
>>   		netdev_dbg(netdev, "Got Collect firmware trace Response\n");
>>   		complete(&adapter->fw_done);
>>   		break;
>> +	case GET_VPD_SIZE_RSP:
>> +		handle_vpd_size_rsp(crq, adapter);
>> +		break;
>> +	case GET_VPD_RSP:
>> +		handle_vpd_rsp(crq, adapter);
>> +		break;
>>   	default:
>>   		netdev_err(netdev, "Got an invalid cmd type 0x%02x\n",
>>   			   gen_crq->cmd);
>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
>> index 4670af8..d3a6959 100644
>> --- a/drivers/net/ethernet/ibm/ibmvnic.h
>> +++ b/drivers/net/ethernet/ibm/ibmvnic.h
>> @@ -558,6 +558,12 @@ struct ibmvnic_multicast_ctrl {
>>   	struct ibmvnic_rc rc;
>>   } __packed __aligned(8);
>>
>> +struct ibmvnic_get_vpd_size {
>> +	u8 first;
>> +	u8 cmd;
>> +	u8 reserved[14];
>> +} __packed __aligned(8);
>> +
>>   struct ibmvnic_get_vpd_size_rsp {
>>   	u8 first;
>>   	u8 cmd;
>> @@ -575,6 +581,13 @@ struct ibmvnic_get_vpd {
>>   	u8 reserved[4];
>>   } __packed __aligned(8);
>>
>> +struct ibmvnic_get_vpd_rsp {
>> +	u8 first;
>> +	u8 cmd;
>> +	u8 reserved[10];
>> +	struct ibmvnic_rc rc;
>> +} __packed __aligned(8);
>> +
>>   struct ibmvnic_acl_change_indication {
>>   	u8 first;
>>   	u8 cmd;
>> @@ -700,10 +713,10 @@ union ibmvnic_crq {
>>   	struct ibmvnic_change_mac_addr change_mac_addr_rsp;
>>   	struct ibmvnic_multicast_ctrl multicast_ctrl;
>>   	struct ibmvnic_multicast_ctrl multicast_ctrl_rsp;
>> -	struct ibmvnic_generic_crq get_vpd_size;
>> +	struct ibmvnic_get_vpd_size get_vpd_size;
>>   	struct ibmvnic_get_vpd_size_rsp get_vpd_size_rsp;
>>   	struct ibmvnic_get_vpd get_vpd;
>> -	struct ibmvnic_generic_crq get_vpd_rsp;
>> +	struct ibmvnic_get_vpd_rsp get_vpd_rsp;
>>   	struct ibmvnic_acl_change_indication acl_change_indication;
>>   	struct ibmvnic_acl_query acl_query;
>>   	struct ibmvnic_generic_crq acl_query_rsp;
>> @@ -937,6 +950,12 @@ struct ibmvnic_error_buff {
>>   	__be32 error_id;
>>   };
>>
>> +struct ibmvnic_vpd {
>> +	unsigned char *buff;
>> +	dma_addr_t dma_addr;
>> +	u64 len;
>> +};
>> +
>>   enum vnic_state {VNIC_PROBING = 1,
>>   		 VNIC_PROBED,
>>   		 VNIC_OPENING,
>> @@ -978,6 +997,10 @@ struct ibmvnic_adapter {
>>   	dma_addr_t ip_offload_ctrl_tok;
>>   	u32 msg_enable;
>>
>> +	/* Vital Product Data (VPD) */
>> +	struct ibmvnic_vpd *vpd;
>> +	char fw_version[32];
>> +
>>   	/* Statistics */
>>   	struct ibmvnic_statistics stats;
>>   	dma_addr_t stats_token;
>>

-- 
Desnes Augusto Nunes do Rosário
------------------------------------------

Linux Developer - IBM / Brazil
M.Sc. in Electrical and Computer Engineering - UFRN

desnesn@br.ibm.com

Re: [PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver

[Nathan Fontenot]

On 11/10/2017 08:41 AM, Desnes Augusto Nunes do Rosário wrote:
> 
> 
> On 11/09/2017 06:31 PM, Nathan Fontenot wrote:
>> On 11/09/2017 01:00 PM, Desnes Augusto Nunes do Rosario wrote:
>>> This patch implements and enables VDP support for the ibmvnic driver.
>>> Moreover, it includes the implementation of suitable structs, signal
>>>   transmission/handling and functions which allows the retrival of firmware
>>>   information from the ibmvnic card through the ethtool command.
>>>
>>> Signed-off-by: Desnes A. Nunes do Rosario <desnesn@linux.vnet.ibm.com>
>>> Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
>>> ---
>>>   drivers/net/ethernet/ibm/ibmvnic.c | 149 ++++++++++++++++++++++++++++++++++++-
>>>   drivers/net/ethernet/ibm/ibmvnic.h |  27 ++++++-
>>>   2 files changed, 173 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
>>> index d0cff28..693b502 100644
>>> --- a/drivers/net/ethernet/ibm/ibmvnic.c
>>> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>>> @@ -573,6 +573,15 @@ static int reset_tx_pools(struct ibmvnic_adapter *adapter)
>>>       return 0;
>>>   }
>>>
>>> +static void release_vpd_data(struct ibmvnic_adapter *adapter)
>>> +{
>>> +    if (!adapter->vpd)
>>> +        return;
>>> +
>>> +    kfree(adapter->vpd->buff);
>>> +    kfree(adapter->vpd);
>>> +}
>>> +
>>>   static void release_tx_pools(struct ibmvnic_adapter *adapter)
>>>   {
>>>       struct ibmvnic_tx_pool *tx_pool;
>>> @@ -753,6 +762,8 @@ static void release_resources(struct ibmvnic_adapter *adapter)
>>>   {
>>>       int i;
>>>
>>> +    release_vpd_data(adapter);
>>> +
>>>       release_tx_pools(adapter);
>>>       release_rx_pools(adapter);
>>>
>>> @@ -833,6 +844,53 @@ static int set_real_num_queues(struct net_device *netdev)
>>>       return rc;
>>>   }
>>>
>>> +static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
>>> +{
>>> +    struct device *dev = &adapter->vdev->dev;
>>> +    union ibmvnic_crq crq;
>>> +    dma_addr_t dma_addr;
>>> +    int len;
>>> +
>>> +    if (adapter->vpd->buff)
>>> +        len = adapter->vpd->len;
>>> +
>>> +    reinit_completion(&adapter->fw_done);
>>> +    crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
>>> +    crq.get_vpd_size.cmd = GET_VPD_SIZE;
>>> +    ibmvnic_send_crq(adapter, &crq);
>>> +    wait_for_completion(&adapter->fw_done);
>>> +
>>
>> Shouldn't there be a check for the return code when getting the
>> vpd size?
> 
> Hello Nathan,
> 
> This check is already being performed on the handle_vpd_size_rsp() function down below.
> 
> In short, a GET_VPD_SIZE signal is sent here through a ibmvnic_crq union in ibmvnic_send_crq(), whereas handle_query_ip_offload_rsp() receives from the VNIC adapter a GET_VPD_SIZE_RSP containing a ibmvnic_crq union with the vpd size information and the rc.code. If successful, a &adapter->fw_done is sent and this part of the code continues; however if not, a dev_error() is thrown. Same logic applies to GET_VPD/GET_VPD_RSP.
> 

Yes, I did see that code. You do a complet of the completion variable for both success and failure,
this then lets this routine continue irregardless of the results of the get vpd size request. The
call to dev_err will print the error message but does not prevent use from bailing if the
get vpd size fails. Perhaps setting vpd->len to -1 to indicate the get vpd call failed which could
then be checked by the requester.

-Nathan


> What I am adding on the next version of the patch is a check if adapter->vpd->len is different than 0 before allocating adapter->vpd->buff, since that in a case of a failure, adapter->vpd->len will be 0.
> 
> Best Regards,
> 
>>
>>
>>> +    if (!adapter->vpd->buff)
>>> +        adapter->vpd->buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
>>> +    else if (adapter->vpd->len != len)
>>> +        adapter->vpd->buff =
>>> +            krealloc(adapter->vpd->buff,
>>> +                 adapter->vpd->len, GFP_KERNEL);
>>> +
>>> +    if (!adapter->vpd->buff) {
>>> +        dev_err(dev, "Could allocate VPD buffer\n");
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    adapter->vpd->dma_addr =
>>> +        dma_map_single(dev, adapter->vpd->buff, adapter->vpd->len,
>>> +                   DMA_FROM_DEVICE);
>>> +    if (dma_mapping_error(dev, dma_addr)) {
>>> +        dev_err(dev, "Could not map VPD buffer\n");
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    reinit_completion(&adapter->fw_done);
>>> +    crq.get_vpd.first = IBMVNIC_CRQ_CMD;
>>> +    crq.get_vpd.cmd = GET_VPD;
>>> +    crq.get_vpd.ioba = cpu_to_be32(adapter->vpd->dma_addr);
>>> +    crq.get_vpd.len = cpu_to_be32((u32)adapter->vpd->len);
>>> +    ibmvnic_send_crq(adapter, &crq);
>>> +    wait_for_completion(&adapter->fw_done);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static int init_resources(struct ibmvnic_adapter *adapter)
>>>   {
>>>       struct net_device *netdev = adapter->netdev;
>>> @@ -850,6 +908,10 @@ static int init_resources(struct ibmvnic_adapter *adapter)
>>>       if (rc)
>>>           return rc;
>>>
>>> +    adapter->vpd = kzalloc(sizeof(*adapter->vpd), GFP_KERNEL);
>>> +    if (!adapter->vpd)
>>> +        return -ENOMEM;
>>> +
>>>       adapter->map_id = 1;
>>>       adapter->napi = kcalloc(adapter->req_rx_queues,
>>>                   sizeof(struct napi_struct), GFP_KERNEL);
>>> @@ -950,6 +1012,10 @@ static int ibmvnic_open(struct net_device *netdev)
>>>
>>>       rc = __ibmvnic_open(netdev);
>>>       netif_carrier_on(netdev);
>>> +
>>> +    /* Vital Product Data (VPD) */
>>> +    ibmvnic_get_vpd(adapter);
>>> +
>>>       mutex_unlock(&adapter->reset_lock);
>>>
>>>       return rc;
>>> @@ -1878,11 +1944,15 @@ static int ibmvnic_get_link_ksettings(struct net_device *netdev,
>>>       return 0;
>>>   }
>>>
>>> -static void ibmvnic_get_drvinfo(struct net_device *dev,
>>> +static void ibmvnic_get_drvinfo(struct net_device *netdev,
>>>                   struct ethtool_drvinfo *info)
>>>   {
>>> +    struct ibmvnic_adapter *adapter = netdev_priv(netdev);
>>> +
>>>       strlcpy(info->driver, ibmvnic_driver_name, sizeof(info->driver));
>>>       strlcpy(info->version, IBMVNIC_DRIVER_VERSION, sizeof(info->version));
>>> +    strlcpy(info->fw_version, adapter->fw_version,
>>> +        sizeof(info->fw_version));
>>>   }
>>>
>>>   static u32 ibmvnic_get_msglevel(struct net_device *netdev)
>>> @@ -3076,6 +3146,77 @@ static void send_cap_queries(struct ibmvnic_adapter *adapter)
>>>       ibmvnic_send_crq(adapter, &crq);
>>>   }
>>>
>>> +static void handle_vpd_size_rsp(union ibmvnic_crq *crq,
>>> +                struct ibmvnic_adapter *adapter)
>>> +{
>>> +    struct device *dev = &adapter->vdev->dev;
>>> +
>>> +    if (crq->get_vpd_size_rsp.rc.code) {
>>> +        dev_err(dev, "Error retrieving VPD size, rc=%x\n",
>>> +            crq->get_vpd_size_rsp.rc.code);
>>> +        complete(&adapter->fw_done);
>>> +        return;
>>> +    }
>>> +
>>> +    adapter->vpd->len = be64_to_cpu(crq->get_vpd_size_rsp.len);
>>> +    complete(&adapter->fw_done);
>>> +}
>>> +
>>> +static void handle_vpd_rsp(union ibmvnic_crq *crq,
>>> +               struct ibmvnic_adapter *adapter)
>>> +{
>>> +    struct device *dev = &adapter->vdev->dev;
>>> +    unsigned char *substr = NULL, *ptr = NULL;
>>> +    u8 fw_level_len = 0;
>>> +
>>> +    dma_unmap_single(dev, adapter->vpd->dma_addr, adapter->vpd->len,
>>> +             DMA_FROM_DEVICE);
>>> +
>>> +    if (crq->get_vpd_rsp.rc.code) {
>>> +        dev_err(dev, "Error retrieving VPD from device, rc=%x\n",
>>> +            crq->get_vpd_rsp.rc.code);
>>> +        memset(adapter->fw_version, 0, 32);
>>> +        goto complete;
>>> +    }
>>> +
>>> +    /* get the position of the firmware version info
>>> +     * located after the ASCII 'RM' substring in the buffer
>>> +     */
>>> +    substr = strnstr(adapter->vpd->buff, "RM", adapter->vpd->len);
>>> +    if (!substr) {
>>> +        dev_info(dev, "No FW level provided by VPD\n");
>>> +        memset(adapter->fw_version, 0, 32);
>>> +        goto complete;
>>> +    }
>>> +
>>> +    /* get length of firmware level ASCII substring */
>>> +    if ((substr + 2) < (adapter->vpd->buff + adapter->vpd->len)) {
>>> +        fw_level_len = *(substr + 2);
>>> +    } else {
>>> +        dev_info(dev, "Length of FW substr extrapolated VDP buff\n");
>>> +        memset(adapter->fw_version, 0, 32);
>>> +        goto complete;
>>> +    }
>>> +
>>> +    /* copy firmware version string from vpd into adapter */
>>> +    if ((substr + 3 + fw_level_len) <
>>> +        (adapter->vpd->buff + adapter->vpd->len)) {
>>> +        ptr = strncpy((char *)adapter->fw_version,
>>> +                  substr + 3, fw_level_len);
>>> +
>>> +        if (!ptr) {
>>> +            dev_err(dev, "Failed to isolate FW level string\n");
>>> +            memset(adapter->fw_version, 0, 32);
>>> +        }
>>> +    } else {
>>> +        dev_info(dev, "FW substr extrapolated VPD buff\n");
>>> +        memset(adapter->fw_version, 0, 32);
>>> +    }
>>> +
>>> +complete:
>>> +    complete(&adapter->fw_done);
>>> +}
>>> +
>>>   static void handle_query_ip_offload_rsp(struct ibmvnic_adapter *adapter)
>>>   {
>>>       struct device *dev = &adapter->vdev->dev;
>>> @@ -3807,6 +3948,12 @@ static void ibmvnic_handle_crq(union ibmvnic_crq *crq,
>>>           netdev_dbg(netdev, "Got Collect firmware trace Response\n");
>>>           complete(&adapter->fw_done);
>>>           break;
>>> +    case GET_VPD_SIZE_RSP:
>>> +        handle_vpd_size_rsp(crq, adapter);
>>> +        break;
>>> +    case GET_VPD_RSP:
>>> +        handle_vpd_rsp(crq, adapter);
>>> +        break;
>>>       default:
>>>           netdev_err(netdev, "Got an invalid cmd type 0x%02x\n",
>>>                  gen_crq->cmd);
>>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
>>> index 4670af8..d3a6959 100644
>>> --- a/drivers/net/ethernet/ibm/ibmvnic.h
>>> +++ b/drivers/net/ethernet/ibm/ibmvnic.h
>>> @@ -558,6 +558,12 @@ struct ibmvnic_multicast_ctrl {
>>>       struct ibmvnic_rc rc;
>>>   } __packed __aligned(8);
>>>
>>> +struct ibmvnic_get_vpd_size {
>>> +    u8 first;
>>> +    u8 cmd;
>>> +    u8 reserved[14];
>>> +} __packed __aligned(8);
>>> +
>>>   struct ibmvnic_get_vpd_size_rsp {
>>>       u8 first;
>>>       u8 cmd;
>>> @@ -575,6 +581,13 @@ struct ibmvnic_get_vpd {
>>>       u8 reserved[4];
>>>   } __packed __aligned(8);
>>>
>>> +struct ibmvnic_get_vpd_rsp {
>>> +    u8 first;
>>> +    u8 cmd;
>>> +    u8 reserved[10];
>>> +    struct ibmvnic_rc rc;
>>> +} __packed __aligned(8);
>>> +
>>>   struct ibmvnic_acl_change_indication {
>>>       u8 first;
>>>       u8 cmd;
>>> @@ -700,10 +713,10 @@ union ibmvnic_crq {
>>>       struct ibmvnic_change_mac_addr change_mac_addr_rsp;
>>>       struct ibmvnic_multicast_ctrl multicast_ctrl;
>>>       struct ibmvnic_multicast_ctrl multicast_ctrl_rsp;
>>> -    struct ibmvnic_generic_crq get_vpd_size;
>>> +    struct ibmvnic_get_vpd_size get_vpd_size;
>>>       struct ibmvnic_get_vpd_size_rsp get_vpd_size_rsp;
>>>       struct ibmvnic_get_vpd get_vpd;
>>> -    struct ibmvnic_generic_crq get_vpd_rsp;
>>> +    struct ibmvnic_get_vpd_rsp get_vpd_rsp;
>>>       struct ibmvnic_acl_change_indication acl_change_indication;
>>>       struct ibmvnic_acl_query acl_query;
>>>       struct ibmvnic_generic_crq acl_query_rsp;
>>> @@ -937,6 +950,12 @@ struct ibmvnic_error_buff {
>>>       __be32 error_id;
>>>   };
>>>
>>> +struct ibmvnic_vpd {
>>> +    unsigned char *buff;
>>> +    dma_addr_t dma_addr;
>>> +    u64 len;
>>> +};
>>> +
>>>   enum vnic_state {VNIC_PROBING = 1,
>>>            VNIC_PROBED,
>>>            VNIC_OPENING,
>>> @@ -978,6 +997,10 @@ struct ibmvnic_adapter {
>>>       dma_addr_t ip_offload_ctrl_tok;
>>>       u32 msg_enable;
>>>
>>> +    /* Vital Product Data (VPD) */
>>> +    struct ibmvnic_vpd *vpd;
>>> +    char fw_version[32];
>>> +
>>>       /* Statistics */
>>>       struct ibmvnic_statistics stats;
>>>       dma_addr_t stats_token;
>>>
> 

Re: [PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver

[Desnes Augusto Nunes do Rosário]

On 11/10/2017 12:54 PM, Nathan Fontenot wrote:
> On 11/10/2017 08:41 AM, Desnes Augusto Nunes do Rosário wrote:
>>
>>
>> On 11/09/2017 06:31 PM, Nathan Fontenot wrote:
>>> On 11/09/2017 01:00 PM, Desnes Augusto Nunes do Rosario wrote:
>>>> This patch implements and enables VDP support for the ibmvnic driver.
>>>> Moreover, it includes the implementation of suitable structs, signal
>>>>    transmission/handling and functions which allows the retrival of firmware
>>>>    information from the ibmvnic card through the ethtool command.
>>>>
>>>> Signed-off-by: Desnes A. Nunes do Rosario <desnesn@linux.vnet.ibm.com>
>>>> Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
>>>> ---
>>>>    drivers/net/ethernet/ibm/ibmvnic.c | 149 ++++++++++++++++++++++++++++++++++++-
>>>>    drivers/net/ethernet/ibm/ibmvnic.h |  27 ++++++-
>>>>    2 files changed, 173 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
>>>> index d0cff28..693b502 100644
>>>> --- a/drivers/net/ethernet/ibm/ibmvnic.c
>>>> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>>>> @@ -573,6 +573,15 @@ static int reset_tx_pools(struct ibmvnic_adapter *adapter)
>>>>        return 0;
>>>>    }
>>>>
>>>> +static void release_vpd_data(struct ibmvnic_adapter *adapter)
>>>> +{
>>>> +    if (!adapter->vpd)
>>>> +        return;
>>>> +
>>>> +    kfree(adapter->vpd->buff);
>>>> +    kfree(adapter->vpd);
>>>> +}
>>>> +
>>>>    static void release_tx_pools(struct ibmvnic_adapter *adapter)
>>>>    {
>>>>        struct ibmvnic_tx_pool *tx_pool;
>>>> @@ -753,6 +762,8 @@ static void release_resources(struct ibmvnic_adapter *adapter)
>>>>    {
>>>>        int i;
>>>>
>>>> +    release_vpd_data(adapter);
>>>> +
>>>>        release_tx_pools(adapter);
>>>>        release_rx_pools(adapter);
>>>>
>>>> @@ -833,6 +844,53 @@ static int set_real_num_queues(struct net_device *netdev)
>>>>        return rc;
>>>>    }
>>>>
>>>> +static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
>>>> +{
>>>> +    struct device *dev = &adapter->vdev->dev;
>>>> +    union ibmvnic_crq crq;
>>>> +    dma_addr_t dma_addr;
>>>> +    int len;
>>>> +
>>>> +    if (adapter->vpd->buff)
>>>> +        len = adapter->vpd->len;
>>>> +
>>>> +    reinit_completion(&adapter->fw_done);
>>>> +    crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
>>>> +    crq.get_vpd_size.cmd = GET_VPD_SIZE;
>>>> +    ibmvnic_send_crq(adapter, &crq);
>>>> +    wait_for_completion(&adapter->fw_done);
>>>> +
>>>
>>> Shouldn't there be a check for the return code when getting the
>>> vpd size?
>>
>> Hello Nathan,
>>
>> This check is already being performed on the handle_vpd_size_rsp() function down below.
>>
>> In short, a GET_VPD_SIZE signal is sent here through a ibmvnic_crq union in ibmvnic_send_crq(), whereas handle_query_ip_offload_rsp() receives from the VNIC adapter a GET_VPD_SIZE_RSP containing a ibmvnic_crq union with the vpd size information and the rc.code. If successful, a &adapter->fw_done is sent and this part of the code continues; however if not, a dev_error() is thrown. Same logic applies to GET_VPD/GET_VPD_RSP.
>>
> 
> Yes, I did see that code. You do a complet of the completion variable for both success and failure,
> this then lets this routine continue irregardless of the results of the get vpd size request. The
> call to dev_err will print the error message but does not prevent use from bailing if the
> get vpd size fails. Perhaps setting vpd->len to -1 to indicate the get vpd call failed which could
> then be checked by the requester.
> 
> -Nathan
> 
>  >> What I am adding on the next version of the patch is a check if 
adapter->vpd->len is different than 0 before allocating 
adapter->vpd->buff, since that in a case of a failure, adapter->vpd->len 
will be 0.

I do concur with your observation that the break is necessary.

If the reception of vpd failed, adapter->vpd->len will be still zeroed 
out since it was created with kzalloc in init_resources().

Thus, do you agree if in the next version I send the following code?

=======
   +       reinit_completion(&adapter->fw_done);
   +       crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
   +       crq.get_vpd_size.cmd = GET_VPD_SIZE;
   +       ibmvnic_send_crq(adapter, &crq);
   +       wait_for_completion(&adapter->fw_done);
   +
->+       if(!adapter->vpd->len)
->+               return -ENODATA;
   +
   +       if (!adapter->vpd->buff)
   +               adapter->vpd->buff = kzalloc(adapter->vpd->len, 
GFP_KERNEL);
   +       else if (adapter->vpd->len != len)
   +               adapter->vpd->buff =
   +                       krealloc(adapter->vpd->buff,
   +                                adapter->vpd->len, GFP_KERNEL);
=======

>>
>> Best Regards,
>>
>>>
>>>
>>>> +    if (!adapter->vpd->buff)
>>>> +        adapter->vpd->buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
>>>> +    else if (adapter->vpd->len != len)
>>>> +        adapter->vpd->buff =
>>>> +            krealloc(adapter->vpd->buff,
>>>> +                 adapter->vpd->len, GFP_KERNEL);
>>>> +
>>>> +    if (!adapter->vpd->buff) {
>>>> +        dev_err(dev, "Could allocate VPD buffer\n");
>>>> +        return -ENOMEM;
>>>> +    }
>>>> +
>>>> +    adapter->vpd->dma_addr =
>>>> +        dma_map_single(dev, adapter->vpd->buff, adapter->vpd->len,
>>>> +                   DMA_FROM_DEVICE);
>>>> +    if (dma_mapping_error(dev, dma_addr)) {
>>>> +        dev_err(dev, "Could not map VPD buffer\n");
>>>> +        return -ENOMEM;
>>>> +    }
>>>> +
>>>> +    reinit_completion(&adapter->fw_done);
>>>> +    crq.get_vpd.first = IBMVNIC_CRQ_CMD;
>>>> +    crq.get_vpd.cmd = GET_VPD;
>>>> +    crq.get_vpd.ioba = cpu_to_be32(adapter->vpd->dma_addr);
>>>> +    crq.get_vpd.len = cpu_to_be32((u32)adapter->vpd->len);
>>>> +    ibmvnic_send_crq(adapter, &crq);
>>>> +    wait_for_completion(&adapter->fw_done);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>    static int init_resources(struct ibmvnic_adapter *adapter)
>>>>    {
>>>>        struct net_device *netdev = adapter->netdev;
>>>> @@ -850,6 +908,10 @@ static int init_resources(struct ibmvnic_adapter *adapter)
>>>>        if (rc)
>>>>            return rc;
>>>>
>>>> +    adapter->vpd = kzalloc(sizeof(*adapter->vpd), GFP_KERNEL);
>>>> +    if (!adapter->vpd)
>>>> +        return -ENOMEM;
>>>> +
>>>>        adapter->map_id = 1;
>>>>        adapter->napi = kcalloc(adapter->req_rx_queues,
>>>>                    sizeof(struct napi_struct), GFP_KERNEL);
>>>> @@ -950,6 +1012,10 @@ static int ibmvnic_open(struct net_device *netdev)
>>>>
>>>>        rc = __ibmvnic_open(netdev);
>>>>        netif_carrier_on(netdev);
>>>> +
>>>> +    /* Vital Product Data (VPD) */
>>>> +    ibmvnic_get_vpd(adapter);
>>>> +
>>>>        mutex_unlock(&adapter->reset_lock);
>>>>
>>>>        return rc;
>>>> @@ -1878,11 +1944,15 @@ static int ibmvnic_get_link_ksettings(struct net_device *netdev,
>>>>        return 0;
>>>>    }
>>>>
>>>> -static void ibmvnic_get_drvinfo(struct net_device *dev,
>>>> +static void ibmvnic_get_drvinfo(struct net_device *netdev,
>>>>                    struct ethtool_drvinfo *info)
>>>>    {
>>>> +    struct ibmvnic_adapter *adapter = netdev_priv(netdev);
>>>> +
>>>>        strlcpy(info->driver, ibmvnic_driver_name, sizeof(info->driver));
>>>>        strlcpy(info->version, IBMVNIC_DRIVER_VERSION, sizeof(info->version));
>>>> +    strlcpy(info->fw_version, adapter->fw_version,
>>>> +        sizeof(info->fw_version));
>>>>    }
>>>>
>>>>    static u32 ibmvnic_get_msglevel(struct net_device *netdev)
>>>> @@ -3076,6 +3146,77 @@ static void send_cap_queries(struct ibmvnic_adapter *adapter)
>>>>        ibmvnic_send_crq(adapter, &crq);
>>>>    }
>>>>
>>>> +static void handle_vpd_size_rsp(union ibmvnic_crq *crq,
>>>> +                struct ibmvnic_adapter *adapter)
>>>> +{
>>>> +    struct device *dev = &adapter->vdev->dev;
>>>> +
>>>> +    if (crq->get_vpd_size_rsp.rc.code) {
>>>> +        dev_err(dev, "Error retrieving VPD size, rc=%x\n",
>>>> +            crq->get_vpd_size_rsp.rc.code);
>>>> +        complete(&adapter->fw_done);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    adapter->vpd->len = be64_to_cpu(crq->get_vpd_size_rsp.len);
>>>> +    complete(&adapter->fw_done);
>>>> +}
>>>> +
>>>> +static void handle_vpd_rsp(union ibmvnic_crq *crq,
>>>> +               struct ibmvnic_adapter *adapter)
>>>> +{
>>>> +    struct device *dev = &adapter->vdev->dev;
>>>> +    unsigned char *substr = NULL, *ptr = NULL;
>>>> +    u8 fw_level_len = 0;
>>>> +
>>>> +    dma_unmap_single(dev, adapter->vpd->dma_addr, adapter->vpd->len,
>>>> +             DMA_FROM_DEVICE);
>>>> +
>>>> +    if (crq->get_vpd_rsp.rc.code) {
>>>> +        dev_err(dev, "Error retrieving VPD from device, rc=%x\n",
>>>> +            crq->get_vpd_rsp.rc.code);
>>>> +        memset(adapter->fw_version, 0, 32);
>>>> +        goto complete;
>>>> +    }
>>>> +
>>>> +    /* get the position of the firmware version info
>>>> +     * located after the ASCII 'RM' substring in the buffer
>>>> +     */
>>>> +    substr = strnstr(adapter->vpd->buff, "RM", adapter->vpd->len);
>>>> +    if (!substr) {
>>>> +        dev_info(dev, "No FW level provided by VPD\n");
>>>> +        memset(adapter->fw_version, 0, 32);
>>>> +        goto complete;
>>>> +    }
>>>> +
>>>> +    /* get length of firmware level ASCII substring */
>>>> +    if ((substr + 2) < (adapter->vpd->buff + adapter->vpd->len)) {
>>>> +        fw_level_len = *(substr + 2);
>>>> +    } else {
>>>> +        dev_info(dev, "Length of FW substr extrapolated VDP buff\n");
>>>> +        memset(adapter->fw_version, 0, 32);
>>>> +        goto complete;
>>>> +    }
>>>> +
>>>> +    /* copy firmware version string from vpd into adapter */
>>>> +    if ((substr + 3 + fw_level_len) <
>>>> +        (adapter->vpd->buff + adapter->vpd->len)) {
>>>> +        ptr = strncpy((char *)adapter->fw_version,
>>>> +                  substr + 3, fw_level_len);
>>>> +
>>>> +        if (!ptr) {
>>>> +            dev_err(dev, "Failed to isolate FW level string\n");
>>>> +            memset(adapter->fw_version, 0, 32);
>>>> +        }
>>>> +    } else {
>>>> +        dev_info(dev, "FW substr extrapolated VPD buff\n");
>>>> +        memset(adapter->fw_version, 0, 32);
>>>> +    }
>>>> +
>>>> +complete:
>>>> +    complete(&adapter->fw_done);
>>>> +}
>>>> +
>>>>    static void handle_query_ip_offload_rsp(struct ibmvnic_adapter *adapter)
>>>>    {
>>>>        struct device *dev = &adapter->vdev->dev;
>>>> @@ -3807,6 +3948,12 @@ static void ibmvnic_handle_crq(union ibmvnic_crq *crq,
>>>>            netdev_dbg(netdev, "Got Collect firmware trace Response\n");
>>>>            complete(&adapter->fw_done);
>>>>            break;
>>>> +    case GET_VPD_SIZE_RSP:
>>>> +        handle_vpd_size_rsp(crq, adapter);
>>>> +        break;
>>>> +    case GET_VPD_RSP:
>>>> +        handle_vpd_rsp(crq, adapter);
>>>> +        break;
>>>>        default:
>>>>            netdev_err(netdev, "Got an invalid cmd type 0x%02x\n",
>>>>                   gen_crq->cmd);
>>>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
>>>> index 4670af8..d3a6959 100644
>>>> --- a/drivers/net/ethernet/ibm/ibmvnic.h
>>>> +++ b/drivers/net/ethernet/ibm/ibmvnic.h
>>>> @@ -558,6 +558,12 @@ struct ibmvnic_multicast_ctrl {
>>>>        struct ibmvnic_rc rc;
>>>>    } __packed __aligned(8);
>>>>
>>>> +struct ibmvnic_get_vpd_size {
>>>> +    u8 first;
>>>> +    u8 cmd;
>>>> +    u8 reserved[14];
>>>> +} __packed __aligned(8);
>>>> +
>>>>    struct ibmvnic_get_vpd_size_rsp {
>>>>        u8 first;
>>>>        u8 cmd;
>>>> @@ -575,6 +581,13 @@ struct ibmvnic_get_vpd {
>>>>        u8 reserved[4];
>>>>    } __packed __aligned(8);
>>>>
>>>> +struct ibmvnic_get_vpd_rsp {
>>>> +    u8 first;
>>>> +    u8 cmd;
>>>> +    u8 reserved[10];
>>>> +    struct ibmvnic_rc rc;
>>>> +} __packed __aligned(8);
>>>> +
>>>>    struct ibmvnic_acl_change_indication {
>>>>        u8 first;
>>>>        u8 cmd;
>>>> @@ -700,10 +713,10 @@ union ibmvnic_crq {
>>>>        struct ibmvnic_change_mac_addr change_mac_addr_rsp;
>>>>        struct ibmvnic_multicast_ctrl multicast_ctrl;
>>>>        struct ibmvnic_multicast_ctrl multicast_ctrl_rsp;
>>>> -    struct ibmvnic_generic_crq get_vpd_size;
>>>> +    struct ibmvnic_get_vpd_size get_vpd_size;
>>>>        struct ibmvnic_get_vpd_size_rsp get_vpd_size_rsp;
>>>>        struct ibmvnic_get_vpd get_vpd;
>>>> -    struct ibmvnic_generic_crq get_vpd_rsp;
>>>> +    struct ibmvnic_get_vpd_rsp get_vpd_rsp;
>>>>        struct ibmvnic_acl_change_indication acl_change_indication;
>>>>        struct ibmvnic_acl_query acl_query;
>>>>        struct ibmvnic_generic_crq acl_query_rsp;
>>>> @@ -937,6 +950,12 @@ struct ibmvnic_error_buff {
>>>>        __be32 error_id;
>>>>    };
>>>>
>>>> +struct ibmvnic_vpd {
>>>> +    unsigned char *buff;
>>>> +    dma_addr_t dma_addr;
>>>> +    u64 len;
>>>> +};
>>>> +
>>>>    enum vnic_state {VNIC_PROBING = 1,
>>>>             VNIC_PROBED,
>>>>             VNIC_OPENING,
>>>> @@ -978,6 +997,10 @@ struct ibmvnic_adapter {
>>>>        dma_addr_t ip_offload_ctrl_tok;
>>>>        u32 msg_enable;
>>>>
>>>> +    /* Vital Product Data (VPD) */
>>>> +    struct ibmvnic_vpd *vpd;
>>>> +    char fw_version[32];
>>>> +
>>>>        /* Statistics */
>>>>        struct ibmvnic_statistics stats;
>>>>        dma_addr_t stats_token;
>>>>
>>

-- 
Desnes Augusto Nunes do Rosário
------------------------------------------

Linux Developer - IBM / Brazil
M.Sc. in Electrical and Computer Engineering - UFRN

(11) 9595-30-900
desnesn@br.ibm.com

Re: [PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver

[Nathan Fontenot]

On 11/10/2017 01:13 PM, Desnes Augusto Nunes do Rosário wrote:
> 
> 
> On 11/10/2017 12:54 PM, Nathan Fontenot wrote:
>> On 11/10/2017 08:41 AM, Desnes Augusto Nunes do Rosário wrote:
>>>
>>>
>>> On 11/09/2017 06:31 PM, Nathan Fontenot wrote:
>>>> On 11/09/2017 01:00 PM, Desnes Augusto Nunes do Rosario wrote:
>>>>> This patch implements and enables VDP support for the ibmvnic driver.
>>>>> Moreover, it includes the implementation of suitable structs, signal
>>>>>    transmission/handling and functions which allows the retrival of firmware
>>>>>    information from the ibmvnic card through the ethtool command.
>>>>>
>>>>> Signed-off-by: Desnes A. Nunes do Rosario <desnesn@linux.vnet.ibm.com>
>>>>> Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
>>>>> ---
>>>>>    drivers/net/ethernet/ibm/ibmvnic.c | 149 ++++++++++++++++++++++++++++++++++++-
>>>>>    drivers/net/ethernet/ibm/ibmvnic.h |  27 ++++++-
>>>>>    2 files changed, 173 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
>>>>> index d0cff28..693b502 100644
>>>>> --- a/drivers/net/ethernet/ibm/ibmvnic.c
>>>>> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>>>>> @@ -573,6 +573,15 @@ static int reset_tx_pools(struct ibmvnic_adapter *adapter)
>>>>>        return 0;
>>>>>    }
>>>>>
>>>>> +static void release_vpd_data(struct ibmvnic_adapter *adapter)
>>>>> +{
>>>>> +    if (!adapter->vpd)
>>>>> +        return;
>>>>> +
>>>>> +    kfree(adapter->vpd->buff);
>>>>> +    kfree(adapter->vpd);
>>>>> +}
>>>>> +
>>>>>    static void release_tx_pools(struct ibmvnic_adapter *adapter)
>>>>>    {
>>>>>        struct ibmvnic_tx_pool *tx_pool;
>>>>> @@ -753,6 +762,8 @@ static void release_resources(struct ibmvnic_adapter *adapter)
>>>>>    {
>>>>>        int i;
>>>>>
>>>>> +    release_vpd_data(adapter);
>>>>> +
>>>>>        release_tx_pools(adapter);
>>>>>        release_rx_pools(adapter);
>>>>>
>>>>> @@ -833,6 +844,53 @@ static int set_real_num_queues(struct net_device *netdev)
>>>>>        return rc;
>>>>>    }
>>>>>
>>>>> +static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
>>>>> +{
>>>>> +    struct device *dev = &adapter->vdev->dev;
>>>>> +    union ibmvnic_crq crq;
>>>>> +    dma_addr_t dma_addr;
>>>>> +    int len;
>>>>> +
>>>>> +    if (adapter->vpd->buff)
>>>>> +        len = adapter->vpd->len;
>>>>> +
>>>>> +    reinit_completion(&adapter->fw_done);
>>>>> +    crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
>>>>> +    crq.get_vpd_size.cmd = GET_VPD_SIZE;
>>>>> +    ibmvnic_send_crq(adapter, &crq);
>>>>> +    wait_for_completion(&adapter->fw_done);
>>>>> +
>>>>
>>>> Shouldn't there be a check for the return code when getting the
>>>> vpd size?
>>>
>>> Hello Nathan,
>>>
>>> This check is already being performed on the handle_vpd_size_rsp() function down below.
>>>
>>> In short, a GET_VPD_SIZE signal is sent here through a ibmvnic_crq union in ibmvnic_send_crq(), whereas handle_query_ip_offload_rsp() receives from the VNIC adapter a GET_VPD_SIZE_RSP containing a ibmvnic_crq union with the vpd size information and the rc.code. If successful, a &adapter->fw_done is sent and this part of the code continues; however if not, a dev_error() is thrown. Same logic applies to GET_VPD/GET_VPD_RSP.
>>>
>>
>> Yes, I did see that code. You do a complet of the completion variable for both success and failure,
>> this then lets this routine continue irregardless of the results of the get vpd size request. The
>> call to dev_err will print the error message but does not prevent use from bailing if the
>> get vpd size fails. Perhaps setting vpd->len to -1 to indicate the get vpd call failed which could
>> then be checked by the requester.
>>
>> -Nathan
>>
>>  >> What I am adding on the next version of the patch is a check if 
> adapter->vpd->len is different than 0 before allocating adapter->vpd->buff, since that in a case of a failure, adapter->vpd->len will be 0.
> 
> I do concur with your observation that the break is necessary.
> 
> If the reception of vpd failed, adapter->vpd->len will be still zeroed out since it was created with kzalloc in init_resources().
> 
> Thus, do you agree if in the next version I send the following code?

Yes, this would be good.

I think you should also add an explicit setting of len to 0 before the call too.
Looking at the code before the get cpd size call, if a vpd buffer already exists
then the len will be non-zero.

-Nathan

> 
> =======
>   +       reinit_completion(&adapter->fw_done);
>   +       crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
>   +       crq.get_vpd_size.cmd = GET_VPD_SIZE;
>   +       ibmvnic_send_crq(adapter, &crq);
>   +       wait_for_completion(&adapter->fw_done);
>   +
> ->+       if(!adapter->vpd->len)
> ->+               return -ENODATA;
>   +
>   +       if (!adapter->vpd->buff)
>   +               adapter->vpd->buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
>   +       else if (adapter->vpd->len != len)
>   +               adapter->vpd->buff =
>   +                       krealloc(adapter->vpd->buff,
>   +                                adapter->vpd->len, GFP_KERNEL);
> =======
> 
>>>
>>> Best Regards,
>>>
>>>>
>>>>
>>>>> +    if (!adapter->vpd->buff)
>>>>> +        adapter->vpd->buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
>>>>> +    else if (adapter->vpd->len != len)
>>>>> +        adapter->vpd->buff =
>>>>> +            krealloc(adapter->vpd->buff,
>>>>> +                 adapter->vpd->len, GFP_KERNEL);
>>>>> +
>>>>> +    if (!adapter->vpd->buff) {
>>>>> +        dev_err(dev, "Could allocate VPD buffer\n");
>>>>> +        return -ENOMEM;
>>>>> +    }
>>>>> +
>>>>> +    adapter->vpd->dma_addr =
>>>>> +        dma_map_single(dev, adapter->vpd->buff, adapter->vpd->len,
>>>>> +                   DMA_FROM_DEVICE);
>>>>> +    if (dma_mapping_error(dev, dma_addr)) {
>>>>> +        dev_err(dev, "Could not map VPD buffer\n");
>>>>> +        return -ENOMEM;
>>>>> +    }
>>>>> +
>>>>> +    reinit_completion(&adapter->fw_done);
>>>>> +    crq.get_vpd.first = IBMVNIC_CRQ_CMD;
>>>>> +    crq.get_vpd.cmd = GET_VPD;
>>>>> +    crq.get_vpd.ioba = cpu_to_be32(adapter->vpd->dma_addr);
>>>>> +    crq.get_vpd.len = cpu_to_be32((u32)adapter->vpd->len);
>>>>> +    ibmvnic_send_crq(adapter, &crq);
>>>>> +    wait_for_completion(&adapter->fw_done);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>    static int init_resources(struct ibmvnic_adapter *adapter)
>>>>>    {
>>>>>        struct net_device *netdev = adapter->netdev;
>>>>> @@ -850,6 +908,10 @@ static int init_resources(struct ibmvnic_adapter *adapter)
>>>>>        if (rc)
>>>>>            return rc;
>>>>>
>>>>> +    adapter->vpd = kzalloc(sizeof(*adapter->vpd), GFP_KERNEL);
>>>>> +    if (!adapter->vpd)
>>>>> +        return -ENOMEM;
>>>>> +
>>>>>        adapter->map_id = 1;
>>>>>        adapter->napi = kcalloc(adapter->req_rx_queues,
>>>>>                    sizeof(struct napi_struct), GFP_KERNEL);
>>>>> @@ -950,6 +1012,10 @@ static int ibmvnic_open(struct net_device *netdev)
>>>>>
>>>>>        rc = __ibmvnic_open(netdev);
>>>>>        netif_carrier_on(netdev);
>>>>> +
>>>>> +    /* Vital Product Data (VPD) */
>>>>> +    ibmvnic_get_vpd(adapter);
>>>>> +
>>>>>        mutex_unlock(&adapter->reset_lock);
>>>>>
>>>>>        return rc;
>>>>> @@ -1878,11 +1944,15 @@ static int ibmvnic_get_link_ksettings(struct net_device *netdev,
>>>>>        return 0;
>>>>>    }
>>>>>
>>>>> -static void ibmvnic_get_drvinfo(struct net_device *dev,
>>>>> +static void ibmvnic_get_drvinfo(struct net_device *netdev,
>>>>>                    struct ethtool_drvinfo *info)
>>>>>    {
>>>>> +    struct ibmvnic_adapter *adapter = netdev_priv(netdev);
>>>>> +
>>>>>        strlcpy(info->driver, ibmvnic_driver_name, sizeof(info->driver));
>>>>>        strlcpy(info->version, IBMVNIC_DRIVER_VERSION, sizeof(info->version));
>>>>> +    strlcpy(info->fw_version, adapter->fw_version,
>>>>> +        sizeof(info->fw_version));
>>>>>    }
>>>>>
>>>>>    static u32 ibmvnic_get_msglevel(struct net_device *netdev)
>>>>> @@ -3076,6 +3146,77 @@ static void send_cap_queries(struct ibmvnic_adapter *adapter)
>>>>>        ibmvnic_send_crq(adapter, &crq);
>>>>>    }
>>>>>
>>>>> +static void handle_vpd_size_rsp(union ibmvnic_crq *crq,
>>>>> +                struct ibmvnic_adapter *adapter)
>>>>> +{
>>>>> +    struct device *dev = &adapter->vdev->dev;
>>>>> +
>>>>> +    if (crq->get_vpd_size_rsp.rc.code) {
>>>>> +        dev_err(dev, "Error retrieving VPD size, rc=%x\n",
>>>>> +            crq->get_vpd_size_rsp.rc.code);
>>>>> +        complete(&adapter->fw_done);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    adapter->vpd->len = be64_to_cpu(crq->get_vpd_size_rsp.len);
>>>>> +    complete(&adapter->fw_done);
>>>>> +}
>>>>> +
>>>>> +static void handle_vpd_rsp(union ibmvnic_crq *crq,
>>>>> +               struct ibmvnic_adapter *adapter)
>>>>> +{
>>>>> +    struct device *dev = &adapter->vdev->dev;
>>>>> +    unsigned char *substr = NULL, *ptr = NULL;
>>>>> +    u8 fw_level_len = 0;
>>>>> +
>>>>> +    dma_unmap_single(dev, adapter->vpd->dma_addr, adapter->vpd->len,
>>>>> +             DMA_FROM_DEVICE);
>>>>> +
>>>>> +    if (crq->get_vpd_rsp.rc.code) {
>>>>> +        dev_err(dev, "Error retrieving VPD from device, rc=%x\n",
>>>>> +            crq->get_vpd_rsp.rc.code);
>>>>> +        memset(adapter->fw_version, 0, 32);
>>>>> +        goto complete;
>>>>> +    }
>>>>> +
>>>>> +    /* get the position of the firmware version info
>>>>> +     * located after the ASCII 'RM' substring in the buffer
>>>>> +     */
>>>>> +    substr = strnstr(adapter->vpd->buff, "RM", adapter->vpd->len);
>>>>> +    if (!substr) {
>>>>> +        dev_info(dev, "No FW level provided by VPD\n");
>>>>> +        memset(adapter->fw_version, 0, 32);
>>>>> +        goto complete;
>>>>> +    }
>>>>> +
>>>>> +    /* get length of firmware level ASCII substring */
>>>>> +    if ((substr + 2) < (adapter->vpd->buff + adapter->vpd->len)) {
>>>>> +        fw_level_len = *(substr + 2);
>>>>> +    } else {
>>>>> +        dev_info(dev, "Length of FW substr extrapolated VDP buff\n");
>>>>> +        memset(adapter->fw_version, 0, 32);
>>>>> +        goto complete;
>>>>> +    }
>>>>> +
>>>>> +    /* copy firmware version string from vpd into adapter */
>>>>> +    if ((substr + 3 + fw_level_len) <
>>>>> +        (adapter->vpd->buff + adapter->vpd->len)) {
>>>>> +        ptr = strncpy((char *)adapter->fw_version,
>>>>> +                  substr + 3, fw_level_len);
>>>>> +
>>>>> +        if (!ptr) {
>>>>> +            dev_err(dev, "Failed to isolate FW level string\n");
>>>>> +            memset(adapter->fw_version, 0, 32);
>>>>> +        }
>>>>> +    } else {
>>>>> +        dev_info(dev, "FW substr extrapolated VPD buff\n");
>>>>> +        memset(adapter->fw_version, 0, 32);
>>>>> +    }
>>>>> +
>>>>> +complete:
>>>>> +    complete(&adapter->fw_done);
>>>>> +}
>>>>> +
>>>>>    static void handle_query_ip_offload_rsp(struct ibmvnic_adapter *adapter)
>>>>>    {
>>>>>        struct device *dev = &adapter->vdev->dev;
>>>>> @@ -3807,6 +3948,12 @@ static void ibmvnic_handle_crq(union ibmvnic_crq *crq,
>>>>>            netdev_dbg(netdev, "Got Collect firmware trace Response\n");
>>>>>            complete(&adapter->fw_done);
>>>>>            break;
>>>>> +    case GET_VPD_SIZE_RSP:
>>>>> +        handle_vpd_size_rsp(crq, adapter);
>>>>> +        break;
>>>>> +    case GET_VPD_RSP:
>>>>> +        handle_vpd_rsp(crq, adapter);
>>>>> +        break;
>>>>>        default:
>>>>>            netdev_err(netdev, "Got an invalid cmd type 0x%02x\n",
>>>>>                   gen_crq->cmd);
>>>>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
>>>>> index 4670af8..d3a6959 100644
>>>>> --- a/drivers/net/ethernet/ibm/ibmvnic.h
>>>>> +++ b/drivers/net/ethernet/ibm/ibmvnic.h
>>>>> @@ -558,6 +558,12 @@ struct ibmvnic_multicast_ctrl {
>>>>>        struct ibmvnic_rc rc;
>>>>>    } __packed __aligned(8);
>>>>>
>>>>> +struct ibmvnic_get_vpd_size {
>>>>> +    u8 first;
>>>>> +    u8 cmd;
>>>>> +    u8 reserved[14];
>>>>> +} __packed __aligned(8);
>>>>> +
>>>>>    struct ibmvnic_get_vpd_size_rsp {
>>>>>        u8 first;
>>>>>        u8 cmd;
>>>>> @@ -575,6 +581,13 @@ struct ibmvnic_get_vpd {
>>>>>        u8 reserved[4];
>>>>>    } __packed __aligned(8);
>>>>>
>>>>> +struct ibmvnic_get_vpd_rsp {
>>>>> +    u8 first;
>>>>> +    u8 cmd;
>>>>> +    u8 reserved[10];
>>>>> +    struct ibmvnic_rc rc;
>>>>> +} __packed __aligned(8);
>>>>> +
>>>>>    struct ibmvnic_acl_change_indication {
>>>>>        u8 first;
>>>>>        u8 cmd;
>>>>> @@ -700,10 +713,10 @@ union ibmvnic_crq {
>>>>>        struct ibmvnic_change_mac_addr change_mac_addr_rsp;
>>>>>        struct ibmvnic_multicast_ctrl multicast_ctrl;
>>>>>        struct ibmvnic_multicast_ctrl multicast_ctrl_rsp;
>>>>> -    struct ibmvnic_generic_crq get_vpd_size;
>>>>> +    struct ibmvnic_get_vpd_size get_vpd_size;
>>>>>        struct ibmvnic_get_vpd_size_rsp get_vpd_size_rsp;
>>>>>        struct ibmvnic_get_vpd get_vpd;
>>>>> -    struct ibmvnic_generic_crq get_vpd_rsp;
>>>>> +    struct ibmvnic_get_vpd_rsp get_vpd_rsp;
>>>>>        struct ibmvnic_acl_change_indication acl_change_indication;
>>>>>        struct ibmvnic_acl_query acl_query;
>>>>>        struct ibmvnic_generic_crq acl_query_rsp;
>>>>> @@ -937,6 +950,12 @@ struct ibmvnic_error_buff {
>>>>>        __be32 error_id;
>>>>>    };
>>>>>
>>>>> +struct ibmvnic_vpd {
>>>>> +    unsigned char *buff;
>>>>> +    dma_addr_t dma_addr;
>>>>> +    u64 len;
>>>>> +};
>>>>> +
>>>>>    enum vnic_state {VNIC_PROBING = 1,
>>>>>             VNIC_PROBED,
>>>>>             VNIC_OPENING,
>>>>> @@ -978,6 +997,10 @@ struct ibmvnic_adapter {
>>>>>        dma_addr_t ip_offload_ctrl_tok;
>>>>>        u32 msg_enable;
>>>>>
>>>>> +    /* Vital Product Data (VPD) */
>>>>> +    struct ibmvnic_vpd *vpd;
>>>>> +    char fw_version[32];
>>>>> +
>>>>>        /* Statistics */
>>>>>        struct ibmvnic_statistics stats;
>>>>>        dma_addr_t stats_token;
>>>>>
>>>
>