Re: [PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver

[Nathan Fontenot]

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 
> 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 = >vdev->dev;
> +    union ibmvnic_crq crq;
> +    dma_addr_t dma_addr;
> +    int len;
> +
> +    if (adapter->vpd->buff)
> +    len = adapter->vpd->len;
> +
> +    reinit_completion(>fw_done);
> +    crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
> +    crq.get_vpd_size.cmd = GET_VPD_SIZE;
> +    ibmvnic_send_crq(adapter, );
> +    wait_for_completion(>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 >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(>fw_done);
>   +   crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
>   +   crq.get_vpd_size.cmd = GET_VPD_SIZE;
>   +   ibmvnic_send_crq(adapter, );
>   +   wait_for_completion(>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)
> 

Re: [PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver

[Desnes Augusto Nunes do Rosário]

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 
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 = >vdev->dev;
+    union ibmvnic_crq crq;
+    dma_addr_t dma_addr;
+    int len;
+
+    if (adapter->vpd->buff)
+    len = adapter->vpd->len;
+
+    reinit_completion(>fw_done);
+    crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
+    crq.get_vpd_size.cmd = GET_VPD_SIZE;
+    ibmvnic_send_crq(adapter, );
+    wait_for_completion(>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 >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(>fw_done);
  +   crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
  +   crq.get_vpd_size.cmd = GET_VPD_SIZE;
  +   ibmvnic_send_crq(adapter, );
  +   wait_for_completion(>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;
+    }
+
+    

Re: [PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver

[Nathan Fontenot]

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 = >vdev->dev;
>>> +    union ibmvnic_crq crq;
>>> +    dma_addr_t dma_addr;
>>> +    int len;
>>> +
>>> +    if (adapter->vpd->buff)
>>> +    len = adapter->vpd->len;
>>> +
>>> +    reinit_completion(>fw_done);
>>> +    crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
>>> +    crq.get_vpd_size.cmd = GET_VPD_SIZE;
>>> +    ibmvnic_send_crq(adapter, );
>>> +    wait_for_completion(>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 >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(>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, );
>>> +    wait_for_completion(>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)
>>>   

Re: [PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver

[Desnes Augusto Nunes do Rosário]

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 = >vdev->dev;
+   union ibmvnic_crq crq;
+   dma_addr_t dma_addr;
+   int len;
+
+   if (adapter->vpd->buff)
+   len = adapter->vpd->len;
+
+   reinit_completion(>fw_done);
+   crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
+   crq.get_vpd_size.cmd = GET_VPD_SIZE;
+   ibmvnic_send_crq(adapter, );
+   wait_for_completion(>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 
>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(>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, );
+   wait_for_completion(>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(>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 

Re: [PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver

[Nathan Fontenot]

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 = >vdev->dev;
> + union ibmvnic_crq crq;
> + dma_addr_t dma_addr;
> + int len;
> +
> + if (adapter->vpd->buff)
> + len = adapter->vpd->len;
> +
> + reinit_completion(>fw_done);
> + crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
> + crq.get_vpd_size.cmd = GET_VPD_SIZE;
> + ibmvnic_send_crq(adapter, );
> + wait_for_completion(>fw_done);
> +

Shouldn't there be a check for the return code when getting the
vpd size?


> + 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(>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, );
> + wait_for_completion(>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(>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, );
>  }
> 
> +static void 

Re: [PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver

[Tushar Dave]

On 11/09/2017 11:00 AM, 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 = >vdev->dev;
+   union ibmvnic_crq crq;
+   dma_addr_t dma_addr;
+   int len;
+
+   if (adapter->vpd->buff)
+   len = adapter->vpd->len;
+
+   reinit_completion(>fw_done);
+   crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
+   crq.get_vpd_size.cmd = GET_VPD_SIZE;
+   ibmvnic_send_crq(adapter, );
+   wait_for_completion(>fw_done);
+
+   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;

In this case, driver should release memory for adapter->vpd->buff.


+   }
+
+   reinit_completion(>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, );
+   wait_for_completion(>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);

May want to check return value from ibmvnic_get_vpd() ?

-Tushar

+
mutex_unlock(>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, );
  }
  
+static void handle_vpd_size_rsp(union ibmvnic_crq *crq,

+