[U-Boot] [PATCH 1/2] [RFC]new command: wol - Wake on LAN
Joe Hershberger
joe.hershberger at ni.com
Thu May 3 00:07:08 UTC 2018
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 at 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 at 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 at 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 at 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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
More information about the U-Boot
mailing list