EXT: Re: [RFC 1/2] net: net_up, net_down

Ian Ray ian.ray at ge.com
Wed May 5 09:04:36 CEST 2021


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 at 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 at 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!



More information about the U-Boot mailing list