On Wed, May 05, 2021 at 05:02:21AM +0300, Ramon Fried wrote:

On Mon, May 3, 2021 at 2:55 PM Ian Ray ian.ray@ge.com wrote:

...

Calls made to eth_halt() by network operations (ping, nfs, tftp, etc.) break netconsole if it is already running.

• Maintain the network up / down state based on a reference count, using new APIs net_up() and net_down().

Note that when network is brought up by netconsole client, then network will remain up until reboot (because there is no way to stop netconsole itself).

• Remove net_loop_last_protocol, eth_is_on_demand_init(), and eth_set_last_protocol(). This functionality is replaced by net_up() / net_down().

• Replace usage of eth_init(), eth_halt() with net_up() and net_down() in net.c, ping.c, tftp.c, and netconsole.c.

Signed-off-by: Ian Ray ian.ray@ge.com

drivers/net/netconsole.c | 26 +++-------------- include/net.h | 23 ++------------- net/net.c | 75 +++++++++++++++++++++++++++++++++--------------- net/ping.c | 1 - net/tftp.c | 4 --- net/wol.c | 1 - 6 files changed, 59 insertions(+), 71 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index f1d0630..b6d2e22 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -27,11 +27,6 @@ static short nc_out_port; /* target output port */ static short nc_in_port; /* source input port */ static const char *output_packet; /* used by first send udp */ static int output_packet_len; -/*

• • Start with a default last protocol.
• • We are only interested in NETCONS or not.
• */

-enum proto_t net_loop_last_protocol = BOOTP;

static void nc_wait_arp_handler(uchar *pkt, unsigned dest, struct in_addr sip, unsigned src, @@ -177,7 +172,6 @@ static void nc_send_packet(const char *buf, int len) #else struct eth_device *eth; #endif

•   int inited = 0;
    uchar *pkt;
    uchar *ether;
    struct in_addr ip;
  

@@ -200,29 +194,17 @@ static void nc_send_packet(const char *buf, int len) return; }

•   if (!eth_is_active(eth)) {
  

•           if (eth_is_on_demand_init()) {
  

•                   if (eth_init() < 0)
  

•                           return;
  

•                   eth_set_last_protocol(NETCONS);
  

•           } else {
  

•                   eth_init_state_only();
  

•           }
  

•           inited = 1;
  

•   if (net_up(NETCONS) < 0) {
  

•           return;
    }
  

•   pkt = (uchar *)net_tx_packet + net_eth_hdr_size() + IP_UDP_HDR_SIZE;
    memcpy(pkt, buf, len);
    ether = nc_ether;
    ip = nc_ip;
    net_send_udp_packet(ether, ip, nc_out_port, nc_in_port, len);
  
  

•   if (inited) {
  

•           if (eth_is_on_demand_init())
  

•                   eth_halt();
  

•           else
  

•                   eth_halt_state_only();
  

•   }
  

•   net_down();
  

}

static int nc_stdio_start(struct stdio_dev *dev) diff --git a/include/net.h b/include/net.h index 1bf9867..cc6be60 100644 --- a/include/net.h +++ b/include/net.h @@ -599,6 +599,9 @@ int net_loop(enum proto_t); /* Load failed. Start again. */ int net_start_again(void);

+int net_up(enum proto_t protocol); +int net_down(void);

/* Get size of the ethernet header when we send */ int net_eth_hdr_size(void);

@@ -706,26 +709,6 @@ int nc_input_packet(uchar *pkt, struct in_addr src_ip, unsigned dest_port, unsigned src_port, unsigned len); #endif

-static __always_inline int eth_is_on_demand_init(void) -{ -#if defined(CONFIG_NETCONSOLE) && !defined(CONFIG_SPL_BUILD)

•   extern enum proto_t net_loop_last_protocol;
  

•   return net_loop_last_protocol != NETCONS;
  

-#else

•   return 1;
  

-#endif -}

-static inline void eth_set_last_protocol(int protocol) -{ -#if defined(CONFIG_NETCONSOLE) && !defined(CONFIG_SPL_BUILD)

•   extern enum proto_t net_loop_last_protocol;
  

•   net_loop_last_protocol = protocol;
  

-#endif -}

/*

• Check if autoload is enabled. If so, use either NFS or TFTP to download
• the boot file.

diff --git a/net/net.c b/net/net.c index 28d9eeb..45d4f19 100644 --- a/net/net.c +++ b/net/net.c @@ -404,6 +404,51 @@ void net_init(void)

• Main network processing loop.
  

*/

+static int up_ref; +static bool up; +static bool keep_up;

+/// Bring net up/active if needed. +int net_up(enum proto_t protocol) +{

•   int ret;
  

•   if (!up) {
  

•           eth_halt();
  

•           eth_set_current();
  

•           ret = eth_init();
  

•           if (ret < 0) {
  

•                   debug_cond(DEBUG_INT_STATE, "net_up failed\n");
  

•                   eth_halt();
  

•                   return ret;
  

•           }
  

•           up = true;
  

•   } else if (up_ref == 0) {
  

•           eth_init_state_only();
  

•   }
  

•   up_ref++;
  

•   if (protocol == NETCONS) {
  

•           keep_up = true;
  

•   }
  

•   return 0;
  

+}

+/// Set net inactive if no clients remain. +int net_down(void) +{

•   if (up_ref > 0) {
  

•           up_ref--;
  

•           if (up_ref == 0) {
  

•                   if (keep_up) {
  

•                           eth_halt_state_only();
  

•                   } else {
  

•                           up = false;
  

•                           eth_halt();
  

•                   }
  

•           }
  

•   }
  

•   return 0;
  

+}

int net_loop(enum proto_t protocol) { int ret = -EINVAL; @@ -420,16 +465,9 @@ int net_loop(enum proto_t protocol)

    bootstage_mark_name(BOOTSTAGE_ID_ETH_START, "eth_start");
    net_init();

•   if (eth_is_on_demand_init() || protocol != NETCONS) {
  

•           eth_halt();
  

•           eth_set_current();
  

•           ret = eth_init();
  

•           if (ret < 0) {
  

•                   eth_halt();
  

•                   return ret;
  

•           }
  

•   } else {
  

•           eth_init_state_only();
  

•   ret = net_up(protocol);
  

•   if (ret < 0) {
  

•           return ret;
    }
  

restart: #ifdef CONFIG_USB_KEYBOARD @@ -448,7 +486,7 @@ restart: switch (net_check_prereq(protocol)) { case 1: /* network not configured */

•           eth_halt();
  

•           net_down();
            net_set_state(prev_net_state);
            return -ENODEV;
  
  

@@ -589,9 +627,7 @@ restart: net_arp_wait_packet_ip.s_addr = 0;

                    net_cleanup_loop();

•                   eth_halt();
  

•                   /* Invalidate the last protocol */
  

•                   eth_set_last_protocol(BOOTP);
  

•                   net_down();
  
                    puts("\nAbort\n");
                    /* include a debug print as well incase the debug
  

@@ -647,12 +683,7 @@ restart: env_set_hex("filesize", net_boot_file_size); env_set_hex("fileaddr", image_load_addr); }

•                   if (protocol != NETCONS)
  

•                           eth_halt();
  

•                   else
  

•                           eth_halt_state_only();
  

•                   eth_set_last_protocol(protocol);
  

•                   net_down();
  
                    ret = net_boot_file_size;
                    debug_cond(DEBUG_INT_STATE, "--- net_loop Success!\n");
  

@@ -660,8 +691,7 @@ restart:

            case NETLOOP_FAIL:
                    net_cleanup_loop();

•                   /* Invalidate the last protocol */
  

•                   eth_set_last_protocol(BOOTP);
  

•                   net_down();
                    debug_cond(DEBUG_INT_STATE, "--- net_loop Fail!\n");
                    ret = -ENONET;
                    goto done;
  

@@ -719,7 +749,6 @@ int net_start_again(void) }

    if ((!retry_forever) && (net_try_count > retrycnt)) {

•           eth_halt();
            net_set_state(NETLOOP_FAIL);
            /*
             * We don't provide a way for the protocol to return an error,
  

diff --git a/net/ping.c b/net/ping.c index 0e33660..2c92806 100644 --- a/net/ping.c +++ b/net/ping.c @@ -64,7 +64,6 @@ static int ping_send(void)

static void ping_timeout_handler(void) {

•   eth_halt();
    net_set_state(NETLOOP_FAIL);    /* we did not get the reply */
  

}

diff --git a/net/tftp.c b/net/tftp.c index 84e970b..dcc387b 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -652,7 +652,6 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, net_set_timeout_handler(timeout_ms, tftp_timeout_handler);

            if (store_block(tftp_cur_block - 1, pkt + 2, len)) {

•                   eth_halt();
                    net_set_state(NETLOOP_FAIL);
                    break;
            }
  

@@ -680,7 +679,6 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, case TFTP_ERR_FILE_NOT_FOUND: case TFTP_ERR_ACCESS_DENIED: puts("Not retrying...\n");

•                   eth_halt();
                    net_set_state(NETLOOP_FAIL);
                    break;
            case TFTP_ERR_UNDEFINED:
  

@@ -829,7 +827,6 @@ void tftp_start(enum proto_t protocol) #endif { if (tftp_init_load_addr()) {

•                   eth_halt();
                    net_set_state(NETLOOP_FAIL);
                    puts("\nTFTP error: ");
                    puts("trying to overwrite reserved memory...\n");
  

@@ -885,7 +882,6 @@ void tftp_start_server(void) tftp_filename[0] = 0;

    if (tftp_init_load_addr()) {

•           eth_halt();
            net_set_state(NETLOOP_FAIL);
            puts("\nTFTP error: trying to overwrite reserved memory...\n");
            return;
  

diff --git a/net/wol.c b/net/wol.c index 0a62566..cd4e67e 100644 --- a/net/wol.c +++ b/net/wol.c @@ -85,7 +85,6 @@ void wol_set_timeout(ulong timeout)

static void wol_timeout_handler(void) {

•   eth_halt();
    net_set_state(NETLOOP_FAIL);
  

}

-- 2.10.1

I like the idea, I think that network protocols should call net_down()

I'm not sure I understand this comment.

My (possibly incomplete) understanding of the design is as follows:

1. A command causes the net_loop() to be entered, and returns when the command has completed (successfully or otherwise).

2. net/net.c calls protocol-specific entry points, which in turn assign handler functions and send a UDP packet.

3. Handler functions are called on receive or timeout, and the protocol implementation is responsible for creating new packets or terminating the net_loop() by calling net_set_state().

Currently the net is brought _down_ by individual protocols (e.g. net/ping.c, net/tftp.c) on failure (shortly before *or* after calling net_set_state()) and of course by net/net.c on success.

In the RFC I tried to simplify this such that: net is brought down in one place net/net.c (essentially on exit from net_loop) -- but sadly drivers/net/netconsole.c needs to do this too. Maybe that could be improved.

Why do you think that network protocols should call net_down() ?

when they're done, however I'm not sure we need reference count.

The reference count does perhaps feel over-engineered. We would need to be *sure* that there are no overlapping scenarios.

net_down() can be a wrapper around eth_halt(), We can just check if netconsole is running and call it eth_halt() only if its' not.

Thanks for the feedback!