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

On Mon, 31 Aug 2020 at 11:27, Philippe Reynes philippe.reynes@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@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@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@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@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