diff mbox series

net: e1000: Fix Unchecked return value coverity

Message ID 20210531152132.25663-1-Zhiqiang.Hou@nxp.com
State Changes Requested
Delegated to: Ramon Fried
Headers show
Series net: e1000: Fix Unchecked return value coverity | expand

Commit Message

Z.Q. Hou May 31, 2021, 3:21 p.m. UTC 
From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

Added check for return value of e1000_read_phy_reg().

Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
---
 drivers/net/e1000.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Ramon Fried June 10, 2021, 5:48 a.m. UTC | #1 
On Mon, May 31, 2021 at 6:12 PM Zhiqiang Hou <Zhiqiang.Hou@nxp.com> wrote:
>
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>
> Added check for return value of e1000_read_phy_reg().
>
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> ---
>  drivers/net/e1000.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
> index 694114eca7..1f0d559415 100644
> --- a/drivers/net/e1000.c
> +++ b/drivers/net/e1000.c
> @@ -4738,12 +4738,16 @@ e1000_phy_init_script(struct e1000_hw *hw)
>                         uint16_t fused, fine, coarse;
>
>                         /* Move to analog registers page */
> -                       e1000_read_phy_reg(hw,
> -                               IGP01E1000_ANALOG_SPARE_FUSE_STATUS, &fused);
> +                       if (e1000_read_phy_reg(hw,
> +                                              IGP01E1000_ANALOG_SPARE_FUSE_STATUS,
> +                                              &fused))
> +                               return;
>
>                         if (!(fused & IGP01E1000_ANALOG_SPARE_FUSE_ENABLED)) {
> -                               e1000_read_phy_reg(hw,
> -                                       IGP01E1000_ANALOG_FUSE_STATUS, &fused);
> +                               if (e1000_read_phy_reg(hw,
> +                                                      IGP01E1000_ANALOG_FUSE_STATUS,
> +                                                      &fused))
> +                                       return;
>
>                                 fine = fused & IGP01E1000_ANALOG_FUSE_FINE_MASK;
>                                 coarse = fused
> --
> 2.17.1
>
What about some error messages ?
Z.Q. Hou June 11, 2021, 2:56 a.m. UTC | #2 
> -----Original Message-----
> From: Ramon Fried <rfried.dev@gmail.com>
> Sent: 2021年6月10日 13:49
> To: Z.Q. Hou <zhiqiang.hou@nxp.com>
> Cc: Joe Hershberger <joe.hershberger@ni.com>; U-Boot Mailing List
> <u-boot@lists.denx.de>
> Subject: Re: [PATCH] net: e1000: Fix Unchecked return value coverity
> 
> On Mon, May 31, 2021 at 6:12 PM Zhiqiang Hou <Zhiqiang.Hou@nxp.com>
> wrote:
> >
> > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> >
> > Added check for return value of e1000_read_phy_reg().
> >
> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > ---
> >  drivers/net/e1000.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c index
> > 694114eca7..1f0d559415 100644
> > --- a/drivers/net/e1000.c
> > +++ b/drivers/net/e1000.c
> > @@ -4738,12 +4738,16 @@ e1000_phy_init_script(struct e1000_hw *hw)
> >                         uint16_t fused, fine, coarse;
> >
> >                         /* Move to analog registers page */
> > -                       e1000_read_phy_reg(hw,
> > -
> IGP01E1000_ANALOG_SPARE_FUSE_STATUS, &fused);
> > +                       if (e1000_read_phy_reg(hw,
> > +
> IGP01E1000_ANALOG_SPARE_FUSE_STATUS,
> > +                                              &fused))
> > +                               return;
> >
> >                         if (!(fused &
> IGP01E1000_ANALOG_SPARE_FUSE_ENABLED)) {
> > -                               e1000_read_phy_reg(hw,
> > -
> IGP01E1000_ANALOG_FUSE_STATUS, &fused);
> > +                               if (e1000_read_phy_reg(hw,
> > +
> IGP01E1000_ANALOG_FUSE_STATUS,
> > +
> &fused))
> > +                                       return;
> >
> >                                 fine = fused &
> IGP01E1000_ANALOG_FUSE_FINE_MASK;
> >                                 coarse = fused
> > --
> > 2.17.1
> >
> What about some error messages ?

CID 43664 (#15 of 15): Unchecked return value (CHECKED_RETURN)
6. check_return: Calling e1000_read_phy_reg without checking return value (as is done elsewhere 44 out of 47 times).

- Zhiqiang
Ramon Fried June 11, 2021, 11:46 p.m. UTC | #3 
On Fri, Jun 11, 2021 at 5:56 AM Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Ramon Fried <rfried.dev@gmail.com>
> > Sent: 2021年6月10日 13:49
> > To: Z.Q. Hou <zhiqiang.hou@nxp.com>
> > Cc: Joe Hershberger <joe.hershberger@ni.com>; U-Boot Mailing List
> > <u-boot@lists.denx.de>
> > Subject: Re: [PATCH] net: e1000: Fix Unchecked return value coverity
> >
> > On Mon, May 31, 2021 at 6:12 PM Zhiqiang Hou <Zhiqiang.Hou@nxp.com>
> > wrote:
> > >
> > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > >
> > > Added check for return value of e1000_read_phy_reg().
> > >
> > > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > > ---
> > >  drivers/net/e1000.c | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c index
> > > 694114eca7..1f0d559415 100644
> > > --- a/drivers/net/e1000.c
> > > +++ b/drivers/net/e1000.c
> > > @@ -4738,12 +4738,16 @@ e1000_phy_init_script(struct e1000_hw *hw)
> > >                         uint16_t fused, fine, coarse;
> > >
> > >                         /* Move to analog registers page */
> > > -                       e1000_read_phy_reg(hw,
> > > -
> > IGP01E1000_ANALOG_SPARE_FUSE_STATUS, &fused);
> > > +                       if (e1000_read_phy_reg(hw,
> > > +
> > IGP01E1000_ANALOG_SPARE_FUSE_STATUS,
> > > +                                              &fused))
> > > +                               return;
> > >
> > >                         if (!(fused &
> > IGP01E1000_ANALOG_SPARE_FUSE_ENABLED)) {
> > > -                               e1000_read_phy_reg(hw,
> > > -
> > IGP01E1000_ANALOG_FUSE_STATUS, &fused);
> > > +                               if (e1000_read_phy_reg(hw,
> > > +
> > IGP01E1000_ANALOG_FUSE_STATUS,
> > > +
> > &fused))
> > > +                                       return;
> > >
> > >                                 fine = fused &
> > IGP01E1000_ANALOG_FUSE_FINE_MASK;
> > >                                 coarse = fused
> > > --
> > > 2.17.1
> > >
> > What about some error messages ?
>
> CID 43664 (#15 of 15): Unchecked return value (CHECKED_RETURN)
> 6. check_return: Calling e1000_read_phy_reg without checking return value (as is done elsewhere 44 out of 47 times).
>
> - Zhiqiang
It's not clear enough why you're leaving the function early, better
add some comments in the commit message and in the code.
Z.Q. Hou June 23, 2021, 2:50 a.m. UTC | #4 
> -----Original Message-----
> From: Ramon Fried [mailto:rfried.dev@gmail.com]
> Sent: 2021年6月12日 7:47
> To: Z.Q. Hou <zhiqiang.hou@nxp.com>
> Cc: Joe Hershberger <joe.hershberger@ni.com>; U-Boot Mailing List
> <u-boot@lists.denx.de>
> Subject: Re: [PATCH] net: e1000: Fix Unchecked return value coverity
> 
> On Fri, Jun 11, 2021 at 5:56 AM Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Ramon Fried <rfried.dev@gmail.com>
> > > Sent: 2021年6月10日 13:49
> > > To: Z.Q. Hou <zhiqiang.hou@nxp.com>
> > > Cc: Joe Hershberger <joe.hershberger@ni.com>; U-Boot Mailing List
> > > <u-boot@lists.denx.de>
> > > Subject: Re: [PATCH] net: e1000: Fix Unchecked return value coverity
> > >
> > > On Mon, May 31, 2021 at 6:12 PM Zhiqiang Hou <Zhiqiang.Hou@nxp.com>
> > > wrote:
> > > >
> > > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > > >
> > > > Added check for return value of e1000_read_phy_reg().
> > > >
> > > > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > > > ---
> > > >  drivers/net/e1000.c | 12 ++++++++----
> > > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c index
> > > > 694114eca7..1f0d559415 100644
> > > > --- a/drivers/net/e1000.c
> > > > +++ b/drivers/net/e1000.c
> > > > @@ -4738,12 +4738,16 @@ e1000_phy_init_script(struct e1000_hw
> *hw)
> > > >                         uint16_t fused, fine, coarse;
> > > >
> > > >                         /* Move to analog registers page */
> > > > -                       e1000_read_phy_reg(hw,
> > > > -
> > > IGP01E1000_ANALOG_SPARE_FUSE_STATUS, &fused);
> > > > +                       if (e1000_read_phy_reg(hw,
> > > > +
> > > IGP01E1000_ANALOG_SPARE_FUSE_STATUS,
> > > > +                                              &fused))
> > > > +                               return;
> > > >
> > > >                         if (!(fused &
> > > IGP01E1000_ANALOG_SPARE_FUSE_ENABLED)) {
> > > > -                               e1000_read_phy_reg(hw,
> > > > -
> > > IGP01E1000_ANALOG_FUSE_STATUS, &fused);
> > > > +                               if (e1000_read_phy_reg(hw,
> > > > +
> > > IGP01E1000_ANALOG_FUSE_STATUS,
> > > > +
> > > &fused))
> > > > +                                       return;
> > > >
> > > >                                 fine = fused &
> > > IGP01E1000_ANALOG_FUSE_FINE_MASK;
> > > >                                 coarse = fused
> > > > --
> > > > 2.17.1
> > > >
> > > What about some error messages ?
> >
> > CID 43664 (#15 of 15): Unchecked return value (CHECKED_RETURN) 6.
> > check_return: Calling e1000_read_phy_reg without checking return value
> (as is done elsewhere 44 out of 47 times).
> >
> > - Zhiqiang
> It's not clear enough why you're leaving the function early, better add some
> comments in the commit message and in the code.

If it should continue to run while it failed to read the PHY registers? Anyone can comments on this?

Thanks,
Zhiqiang
diff mbox series

Patch

diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
index 694114eca7..1f0d559415 100644
--- a/drivers/net/e1000.c
+++ b/drivers/net/e1000.c
@@ -4738,12 +4738,16 @@  e1000_phy_init_script(struct e1000_hw *hw)
 			uint16_t fused, fine, coarse;
 
 			/* Move to analog registers page */
-			e1000_read_phy_reg(hw,
-				IGP01E1000_ANALOG_SPARE_FUSE_STATUS, &fused);
+			if (e1000_read_phy_reg(hw,
+					       IGP01E1000_ANALOG_SPARE_FUSE_STATUS,
+					       &fused))
+				return;
 
 			if (!(fused & IGP01E1000_ANALOG_SPARE_FUSE_ENABLED)) {
-				e1000_read_phy_reg(hw,
-					IGP01E1000_ANALOG_FUSE_STATUS, &fused);
+				if (e1000_read_phy_reg(hw,
+						       IGP01E1000_ANALOG_FUSE_STATUS,
+						       &fused))
+					return;
 
 				fine = fused & IGP01E1000_ANALOG_FUSE_FINE_MASK;
 				coarse = fused