[v3,8/9] net/ionic: minor refactorings and helper variables
diff mbox series

Message ID 20201204201646.51746-9-aboyer@pensando.io
State Changes Requested
Delegated to: Ferruh Yigit
Headers show
Series
  • net/ionic: minor updates and documentation
Related show

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Andrew Boyer Dec. 4, 2020, 8:16 p.m. UTC 
This makes the code clearer and conserves resources.

Signed-off-by: Andrew Boyer <aboyer@pensando.io>
---
 drivers/net/ionic/ionic_ethdev.c |  5 ++---
 drivers/net/ionic/ionic_lif.c    | 15 ++++++++++-----
 drivers/net/ionic/ionic_main.c   | 18 +++++++-----------
 3 files changed, 19 insertions(+), 19 deletions(-)

Comments

Ferruh Yigit Dec. 9, 2020, 1:04 p.m. UTC | #1 
On 12/4/2020 8:16 PM, Andrew Boyer wrote:
> This makes the code clearer and conserves resources.
> 
> Signed-off-by: Andrew Boyer <aboyer@pensando.io>
> ---
>   drivers/net/ionic/ionic_ethdev.c |  5 ++---
>   drivers/net/ionic/ionic_lif.c    | 15 ++++++++++-----
>   drivers/net/ionic/ionic_main.c   | 18 +++++++-----------
>   3 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/ionic/ionic_ethdev.c b/drivers/net/ionic/ionic_ethdev.c
> index ce6ca9671..a1c35ace3 100644
> --- a/drivers/net/ionic/ionic_ethdev.c
> +++ b/drivers/net/ionic/ionic_ethdev.c
> @@ -901,7 +901,7 @@ ionic_dev_start(struct rte_eth_dev *eth_dev)
>   	struct ionic_lif *lif = IONIC_ETH_DEV_TO_LIF(eth_dev);
>   	struct ionic_adapter *adapter = lif->adapter;
>   	struct ionic_dev *idev = &adapter->idev;
> -	uint32_t allowed_speeds;
> +	uint32_t speed, allowed_speeds;
>   	int err;
>   
>   	IONIC_PRINT_CALL();
> @@ -929,8 +929,7 @@ ionic_dev_start(struct rte_eth_dev *eth_dev)
>   	}
>   
>   	if (eth_dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_FIXED) {
> -		uint32_t speed = ionic_parse_link_speeds(dev_conf->link_speeds);
> -
> +		speed = ionic_parse_link_speeds(dev_conf->link_speeds);
>   		if (speed)
>   			ionic_dev_cmd_port_speed(idev, speed);
>   	}

Same comment from previous version, what is the reason to increase the scope of 
the 'speed' variable?
Functionality is same and isn't it better to have reduced scope?
Andrew Boyer Dec. 9, 2020, 2:39 p.m. UTC | #2 
> On Dec 9, 2020, at 8:04 AM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> On 12/4/2020 8:16 PM, Andrew Boyer wrote:
>> This makes the code clearer and conserves resources.
>> Signed-off-by: Andrew Boyer <aboyer@pensando.io>
>> ---
>>  drivers/net/ionic/ionic_ethdev.c |  5 ++---
>>  drivers/net/ionic/ionic_lif.c    | 15 ++++++++++-----
>>  drivers/net/ionic/ionic_main.c   | 18 +++++++-----------
>>  3 files changed, 19 insertions(+), 19 deletions(-)
>> diff --git a/drivers/net/ionic/ionic_ethdev.c b/drivers/net/ionic/ionic_ethdev.c
>> index ce6ca9671..a1c35ace3 100644
>> --- a/drivers/net/ionic/ionic_ethdev.c
>> +++ b/drivers/net/ionic/ionic_ethdev.c
>> @@ -901,7 +901,7 @@ ionic_dev_start(struct rte_eth_dev *eth_dev)
>>  	struct ionic_lif *lif = IONIC_ETH_DEV_TO_LIF(eth_dev);
>>  	struct ionic_adapter *adapter = lif->adapter;
>>  	struct ionic_dev *idev = &adapter->idev;
>> -	uint32_t allowed_speeds;
>> +	uint32_t speed, allowed_speeds;
>>  	int err;
>>    	IONIC_PRINT_CALL();
>> @@ -929,8 +929,7 @@ ionic_dev_start(struct rte_eth_dev *eth_dev)
>>  	}
>>    	if (eth_dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_FIXED) {
>> -		uint32_t speed = ionic_parse_link_speeds(dev_conf->link_speeds);
>> -
>> +		speed = ionic_parse_link_speeds(dev_conf->link_speeds);
>>  		if (speed)
>>  			ionic_dev_cmd_port_speed(idev, speed);
>>  	}
> 
> Same comment from previous version, what is the reason to increase the scope of the 'speed' variable?
> Functionality is same and isn't it better to have reduced scope?

In a future patch I will be redesigning this code block and speed will have function scope.

I have tried to break things up into digestible bits. Is this not acceptable?

-Andrew
Ferruh Yigit Dec. 9, 2020, 3:25 p.m. UTC | #3 
On 12/9/2020 2:39 PM, Andrew Boyer wrote:
> 
> 
>> On Dec 9, 2020, at 8:04 AM, Ferruh Yigit <ferruh.yigit@intel.com 
>> <mailto:ferruh.yigit@intel.com>> wrote:
>>
>> On 12/4/2020 8:16 PM, Andrew Boyer wrote:
>>> This makes the code clearer and conserves resources.
>>> Signed-off-by: Andrew Boyer <aboyer@pensando.io <mailto:aboyer@pensando.io>>
>>> ---
>>>  drivers/net/ionic/ionic_ethdev.c |  5 ++---
>>>  drivers/net/ionic/ionic_lif.c    | 15 ++++++++++-----
>>>  drivers/net/ionic/ionic_main.c   | 18 +++++++-----------
>>>  3 files changed, 19 insertions(+), 19 deletions(-)
>>> diff --git a/drivers/net/ionic/ionic_ethdev.c b/drivers/net/ionic/ionic_ethdev.c
>>> index ce6ca9671..a1c35ace3 100644
>>> --- a/drivers/net/ionic/ionic_ethdev.c
>>> +++ b/drivers/net/ionic/ionic_ethdev.c
>>> @@ -901,7 +901,7 @@ ionic_dev_start(struct rte_eth_dev *eth_dev)
>>> struct ionic_lif *lif = IONIC_ETH_DEV_TO_LIF(eth_dev);
>>> struct ionic_adapter *adapter = lif->adapter;
>>> struct ionic_dev *idev = &adapter->idev;
>>> -uint32_t allowed_speeds;
>>> +uint32_t speed, allowed_speeds;
>>> int err;
>>> IONIC_PRINT_CALL();
>>> @@ -929,8 +929,7 @@ ionic_dev_start(struct rte_eth_dev *eth_dev)
>>> }
>>> if (eth_dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_FIXED) {
>>> -uint32_t speed = ionic_parse_link_speeds(dev_conf->link_speeds);
>>> -
>>> +speed = ionic_parse_link_speeds(dev_conf->link_speeds);
>>> if (speed)
>>> ionic_dev_cmd_port_speed(idev, speed);
>>> }
>>
>> Same comment from previous version, what is the reason to increase the scope 
>> of the 'speed' variable?
>> Functionality is same and isn't it better to have reduced scope?
> 
> In a future patch I will be redesigning this code block and speed will have 
> function scope.
> 
> I have tried to break things up into digestible bits. Is this not acceptable?
> 

On its own this is not a good change, please do the update in the future patch.

Patch
diff mbox series 

diff --git a/drivers/net/ionic/ionic_ethdev.c b/drivers/net/ionic/ionic_ethdev.c
index ce6ca9671..a1c35ace3 100644
--- a/drivers/net/ionic/ionic_ethdev.c
+++ b/drivers/net/ionic/ionic_ethdev.c
@@ -901,7 +901,7 @@  ionic_dev_start(struct rte_eth_dev *eth_dev)
 	struct ionic_lif *lif = IONIC_ETH_DEV_TO_LIF(eth_dev);
 	struct ionic_adapter *adapter = lif->adapter;
 	struct ionic_dev *idev = &adapter->idev;
-	uint32_t allowed_speeds;
+	uint32_t speed, allowed_speeds;
 	int err;
 
 	IONIC_PRINT_CALL();
@@ -929,8 +929,7 @@  ionic_dev_start(struct rte_eth_dev *eth_dev)
 	}
 
 	if (eth_dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_FIXED) {
-		uint32_t speed = ionic_parse_link_speeds(dev_conf->link_speeds);
-
+		speed = ionic_parse_link_speeds(dev_conf->link_speeds);
 		if (speed)
 			ionic_dev_cmd_port_speed(idev, speed);
 	}
diff --git a/drivers/net/ionic/ionic_lif.c b/drivers/net/ionic/ionic_lif.c
index 2e33fb8d9..722a89565 100644
--- a/drivers/net/ionic/ionic_lif.c
+++ b/drivers/net/ionic/ionic_lif.c
@@ -77,11 +77,14 @@  void
 ionic_lif_reset(struct ionic_lif *lif)
 {
 	struct ionic_dev *idev = &lif->adapter->idev;
+	int err;
 
 	IONIC_PRINT_CALL();
 
 	ionic_dev_cmd_lif_reset(idev, lif->index);
-	ionic_dev_cmd_wait_check(idev, IONIC_DEVCMD_TIMEOUT);
+	err = ionic_dev_cmd_wait_check(idev, IONIC_DEVCMD_TIMEOUT);
+	if (err)
+		IONIC_PRINT(WARNING, "Failed to reset lif");
 }
 
 static void
@@ -305,10 +308,11 @@  ionic_dev_add_mac(struct rte_eth_dev *eth_dev,
 }
 
 void
-ionic_dev_remove_mac(struct rte_eth_dev *eth_dev, uint32_t index __rte_unused)
+ionic_dev_remove_mac(struct rte_eth_dev *eth_dev, uint32_t index)
 {
 	struct ionic_lif *lif = IONIC_ETH_DEV_TO_LIF(eth_dev);
 	struct ionic_adapter *adapter = lif->adapter;
+	struct rte_ether_addr *mac_addr;
 
 	IONIC_PRINT_CALL();
 
@@ -319,11 +323,12 @@  ionic_dev_remove_mac(struct rte_eth_dev *eth_dev, uint32_t index __rte_unused)
 		return;
 	}
 
-	if (!rte_is_valid_assigned_ether_addr(&eth_dev->data->mac_addrs[index]))
+	mac_addr = &eth_dev->data->mac_addrs[index];
+
+	if (!rte_is_valid_assigned_ether_addr(mac_addr))
 		return;
 
-	ionic_lif_addr_del(lif, (const uint8_t *)
-		&eth_dev->data->mac_addrs[index]);
+	ionic_lif_addr_del(lif, (const uint8_t *)mac_addr);
 }
 
 int
diff --git a/drivers/net/ionic/ionic_main.c b/drivers/net/ionic/ionic_main.c
index f77bddaa4..92cf0f398 100644
--- a/drivers/net/ionic/ionic_main.c
+++ b/drivers/net/ionic/ionic_main.c
@@ -188,8 +188,7 @@  ionic_adminq_post_wait(struct ionic_lif *lif, struct ionic_admin_ctx *ctx)
 	done = ionic_wait_ctx_for_completion(lif, qcq, ctx,
 		IONIC_DEVCMD_TIMEOUT);
 
-	err = ionic_adminq_check_err(ctx, !done /* timed out */);
-	return err;
+	return ionic_adminq_check_err(ctx, !done /* timed out */);
 }
 
 static int
@@ -241,10 +240,11 @@  ionic_dev_cmd_wait_check(struct ionic_dev *idev, unsigned long max_wait)
 	int err;
 
 	err = ionic_dev_cmd_wait(idev, max_wait);
-	if (err)
-		return err;
 
-	return ionic_dev_cmd_check_error(idev);
+	if (!err)
+		err = ionic_dev_cmd_check_error(idev);
+
+	return err;
 }
 
 int
@@ -299,22 +299,18 @@  int
 ionic_init(struct ionic_adapter *adapter)
 {
 	struct ionic_dev *idev = &adapter->idev;
-	int err;
 
 	ionic_dev_cmd_init(idev);
-	err = ionic_dev_cmd_wait_check(idev, IONIC_DEVCMD_TIMEOUT);
-	return err;
+	return ionic_dev_cmd_wait_check(idev, IONIC_DEVCMD_TIMEOUT);
 }
 
 int
 ionic_reset(struct ionic_adapter *adapter)
 {
 	struct ionic_dev *idev = &adapter->idev;
-	int err;
 
 	ionic_dev_cmd_reset(idev);
-	err = ionic_dev_cmd_wait_check(idev, IONIC_DEVCMD_TIMEOUT);
-	return err;
+	return ionic_dev_cmd_wait_check(idev, IONIC_DEVCMD_TIMEOUT);
 }
 
 int