From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Ray Date: Wed, 5 May 2021 10:04:36 +0300 Subject: EXT: Re: [RFC 1/2] net: net_up, net_down In-Reply-To: References: <48c36f0a30192f3fa39dcecbec45dbf7c734db80.1620039770.git.ian.ray@ge.com> Message-ID: <20210505070436.GA10509@zoo6.em.health.ge.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Wed, May 05, 2021 at 05:02:21AM +0300, Ramon Fried wrote: > > On Mon, May 3, 2021 at 2:55 PM Ian Ray 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 > > --- > > 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!