Re: [PATCH, net] ibmvnic: fix firmware version when no firmware level has been provided by the VIOS server

[Tyrel Datwyler]

On 02/02/2018 06:37 AM, Desnes Augusto Nunes do Rosário wrote:
> Hello Tyrel,
> 
> I concur with your observations, but since this patch has already been 
> merged, I'll address them in another patch.

Fair enough. I didn't realize David had already merged it till after I sent my 
review.

-Tyrel

> 
> Thank you for your review,
> 
> On 02/01/2018 07:02 PM, Tyrel Datwyler wrote:
>> On 02/01/2018 10:04 AM, Desnes Augusto Nunes do Rosario wrote:
>>> Older versions of VIOS servers do not send the firmware level in the VPD
>>> buffer for the ibmvnic driver. Thus, not only the current message is mis-
>>> leading but the firmware version in the ethtool will be NULL. Therefore,
>>> this patch fixes the firmware string and its warning.
>>>
>>> Fixes: 4e6759be28e4 ("ibmvnic: Feature implementation of VPD for the 
>>> ibmvnic driver")
>>>
>>> Signed-off-by: Desnes A. Nunes do Rosario 
>>> ---
>>>   drivers/net/ethernet/ibm/ibmvnic.c | 6 +-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c 
>>> b/drivers/net/ethernet/ibm/ibmvnic.c
>>> index b65f5f3ac034..2b3e71b63a7a 100644
>>> --- a/drivers/net/ethernet/ibm/ibmvnic.c
>>> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>>> @@ -3290,7 +3290,11 @@ static void handle_vpd_rsp(union ibmvnic_crq *crq,
>>>    */
>>>   substr = strnstr(adapter->vpd->buff, "RM", adapter->vpd->len);
>>>   if (!substr) {
>>> -    dev_info(dev, "No FW level provided by VPD\n");
>>> +    dev_info(dev, "Warning - No FW level has been provided in the VPD 
>>> buffer by the VIOS Server\n");
>>> +    ptr = strncpy((char *)adapter->fw_version, "N/A",
>>
>> Is "N/A" the right thing to report? Would something like "Unknown" or 
>> "Unreported" be better?
>>
>>> +  3 * sizeof(char));
>>> +    if (!ptr)
>>> +    dev_err(dev, "Failed to inform that firmware version is 
>>> unavailable to the adapter\n");
>>
>> The sentence structure here seems awkward. I would probably just get rid of 
>> this error and this one later in the function.
>>
>> dev_err(dev, "Failed to isolate FW level string\n");
>>
>> Instead just check and report if adapter->fw_version == NULL in the 
>> complete: label section.
>>
>> -Tyrel
>>
>>>   goto complete;
>>>   }
>>>
>>
> 

Re: [PATCH, net] ibmvnic: fix firmware version when no firmware level has been provided by the VIOS server

[Desnes Augusto Nunes do Rosário]

Hello Tyrel,

I concur with your observations, but since this patch has already been 
merged, I'll address them in another patch.


Thank you for your review,

On 02/01/2018 07:02 PM, Tyrel Datwyler wrote:

On 02/01/2018 10:04 AM, Desnes Augusto Nunes do Rosario wrote:

Older versions of VIOS servers do not send the firmware level in the VPD
buffer for the ibmvnic driver. Thus, not only the current message is mis-
leading but the firmware version in the ethtool will be NULL. Therefore,
this patch fixes the firmware string and its warning.

Fixes: 4e6759be28e4 ("ibmvnic: Feature implementation of VPD for the ibmvnic 
driver")

Signed-off-by: Desnes A. Nunes do Rosario 
---
  drivers/net/ethernet/ibm/ibmvnic.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c 
b/drivers/net/ethernet/ibm/ibmvnic.c
index b65f5f3ac034..2b3e71b63a7a 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -3290,7 +3290,11 @@ static void handle_vpd_rsp(union ibmvnic_crq *crq,
 */
substr = strnstr(adapter->vpd->buff, "RM", adapter->vpd->len);
if (!substr) {
-   dev_info(dev, "No FW level provided by VPD\n");
+   dev_info(dev, "Warning - No FW level has been provided in the VPD 
buffer by the VIOS Server\n");
+   ptr = strncpy((char *)adapter->fw_version, "N/A",


Is "N/A" the right thing to report? Would something like "Unknown" or 
"Unreported" be better?


+ 3 * sizeof(char));
+   if (!ptr)
+   dev_err(dev, "Failed to inform that firmware version is 
unavailable to the adapter\n");


The sentence structure here seems awkward. I would probably just get rid of 
this error and this one later in the function.

dev_err(dev, "Failed to isolate FW level string\n");

Instead just check and report if adapter->fw_version == NULL in the complete: 
label section.

-Tyrel


goto complete;
}





--
Desnes Augusto Nunes do Rosário
--

Linux Developer - IBM / Brazil
M.Sc. in Electrical and Computer Engineering - UFRN

Re: [PATCH, net] ibmvnic: fix firmware version when no firmware level has been provided by the VIOS server

[Desnes Augusto Nunes do Rosário]

Hello David,

Thank you for your review and the heads up about protocol.

On 02/01/2018 05:59 PM, David Miller wrote:

From: Desnes Augusto Nunes do Rosario 
Date: Thu,  1 Feb 2018 16:04:30 -0200


Older versions of VIOS servers do not send the firmware level in the VPD
buffer for the ibmvnic driver. Thus, not only the current message is mis-
leading but the firmware version in the ethtool will be NULL. Therefore,
this patch fixes the firmware string and its warning.

Fixes: 4e6759be28e4 ("ibmvnic: Feature implementation of VPD for the ibmvnic 
driver")

Signed-off-by: Desnes A. Nunes do Rosario 


Applied.

Please do not put empty lines between Fixes: and Signed-off-by: and other
tags, all tags are equal and are placed together in an uninterrupted
sequences of consequetive lines.

Thank you.



--
Desnes Augusto Nunes do Rosário
--

Linux Developer - IBM / Brazil
M.Sc. in Electrical and Computer Engineering - UFRN

Re: [PATCH, net] ibmvnic: fix firmware version when no firmware level has been provided by the VIOS server

[Tyrel Datwyler]

On 02/01/2018 10:04 AM, Desnes Augusto Nunes do Rosario wrote:
> Older versions of VIOS servers do not send the firmware level in the VPD
> buffer for the ibmvnic driver. Thus, not only the current message is mis-
> leading but the firmware version in the ethtool will be NULL. Therefore,
> this patch fixes the firmware string and its warning.
> 
> Fixes: 4e6759be28e4 ("ibmvnic: Feature implementation of VPD for the ibmvnic 
> driver")
> 
> Signed-off-by: Desnes A. Nunes do Rosario 
> ---
>  drivers/net/ethernet/ibm/ibmvnic.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c 
> b/drivers/net/ethernet/ibm/ibmvnic.c
> index b65f5f3ac034..2b3e71b63a7a 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -3290,7 +3290,11 @@ static void handle_vpd_rsp(union ibmvnic_crq *crq,
>*/
>   substr = strnstr(adapter->vpd->buff, "RM", adapter->vpd->len);
>   if (!substr) {
> - dev_info(dev, "No FW level provided by VPD\n");
> + dev_info(dev, "Warning - No FW level has been provided in the 
> VPD buffer by the VIOS Server\n");
> + ptr = strncpy((char *)adapter->fw_version, "N/A",

Is "N/A" the right thing to report? Would something like "Unknown" or 
"Unreported" be better?

> +   3 * sizeof(char));
> + if (!ptr)
> + dev_err(dev, "Failed to inform that firmware version is 
> unavailable to the adapter\n");

The sentence structure here seems awkward. I would probably just get rid of 
this error and this one later in the function.

dev_err(dev, "Failed to isolate FW level string\n");

Instead just check and report if adapter->fw_version == NULL in the complete: 
label section.

-Tyrel

>   goto complete;
>   }
> 

Re: [PATCH, net] ibmvnic: fix firmware version when no firmware level has been provided by the VIOS server

[David Miller]

From: Desnes Augusto Nunes do Rosario 
Date: Thu,  1 Feb 2018 16:04:30 -0200

> Older versions of VIOS servers do not send the firmware level in the VPD
> buffer for the ibmvnic driver. Thus, not only the current message is mis-
> leading but the firmware version in the ethtool will be NULL. Therefore,
> this patch fixes the firmware string and its warning.
> 
> Fixes: 4e6759be28e4 ("ibmvnic: Feature implementation of VPD for the ibmvnic 
> driver")
> 
> Signed-off-by: Desnes A. Nunes do Rosario 

Applied.

Please do not put empty lines between Fixes: and Signed-off-by: and other
tags, all tags are equal and are placed together in an uninterrupted
sequences of consequetive lines.

Thank you.

[PATCH, net] ibmvnic: fix firmware version when no firmware level has been provided by the VIOS server

[Desnes Augusto Nunes do Rosario]

Older versions of VIOS servers do not send the firmware level in the VPD
buffer for the ibmvnic driver. Thus, not only the current message is mis-
leading but the firmware version in the ethtool will be NULL. Therefore,
this patch fixes the firmware string and its warning.

Fixes: 4e6759be28e4 ("ibmvnic: Feature implementation of VPD for the ibmvnic 
driver")

Signed-off-by: Desnes A. Nunes do Rosario 
---
 drivers/net/ethernet/ibm/ibmvnic.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c 
b/drivers/net/ethernet/ibm/ibmvnic.c
index b65f5f3ac034..2b3e71b63a7a 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -3290,7 +3290,11 @@ static void handle_vpd_rsp(union ibmvnic_crq *crq,
 */
substr = strnstr(adapter->vpd->buff, "RM", adapter->vpd->len);
if (!substr) {
-   dev_info(dev, "No FW level provided by VPD\n");
+   dev_info(dev, "Warning - No FW level has been provided in the 
VPD buffer by the VIOS Server\n");
+   ptr = strncpy((char *)adapter->fw_version, "N/A",
+ 3 * sizeof(char));
+   if (!ptr)
+   dev_err(dev, "Failed to inform that firmware version is 
unavailable to the adapter\n");
goto complete;
}
 
-- 
2.14.3