[PATCH] net: lwip: introduce net_lwip_eth_stop() function

[Simon Glass]

Hi David,

On Mon, 11 May 2026 at 15:48, David Lechner <dlechner at baylibre.com> wrote:
>
> On 5/11/26 1:08 PM, Simon Glass wrote:
> > Hi David,
> >
> > On 2026-05-09T17:36:26, David Lechner <dlechner at baylibre.com> wrote:
> >> net: lwip: introduce net_lwip_eth_stop() function
> >>
> >> Add a introduce net_lwip_eth_stop() function and use that to stop the
> >> network interface after each command that uses the network.
> >>
> >> This makes the behavior the same as the legacy net code and avoids
> >> potential issues with the network interface being left in an active
> >> state after a command finishes.
> >>
> >> Signed-off-by: David Lechner <dlechner at baylibre.com>
> >>
> >> cmd/lwip/ping.c     | 10 ++++++++--
> >>  cmd/lwip/sntp.c     | 10 ++++++++--
> >>  include/net-lwip.h  |  1 +
> >>  net/lwip/dhcp.c     | 15 +++++++++++----
> >>  net/lwip/dns.c      |  6 +++++-
> >>  net/lwip/net-lwip.c |  6 +++++-
> >>  net/lwip/nfs.c      |  4 ++++
> >>  net/lwip/tftp.c     |  4 ++++
> >>  net/lwip/wget.c     | 34 ++++++++++++++++++++++------------
> >>  9 files changed, 68 insertions(+), 22 deletions(-)
> >
> >> diff --git a/net/lwip/wget.c b/net/lwip/wget.c
> >> @@ -302,6 +302,7 @@ int wget_do_request(ulong dst_addr, char *uri)
> >>       struct udevice *udev;
> >>       struct netif *netif;
> >>       struct wget_ctx ctx;
> >> +     int ret = CMD_RET_USAGE;
> >>       char *path;
> >>       bool is_https;
> >
> > Initialising ret to CMD_RET_USAGE changes error paths:
> >
> >    netif = net_lwip_new_netif(udev);
> >    if (!netif)
> >         goto out_stop;          /* ret is still CMD_RET_USAGE */
> >
> >    if (!tls_allocator.arg) {
> >         log_err(...);
> >         goto out_remove_netif;  /* ret is still CMD_RET_USAGE */
> >    }
> >
> > The fall-through after 'Certificate verification failed' also lands on
> > out_remove_netif with ret unchanged. These paths previously returned
> > -1; now they return CMD_RET_USAGE (1), so the command framework prints
> > usage for a network/TLS failure.
>
> The code I am looking at says:
>
> enum command_ret_t {
>         CMD_RET_SUCCESS,        /* 0 = Success */
>         CMD_RET_FAILURE,        /* 1 = Failure */
>         CMD_RET_USAGE = -1,     /* Failure, please report 'usage' error */
> };
>
> So I don't think these changed. CMD_RET_USAGE is -1.
>
> Happy to init it -1 though as you suggest if the name doesn't make
> sense here. That is might be why it was -1 before instead of using
> the macro anyway.

Ah yes I missed that this is a pre-existing bug. We should return
usage only when parsing is wrong, not when something fails.

So what you have here is fine and actually makes the bug more obvious
in the code, which is good.

Reviewed-by: Simon Glass <sjg at chromium.org>

>
> >
> > wget_do_request() isn't a command handler either, so CMD_RET_USAGE is
> > doubly out of place. Please initialise ret to -1 and set ret =
> > CMD_RET_FAILURE explicitly only in the paths that previously returned
> > that.
> >
> > It might make sense to split this up function (perhaps in a prior
> > patch) to try to make the error handling easier?
> >
> Sure. Is there a common naming pattern for that in U-Boot. I would
> call the inner function __wget_do_request(). But I know opinions on
> such things can vary widely. :-)

If you do take this on, I would say better to rename it, e.g.
handle_request() - strictly speaking we are supposed to avoid leading
underscores, since they indicate symbols used by the
compiler/toolchain, particularly the double underscore. In practice
you will see use of a leading underscore in the code.

Separately, I am taking a look at more use of getopt(), which would
make parsing more regular, but unfortunately adds code size.

Regards,
Simon