Message ID | 20170809214746.28139-4-jeffrey.t.kirsher@intel.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
On Wed, 2017-08-09 at 14:47 -0700, Jeff Kirsher wrote: > From: Gustavo A R Silva <garsilva@embeddedor.com> > > Check return value from call to e1e_wphy(). This value is being > checked during previous calls to function e1e_wphy() and it seems > a check was missing here. The use of "it seems" here is less than compelling. Perhaps the write of 0x3140 to MII_BMCR takes too long for the return value used. Many other uses of e1e_wphy.*MII_BMCR are also not checked. For instance: the e100e/ethtool uses. > diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c [] > @@ -2437,6 +2437,8 @@ static s32 e1000_hv_phy_workarounds_ich8lan(struct e1000_hw *hw) > if (hw->phy.revision < 2) { > e1000e_phy_sw_reset(hw); > ret_val = e1e_wphy(hw, MII_BMCR, 0x3140); > + if (ret_val) > + return ret_val; > } > } >
Hello everybody, I'm a little confused. Is this patch causing any trouble? On 08/10/2017 12:56 PM, Joe Perches wrote: > On Wed, 2017-08-09 at 14:47 -0700, Jeff Kirsher wrote: >> From: Gustavo A R Silva <garsilva@embeddedor.com> >> >> Check return value from call to e1e_wphy(). This value is being >> checked during previous calls to function e1e_wphy() and it seems >> a check was missing here. > > The use of "it seems" here is less than compelling. > This is one of the first patches I sent. Maybe I should have added a note saying that this patch needed some testing, as I don't have the hardware to test it. > Perhaps the write of 0x3140 to MII_BMCR takes too long for > the return value used. > > Many other uses of e1e_wphy.*MII_BMCR are also not checked. > > For instance: the e100e/ethtool uses. > >> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c > [] >> @@ -2437,6 +2437,8 @@ static s32 e1000_hv_phy_workarounds_ich8lan(struct e1000_hw *hw) >> if (hw->phy.revision < 2) { >> e1000e_phy_sw_reset(hw); >> ret_val = e1e_wphy(hw, MII_BMCR, 0x3140); >> + if (ret_val) >> + return ret_val; >> } >> } >> Thanks
On Thu, 2017-08-10 at 21:47 -0500, Gustavo A. R. Silva wrote: > Hello everybody, > > I'm a little confused. Is this patch causing any trouble? Not to me. I no longer have an e1000e. Given the commit message, this just seemed to be a patch that _might_ cause an issue if this code patch is actually untested. Compilation wise, it's obviously fine. > On 08/10/2017 12:56 PM, Joe Perches wrote: > > On Wed, 2017-08-09 at 14:47 -0700, Jeff Kirsher wrote: > > > From: Gustavo A R Silva <garsilva@embeddedor.com> > > > > > > Check return value from call to e1e_wphy(). This value is being > > > checked during previous calls to function e1e_wphy() and it seems > > > a check was missing here. > > > > The use of "it seems" here is less than compelling. > > > > This is one of the first patches I sent. Maybe I should have added a > note saying that this patch needed some testing, as I don't have the > hardware to test it. > > > Perhaps the write of 0x3140 to MII_BMCR takes too long for > > the return value used. > > > > Many other uses of e1e_wphy.*MII_BMCR are also not checked. > > > > For instance: the e100e/ethtool uses. > > > > > diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c > > > > [] > > > @@ -2437,6 +2437,8 @@ static s32 e1000_hv_phy_workarounds_ich8lan(struct e1000_hw *hw) > > > if (hw->phy.revision < 2) { > > > e1000e_phy_sw_reset(hw); > > > ret_val = e1e_wphy(hw, MII_BMCR, 0x3140); > > > + if (ret_val) > > > + return ret_val; > > > } > > > } > > > > > Thanks
On Thu, 2017-08-10 at 19:53 -0700, Joe Perches wrote: > On Thu, 2017-08-10 at 21:47 -0500, Gustavo A. R. Silva wrote: > > Hello everybody, > > > > I'm a little confused. Is this patch causing any trouble? > > Not to me. I no longer have an e1000e. > > Given the commit message, this just seemed to be a patch > that _might_ cause an issue if this code patch is actually > untested. > > Compilation wise, it's obviously fine. Fortunately, I have all patches submitted to me thoroughly tested. It was a good catch, and we confirmed it does provide a valid fix.
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c index 68ea8b4555ab..d6d4ed7acf03 100644 --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c @@ -2437,6 +2437,8 @@ static s32 e1000_hv_phy_workarounds_ich8lan(struct e1000_hw *hw) if (hw->phy.revision < 2) { e1000e_phy_sw_reset(hw); ret_val = e1e_wphy(hw, MII_BMCR, 0x3140); + if (ret_val) + return ret_val; } }