From: Ian Ray <ian.ray@ge.com>
To: u-boot@lists.denx.de
Subject: EXT: Re: [RFC 1/2] net: net_up, net_down
Date: Wed, 5 May 2021 10:04:36 +0300 [thread overview]
Message-ID: <20210505070436.GA10509@zoo6.em.health.ge.com> (raw)
In-Reply-To: <CAGi-RUJDLOyx63ftc+KE03B1HsVNGPDr-TitnO3ruoj80QcKZw@mail.gmail.com>
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!
next prev parent reply other threads:[~2021-05-05 7:04 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-03 11:55 [RFC 0/2] netconsole and networking co-existence Ian Ray
2021-05-03 11:55 ` [RFC 1/2] net: net_up, net_down Ian Ray
2021-05-05 2:02 ` Ramon Fried
2021-05-05 7:04 ` Ian Ray [this message]
2021-05-03 11:55 ` [RFC 2/2] netconsole and networking co-existence Ian Ray
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210505070436.GA10509@zoo6.em.health.ge.com \
--to=ian.ray@ge.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.