[PATCH v2 1/2] net: add a generic udp protocol

[Simon Glass]

On Mon, 31 Aug 2020 at 11:27, Philippe Reynes
<philippe.reynes at softathome.com> wrote:
>
> This commit adds a generic udp protocol framework in the
> network loop. So protocol based on udp may be implemented
> without modifying the network loop (for example custom
> wait magic packet).
>
> Signed-off-by: Philippe Reynes <philippe.reynes at softathome.com>
> ---
>
> Changelog:
> v2:
> - no change
>
>  include/net.h     |  2 +-
>  include/net/udp.h | 31 +++++++++++++++++++++++++++++++
>  net/Kconfig       |  6 ++++++
>  net/Makefile      |  1 +
>  net/net.c         | 15 ++++++++++++++-
>  net/udp.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 101 insertions(+), 2 deletions(-)
>  create mode 100644 include/net/udp.h
>  create mode 100644 net/udp.c
>

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

Needs a mention in docs somewhere.

nits below

> diff --git a/include/net.h b/include/net.h
> index 1bf9867..2191071 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -551,7 +551,7 @@ extern int          net_restart_wrap;       /* Tried all network devices */
>
>  enum proto_t {
>         BOOTP, RARP, ARP, TFTPGET, DHCP, PING, DNS, NFS, CDP, NETCONS, SNTP,
> -       TFTPSRV, TFTPPUT, LINKLOCAL, FASTBOOT, WOL
> +       TFTPSRV, TFTPPUT, LINKLOCAL, FASTBOOT, WOL, UDP
>  };
>
>  extern char    net_boot_file_name[1024];/* Boot File name */
> diff --git a/include/net/udp.h b/include/net/udp.h
> new file mode 100644
> index 0000000..f97df8d
> --- /dev/null
> +++ b/include/net/udp.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2020 Philippe Reynes <philippe.reynes at softathome.com>
> + */
> +
> +#ifndef __UDP
> +#define __UDP
> +
> +/**
> + * struct udp_ops - function to handle udp packet
> + *
> + * This structure provides the function to handle udp packet in
> + * the network loop.
> + *
> + * @prereq: callback called to check the requirement
> + * @start: callback called to start the protocol/feature
> + * @data: pointer to store private data (used by prereq and start)
> + */
> +struct udp_ops {
> +       int (*prereq)(void *data);
> +       int (*start)(void *data);
> +       void *data;
> +};
> +
> +int udp_prereq(void);
> +
> +int udp_start(void);
> +
> +int udp_loop(struct udp_ops *h);

Need function comments for these. Also how about 'ops' instead of 'h'?

> +
> +#endif
> diff --git a/net/Kconfig b/net/Kconfig
> index 6874b55..db8d796 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -8,6 +8,12 @@ menuconfig NET
>
>  if NET
>
> +config PROT_UDP
> +       bool "Enable generic udp framework"
> +       help
> +         Enable a generic udp framework that allow to define custom

that allows defining a custom

> +         handler for udp protocol.
> +
>  config BOOTP_SEND_HOSTNAME
>         bool "Send hostname to DNS server"
>         help
> diff --git a/net/Makefile b/net/Makefile
> index fef71b9..76527f7 100644
> --- a/net/Makefile
> +++ b/net/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_CMD_SNTP) += sntp.o
>  obj-$(CONFIG_CMD_TFTPBOOT) += tftp.o
>  obj-$(CONFIG_UDP_FUNCTION_FASTBOOT)  += fastboot.o
>  obj-$(CONFIG_CMD_WOL)  += wol.o
> +obj-$(CONFIG_PROT_UDP) += udp.o
>
>  # Disable this warning as it is triggered by:
>  # sprintf(buf, index ? "foo%d" : "foo", index)
> diff --git a/net/net.c b/net/net.c
> index 28d9eeb..f6ae814 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -102,6 +102,7 @@
>  #if defined(CONFIG_CMD_PCAP)
>  #include <net/pcap.h>
>  #endif
> +#include <net/udp.h>
>  #if defined(CONFIG_LED_STATUS)
>  #include <miiphy.h>
>  #include <status_led.h>
> @@ -540,6 +541,11 @@ restart:
>                         wol_start();
>                         break;
>  #endif
> +#if defined(CONFIG_PROT_UDP)

I think you can get the same effect by putting if() after the case

> +               case UDP:
> +                       udp_start();
> +                       break;
> +#endif
>                 default:
>                         break;
>                 }
> @@ -1364,6 +1370,13 @@ static int net_check_prereq(enum proto_t protocol)
>                 }
>                 goto common;
>  #endif
> +#if defined(CONFIG_PROT_UDP)

Same here

> +       case UDP:
> +               if (udp_prereq())
> +                       return 1;
> +               goto common;
> +#endif
> +
>  #if defined(CONFIG_CMD_NFS)
>         case NFS:
>  #endif
> @@ -1375,7 +1388,7 @@ static int net_check_prereq(enum proto_t protocol)
>                         return 1;
>                 }
>  #if    defined(CONFIG_CMD_PING) || defined(CONFIG_CMD_SNTP) || \
> -       defined(CONFIG_CMD_DNS)
> +       defined(CONFIG_CMD_DNS) || defined(CONFIG_PROT_UDP)
>  common:
>  #endif
>                 /* Fall through */
> diff --git a/net/udp.c b/net/udp.c
> new file mode 100644
> index 0000000..2409ce4
> --- /dev/null
> +++ b/net/udp.c
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2020 Philippe Reynes <philippe.reynes at softathome.com>
> + */
> +
> +#include <common.h>
> +#include <net.h>
> +#include <net/udp.h>
> +
> +static struct udp_ops *udp_ops;
> +
> +int udp_prereq(void)
> +{
> +       int ret = 0;
> +
> +       if (udp_ops && udp_ops->prereq)

You already check udp_ops is non-NULL below.

> +               ret = udp_ops->prereq(udp_ops->data);
> +
> +       return ret;
> +}
> +
> +int udp_start(void)
> +{
> +       int ret = -1;
> +
> +       if (udp_ops && udp_ops->start)
> +               ret = udp_ops->start(udp_ops->data);
> +       else
> +               printf("%s: no start function defined\n", __func__);

Should return this error so the caller can print it.

> +
> +       return ret;
> +}
> +
> +int udp_loop(struct udp_ops *ops)
> +{
> +       int ret = -1;
> +
> +       if (!ops) {
> +               printf("%s: ops should not be null\n", __func__);
> +               goto out;
> +       }
> +
> +       udp_ops = ops;
> +       ret = net_loop(UDP);
> +
> + out:
> +       return ret;
> +}
> --
> 2.7.4
>

Regards,
Simon