Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220823060200.1452663-1-floridsleeves@gmail.com> <0056a39d-d7dc-34ea-3a71-6d5d3835c2d5@intel.com>
In-Reply-To: <0056a39d-d7dc-34ea-3a71-6d5d3835c2d5@intel.com>
From:   Li Zhong <floridsleeves@gmail.com>
Date:   Thu, 25 Aug 2022 22:42:48 -0700
Message-ID: <CAMEuxRraM31C1k9u37ZyxrYVUtKuWdiYUfhw+g=p7_oq-MrMEA@mail.gmail.com>
Subject: Re: [PATCH v1] drivers/net/ethernet: check return value of e1e_rphy()
To:     Jesse Brandeburg <jesse.brandeburg@intel.com>
Cc:     intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, anthony.l.nguyen@intel.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Aug 23, 2022 at 8:19 AM Jesse Brandeburg
<jesse.brandeburg@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@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.