[U-Boot] [PATCH v2 08/18] net: Refactor to protect access to the NetState variable

Simon Glass sjg at chromium.org
Wed Apr 4 03:11:56 CEST 2012


Hi Joe,

On Tue, Mar 27, 2012 at 4:42 PM, Joe Hershberger <joe.hershberger at ni.com> wrote:
> Changes to NetState now go through an accessor function called
> net_set_state()
>
> Signed-off-by: Joe Hershberger <joe.hershberger at ni.com>
> Cc: Joe Hershberger <joe.hershberger at gmail.com>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Mike Frysinger <vapier at gentoo.org>
> ---
> Changes for v2:
>   - net_set_state changed to static inline
>   - States changed to an enum
>   - Eliminate CamelCase in new functions.
>
>  drivers/net/netconsole.c |    6 +++---
>  include/net.h            |   16 ++++++++++------
>  net/cdp.c                |    2 +-
>  net/dns.c                |    8 ++++----
>  net/net.c                |   21 ++++++++++++---------
>  net/nfs.c                |    8 ++++----
>  net/ping.c               |    4 ++--
>  net/sntp.c               |    4 ++--
>  net/tftp.c               |    8 ++++----
>  9 files changed, 42 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index ba02fd7..28bb955 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -44,19 +44,19 @@ static void nc_wait_arp_handler(uchar *pkt, unsigned dest,
>                                 IPaddr_t sip, unsigned src,
>                                 unsigned len)
>  {
> -       NetState = NETLOOP_SUCCESS; /* got arp reply - quit net loop */
> +       net_set_state(NETLOOP_SUCCESS); /* got arp reply - quit net loop */
>  }
>
>  static void nc_handler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
>                        unsigned len)
>  {
>        if (input_size)
> -               NetState = NETLOOP_SUCCESS; /* got input - quit net loop */
> +               net_set_state(NETLOOP_SUCCESS); /* got input - quit net loop */
>  }
>
>  static void nc_timeout(void)
>  {
> -       NetState = NETLOOP_SUCCESS;
> +       net_set_state(NETLOOP_SUCCESS);
>  }
>
>  void NcStart(void)
> diff --git a/include/net.h b/include/net.h
> index f84511c..c33cd28 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -390,12 +390,6 @@ extern uchar               NetEtherNullAddr[6];
>  extern ushort          NetOurVLAN;             /* Our VLAN */
>  extern ushort          NetOurNativeVLAN;       /* Our Native VLAN */
>
> -extern int             NetState;               /* Network loop state */
> -#define NETLOOP_CONTINUE       1
> -#define NETLOOP_RESTART                2
> -#define NETLOOP_SUCCESS                3
> -#define NETLOOP_FAIL           4
> -
>  extern int             NetRestartWrap;         /* Tried all network devices */
>
>  enum proto_t {
> @@ -465,6 +459,16 @@ extern void        NetSetHandler(rxhand_f *);      /* Set RX packet handler */
>  extern void net_set_icmp_handler(rxhand_icmp_f *f); /* Set ICMP RX handler */
>  extern void    NetSetTimeout(ulong, thand_f *);/* Set timeout handler */
>
> +/* Network loop state */
> +enum net_loop_state {
> +       NETLOOP_CONTINUE, NETLOOP_RESTART, NETLOOP_SUCCESS, NETLOOP_FAIL

Do we need these all on one line?

> +};
> +static inline void net_set_state(enum net_loop_state state)
> +{
> +       extern enum net_loop_state net_state;

blank line?

> +       net_state = state;
> +}
> +
>  /* Transmit "NetTxPacket" */
>  static inline void NetSendPacket(uchar *pkt, int len)
>  {
> diff --git a/net/cdp.c b/net/cdp.c
> index 34c5401..f1c1c14 100644
> --- a/net/cdp.c
> +++ b/net/cdp.c
> @@ -234,7 +234,7 @@ CDPTimeout(void)
>        if (!CDPOK)
>                NetStartAgain();
>        else
> -               NetState = NETLOOP_SUCCESS;
> +               net_set_state(NETLOOP_SUCCESS);
>  }
>
>  static void
> diff --git a/net/dns.c b/net/dns.c
> index cc7ed51..cc0aa0a 100644
> --- a/net/dns.c
> +++ b/net/dns.c
> @@ -98,7 +98,7 @@ static void
>  DnsTimeout(void)
>  {
>        puts("Timeout\n");
> -       NetState = NETLOOP_FAIL;
> +       net_set_state(NETLOOP_FAIL);
>  }
>
>  static void
> @@ -128,7 +128,7 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
>        /* Received 0 answers */
>        if (header->nanswers == 0) {
>                puts("DNS: host not found\n");
> -               NetState = NETLOOP_SUCCESS;
> +               net_set_state(NETLOOP_SUCCESS);
>                return;
>        }
>
> @@ -141,7 +141,7 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
>        /* We sent query class 1, query type 1 */
>        if (&p[5] > e || get_unaligned_be16(p+1) != DNS_A_RECORD) {
>                puts("DNS: response was not an A record\n");
> -               NetState = NETLOOP_SUCCESS;
> +               net_set_state(NETLOOP_SUCCESS);
>                return;
>        }
>
> @@ -191,7 +191,7 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
>                        puts("server responded with invalid IP number\n");
>        }
>
> -       NetState = NETLOOP_SUCCESS;
> +       net_set_state(NETLOOP_SUCCESS);
>  }
>
>  void
> diff --git a/net/net.c b/net/net.c
> index 0d14e2c..8076e5f 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -149,7 +149,7 @@ uchar               NetEtherNullAddr[6];
>  void           (*push_packet)(void *, int len) = 0;
>  #endif
>  /* Network loop state */
> -int            NetState;
> +enum net_loop_state net_state;
>  /* Tried all network devices */
>  int            NetRestartWrap;
>  /* Network loop restarted */
> @@ -212,7 +212,7 @@ void net_auto_load(void)
>                         * Just use BOOTP/RARP to configure system;
>                         * Do not use TFTP to load the bootfile.
>                         */
> -                       NetState = NETLOOP_SUCCESS;
> +                       net_set_state(NETLOOP_SUCCESS);
>                        return;
>                }
>  #if defined(CONFIG_CMD_NFS)
> @@ -292,7 +292,7 @@ int NetLoop(enum proto_t protocol)
>  restart:
>        memcpy(NetOurEther, eth_get_dev()->enetaddr, 6);
>
> -       NetState = NETLOOP_CONTINUE;
> +       net_set_state(NETLOOP_CONTINUE);
>
>        /*
>         *      Start the ball rolling with the given start function.  From
> @@ -401,7 +401,7 @@ restart:
>
>        /*
>         *      Main packet reception loop.  Loop receiving packets until
> -        *      someone sets `NetState' to a state that terminates.
> +        *      someone sets `net_state' to a state that terminates.
>         */
>        for (;;) {
>                WATCHDOG_RESET();
> @@ -453,7 +453,7 @@ restart:
>                }
>
>
> -               switch (NetState) {
> +               switch (net_state) {
>
>                case NETLOOP_RESTART:
>                        NetRestarted = 1;
> @@ -477,6 +477,9 @@ restart:
>
>                case NETLOOP_FAIL:
>                        goto done;
> +
> +               case NETLOOP_CONTINUE:
> +                       continue;
>                }
>        }
>
> @@ -494,7 +497,7 @@ done:
>  static void
>  startAgainTimeout(void)
>  {
> -       NetState = NETLOOP_RESTART;
> +       net_set_state(NETLOOP_RESTART);
>  }
>
>  static void
> @@ -525,7 +528,7 @@ void NetStartAgain(void)
>
>        if ((!retry_forever) && (NetTryCount >= retrycnt)) {
>                eth_halt();
> -               NetState = NETLOOP_FAIL;
> +               net_set_state(NETLOOP_FAIL);
>                return;
>        }
>
> @@ -542,10 +545,10 @@ void NetStartAgain(void)
>                        NetSetTimeout(10000UL, startAgainTimeout);
>                        NetSetHandler(startAgainHandler);
>                } else {
> -                       NetState = NETLOOP_FAIL;
> +                       net_set_state(NETLOOP_FAIL);
>                }
>        } else {
> -               NetState = NETLOOP_RESTART;
> +               net_set_state(NETLOOP_RESTART);
>        }
>  }
>
> diff --git a/net/nfs.c b/net/nfs.c
> index b6188fe..4f2041c 100644
> --- a/net/nfs.c
> +++ b/net/nfs.c
> @@ -41,7 +41,7 @@ static int nfs_len;
>  static char dirfh[NFS_FHSIZE]; /* file handle of directory */
>  static char filefh[NFS_FHSIZE]; /* file handle of kernel image */
>
> -static int     NfsDownloadState;
> +static enum net_loop_state NfsDownloadState;

Can you use nfs_download_state instead?

>  static IPaddr_t NfsServerIP;
>  static int     NfsSrvMountPort;
>  static int     NfsSrvNfsPort;
> @@ -613,10 +613,10 @@ NfsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
>        case STATE_UMOUNT_REQ:
>                if (nfs_umountall_reply(pkt, len)) {
>                        puts("*** ERROR: Cannot umount\n");
> -                       NetState = NETLOOP_FAIL;
> +                       net_set_state(NETLOOP_FAIL);
>                } else {
>                        puts("\ndone\n");
> -                       NetState = NfsDownloadState;
> +                       net_set_state(NfsDownloadState);
>                }
>                break;
>
> @@ -679,7 +679,7 @@ NfsStart(void)
>        nfs_path = (char *)nfs_path_buff;
>
>        if (nfs_path == NULL) {
> -               NetState = NETLOOP_FAIL;
> +               net_set_state(NETLOOP_FAIL);
>                puts("*** ERROR: Fail allocate memory\n");
>                return;
>        }
> diff --git a/net/ping.c b/net/ping.c
> index 4896bce..6cdcdf0 100644
> --- a/net/ping.c
> +++ b/net/ping.c
> @@ -72,7 +72,7 @@ static int ping_send(void)
>  static void ping_timeout(void)
>  {
>        eth_halt();
> -       NetState = NETLOOP_FAIL;        /* we did not get the reply */
> +       net_set_state(NETLOOP_FAIL);    /* we did not get the reply */
>  }
>
>  void ping_start(void)
> @@ -92,7 +92,7 @@ void ping_receive(struct Ethernet_hdr *et, struct IP_UDP_hdr *ip, int len)
>        case ICMP_ECHO_REPLY:
>                src_ip = NetReadIP((void *)&ip->ip_src);
>                if (src_ip == NetPingIP)
> -                       NetState = NETLOOP_SUCCESS;
> +                       net_set_state(NETLOOP_SUCCESS);
>                return;
>        case ICMP_ECHO_REQUEST:
>                debug("Got ICMP ECHO REQUEST, return "
> diff --git a/net/sntp.c b/net/sntp.c
> index 7997f98..2adcc6f 100644
> --- a/net/sntp.c
> +++ b/net/sntp.c
> @@ -45,7 +45,7 @@ static void
>  SntpTimeout(void)
>  {
>        puts("Timeout\n");
> -       NetState = NETLOOP_FAIL;
> +       net_set_state(NETLOOP_FAIL);
>        return;
>  }
>
> @@ -76,7 +76,7 @@ SntpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
>                tm.tm_year, tm.tm_mon, tm.tm_mday,
>                tm.tm_hour, tm.tm_min, tm.tm_sec);
>
> -       NetState = NETLOOP_SUCCESS;
> +       net_set_state(NETLOOP_SUCCESS);
>  }
>
>  void
> diff --git a/net/tftp.c b/net/tftp.c
> index a04a832..bf32eab 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -177,7 +177,7 @@ store_block(unsigned block, uchar *src, unsigned len)
>                rc = flash_write((char *)src, (ulong)(load_addr+offset), len);
>                if (rc) {
>                        flash_perror(rc);
> -                       NetState = NETLOOP_FAIL;
> +                       net_set_state(NETLOOP_FAIL);
>                        return;
>                }
>        } else
> @@ -300,7 +300,7 @@ static void tftp_complete(void)
>        }
>  #endif
>        puts("\ndone\n");
> -       NetState = NETLOOP_SUCCESS;
> +       net_set_state(NETLOOP_SUCCESS);
>  }
>
>  static void
> @@ -627,7 +627,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
>                        if (MasterClient && (TftpBlock >= TftpEndingBlock)) {
>                                puts("\nMulticast tftp done\n");
>                                mcast_cleanup();
> -                               NetState = NETLOOP_SUCCESS;
> +                               net_set_state(NETLOOP_SUCCESS);
>                        }
>                } else
>  #endif
> @@ -644,7 +644,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
>                case TFTP_ERR_ACCESS_DENIED:
>                        puts("Not retrying...\n");
>                        eth_halt();
> -                       NetState = NETLOOP_FAIL;
> +                       net_set_state(NETLOOP_FAIL);
>                        break;
>                case TFTP_ERR_UNDEFINED:
>                case TFTP_ERR_DISK_FULL:
> --
> 1.6.0.2
>

Regards,
Simon


More information about the U-Boot mailing list