| Message ID | 20240103085044.283741-1-vitaly.lifshits@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [PATH,iwl-next,v1,1/1] e1000e: Minor flow correction in e1000_shutdown function | expand |
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
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
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
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(-)