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 <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?
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;
>>>>>
>>>
>
