[U-Boot] [PATCH v2 09/18] net: Refactor to separate the UDP handler from the ARP handler

Simon Glass sjg at chromium.org
Thu Apr 12 06:22:11 CEST 2012


Hi Joe,

On Tue, Mar 27, 2012 at 4:42 PM, Joe Hershberger <joe.hershberger at ni.com> wrote:
> Call a built-in dummy if none is registered... don't require
> protocols to register a handler (eliminating dummies)
> NetConsole now uses the ARP handler when waiting on arp
> (instead of needing a #define hack in arp.c)
> Clear handlers at the end of net loop
>
> 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>

Couple of questions, but not important.

Acked-by:: Simon Glass <sjg at chromium.org>

> ---
> Changes for v2:
>   - Change NULL checks into dummy functions
>   - Must now call net_set_*_handler(NULL); in init to set dummies
>   - Eliminate CamelCase for new functions
>
>  drivers/net/netconsole.c |    4 +-
>  include/net.h            |    9 +++-
>  net/arp.c                |    6 +-
>  net/bootp.c              |    4 +-
>  net/cdp.c                |    8 ----
>  net/dns.c                |    2 +-
>  net/net.c                |   95 ++++++++++++++++++++++++++++++----------------
>  net/nfs.c                |    2 +-
>  net/sntp.c               |    2 +-
>  net/tftp.c               |    4 +-
>  10 files changed, 80 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 28bb955..744f4d1 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -63,12 +63,12 @@ void NcStart(void)
>  {
>        if (!output_packet_len || memcmp(nc_ether, NetEtherNullAddr, 6)) {
>                /* going to check for input packet */
> -               NetSetHandler(nc_handler);
> +               net_set_udp_handler(nc_handler);
>                NetSetTimeout(net_timeout, nc_timeout);
>        } else {
>                /* send arp request */
>                uchar *pkt;
> -               NetSetHandler(nc_wait_arp_handler);
> +               net_set_arp_handler(nc_wait_arp_handler);
>                pkt = (uchar *)NetTxPacket + NetEthHdrSize() + IP_UDP_HDR_SIZE;
>                memcpy(pkt, output_packet, output_packet_len);
>                NetSendUDPPacket(nc_ether, nc_ip, nc_port, nc_port,
> diff --git a/include/net.h b/include/net.h
> index c33cd28..796d59f 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -454,8 +454,10 @@ extern int NetCksumOk(uchar *, int);       /* Return true if cksum OK */
>  extern uint    NetCksum(uchar *, int);         /* Calculate the checksum */
>
>  /* Callbacks */
> -extern rxhand_f *NetGetHandler(void);          /* Get RX packet handler */
> -extern void    NetSetHandler(rxhand_f *);      /* Set RX packet handler */
> +extern rxhand_f *net_get_udp_handler(void);    /* Get UDP RX packet handler */
> +extern void net_set_udp_handler(rxhand_f *);   /* Set UDP RX packet handler */
> +extern rxhand_f *net_get_arp_handler(void);    /* Get ARP RX packet handler */
> +extern void net_set_arp_handler(rxhand_f *);   /* Set ARP 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 */
>
> @@ -475,7 +477,8 @@ static inline void NetSendPacket(uchar *pkt, int len)
>        (void) eth_send(pkt, len);
>  }
>
> -/* Transmit UDP packet, performing ARP request if needed */
> +/* Transmit UDP packet, performing ARP request if needed
> +   (ether will be populated) */

comment style

>  extern int     NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport,
>                        int sport, int payload_len);
>
> diff --git a/net/arp.c b/net/arp.c
> index 90a6b01..90bcc09 100644
> --- a/net/arp.c
> +++ b/net/arp.c
> @@ -192,9 +192,9 @@ void ArpReceive(struct Ethernet_hdr *et, struct IP_UDP_hdr *ip, int len)
>                        memcpy(NetArpWaitPacketMAC,
>                                &arp->ar_sha, ARP_HLEN);
>
> -#ifdef CONFIG_NETCONSOLE
> -                       NetGetHandler()(0, 0, 0, 0, 0);
> -#endif
> +                       net_get_arp_handler()((uchar *)arp, 0,
> +                               reply_ip_addr, 0, len);

I presume this is too long for one line?

> +
>                        /* modify header, and transmit it */
>                        memcpy(((struct Ethernet_hdr *)NetArpWaitTxPacket)->
>                                et_dest, NetArpWaitPacketMAC, ARP_HLEN);
> diff --git a/net/bootp.c b/net/bootp.c
> index 2eeb14e..0cf8beb 100644
> --- a/net/bootp.c
> +++ b/net/bootp.c
> @@ -671,9 +671,9 @@ BootpRequest(void)
>
>  #if defined(CONFIG_CMD_DHCP)
>        dhcp_state = SELECTING;
> -       NetSetHandler(DhcpHandler);
> +       net_set_udp_handler(DhcpHandler);
>  #else
> -       NetSetHandler(BootpHandler);
> +       net_set_udp_handler(BootpHandler);
>  #endif
>        NetSendPacket(NetTxPacket, pktlen);
>  }
> diff --git a/net/cdp.c b/net/cdp.c
> index f1c1c14..3b8a9a1 100644
> --- a/net/cdp.c
> +++ b/net/cdp.c
> @@ -237,13 +237,6 @@ CDPTimeout(void)
>                net_set_state(NETLOOP_SUCCESS);
>  }
>
> -static void
> -CDPDummyHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
> -               unsigned len)
> -{
> -       /* nothing */
> -}
> -
>  void
>  CDPReceive(const uchar *pkt, unsigned len)
>  {
> @@ -368,7 +361,6 @@ CDPStart(void)
>        CDPApplianceVLAN = htons(-1);
>
>        NetSetTimeout(CDP_TIMEOUT, CDPTimeout);
> -       NetSetHandler(CDPDummyHandler);
>
>        CDPSendTrigger();
>  }
> diff --git a/net/dns.c b/net/dns.c
> index cc0aa0a..ff9ddff 100644
> --- a/net/dns.c
> +++ b/net/dns.c
> @@ -200,7 +200,7 @@ DnsStart(void)
>        debug("%s\n", __func__);
>
>        NetSetTimeout(DNS_TIMEOUT, DnsTimeout);
> -       NetSetHandler(DnsHandler);
> +       net_set_udp_handler(DnsHandler);
>
>        DnsSend();
>  }
> diff --git a/net/net.c b/net/net.c
> index 8076e5f..cac9406 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -178,10 +178,13 @@ uchar PktBuf[(PKTBUFSRX+1) * PKTSIZE_ALIGN + PKTALIGN];
>  /* Receive packet */
>  uchar *NetRxPackets[PKTBUFSRX];
>
> -/* Current RX packet handler */
> -static rxhand_f *packetHandler;
> +/* Current UDP RX packet handler */
> +static rxhand_f *udp_packet_handler;
> +/* Current ARP RX packet handler */
> +static rxhand_f *arp_packet_handler;
>  #ifdef CONFIG_CMD_TFTPPUT
> -static rxhand_icmp_f *packet_icmp_handler;     /* Current ICMP rx handler */
> +/* Current ICMP rx handler */
> +static rxhand_icmp_f *packet_icmp_handler;
>  #endif
>  /* Current timeout handler */
>  static thand_f *timeHandler;
> @@ -252,6 +255,18 @@ static void NetInitLoop(enum proto_t protocol)
>        return;
>  }
>
> +static void net_clear_handlers(void)
> +{
> +       net_set_udp_handler(NULL);
> +       net_set_arp_handler(NULL);
> +       NetSetTimeout(0, NULL);
> +}
> +
> +static void net_cleanup_loop(void)
> +{
> +       net_clear_handlers();

If this is static anyway, and net_clear_handlers() also clear the
timeout, why not just put the three lines of code in here instead of a
separate function?

> +}
> +
>  /**********************************************************************/
>  /*
>  *     Main network processing loop.
> @@ -259,6 +274,7 @@ static void NetInitLoop(enum proto_t protocol)
>
>  int NetLoop(enum proto_t protocol)
>  {
> +       int     i;
>        bd_t *bd = gd->bd;
>        int ret = -1;
>
> @@ -269,17 +285,15 @@ int NetLoop(enum proto_t protocol)
>        NetTryCount = 1;
>
>        ArpInit();
> +       net_clear_handlers();
>
> -       if (!NetTxPacket) {
> -               int     i;
> -               /*
> -                *      Setup packet buffers, aligned correctly.
> -                */
> -               NetTxPacket = &PktBuf[0] + (PKTALIGN - 1);
> -               NetTxPacket -= (ulong)NetTxPacket % PKTALIGN;
> -               for (i = 0; i < PKTBUFSRX; i++)
> -                       NetRxPackets[i] = NetTxPacket + (i+1)*PKTSIZE_ALIGN;
> -       }
> +       /*
> +        *      Setup packet buffers, aligned correctly.
> +        */
> +       NetTxPacket = &PktBuf[0] + (PKTALIGN - 1);
> +       NetTxPacket -= (ulong)NetTxPacket % PKTALIGN;
> +       for (i = 0; i < PKTBUFSRX; i++)
> +               NetRxPackets[i] = NetTxPacket + (i+1)*PKTSIZE_ALIGN;
>
>        bootstage_mark_name(BOOTSTAGE_ID_ETH_START, "eth_start");
>        eth_halt();
> @@ -418,6 +432,7 @@ restart:
>                 *      Abort if ctrl-c was pressed.
>                 */
>                if (ctrlc()) {
> +                       net_cleanup_loop();
>                        eth_halt();
>                        puts("\nAbort\n");
>                        goto done;
> @@ -460,6 +475,7 @@ restart:
>                        goto restart;
>
>                case NETLOOP_SUCCESS:
> +                       net_cleanup_loop();
>                        if (NetBootFileXferSize > 0) {
>                                char buf[20];
>                                printf("Bytes transferred = %ld (%lx hex)\n",
> @@ -476,6 +492,7 @@ restart:
>                        goto done;
>
>                case NETLOOP_FAIL:
> +                       net_cleanup_loop();
>                        goto done;
>
>                case NETLOOP_CONTINUE:
> @@ -486,7 +503,7 @@ restart:
>  done:
>  #ifdef CONFIG_CMD_TFTPPUT
>        /* Clear out the handlers */
> -       NetSetHandler(NULL);
> +       net_set_upd_handler(NULL);
>        net_set_icmp_handler(NULL);
>  #endif
>        return ret;
> @@ -500,13 +517,6 @@ startAgainTimeout(void)
>        net_set_state(NETLOOP_RESTART);
>  }
>
> -static void
> -startAgainHandler(uchar *pkt, unsigned dest, IPaddr_t sip,
> -                 unsigned src, unsigned len)
> -{
> -       /* Totally ignore the packet */
> -}
> -
>  void NetStartAgain(void)
>  {
>        char *nretry;
> @@ -543,7 +553,7 @@ void NetStartAgain(void)
>                NetRestartWrap = 0;
>                if (NetDevExists) {
>                        NetSetTimeout(10000UL, startAgainTimeout);
> -                       NetSetHandler(startAgainHandler);
> +                       net_set_udp_handler(NULL);
>                } else {
>                        net_set_state(NETLOOP_FAIL);
>                }
> @@ -557,17 +567,36 @@ void NetStartAgain(void)
>  *     Miscelaneous bits.
>  */
>
> -rxhand_f *
> -NetGetHandler(void)
> +static void dummy_handler(uchar *pkt, unsigned dport,
> +                       IPaddr_t sip, unsigned sport,
> +                       unsigned len)
>  {
> -       return packetHandler;
>  }
>
> +rxhand_f *net_get_udp_handler(void)
> +{
> +       return udp_packet_handler;
> +}
>
> -void
> -NetSetHandler(rxhand_f *f)
> +void net_set_udp_handler(rxhand_f *f)
> +{
> +       if (f == NULL)
> +               udp_packet_handler = dummy_handler;
> +       else
> +               udp_packet_handler = f;
> +}
> +
> +rxhand_f *net_get_arp_handler(void)
>  {
> -       packetHandler = f;
> +       return arp_packet_handler;
> +}
> +
> +void net_set_arp_handler(rxhand_f *f)
> +{
> +       if (f == NULL)
> +               arp_packet_handler = dummy_handler;
> +       else
> +               arp_packet_handler = f;
>  }
>
>  #ifdef CONFIG_CMD_TFTPPUT
> @@ -1094,11 +1123,11 @@ NetReceive(uchar *inpkt, int len)
>                /*
>                 *      IP header OK.  Pass the packet to the current handler.
>                 */
> -               (*packetHandler)((uchar *)ip + IP_UDP_HDR_SIZE,
> -                                       ntohs(ip->udp_dst),
> -                                       src_ip,
> -                                       ntohs(ip->udp_src),
> -                                       ntohs(ip->udp_len) - UDP_HDR_SIZE);
> +               (*udp_packet_handler)((uchar *)ip + IP_UDP_HDR_SIZE,
> +                               ntohs(ip->udp_dst),
> +                               src_ip,
> +                               ntohs(ip->udp_src),
> +                               ntohs(ip->udp_len) - UDP_HDR_SIZE);
>                break;
>        }
>  }
> diff --git a/net/nfs.c b/net/nfs.c
> index 4f2041c..acae4de 100644
> --- a/net/nfs.c
> +++ b/net/nfs.c
> @@ -735,7 +735,7 @@ NfsStart(void)
>                "Loading: *\b", load_addr);
>
>        NetSetTimeout(NFS_TIMEOUT, NfsTimeout);
> -       NetSetHandler(NfsHandler);
> +       net_set_udp_handler(NfsHandler);
>
>        NfsTimeoutCount = 0;
>        NfsState = STATE_PRCLOOKUP_PROG_MOUNT_REQ;
> diff --git a/net/sntp.c b/net/sntp.c
> index 2adcc6f..73ade8b 100644
> --- a/net/sntp.c
> +++ b/net/sntp.c
> @@ -85,7 +85,7 @@ SntpStart(void)
>        debug("%s\n", __func__);
>
>        NetSetTimeout(SNTP_TIMEOUT, SntpTimeout);
> -       NetSetHandler(SntpHandler);
> +       net_set_udp_handler(SntpHandler);
>        memset(NetServerEther, 0, 6);
>
>        SntpSend();
> diff --git a/net/tftp.c b/net/tftp.c
> index bf32eab..b2e08b4 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -778,7 +778,7 @@ void TftpStart(enum proto_t protocol)
>        TftpTimeoutCountMax = TftpRRQTimeoutCountMax;
>
>        NetSetTimeout(TftpTimeoutMSecs, TftpTimeout);
> -       NetSetHandler(TftpHandler);
> +       net_set_udp_handler(TftpHandler);
>  #ifdef CONFIG_CMD_TFTPPUT
>        net_set_icmp_handler(icmp_handler);
>  #endif
> @@ -840,7 +840,7 @@ TftpStartServer(void)
>  #endif
>
>        TftpState = STATE_RECV_WRQ;
> -       NetSetHandler(TftpHandler);
> +       net_set_udp_handler(TftpHandler);
>  }
>  #endif /* CONFIG_CMD_TFTPSRV */
>
> --
> 1.6.0.2
>

Regards,
Simon


More information about the U-Boot mailing list