[v1] drivers/net/ethernet: check return value of e1e_rphy()

[PATCH v1] drivers/net/ethernet: check return value of e1e_rphy()

Posted by lily 3 years, 7 months ago

e1e_rphy() could return error value, which need to be checked.

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;
 	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;
 	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;
 
-- 
2.25.1

Re: [Intel-wired-lan] [PATCH v1] drivers/net/ethernet: check return value of e1e_rphy()

Posted by Paul Menzel 3 years, 7 months ago

Dear Li,


Thank you for your patch.

Am 23.08.22 um 08:02 schrieb lily:
> e1e_rphy() could return error value, which need to be checked.

need*s*

> 
> Signed-off-by: Li Zhong <floridsleeves@gmail.com>

The From header field does not match the Signed-off-by line. Could you 
configure git with your user name?

     $ git config --global user.name "Li Zhong"
     $ git commit --amend --author="Li Zhong <floridsleeves@gmail.com>"

> ---
>   drivers/net/ethernet/intel/e1000e/phy.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)

[…]


Kind regards,

Paul

Re: [PATCH v1] drivers/net/ethernet: check return value of e1e_rphy()

Posted by Jesse Brandeburg 3 years, 7 months ago

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?

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?

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...

Re: [PATCH v1] drivers/net/ethernet: check return value of e1e_rphy()

Posted by Li Zhong 3 years, 7 months ago

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.