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 <desn...@linux.vnet.ibm.com>
Signed-off-by: Thomas Falcon <tlfal...@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
desn...@br.ibm.com

Reply via email to