On Wed, Aug 26, 2026 at 1:42 PM Tom Rini <[email protected]> wrote:
> 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 > Sorry for the late reply. I checked the commit 7ad5e878cd850102e13ee2a5f8a5fb69742f70b0 and looks like the issue still exists before that commit. If the check "device_active(current)" failed then the errno is also not set and we still return 0. I am not sure if "eth_errno = eth_start_udev();" since that just ignores the real errno. The fix I proposed is just to capture the corner case that errno is 0 but eth_start_udev still failed for whatever reason. Otherwise the logic keeps the same as before. Thanks, -Yuxiao
