From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3yYNN71nrtzDrKG for ; Sat, 11 Nov 2017 01:54:15 +1100 (AEDT) Received: from pps.filterd (m0098404.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id vAAEs64D070614 for ; Fri, 10 Nov 2017 09:54:13 -0500 Received: from e32.co.us.ibm.com (e32.co.us.ibm.com [32.97.110.150]) by mx0a-001b2d01.pphosted.com with ESMTP id 2e5dqy1p74-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 10 Nov 2017 09:54:12 -0500 Received: from localhost by e32.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 10 Nov 2017 07:54:11 -0700 Subject: Re: [PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver To: =?UTF-8?Q?Desnes_Augusto_Nunes_do_Ros=c3=a1rio?= , netdev@vger.kernel.org Cc: tlfalcon@linux.vnet.ibm.com, linuxppc-dev@lists.ozlabs.org, jallen@linux.vnet.ibm.com References: <20171109190012.17981-1-desnesn@linux.vnet.ibm.com> From: Nathan Fontenot Date: Fri, 10 Nov 2017 08:54:06 -0600 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Message-Id: <38a668a8-fab7-549a-d3d8-e98e5d875ca5@linux.vnet.ibm.com> List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 >>> Signed-off-by: Thomas Falcon >>> --- >>>   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; >>> >