[Intel-wired-lan] [PATH iwl-next v1 1/1] e1000e: Minor flow correction in e1000_shutdown function

[Intel-wired-lan] [PATH iwl-next v1 1/1] e1000e: Minor flow correction in e1000_shutdown function

[Vitaly Lifshits]

Added a missing curly braces to avoid entering to an if statement
where it is not always required in e1000_shutdown function.
This improves code readability and might prevent a non-deterministic
behaviour in the future.

Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index af5d9d97a0d6..cc8c531ec3df 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -6688,14 +6688,14 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime)
 	if (adapter->hw.phy.type == e1000_phy_igp_3) {
 		e1000e_igp3_phy_powerdown_workaround_ich8lan(&adapter->hw);
 	} else if (hw->mac.type >= e1000_pch_lpt) {
-		if (wufc && !(wufc & (E1000_WUFC_EX | E1000_WUFC_MC | E1000_WUFC_BC)))
+		if (wufc && !(wufc & (E1000_WUFC_EX | E1000_WUFC_MC | E1000_WUFC_BC))) {
 			/* ULP does not support wake from unicast, multicast
 			 * or broadcast.
 			 */
 			retval = e1000_enable_ulp_lpt_lp(hw, !runtime);
-
-		if (retval)
-			return retval;
+			if (retval)
+				return retval;
+		}
 	}
 
 	/* Ensure that the appropriate bits are set in LPI_CTRL
-- 
2.34.1

Re: [Intel-wired-lan] [PATH iwl-next v1 1/1] e1000e: Minor flow correction in e1000_shutdown function

[Paul Menzel]

Dear Vitaly,


Thank you for the patch.

In the commit message summary, it’d be great if you used a statement by 
adding a verb in imperative mood. Maybe:

Correct flow in e1000_shutdown()

Am 03.01.24 um 09:50 schrieb Vitaly Lifshits:
> Added a missing curly braces to avoid entering to an if statement

s/Added/Add/
s/entering to/entering/

The curly braces are not missing though.

> where it is not always required in e1000_shutdown function.
> This improves code readability and might prevent a non-deterministic
> behaviour in the future.

Looking at the diff, it’s not clear to me, how `retval` is used/set 
before. Could you please elaborate, what the problem is?

Missing Fixes tag?

> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> ---
>   drivers/net/ethernet/intel/e1000e/netdev.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index af5d9d97a0d6..cc8c531ec3df 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -6688,14 +6688,14 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime)
>   	if (adapter->hw.phy.type == e1000_phy_igp_3) {
>   		e1000e_igp3_phy_powerdown_workaround_ich8lan(&adapter->hw);
>   	} else if (hw->mac.type >= e1000_pch_lpt) {
> -		if (wufc && !(wufc & (E1000_WUFC_EX | E1000_WUFC_MC | E1000_WUFC_BC)))
> +		if (wufc && !(wufc & (E1000_WUFC_EX | E1000_WUFC_MC | E1000_WUFC_BC))) {
>   			/* ULP does not support wake from unicast, multicast
>   			 * or broadcast.
>   			 */
>   			retval = e1000_enable_ulp_lpt_lp(hw, !runtime);
> -
> -		if (retval)
> -			return retval;
> +			if (retval)
> +				return retval;
> +		}
>   	}
>   
>   	/* Ensure that the appropriate bits are set in LPI_CTRL


Kind regards,

Paul

Re: [Intel-wired-lan] [PATH iwl-next v1 1/1] e1000e: Minor flow correction in e1000_shutdown function

[Lifshits, Vitaly]

Dear Paul,

Thank you for your comments.

On 1/3/2024 5:27 PM, Paul Menzel wrote:
> Dear Vitaly,
> 
> 
> Thank you for the patch.
> 
> In the commit message summary, it’d be great if you used a statement by 
> adding a verb in imperative mood. Maybe:
> 
> Correct flow in e1000_shutdown()
> 
> Am 03.01.24 um 09:50 schrieb Vitaly Lifshits:
>> Added a missing curly braces to avoid entering to an if statement
> 
> s/Added/Add/
> s/entering to/entering/

Will take care of it in v2.
> 
> The curly braces are not missing though.
> 
>> where it is not always required in e1000_shutdown function.
>> This improves code readability and might prevent a non-deterministic
>> behaviour in the future.
> 
> Looking at the diff, it’s not clear to me, how `retval` is used/set 
> before. Could you please elaborate, what the problem is?
In e1000_shutdown function, at the beginning retval is initialized to 0. 
Before the if on line 6694
retval is being used to evaluate the output of e1000_init_phy_wakeup
on line 6676.
There if retval is not 0 the driver exits that function, otherwise, it 
still holds 0 as a value.

Before the patch what could have happened is that if the condition on 
line 6694 had not been met, the driver would have entered the next if 
statement on line 6700 even though it was meant to evaluate only the 
output of e1000_enable_ulp_lpt_lp function. Though it is not causing a 
bug since retval at that point is still 0, that evaluation is unnecessary.
Therefore it is not a bug, but, rather an improvement to the code by 
making it more obvious and removing the unnecessary evaluation.
> 
> Missing Fixes tag?

Since it is not a bug, then a Fixes tag is not required.
> 
>> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>> ---
>>   drivers/net/ethernet/intel/e1000e/netdev.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c 
>> b/drivers/net/ethernet/intel/e1000e/netdev.c
>> index af5d9d97a0d6..cc8c531ec3df 100644
>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>> @@ -6688,14 +6688,14 @@ static int __e1000_shutdown(struct pci_dev 
>> *pdev, bool runtime)
>>       if (adapter->hw.phy.type == e1000_phy_igp_3) {
>>           e1000e_igp3_phy_powerdown_workaround_ich8lan(&adapter->hw);
>>       } else if (hw->mac.type >= e1000_pch_lpt) {
>> -        if (wufc && !(wufc & (E1000_WUFC_EX | E1000_WUFC_MC | 
>> E1000_WUFC_BC)))
>> +        if (wufc && !(wufc & (E1000_WUFC_EX | E1000_WUFC_MC | 
>> E1000_WUFC_BC))) {
>>               /* ULP does not support wake from unicast, multicast
>>                * or broadcast.
>>                */
>>               retval = e1000_enable_ulp_lpt_lp(hw, !runtime);
>> -
>> -        if (retval)
>> -            return retval;
>> +            if (retval)
>> +                return retval;
>> +        }
>>       }
>>       /* Ensure that the appropriate bits are set in LPI_CTRL
> 
> 
> Kind regards,
> 
> Paul