Re: [PATCH] u-boot: fix error check in eth init

On Wed, Aug 26, 2026 at 01:12:55PM -0700, Yuxiao Zhang wrote:

> IMHO at least the activation check is a valid case. It just check the
> status flag and returns EINVAL without setting errno:
> 
> 292         if (!device_active(dev))
> 293                 return -EINVAL;

Please don't top-post, it makes following context harder. And, this
isn't quite what I was asking about. If we look at commit:
commit 7ad5e878cd850102e13ee2a5f8a5fb69742f70b0
Author: Jerome Forissier <[email protected]>
Date:   Wed Oct 16 12:04:01 2024 +0200

    net: eth-uclass: add function eth_start_udev()
    
    Add a function to start a given network device, and update eth_init()
    to use it.
    
    Signed-off-by: Jerome Forissier <[email protected]>
    Reviewed-by: Ilias Apalodimas <[email protected]>

Which introduces this function, and examine the code before/after, we
aren't quite doing the same things as before.

So yes, we aren't handling the error case you describe correctly, but my
first thought is that the caller here should be doing eth_errno =
eth_start_udev(); and then setting ret = eth_errno if we get a problem.
But that might not be quite right either, I've only spent a few minutes
on this,

> 
> Thanks,
> -Yuxiao
> 
> 
> 
> On Wed, Aug 26, 2026 at 1:02 PM Tom Rini <[email protected]> wrote:
> 
> > On Wed, Aug 26, 2026 at 11:14:19AM -0700, Yuxiao Zhang wrote:
> >
> > > eth_start_udev can fail but errno could still be zero which causes the
> > > function ignores the error silently. This fix catches the error
> > > properly.
> > >
> > > Signed-off-by: Yuxiao Zhang <[email protected]>
> > > ---
> > >
> > >  net/eth-uclass.c | 11 ++++++-----
> > >  1 file changed, 6 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> > > index 5c437143a30..5518268bd68 100644
> > > --- a/net/eth-uclass.c
> > > +++ b/net/eth-uclass.c
> > > @@ -347,12 +347,13 @@ int eth_init(void)
> > >                       debug("Trying %s\n", current->name);
> > >
> > >                       ret = eth_start_udev(current);
> > > -                     if (ret < 0)
> > > -                             ret = eth_errno;
> > > -                     else
> > > +                     if (ret < 0) {
> > > +                             if (eth_errno < 0)
> > > +                                     ret = eth_errno;
> > > +                             debug("%s start failed (ret=%d)\n",
> > current->name, ret);
> > > +                     } else {
> > >                               break;
> > > -
> > > -                     debug("FAIL\n");
> > > +                     }
> > >               } else {
> > >                       debug("PROBE FAIL\n");
> > >               }
> >
> > Is it a valid case for eth_start_udev to have failed and not set eth_errno?
> >
> > --
> > Tom
> >

-- 
Tom

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQTzzqh0PWDgGS+bTHor4qD1Cr/kCgUCao9PvQAKCRAr4qD1Cr/k
Cg3WAQDXe+NoQscQZU3CrfyZQKP7k4pdRPap+cAbVLdpyQNsSgEAraM4C1XyrbBu
dqFr+2TDgZOW9PnIqMViNx1d1BRT5Ag=
=vijq
-----END PGP SIGNATURE-----