On Mon, Aug 31, 2026 at 02:00:22PM -0700, Yuxiao Zhang wrote: > 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.
Yes, before if device_active(current) is false (it's just a flag check), we would only end up with something in eth_errno if eth_get_dev() had failed. So at this point, it should be clear that, as I had asked originally, this isn't some special case but rather a problem in handling errors from eth_start_udev to solve. Perhaps it should be setting eth_errno instead of directly returning? Or perhaps as I suggested at first we should be doing 'eth_errno = eth_start_udev();' and going from there? -- Tom
signature.asc
Description: PGP signature
