[Intel-wired-lan] [PATH iwl-next v1 1/1] e1000e: Minor flow correction in e1000_shutdown function
Lifshits, Vitaly
vitaly.lifshits at intel.com
Thu Jan 4 19:30:53 UTC 2024
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 at 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
More information about the Intel-wired-lan
mailing list