[PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver
Desnes Augusto Nunes do Rosário
desnesn at linux.vnet.ibm.com
Sat Nov 11 06:13:34 AEDT 2017
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 at linux.vnet.ibm.com>
>>>> Signed-off-by: Thomas Falcon <tlfalcon at 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 at br.ibm.com
More information about the Linuxppc-dev
mailing list