e1e_rphy() could return error value, which need to be checked.
Signed-off-by: Li Zhong <[email protected]>
---
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
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 <[email protected]>
> ---
> 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...
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 <[email protected]>
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 <[email protected]>"
> ---
> drivers/net/ethernet/intel/e1000e/phy.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
[…]
Kind regards,
Paul
On Tue, Aug 23, 2022 at 8:19 AM Jesse Brandeburg
<[email protected]> 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 <[email protected]>
> > ---
> > 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.