Andre Przywara <andre.przywara@arm.com> says:
C's implicit fallthrough behaviour in switch/case statements can lead to
subtle bugs. Quite some while ago many compilers introduced warnings in
those cases, requiring intentional fallthrough's to be annotated.
So far we were not enabling that compiler option, so many ambiguities
and some bugs in the code went unnoticed.
This series adds the required annotations in code paths that the first
stage of the U-Boot CI covers. There is a large number of cases left
in the libbz2 code. The usage of switch/case is borderline insane there,
labels are hidden in macros, and there are no breaks, but just goto's.
Upstream still uses very similar code, without any annotations. I still
am not 100% sure those are meant to fall through or not, and plan to do
further investigations, but didn't want to hold the rest of the patches
back. You can see for yourself by applying patch 18/18 and building for
sandbox64, for instance.
Because of this we cannot quite enable the warning in the Makefile yet,
but those fixes are worth regardless, and be it to increase readability.
Please note that those patches do not fix anything, really, they just add
those fallthrough annotations, so the series is not really critical.
Link: https://lore.kernel.org/r/20250327153313.2105227-1-andre.przywara@arm.com
In some cases in the generic code, we were already using switch/case
fallthrough annotations comments, though in a way which might not be
understood by most compilers.
Replace two non-standard /* no break */ comments with our fallthrough;
statement-like macro, to make this visible to the compiler.
Also use this macro in place of an /* Fall through */ comment, to be
more consistent.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Tom Rini <trini@konsulko.com>
Note that this undoes the changes of commit cf6d4535cc ("x86:
emulation: Disable bloblist for now") as that was intended only for the
release due to time.
When an ethernet driver fails to send a frame we print an error in lwIP.
But depending on how often that error is it might significantly delay
transmissions.
For example downloading a big file with the rpi4 spams the console with
'send error: -101', but removing the print makes the file download with
an average speed of ~8.5MiB/s since the packets are retransmitted.
So let's move it to a 'debug' in lwIP and expect ethernet drivers to handle
the failure if it's severe.
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
do_ping() expects ping_loop() to return a negative value on error, so
that it can propagate it to the caller as CMD_RET_FAILURE. This is not
the case when no ethernet device is found, so fix that.
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Simon Glass <sjg@chromium.org> says:
This series includes some patches related to allowing read_all() to be
used with the extlinux / PXE bootmeths.
These patches were split out from the stb4 series, since it will need to
have additional patches for LWIP, to avoid breaking PXE booting when
LWIP is used.
Link: https://lore.kernel.org/r/20250306002533.2380866-1-sjg@chromium.org
Add a new netboot_run() function which can be used for simple network
operations, such as loading a file. Put the implementation in an
internal function, used by the existing code.
Place this function into the net/ code, so that it does not need the
command line to be available.
Document which network operations are supported, i.e. a limited subset,
for now.
For the one board which uses lwip, it is not quite clear how to avoid
using the cmdline interface. This will need some discussion.
Signed-off-by: Simon Glass <sjg@chromium.org>
Add Kconfig symbol LWIP_DEBUG_RXTX to dump the incoming and outgoing
packets when NET_LWIP=y.
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Rename static function linkoutput() as net_lwip_tx() for consistency
with net_lwip_rx().
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
When enabling net console and console multiplexing, a boot crash was
observed using mtk_eth driver with stdin/stdout set to "serial,nc"
in persistent environment:
> CPU: MediaTek MT7981
> Model: OpenWrt One
> DRAM: 1 GiB
> Core: 35 devices, 15 uclasses, devicetree: separate
> spi-nand: spi_nand spi_nand@0: Winbond SPI NAND was found.
> spi-nand: spi_nand spi_nand@0: 128 MiB, block size: 128 KiB, page size: 2048, OOB size: 64
> Loading Environment from UBI... SF: Detected w25q128 with page size 256 Bytes, erase size 4 KiB, total 16 MiB
> mtd: partition "ubi" extends beyond the end of device "spi-nand0" -- size truncated to 0x7f00000
> Read 126976 bytes from volume ubootenv to 000000007f7bf0c0
> Read 126976 bytes from volume ubootenv2 to 000000007f7de100
> OK
> "Synchronous Abort" handler, esr 0x96000004, far 0xeafffffeea000018
> elr: 0000000041e63cd4 lr : 0000000041e1b844 (reloc)
> elr: 000000007ff9ecd4 lr : 000000007ff56844
> x0 : eafffffeea000018 x1 : 000000007fb552e0
> x2 : 00000000000000fe x3 : 0000000000000000
The cause is that "serial,nc" forced the console subsystem to
initialize the ethernet driver before ethernet subsystem
initialization (console_init_r() is called before initr_net()).
During the mtk_eth driver initialization, mdio_register() will be
called, and miiphy_get_dev_by_name() will then be called.
The miiphy_get_dev_by_name() will check the list "mii_devs" to see
if the passed device name exists. However the mii_devs is defined
without initialization:
> static struct list_head mii_devs;
and the actual initialization is done in the following chain:
initr_net -> eth_initialize -> eth_common_init -> miiphy_init
Since initr_net() hasn't be called, iterating over the mii_devs
will access to physical address 0 (mii_devs.next == NULL) and will
cause the crash.
The fix is to define mii_devs using:
> static LIST_HEAD(mii_devs);
As the "current_mii" is defined as a static variable, it will
always be NULL in board_r stage and initializing it will NULL is
unnecessary. So the entire miiphy_init() can be remove.
Signed-off-by: Weijie Gao <hackpascal@gmail.com>
Check wget_info->buffer_size for overflow and do not clean the wget_info struct
on failure, let the owner of the struct handle the error. The latter is necesary
, e.g., for when a request fails because the provided buffer was too small.
Signed-off-by: Adriano Cordova <adriano.cordova@canonical.com>
net/lwip/wget.c/mbedtls_hardware_poll() is calling dm_rng_read() but
dependency is not recorded anywhere that's why depend on DM_RNG
when WGET_HTTPS is used.
Suggested-by: Michal Simek <michal.simek@amd.com>
Co-developed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Tested-by: Michal Simek <michal.simek@amd.com>
Move the initialization of the ethernet devices out of the new_netif()
function. Indeed, new_netif() accepts a struct device argument, which
is expected to be valid and active. The activation and selection of
this device are achieved by eth_init() (on first time the network
stack is used) and eth_set_current(). This is what takes care of the
ethrotate and ethact environment variables. Therefore, move these calls
to a new function: net_lwip_set_current(), and use it whenever a
net-lwip command is run.
This patch hopefully fixes the incorrect net-lwip behavior observed on
boards with multiple ethernet interfaces [1].
Tested on an i.MX8MPlus EVK equipped wih two ethernet ports. The dhcp
command succeeds whether the cable is plugged into the first or second
port.
[1] https://lists.denx.de/pipermail/u-boot/2025-January/576326.html
Reported-by: E Shattow <e@freeshell.de>
Tested-by: E Shattow <e@freeshell.de>
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Currently when booting dhcp_run() may be executed multiple times:
once in eth_bootdev_hunt() and once in the network booting bootmeth.
We need to call eth_bootdev_hunt() when setting up the EFI sub-system to
supply the simple network protocol. We don't need an IP address set up.
We can reduce the bootime by not executing dhcp_run() in
eth_bootdev_hunt().
Furthermore eth_bootdev_hunt() with autostart=yes leads on the legacy
network stack leads to downloading a file via TFTP and to booting the
downloaded file.
Instead of running dchp_run() just check that there is a network device
in eth_bootdev_hunt().
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
When a successful neighbor advertisement is received, the ethernet
address should be saved for later use to avoid having to redo the
neighbor discovery process.
For example, with TFTP the address should get saved into
"net_server_ethaddr". This is being done correctly with ARP for IPv4,
but not for neighbor discovery with IPv6.
Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
Reviewed-by: Viacheslav Mitrofanov <v.v.mitrofanov@yadro.com>
timeout_count is never reset once a tftpput transfer has started. If for
whatever reason timeouts occur frequently, but the server keeps replying
nonetheless, the transfer may be needlessly aborted.
Reset timer_count on reception of an ACK to avoid this situation.
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
We have a some boards that rarely starts networking abnormally, so there are
many timeouts during file transfer. In the same time there is a normal transfer
between timeouts. In this case we can continue transfer (instead of connection
aborting) by just clearing timeout counter on every successful block.
This patch does not affect the case when several timeouts happen one after
another. The transfer will be aborted. Thus the transfer will be
continued in the case of unstable link, but will be aborted in the case
of inaccessible server.
Feature downside: it may greatly slowdown (instead of abort) file transfer
in the case of unstable link.
Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
Some driver implements it's own network packet pool, so PKTBUFSRX is zero.
This results in zero-size TCP receive window, so data transfer doesn't
work. Avoid it by setting a reasonable fallback value.
Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
Reviewed-by: Simon Glass <sjg@chromium.org>
This patch:
* remove useless code,
* use a special function for pretty printing of tcp flags,
* simplify the code
The behavior should not be changed.
Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
Reviewed-by: Simon Glass <sjg@chromium.org>
Changes:
* Fix initial send sequence always zero issue
* Use state machine close to RFC 9293. This should make TCP
transfers more reliable (now we can upload a huge array
of data from the board to external server)
* Improve TCP framework a lot. This should make tcp client
code much more simple.
* rewrite wget with new tcp stack
* rewrite fastboot_tcp with new tcp stack
It's quite hard to fix the initial send sequence (ISS) issue
with the separate patch. A naive attempt to fix an issue
inside the tcp_set_tcp_header() function will break tcp packet
retransmit logic in wget and other clients.
Example:
Wget stores tcp_seq_num value before tcp_set_tcp_header() will
be called and (on failure) retransmit the packet with the stored
tcp_seq_num value. Thus:
* the same ISS must allways be used (current case)
* or tcp clients needs to generate a proper ISS when
required.
A proper ISS fix will require a big redesing comparable with
a this one.
Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
Reviewed-by: Simon Glass <sjg@chromium.org>
Changes:
* Avoid use net_server_ip in tcp code, use tcp_stream data instead
* Ignore packets from other connections if connection already created.
This prevents us from connection break caused by other tcp stream.
Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
Reviewed-by: Simon Glass <sjg@chromium.org>
Current code assume that all (except last) packets are of the same size.
This is definitely wrong. Replace SACK code with a new one, that does
not rely on this assumption. Also this code uses less memory.
Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
Reviewed-by: Simon Glass <sjg@chromium.org>
Current TCP code may miss an option if TCP_O_NOP option was used before
it for proper aligning.
Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
Reviewed-by: Simon Glass <sjg@chromium.org>
TFTP transfer size can be used to re-size the TFTP progress bar on
single line based on the server reported file size. Enable it by
default for Renesas hardware to avoid long scrolling walls of '#'
character during long TFTP transfers.
Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
eth_get_dev() returns NULL if no network device is available.
Not checking the return value leads to a crash when the device
pointer is dereferenced.
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
If the dns command cannot find a network interface, we should return
CMD_RETFAIURE and not -1 (CMD_RET_USAGE).
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Set the device path of the efi boot device to an HTTP device path
(as formed by efi_dp_from_http) when the next boot stage is loaded
using wget (i.e., when wget is used with wget_info.set_bootdev=1).
When loaded from HTTP, the device path should account for it so that
the next boot stage is aware (e.g. grub only loads its http stack if
it itself was loaded from http, and it checks this from its device path).
Signed-off-by: Adriano Cordova <adrianox@gmail.com>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
This was marked as TODO in the code:
- Enable use of wget_with_dns even if CMD_DNS is disabled if
the given uri has the ip address for the http server.
- Move the check for CMD_DNS inside wget_with_dns.
- Rename wget_with_dns to wget_do_request
Signed-off-by: Adriano Cordova <adrianox@gmail.com>
Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
This patch comes as a companion to the same patch but for the legacy
net stack. Commit 1327c2a8d6 ("net/lwip: wget: integrate struct wget_info
into wget code") introduced function wget_fill_info() which retrieves
the headers from the HTTP server response. As we want to parse the
string in later patches we need to ensure that it is NUL terminated.
We must further check that wget_info->headers in not NULL.
Otherwise a crash occurs.
Signed-off-by: Adriano Cordova <adrianox@gmail.com>
Commit 2dd076a9c1 ("net: wget: integrate struct wget_info into legacy
wget code") introduced function wget_fill_info() which retrieves the
headers from the HTTP server response. As we want to parse the string in
later patches we need to ensure that it is NUL terminated.
We must further check that wget_info->headers in not NULL.
Otherwise a crash occurs.
Fixes: 2dd076a9c1 ("net: wget: integrate struct wget_info into legacy wget code")
Signed-off-by: Adriano Cordova <adrianox@gmail.com>
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Commit 5907c81 ("net: lwip: Enable https:// support for wget") was not
correctly rebased on top of the changes introduced by Commit 6cc4d04
("net/lwip: wget: put server_name and port into wget_ctx") in next.
This commit re-applies a couple of lines from 6cc4d04.
Fixes: Commit 5907c81 ("net: lwip: Enable https:// support for wget")
Signed-off-by: Adriano Cordova <adrianox@gmail.com>
Commit 356011f7ac ("lwip: fix code style issues") has inadvertently
broken the support for the legacy syntax:
=> wget 192.168.0.16:test.bin
invalid uri, no file path
Invalid URL. Use http(s)://
The reason is two calls to strncpy() were replaced by strlcpy() without
paying attention to the fact that they are not equivalent in the present
case. Since we are using a character counter (n) and since we do not
depend on having a properly null-terminated string at each step in the
parsing, strlcpy() is not justified and strncpy() is the right tool for
the job. So use it again.
Reported-by: Tom Rini <trini@konsulko.com>
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
The dhcp command is supposed to have the following syntax as per
"help dhcp":
dhcp [loadAddress] [[hostIPaddr:]bootfilename]
In other words, any arguments should be passed to an implicit
tftpboot command after the DHCP exchange has occurred.
Add the missing code to the lwIP version of do_dhcp().
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
We currently provide entropy to mbedTLS using 8b chunks.
Take into account the 'len' parameter passed by MBed TLS to the entropy
gathering function instead. Note that the current code works because len
is always 128 (defined at compile time), therefore mbedtls_hardware_poll()
is called repeatedly and the buffer is filled correctly. But passing 'len'
to dm_rng_read() is both better and simpler.
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Suggested-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
The local variables ipstr, maskstr and gwstr in static function
get_udev_ipv4_info() cannot be pointers to read-only data, since
they may be written to in case the device index is > 0. Therefore
make them char arrays allocated on the stack.
Reported-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reported-by: Adriano Cordova <adrianox@gmail.com>
Link: https://lists.denx.de/pipermail/u-boot/2024-November/572066.html
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
The local variables ipstr, maskstr and gwstr in static function
dhcp_loop() cannot be pointers to read-only data, since they may be
written to in case the device index is > 0. Therefore make them char
arrays allocated on the stack.
Reported-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Some Kconfig symbols introduced in commit 8cb330355b ("net: introduce
alternative implementation as net/lwip/") need a full description. The
NET symbol needs one, too.
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
The maximum length of a domain name is 253 as defined in RFC 1035.
So SERVER_NAME_SIZE should be 254 including NUL.
Fixes: 3c656c928b ("net: lwip: add wget command")
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
The function string_to_ip is already in net_utils, which is
compiled unconditionally, but ip_to_string is currently only
accessible if the legacy network stack is selected. This
commit puts ip_to_string in net_utils.c and removes it from the
legacy network code.
Signed-off-by: Adriano Cordova <adrianox@gmail.com>
Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Each wget request now fills the struct wget_info. Also, the
efi bootdevice is now set conditionally to the set_bootdevice
variable in wget_info and a buffer size check is performed if
check_buffer_size is set.
Signed-off-by: Adriano Cordova <adrianox@gmail.com>
Currently server_name and port are local variables in wget_loop.
This commit puts them inside ctx, so that they are accessible
from the http callbacks.
Signed-off-by: Adriano Cordova <adrianox@gmail.com>
