From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Desnes_Augusto_Nunes_do_Ros=c3=a1rio?= Subject: Re: [PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver Date: Fri, 10 Nov 2017 12:41:25 -0200 Message-ID: References: <20171109190012.17981-1-desnesn@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: linuxppc-dev@lists.ozlabs.org, tlfalcon@linux.vnet.ibm.com, jallen@linux.vnet.ibm.com To: Nathan Fontenot , netdev@vger.kernel.org Return-path: Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:51798 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752852AbdKJOlb (ORCPT ); Fri, 10 Nov 2017 09:41:31 -0500 Received: from pps.filterd (m0098416.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id vAAEee9Q178420 for ; Fri, 10 Nov 2017 09:41:30 -0500 Received: from e15.ny.us.ibm.com (e15.ny.us.ibm.com [129.33.205.205]) by mx0b-001b2d01.pphosted.com with ESMTP id 2e5e1y83u1-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 10 Nov 2017 09:41:30 -0500 Received: from localhost by e15.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 10 Nov 2017 09:41:29 -0500 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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. 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