[U-Boot,1/2,RFC] new command: wol - Wake on LAN

This patch adds a new command 'wol': It waits for an incoming
Wake-on-LAN
packet or times out if no WoL packed is received.
If the WoL packet contains a password, it is saved in the environment
variable 'wolpassword' using the etherwake format (dot separated
decimals).

Intended use case: a networked device should boot an alternate image.
It's attached to a network on a client site, modifying the DHCP server
configuration or setup of a tftp server is not allowed.
After power on the device waits a few seconds for a WoL packet. If a
packet is received, the device boots the alternate image. Otherwise
it boots the default image.

This method is a simple way to interact with a system via network even
if only the MAC address is known. Tools to send WoL packets are
available on all common platforms.

Signed-off-by: Lothar Felten <lothar.felten@gmail.com>
---
 net/wol.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 net/wol.h | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+)
 create mode 100644 net/wol.c
 create mode 100644 net/wol.h

Hi Lothar,

On 23 April 2018 at 11:47, Lothar Felten <lothar.felten@gmail.com> wrote:
> This patch adds a new command 'wol': It waits for an incoming
> Wake-on-LAN
> packet or times out if no WoL packed is received.
> If the WoL packet contains a password, it is saved in the environment
> variable 'wolpassword' using the etherwake format (dot separated
> decimals).
>
> Intended use case: a networked device should boot an alternate image.
> It's attached to a network on a client site, modifying the DHCP server
> configuration or setup of a tftp server is not allowed.
> After power on the device waits a few seconds for a WoL packet. If a
> packet is received, the device boots the alternate image. Otherwise
> it boots the default image.
>
> This method is a simple way to interact with a system via network even
> if only the MAC address is known. Tools to send WoL packets are
> available on all common platforms.
>
> Signed-off-by: Lothar Felten <lothar.felten@gmail.com>
> ---
>  net/wol.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  net/wol.h | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+)
>  create mode 100644 net/wol.c
>  create mode 100644 net/wol.h
>
> diff --git a/net/wol.c b/net/wol.c
> new file mode 100644
> index 0000000000..0cedbaed85
> --- /dev/null
> +++ b/net/wol.c
> @@ -0,0 +1,54 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2018 Lothar Felten, lothar.felten@gmail.com
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <net.h>
> +#include "wol.h"
> +
> +/*
> + *     Handle a received wake-on-lan packet.
> + */
> +void wol_receive(struct ip_udp_hdr *ip, unsigned int len)
> +{
> +       char buf[4];
> +       int i;
> +       struct wol_hdr *wol;
> +
> +       wol = (struct wol_hdr *)ip;
> +
> +       if (len < 102)
> +               return;
> +
> +       for (i = 0; i < 6; i++)
> +               if (wol->wol_sync[i] != 0xff)
> +                       return;
> +
> +       for (i = 0; i < 16; i++)
> +               if (memcmp(&wol->wol_dest[i * 6], net_ethaddr, 6) != 0)
> +                       return;
> +
> +       /* save the optional password using the etherwake format */
> +       if (len >= 106) {

Is it possible to remove these four open-coded values and use values
from the header file instead?

> +               sprintf(buf, "%i.%i.%i.%i",
> +                       wol->wol_passwd[0], wol->wol_passwd[1],
> +                       wol->wol_passwd[2], wol->wol_passwd[3]);
> +               env_set("wolpassword", buf);
> +       }
> +       net_set_state(NETLOOP_SUCCESS);
> +}
> +
> +static void wol_timeout_handler(void)
> +{
> +       eth_halt();
> +       net_set_state(NETLOOP_FAIL);
> +}
> +
> +void wol_start(void)
> +{
> +       ulong timeout = env_get_ulong("woltimeout", 10, 5);
> +
> +       net_set_timeout_handler(timeout * 1000, wol_timeout_handler);
> +}
> diff --git a/net/wol.h b/net/wol.h
> new file mode 100644
> index 0000000000..e34767c733
> --- /dev/null
> +++ b/net/wol.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2018 Lothar Felten, lothar.felten@gmail.com
> + */
> +
> +#if defined(CONFIG_CMD_WOL)
> +
> +#ifndef __WOL_H__
> +#define __WOL_H__
> +
> +#include <net.h>
> +
> +/**********************************************************************/
> +
> +/*
> + *     Wake-on-LAN header.
> + */
> +struct wol_hdr {
> +       u8      wol_sync[6];            /* sync bytes                   */
> +       u8      wol_dest[16 * 6];       /* 16x destination MAC address  */
> +       u8      wol_passwd[4];          /* optional password            */
> +};
> +
> +/*
> + * Initialize wol (beginning of netloop)
> + */
> +void wol_start(void);
> +
> +/*
> + * Deal with the receipt of a wol packet
> + *
> + * @param ip IP header in the packet
> + * @param len Packet length
> + */
> +void wol_receive(struct ip_udp_hdr *ip, unsigned int len);

It seems like this should return an error code if the packet does not
match as expected (the 'return' in your impl)

Also you could expand a bit on what 'deal with' means. What exactly happens?

Seems like there should be a README addition somewhere too.

> +
> +/**********************************************************************/
> +
> +#endif /* __WOL_H__ */
> +#endif
> --
> 2.14.1
>

Regards,
Simon

Hi Lothar,

I generally like this. Thanks!

Please use the "net: " prefix to the subject of your patches. Also
follow http://www.denx.de/wiki/U-Boot/Patches - Commit message
conventions

"
Use the imperative tense in your summary line (e.g., "Add support for
X" rather than "Adds support for X"). In general, you can think of the
summary line as "this commit is meant to 'Add support for X'"
"

I'm not seeing any value in the way you've split up these two patches.
It seems like they should be squashed into 1. I've commented in
context in each patch.

On Mon, Apr 23, 2018 at 12:47 PM, Lothar Felten <lothar.felten@gmail.com> wrote:
> This patch adds a new command 'wol': It waits for an incoming
> Wake-on-LAN
> packet or times out if no WoL packed is received.
> If the WoL packet contains a password, it is saved in the environment
> variable 'wolpassword' using the etherwake format (dot separated
> decimals).
>
> Intended use case: a networked device should boot an alternate image.
> It's attached to a network on a client site, modifying the DHCP server
> configuration or setup of a tftp server is not allowed.
> After power on the device waits a few seconds for a WoL packet. If a
> packet is received, the device boots the alternate image. Otherwise
> it boots the default image.
>
> This method is a simple way to interact with a system via network even
> if only the MAC address is known. Tools to send WoL packets are
> available on all common platforms.

This seems pretty good.

Please enable this on some board so that this is build-tested and
provides others an example of how it can be used.

>
> Signed-off-by: Lothar Felten <lothar.felten@gmail.com>
> ---
>  net/wol.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  net/wol.h | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+)
>  create mode 100644 net/wol.c
>  create mode 100644 net/wol.h
>
> diff --git a/net/wol.c b/net/wol.c
> new file mode 100644
> index 0000000000..0cedbaed85
> --- /dev/null
> +++ b/net/wol.c
> @@ -0,0 +1,54 @@
> +// SPDX-License-Identifier: GPL-2.0+

Please use C-style comment.

> +/*
> + * Copyright 2018 Lothar Felten, lothar.felten@gmail.com
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <net.h>
> +#include "wol.h"
> +
> +/*
> + *     Handle a received wake-on-lan packet.
> + */
> +void wol_receive(struct ip_udp_hdr *ip, unsigned int len)
> +{
> +       char buf[4];

This is broken. You write at least 8 bytes to this buffer and at most
16 bytes. Switching to ip_to_string() below will avoid this bug.

> +       int i;
> +       struct wol_hdr *wol;
> +
> +       wol = (struct wol_hdr *)ip;
> +
> +       if (len < 102)

Use sizeof(struct wol_hdr)

> +               return;
> +
> +       for (i = 0; i < 6; i++)

Use #defines throughout.

> +               if (wol->wol_sync[i] != 0xff)
> +                       return;
> +
> +       for (i = 0; i < 16; i++)
> +               if (memcmp(&wol->wol_dest[i * 6], net_ethaddr, 6) != 0)
> +                       return;
> +
> +       /* save the optional password using the etherwake format */

etherwake -> ether-wake

> +       if (len >= 106) {

The password is supposed to be 0, 4, or 6 bytes. You should check the
actual length of the packet (instead of just "greater").

That would be "sizeof(struct wol_hdr) + ARP_PLEN" and "sizeof(struct
wol_hdr) + ARP_HLEN"

> +               sprintf(buf, "%i.%i.%i.%i",
> +                       wol->wol_passwd[0], wol->wol_passwd[1],
> +                       wol->wol_passwd[2], wol->wol_passwd[3]);

This sprintf should be replaced with ip_to_string().

> +               env_set("wolpassword", buf);

You should encode the password as a MAC address if it is 6 bytes. For
that you should use eth_env_set_enetaddr() instead of this.

> +       }
> +       net_set_state(NETLOOP_SUCCESS);
> +}
> +
> +static void wol_timeout_handler(void)
> +{
> +       eth_halt();
> +       net_set_state(NETLOOP_FAIL);
> +}
> +
> +void wol_start(void)
> +{
> +       ulong timeout = env_get_ulong("woltimeout", 10, 5);

I think it would be nicer if this were a parameter to the wol command
(as you have documented in the help for the command, but it doesn't
actually do anything as is).

This function should also call net_set_udp_handler() for the UDP case.
You'll probably need a static common function that does the meat of
receive, and then a stub for UDP and a stub for ether-wake.

> +
> +       net_set_timeout_handler(timeout * 1000, wol_timeout_handler);
> +}
> diff --git a/net/wol.h b/net/wol.h
> new file mode 100644
> index 0000000000..e34767c733
> --- /dev/null
> +++ b/net/wol.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2018 Lothar Felten, lothar.felten@gmail.com
> + */
> +
> +#if defined(CONFIG_CMD_WOL)
> +
> +#ifndef __WOL_H__
> +#define __WOL_H__
> +
> +#include <net.h>
> +
> +/**********************************************************************/
> +
> +/*
> + *     Wake-on-LAN header.
> + */
> +struct wol_hdr {
> +       u8      wol_sync[6];            /* sync bytes                   */

Make a #define for this sync size so that you can use it above in the
implementation.

> +       u8      wol_dest[16 * 6];       /* 16x destination MAC address  */

Make a #define for the "16" that you can use in the implementation
above and use ARP_HLEN for the MAC length.

> +       u8      wol_passwd[4];          /* optional password            */

Make this size 0 so you can get the size of this structure without a
password. You only ever use a pointer to this structure anyway.

> +};
> +
> +/*
> + * Initialize wol (beginning of netloop)
> + */
> +void wol_start(void);
> +
> +/*
> + * Deal with the receipt of a wol packet
> + *
> + * @param ip IP header in the packet
> + * @param len Packet length
> + */
> +void wol_receive(struct ip_udp_hdr *ip, unsigned int len);

You should add a function here that you can use to set the timeout.
Then in the net/wol.c you can implement it and write it to a static
global variable.

> +
> +/**********************************************************************/
> +
> +#endif /* __WOL_H__ */
> +#endif
> --
> 2.14.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot