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.
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?
> diff --git a/net/lwip/net-lwip.c b/net/lwip/net-lwip.c
> @@ -181,7 +181,6 @@ int net_lwip_eth_start(void)
> int ret;
>
> net_init();
> - eth_halt();
> eth_set_current();
> ret = eth_init();
Removing the eth_halt() changes the contract of net_lwip_eth_start().
The legacy code in net/net.c still does eth_halt() before eth_init()
in the on-demand-init path, so the claim that this matches legacy
behaviour is only half true.
do_dhcp() with argc > 1 calls do_tftpb() while the interface is still
up, so do_tftpb() now calls net_lwip_eth_start() -> eth_init() without
an intervening halt. I wonder what happens if we have 'dhcp <addr>
<file>' on a board where the driver doesn't support a second
eth_init() without a halt? It may be safer to keep the halt in
net_lwip_eth_start() and treat net_lwip_eth_stop() as the symmetric
counterpart.
> Add a introduce net_lwip_eth_stop() function and use that to stop the
> network interface after each command that uses the network.
Commit message: 'Add a introduce' should be just 'Introduce' (or 'Add a').
Regards,
Simon