[Intel-wired-lan] [PATCH v1] drivers/net/ethernet: check return value of e1e_rphy()
Li Zhong
floridsleeves at gmail.com
Fri Aug 26 05:42:48 UTC 2022
On Tue, Aug 23, 2022 at 8:19 AM Jesse Brandeburg
<jesse.brandeburg at intel.com> wrote:
>
> On 8/22/2022 11:02 PM, lily wrote:
> > e1e_rphy() could return error value, which need to be checked.
>
> Thanks for having a look at the e1000e driver. Was there some bug you
> found or is this just a fix based on a tool or observation?
>
> If a tool was used, what tool?
>
These bugs are detected by a static analysis tool to check whether a
return error is handled.
> For networking patches please follow the guidance at
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
>
>
> > Signed-off-by: Li Zhong <floridsleeves at gmail.com>
> > ---
> > drivers/net/ethernet/intel/e1000e/phy.c | 14 +++++++++++---
> > 1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/e1000e/phy.c b/drivers/net/ethernet/intel/e1000e/phy.c
> > index fd07c3679bb1..15ac302fdee0 100644
> > --- a/drivers/net/ethernet/intel/e1000e/phy.c
> > +++ b/drivers/net/ethernet/intel/e1000e/phy.c
> > @@ -2697,9 +2697,12 @@ static s32 e1000_access_phy_wakeup_reg_bm(struct e1000_hw *hw, u32 offset,
> > void e1000_power_up_phy_copper(struct e1000_hw *hw)
> > {
> > u16 mii_reg = 0;
> > + int ret;
> >
> > /* The PHY will retain its settings across a power down/up cycle */
> > - e1e_rphy(hw, MII_BMCR, &mii_reg);
> > + ret = e1e_rphy(hw, MII_BMCR, &mii_reg);
> > + if (ret)
> > + return ret;
>
> Can't return value to a void declared function, did you even compile
> test this?
Sorry for the compilation error. We will fix it in patch v2.
>
> Maybe it should be like:
> if (ret) {
> // this is psuedo code
> dev_warn(..., "PHY read failed during power up\n");
> return;
> }
>
> > mii_reg &= ~BMCR_PDOWN;
> > e1e_wphy(hw, MII_BMCR, mii_reg);
> > }
> > @@ -2715,9 +2718,12 @@ void e1000_power_up_phy_copper(struct e1000_hw *hw)
> > void e1000_power_down_phy_copper(struct e1000_hw *hw)
> > {
> > u16 mii_reg = 0;
> > + int ret;
> >
> > /* The PHY will retain its settings across a power down/up cycle */
> > - e1e_rphy(hw, MII_BMCR, &mii_reg);
> > + ret = e1e_rphy(hw, MII_BMCR, &mii_reg);
> > + if (ret)
> > + return ret;
>
> same here.
>
> > mii_reg |= BMCR_PDOWN;
> > e1e_wphy(hw, MII_BMCR, mii_reg);
> > usleep_range(1000, 2000);
> > @@ -3037,7 +3043,9 @@ s32 e1000_link_stall_workaround_hv(struct e1000_hw *hw)
> > return 0;
> >
> > /* Do not apply workaround if in PHY loopback bit 14 set */
> > - e1e_rphy(hw, MII_BMCR, &data);
> > + ret_val = e1e_rphy(hw, MII_BMCR, &data);
> > + if (ret_val)
> > + return ret_val;
> > if (data & BMCR_LOOPBACK)
> > return 0;
> >
>
> Did any of the callers of the above function care about the return code
> being an error value? This has been like this for a long time...
We manually check this function e1e_rphy(). We think it's possible that
this function fails and it would be better if we can check the error and
report it for debugging and diagnosing. Though the possibility of error
may be low and that's why it has been here for a long time.
More information about the Intel-wired-lan
mailing list