[PATCH 2/2] usb:gadget: use helper function to queue composite requests
From: Enrico Scholz
Fixes bookkeeping in composite driver.
Signed-off-by: Enrico Scholz
---
drivers/usb/gadget/function/dfu.c | 2 +-
drivers/usb/gadget/function/f_acm.c | 4 +++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/dfu.c
b/drivers/usb/gadget/function/dfu.c
index 3a6d2cf385b1..a41ff22dcebc 100644
--- a/drivers/usb/gadget/function/dfu.c
+++ b/drivers/usb/gadget/function/dfu.c
@@ -807,7 +807,7 @@ static int dfu_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
if (value >= 0) {
req->zero = 0;
req->length = value;
- value = usb_ep_queue(cdev->gadget->ep0, req);
+ value = composite_queue_setup_request(cdev);
if (value < 0)
ERROR(cdev, "dfu response on ttyGS%d, err %d\n",
dfu->port_num, value);
diff --git a/drivers/usb/gadget/function/f_acm.c
b/drivers/usb/gadget/function/f_acm.c
index 3532fd589274..7fa0a4358fe6 100644
--- a/drivers/usb/gadget/function/f_acm.c
+++ b/drivers/usb/gadget/function/f_acm.c
@@ -309,6 +309,8 @@ static void acm_complete_set_line_coding(struct usb_ep *ep,
struct f_acm*acm = ep->driver_data;
struct usb_composite_dev *cdev = acm->port.func.config->cdev;
+ composite_setup_complete(ep, req);
+
if (req->status != 0) {
DBG(cdev, "acm ttyGS%d completion, err %d\n",
acm->port_num, req->status);
@@ -406,7 +408,7 @@ static int acm_setup(struct usb_function *f, const struct
usb_ctrlrequest *ctrl)
w_value, w_index, w_length);
req->zero = 0;
req->length = value;
- value = usb_ep_queue(cdev->gadget->ep0, req);
+ value = composite_queue_setup_request(cdev);
if (value < 0)
ERROR(cdev, "acm response on ttyGS%d, err %d\n",
acm->port_num, value);
--
2.45.0
[PATCH 1/2] usb:gadget:composite: add public functions for managing setup requests
From: Enrico Scholz
The composite driver does some bookkeeping about pending requests and
decides in its cleanup function whether requests must be dequeued.
There are some function drivers (dfu, acm) which queue the requests
directly which causes e.g.
| :/ dfu /tmp/img(img)c
| ...
| g_multi gadget0: high-speed config #1: Multifunction Composite Gadget
| fsl_free_request: Freeing queued request
| [<2fd8d8e5>] (unwind_backtrace+0x1/0x78) from [<2fd34b1f>]
(fsl_free_request+0x1f/0x34)
| [<2fd34b1f>] (fsl_free_request+0x1f/0x34) from [<2fd337cf>]
(composite_dev_cleanup+0x77/0xc0)
| [<2fd337cf>] (composite_dev_cleanup+0x77/0xc0) from [<2fd33867>]
(__composite_unbind+0x4f/0x94)
| [<2fd33867>] (__composite_unbind+0x4f/0x94) from [<2fd3432b>]
(gadget_unbind_driver+0x37/0x70)
| [<2fd3432b>] (gadget_unbind_driver+0x37/0x70) from [<2fd1275f>]
(device_remove+0xf/0x20)
| [<2fd1275f>] (device_remove+0xf/0x20) from [<2fd1289b>]
(unregister_driver+0x47/0x60)
| [<2fd1289b>] (unregister_driver+0x47/0x60) from [<2fd34663>]
(usb_gadget_unregister_driver+0xf/0x18)
| [<2fd34663>] (usb_gadget_unregister_driver+0xf/0x18) from [<2fd37c5b>]
(usb_multi_unregister+0x13/0x30)
| [<2fd37c5b>] (usb_multi_unregister+0x13/0x30) from [<2fd59f67>]
(do_dfu+0x47/0x68)
| [<2fd59f67>] (do_dfu+0x47/0x68) from [<2fd04fdf>] (execute_command+0x23/0x4c)
| [<2fd04fdf>] (execute_command+0x23/0x4c) from [<2fd0a737>]
(run_list_real+0x5ef/0x690)
| [<2fd0a737>] (run_list_real+0x5ef/0x690) from [<2fd0a00b>]
(parse_stream_outer+0xc7/0x154)
| [<2fd0a00b>] (parse_stream_outer+0xc7/0x154) from [<2fd0a927>]
(run_shell+0x3f/0x6c)
| [<2fd0a927>] (run_shell+0x3f/0x6c) from [<2fd01103>] (run_init+0xeb/0x210)
| [<2fd01103>] (run_init+0xeb/0x210) from [<2fd01253>] (start_barebox+0x2b/0x6c)
| [<2fd01253>] (start_barebox+0x2b/0x6c) from [<2fd89b37>]
(barebox_non_pbl_start+0xc3/0x108)
| [<2fd89b37>] (barebox_non_pbl_start+0xc3/0x108) from [<2fd5>]
(__bare_init_start+0x1/0xc)
and related NULL pointer dereferences after 'dfu-util -e'.
Add a helper function which can be called by function drivers and
export the complete method.
*NOTE*: kernel uses the same code and probably suffers from the same
problem.
Signed-off-by: Enrico Scholz
---
drivers/usb/gadget/composite.c | 7 ++-
include/linux/usb/composite.h | 4
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index f55ae5698e08..98f7b5bf7fb4 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1509,7 +1509,7 @@ EXPORT_SYMBOL_GPL(usb_string_ids_n);
/*-*/
-static void composite_setup_complete(struct usb_ep *ep, struct usb_request
*req)
+void composite_setup_complete(struct usb_ep *ep, struct usb_request *req)
{
struct usb_composite_dev *cdev;
@@ -1556,6 +1556,11 @@ static int composite_ep0_queue(struct usb_composite_dev
*cdev,
return ret;
}
+int composite_queue_setup_request(struct usb_composite_dev *cdev)
+{
+ return composite_ep0_queue(cdev, cdev->req);
+}
+
static int count_ext_compat(struct usb_configuration *c)
{
int i, res;
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index c3ee403abfe9..cc570657e55f 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -521,6 +521,10 @@ extern struct usb_string *usb_gstrings_attach(struct
usb_composite_dev *cdev,
extern int usb_string_ids_n(struct usb_composite_dev *c, unsigned n);
+extern void composite_setup_complete(struct usb_ep *ep,
+ struct usb_request *req);
+extern int composite_queue_setup_request(struct usb_composite_dev *cdev);
+
extern void composite_disconnect(struct usb_gadget *gadget);
extern void composite_reset(struct usb_gadget *gadget);
--
2.45.0
imx8: 'struct dram_timing_info' incompatibilities
Hi,
commit e6234f90 ("ddr: Initial i.MX9 support") adds unconditionally
fields to the dram timing info object (include/soc/imx/ddr.h):
| struct dram_timing_info {
| ...
| + /* fsp config */
| + struct dram_fsp_cfg *fsp_cfg;
| + unsigned int fsp_cfg_num;
However, the iMX8 ATF ([1]) does not have this field and DRAM related
SMC calls like
| arm_smccc_smc(FSL_SIP_DDR_DVFS, 0x11, i, 0, 0, 0, 0, 0, );
in the NXP busfreq-imx8mq.c driver will fail [2].
Am I missing something here or should the dram_timing_info table somehow
translated between these architectures (e.g. in dram_config_save())?
Enrico
Footnotes:
[1]
https://github.com/nxp-imx/imx-atf/blob/lf_v2.8/plat/imx/imx8m/include/dram.h
[2] typical message: "imx_busfreq: probe of busfreq failed with error -22"
Re: [PATCH] ARM64: let 'end' point after the range in cache functions
Sascha Hauer writes: > So 129 bytes are sent from barebox, right? Which network driver driver > is involved on the barebox side here? How did you force sending excatly > 129 bytes? drivers/net/bcmgenet.c; I made a diff --git a/drivers/net/bcmgenet.c b/drivers/net/bcmgenet.c index 9e0bacb31adf..988324cd22d4 100644 --- a/drivers/net/bcmgenet.c +++ b/drivers/net/bcmgenet.c @@ -272,6 +272,10 @@ static int bcmgenet_gmac_eth_send(struct eth_device *edev, void *packet, int len u32 tries = 100; dma_addr_t dma; + if (length == 129) + print_hex_dump(KERN_INFO, "D ", DUMP_PREFIX_OFFSET, + 16, 4, packet + 125, 4, 1); + prod_index = readl(priv->mac_reg + TDMA_PROD_INDEX); dma = dma_map_single(priv->dev, packet, length, DMA_TO_DEVICE); there to verify the input data and checked with tcpdump on the other end (which differed in around 70% of the cases in the last byte). Packets with arbitrary length can be constructed easily by custom tftp filenames. Enrico
[PATCH] of: do not acccess 'prop->value' directly
From: Enrico Scholz
Use of_property_get_value() accessor. Else, wrong results are
returned when working with fit images.
Signed-off-by: Enrico Scholz
---
drivers/of/base.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index b22959dabebf..9bd0cdaac2ad 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -853,7 +853,7 @@ int of_property_count_elems_of_size(const struct
device_node *np,
if (!prop)
return -EINVAL;
- if (!prop->value)
+ if (!of_property_get_value(prop))
return -ENODATA;
if (prop->length % elem_size != 0) {
@@ -2009,9 +2009,9 @@ int of_property_read_string_helper(const struct
device_node *np,
if (!prop)
return -EINVAL;
- if (!prop->value)
+ p = of_property_get_value(prop);
+ if (!p)
return -ENODATA;
- p = prop->value;
end = p + prop->length;
for (i = 0; p < end && (!out_strs || i < skip + sz); i++, p += l) {
--
2.44.0
[PATCH] ARM64: let 'end' point after the range in cache functions
From: Enrico Scholz
v8_flush_dcache_range() and v8_inv_dcache_range() are implemented
under the assumption that their 'end' parameter points *after* the
range.
Fix callers to use it in this way.
This fixes e.g. spurious corruptions in the last octet when sending
129 bytes over ethernet.
Signed-off-by: Enrico Scholz
---
arch/arm/cpu/dma_64.c | 2 +-
arch/arm/cpu/mmu_64.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/cpu/dma_64.c b/arch/arm/cpu/dma_64.c
index 74d7167860c2..b50572f5e601 100644
--- a/arch/arm/cpu/dma_64.c
+++ b/arch/arm/cpu/dma_64.c
@@ -6,7 +6,7 @@ void arch_sync_dma_for_device(void *vaddr, size_t size,
enum dma_data_direction dir)
{
unsigned long start = (unsigned long)vaddr;
- unsigned long end = start + size - 1;
+ unsigned long end = start + size;
if (dir == DMA_FROM_DEVICE)
v8_inv_dcache_range(start, end);
diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c
index 12cd644de0c7..b48e4732b86d 100644
--- a/arch/arm/cpu/mmu_64.c
+++ b/arch/arm/cpu/mmu_64.c
@@ -282,7 +282,7 @@ void mmu_disable(void)
void dma_inv_range(void *ptr, size_t size)
{
unsigned long start = (unsigned long)ptr;
- unsigned long end = start + size - 1;
+ unsigned long end = start + size;
v8_inv_dcache_range(start, end);
}
@@ -290,7 +290,7 @@ void dma_inv_range(void *ptr, size_t size)
void dma_flush_range(void *ptr, size_t size)
{
unsigned long start = (unsigned long)ptr;
- unsigned long end = start + size - 1;
+ unsigned long end = start + size;
v8_flush_dcache_range(start, end);
}
--
2.44.0
[PATCH] fdt: copy terminating '\0' in lstrcpy()
From: Enrico Scholz
On large string tables (>64K), a
| fdt->strings = realloc(fdt->strings, fdt->str_size * 2);
operation is executed. This 'realloc()' does not zero the memory so
there is no guarantee that the strings will be terminated properly.
Modify 'lstrcpy()' so that it also copies the terminating '\0'.
Signed-off-by: Enrico Scholz
---
drivers/of/fdt.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 01d7dc37439f..9d72fafd3669 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -302,15 +302,15 @@ static int lstrcpy(char *dest, const char *src)
int len = 0;
int maxlen = 1023;
- while (*src) {
- *dest++ = *src++;
+ do {
+ *dest++ = *src;
len++;
if (!maxlen)
return -ENOSPC;
maxlen--;
- }
+ } while (*src++);
- return len;
+ return len - 1;
}
static void *memalign_realloc(void *orig, size_t oldsize, size_t newsize)
--
2.41.0
[PATCH] imx:boot: adapt boot device detection for imx8mp
imx8mp uses sbmr2[27..24] for encoding the bootmode while existing
code reads only sbmr2[25..24].
This can detect BOOTSOURCE_SERIAL for the wrong mode.
Signed-off-by: Enrico Scholz
---
arch/arm/mach-imx/boot.c | 34 +++---
1 file changed, 31 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index 8c9febb50a65..daf1951a36c2 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -202,7 +202,8 @@ void imx51_boot_save_loc(void)
}
#define IMX53_SRC_SBMR 0x4
-#define SRC_SBMR_BMOD GENMASK(25, 24)
+#define IMX53_SRC_SBMR_BMODGENMASK(25, 24)
+#define IMX8MP_SRC_SBMR_BMOD GENMASK(27, 24)
#define IMX53_BMOD_SERIAL 0b11
#define __BOOT_CFG(n, m, l)GENMASK((m) + ((n) - 1) * 8, \
@@ -234,7 +235,12 @@ __MAKE_BOOT_CFG_BITS(4)
static unsigned int imx53_get_bmod(uint32_t r)
{
- return FIELD_GET(SRC_SBMR_BMOD, r);
+ return FIELD_GET(IMX53_SRC_SBMR_BMOD, r);
+}
+
+static unsigned int imx8mp_get_bmod(uint32_t r)
+{
+ return FIELD_GET(IMX8MP_SRC_SBMR_BMOD, r);
}
static int imx53_bootsource_internal(uint32_t r)
@@ -318,6 +324,8 @@ void imx53_boot_save_loc(void)
#define IMX6_BMOD_SERIAL 0b01
#define IMX6_BMOD_RESERVED 0b11
#define IMX6_BMOD_FUSES0b00
+#define IMX8MP_BMOD_FUSES 0b
+#define IMX8MP_BMOD_SERIAL 0b0001
#define BT_FUSE_SELBIT(4)
#define GPR10_BOOT_FROM_GPR9 BIT(28)
@@ -338,6 +346,26 @@ static bool imx6_bootsource_serial(uint32_t sbmr2)
!(sbmr2 & BT_FUSE_SEL));
}
+static bool imx8mp_bootsource_serial(uint32_t sbmr2)
+{
+ return imx8mp_get_bmod(sbmr2) == IMX8MP_BMOD_SERIAL ||
+ /*
+* If boot from fuses is selected and fuses are not
+* programmed by setting BT_FUSE_SEL, ROM code will
+* fallback to serial mode
+*/
+ (imx8mp_get_bmod(sbmr2) == IMX8MP_BMOD_FUSES &&
+!(sbmr2 & BT_FUSE_SEL));
+}
+
+static bool imx_bootsource_serial(uint32_t sbmr2)
+{
+ if (cpu_is_mx8mp())
+ return imx8mp_bootsource_serial(sbmr2);
+ else
+ return imx6_bootsource_serial(sbmr2);
+}
+
static bool imx6_bootsource_serial_forced(uint32_t bootmode)
{
if (cpu_mx6_is_mx6ul() || cpu_mx6_is_mx6ull())
@@ -497,7 +525,7 @@ static void __imx7_get_boot_source(enum bootsource *src,
int *instance,
{
const struct imx_boot_sw_info *info;
- if (imx6_bootsource_serial(sbmr2)) {
+ if (imx_bootsource_serial(sbmr2)) {
*src = BOOTSOURCE_SERIAL;
return;
}
--
2.37.3
Re: [PATCH 0/9] tlsf: use 8-byte alignment for normal malloc allocations
Ahmad Fatoum writes: > TLSF currently uses only 4-byte alignment on 32-bit platforms, which isn't > enough for ldrd/strd on ARMv7. This series reworks TLSF a bit, so we always > have at least 8 byte alignment. dlmalloc already has 8 byte alignment > minimum, so nothing to do there. I am wondering whether alignment should be increased on 64 bit archs to 16 bytes as well. ARMv8 spec [1] says | exclusive pair access must be aligned to twice the data size, that is, | 128 bits for a pair of 64-bit values. A github issue [2] mentions this alignment too. > While this fixes real issues like what Enrico ran into, I'd suggest we only > this be taken into next only after v2022.10.0 is tagged, This is ok for me; the issue disappeared with reverting the zstd patch. Enrico Footnotes: [1] https://developer.arm.com/documentation/den0024/a/An-Introduction-to-the-ARMv8-Instruction-Sets/The-ARMv8-instruction-sets/Addressing [2] https://github.com/mattconte/tlsf/issues/16 -- SIGMA Chemnitz GmbH Registergericht: Amtsgericht Chemnitz HRB 1750 Am Erlenwald 13 Geschaeftsfuehrer: Grit Freitag, Frank Pyritz 09128 Chemnitz
Re: [PATCH 4/5] commands: gpio: add -d argument to set/get commands
Ahmad Fatoum writes: > For debugging, it can be useful to reference GPIOs relative to a > controller, e.g.: > > gpio_direction_output -d gpio4 20 1 > > - if (argc < count) > + if (optind < count) > return COMMAND_ERROR_USAGE; This change seems to make '-d ...' mandatory. E.g. | :/ gpio_get_value 65 | | gpio_get_value - return value of a GPIO pin | | Usage: gpio_get_value [-d CONTROLLER] GPIO Enrico
Re: [PATCH 5/5] gpiolib: gpioinfo: add optional CONTROLLER command line argument
Ahmad Fatoum writes:
> static int do_gpiolib(int argc, char *argv[])
> {
> + if (argc == 1) {
> + dev = find_device(argv[1]);
'gpioinfo' without arguments will crash because this calls
'find_device(NULL)'
| :/ gpioinfo
| BUG: failure at lib/string.c:226/strcmp()!
| BUG!
Enrico
Re: malloc() alignment on 32 bit
Sascha Hauer writes: > So it seems it's really a good idea to increase malloc alignment > accordingly. But this does not seem to be trivial :( A naive | - ALIGN_SIZE_LOG2 = 2, | + ALIGN_SIZE_LOG2 = 3, triggers a lot of warnings. I think, ALIGN_SIZE in tlsf_add_pool() and in the higher level functions (tlsf_malloc(), _memalign(), _realloc()) should be replaced by a new, bigger constant. But this appears invasive and I have no idea about tlsf and the sideeffects :( There exists https://github.com/mattconte/tlsf/issues/16 which asks for proper alignment; but project seems to be unmaintained. Replacing 'malloc()' with 'memalign()' on problems, seems the best idea for now. Enrico
Re: [PATCH] zstd: fix assert() logic
Sascha Hauer writes: > On Tue, Sep 27, 2022 at 07:22:17PM +0200, Enrico Scholz wrote: >> It should warn when condition does **not** hold > > That fixes only the warnings, not the regression, right? no; sorry. I stumpled over it while looking at this regression. Enrico
[PATCH] zstd: fix assert() logic
It should warn when condition does **not** hold
Signed-off-by: Enrico Scholz
---
lib/zstd/common/zstd_deps.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/zstd/common/zstd_deps.h b/lib/zstd/common/zstd_deps.h
index 7a5bf44839c9..f06df065dec0 100644
--- a/lib/zstd/common/zstd_deps.h
+++ b/lib/zstd/common/zstd_deps.h
@@ -84,7 +84,7 @@ static uint64_t ZSTD_div64(uint64_t dividend, uint32_t
divisor) {
#include
-#define assert(x) WARN_ON((x))
+#define assert(x) WARN_ON(!(x))
#endif /* ZSTD_DEPS_ASSERT */
#endif /* ZSTD_DEPS_NEED_ASSERT */
--
2.37.3
Broken ubifs-zstd
Hello, trying to mount a zstd compressed ubifs fails on an iMX6ULL with | ERROR: UBIFS error (ubi0:0): 9fe6691d: cannot decompress 2612 bytes, compressor zstd, error -22 | ERROR: UBIFS error (ubi0:0): 9fe6692d: cannot decompress 2612 bytes, compressor zstd, error -22 | ERROR: UBIFS error (ubi0:0): 9fe66a65: bad data node (block 0, inode 815) | ERROR: magic 0x6101831 (2022.09 and master). After reverting | commit b4a9782d4f56333e897dccc35c2c27e2605f6b93 | Author: Ahmad Fatoum | Date: Wed Jul 13 12:09:18 2022 +0200 | | lib: zstd: sync with Linux it works again (both in Linux 5.15 and barebox). ubifs was created by recent OpenEmbedded kirkstone with | MKUBIFS_ARGS = "-F -m 2048 -e 126976 -c 2048 --compr=zstd" Warnings are triggered (after fixing assert()) at | WARNING: at lib/zstd/decompress/../common/bitstream.h:390/BIT_reloadDStreamFast()! Is anybody else experiencing these problems? Enrico
Re: malloc() alignment on 32 bit
Sascha Hauer writes: >> | zstd_decomp_init:536 workspace=8ff1a004+161320 >> | ERROR: initcall ubifs_init+0x1/0xc4 failed: Invalid argument >> > If you had asked me which alignment we have then I would have said it's > bigger. OTOH I never received any reports about insufficient alignment > on ARM or any other 32bit architecture. The code which failed for me was added 3 months ago | commit b4a9782d4f56333e897dccc35c2c27e2605f6b93 | Author: Ahmad Fatoum | Date: Wed Jul 13 12:09:18 2022 +0200 | | lib: zstd: sync with Linux and the Kconfig options (FS_UBIFS_COMPRESSION_ZSTD) are set to "off"... > I suspect we could just drop the check without any harm, but that's just > a gut feeling because we never had any alignment issues. > > BTW are you sure ldrd/strd need 8 byte alignment? Their EX variants (LDREXD + STREXD; see [1]). Unaligned access on plain LDRD/STRD is allowed on ARMv7-A. But not on ARMv7-M or ARMv6 and earlier. Enrico Footnotes: [1] https://developer.arm.com/documentation/ddi0406/c/Application-Level-Architecture/Application-Level-Memory-Model/Alignment-support/Unaligned-data-access
malloc() alignment on 32 bit
Hello,
on an iMX6ull I stumpled across
| zstd_decomp_init:536 workspace=8ff1a004+161320
| ERROR: initcall ubifs_init+0x1/0xc4 failed: Invalid argument
which is caused by
| static int zstd_decomp_init(void)
| void *wksp = malloc(wksp_size);
| ...
| ZSTD_DCtx* ZSTD_initStaticDCtx(void *workspace, size_t workspaceSize)
|if ((size_t)workspace & 7) return NULL; /* 8-aligned */
Trivial fix would be 'memalign(8, wksp_size)', but is it really ok that
malloc() for 32 bit has only an alignment of 4?
Relevant code seems to be in common/tlsf.c
| enum tlsf_private
| {
| #if defined (TLSF_64BIT)
| /* All allocation sizes and addresses are aligned to 8 bytes. */
| ALIGN_SIZE_LOG2 = 3,
| #else
| /* All allocation sizes and addresses are aligned to 4 bytes. */
| ALIGN_SIZE_LOG2 = 2,
| #endif
'ldrd/strd' require 8 byte alignment which might break with such
alignment.
Enrico
Re: [PATCH 2/3] tftp: implement UDP reorder cache using lists
Sascha Hauer writes:
> The UDP reorder cache can be much easier implemented using lists.
> As a bonus the cache grows and shrinks on demand and no fixed size
> has to be configured at compile time.
There are three variants of the cache
- small, fixed sized array with linear search (very first
implementation) --> O(n)
- list --> O(n) on insert, O(1) on lookup
- bitmap --> O(1)
Performance wise, I think the list implementation is the slowest one
(although the fixed sized array is O(n) on every operation, this should
be still faster for small n than the list operations and the related
memory management).
>From code size, the list implementation is in the middle.
I am not sure whether dynamic shrinking/growing of the cache is so
important or whether a small, fixed sized cache suffices. In my
experience, only the first couple of packets really matter regarding
reordering.
Of course, the 'kfifo' could be replaced completely by a custom buffer
implementation where packets are inserted at the correct position. O(1)
for every operation + zero additional memory.
> -static inline bool is_block_before(uint16_t a, uint16_t b)
> -{
> - return (int16_t)(b - a) > 0;
> -}
> -
>
> static int tftp_window_cache_insert(struct tftp_cache *cache, uint16_t id,
> void const *data, size_t len)
> {
> ...
> + list_for_each_entry(block, >blocks, list) {
fwiw, iterating in the reverse direction will find the position probably
faster
> + if (block->id == id)
> + return 0;
> + if (block->id < id)
This will break when wrapping at 65535; the removed 'is_block_before()'
function was written for this case.
> @@ -614,12 +431,26 @@ static void tftp_apply_window_cache(struct file_priv
> *priv)
> if (priv->state != STATE_RDATA)
> return;
>
> - block = tftp_window_cache_pop(cache);
> + if (list_empty(>blocks))
> + return;
>
> - debug_assert(block);
> - debug_assert(block->id == (uint16_t)(priv->last_block + 1));
> + block = list_first_entry(>blocks, struct tftp_block,
> list);
> +
> + if (block->id < priv->last_block + 1) {
ditto; wrapping at 65535
> + /* shouldn't happen, but be sure */
> + list_del(>list);
> + free(block);
> + continue;
> + }
> +
> + if (block->id != priv->last_block + 1)
ditto; wrapping at 65535. Resp. should be written as
| if (block->id != (uint16_t)(priv->last_block + 1))
Enrico
Re: [PATCH] tftp: allocate at least 4096 bytes for FIFO
Ahmad Fatoum writes: > On one board, boots from /mnt/tftp currently fail for me with: > > ERROR: tftp: tftp: not enough room in kfifo (only 1376 out of 1432 written Thanks for noticing that; I sent fixups for the recent patchset. Perhaps, can you test it on your failing platform with changing | #define TFTP_EXTRA_BLOCKS 2 to | #define TFTP_EXTRA_BLOCKS 0 ? This should turn off the workaround of over-allocating memory and isolate (and verify) the fix for the real problem. Enrico
[PATCH 1/3] fixup! tftp: implement 'windowsize' (RFC 7440) support
avoid fifo overflow on read() with small buffer sizes. Signed-off-by: Enrico Scholz --- fs/tftp.c | 13 +++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/fs/tftp.c b/fs/tftp.c index 2bffae2bf36e..a3fe7dfd590e 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -78,6 +78,9 @@ /* size of cache which deals with udp reordering */ #define TFTP_WINDOW_CACHE_NUM CONFIG_FS_TFTP_REORDER_CACHE_SIZE +/* allocate this number of blocks more than needed in the fifo */ +#define TFTP_EXTRA_BLOCKS 2 + /* marker for an emtpy 'tftp_cache' */ #define TFTP_CACHE_NO_ID (-1) @@ -546,7 +549,8 @@ static int tftp_allocate_transfer(struct file_priv *priv) /* multiplication is safe; both operands were checked in tftp_parse_oack() and are small integers */ - priv->fifo = kfifo_alloc(priv->blocksize * priv->windowsize); + priv->fifo = kfifo_alloc(priv->blocksize * +(priv->windowsize + TFTP_EXTRA_BLOCKS)); if (!priv->fifo) goto err; @@ -1025,7 +1029,12 @@ static int tftp_read(struct device_d *dev, FILE *f, void *buf, size_t insize) if (priv->state == STATE_DONE) return outsize; - if (priv->last_block == priv->ack_block) + /* send the ACK only when fifo has been nearly depleted; else, + when tftp_read() is called with small 'insize' values, it + is possible that there is read more data from the network + than consumed by kfifo_get() and the fifo overflows */ + if (priv->last_block == priv->ack_block && + kfifo_len(priv->fifo) <= TFTP_EXTRA_BLOCKS * priv->blocksize) tftp_send(priv); ret = tftp_poll(priv); -- 2.37.2
[PATCH 3/3] tftp: make read() fail in error case
when tftp transfer goes in error state e.g. due to error packets sent
from the server or (unexpected) internal problems, let the read() fail
instead of ignoring these errors silently and corrupting the output.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 9a3753e50e37..2592da94dd34 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -1017,7 +1017,7 @@ static int tftp_read(struct device_d *dev, FILE *f, void
*buf, size_t insize)
{
struct file_priv *priv = f->priv;
size_t outsize = 0, now;
- int ret;
+ int ret = 0;
pr_vdebug("%s %zu\n", __func__, insize);
@@ -1026,8 +1026,11 @@ static int tftp_read(struct device_d *dev, FILE *f, void
*buf, size_t insize)
outsize += now;
buf += now;
insize -= now;
- if (priv->state == STATE_DONE)
- return outsize;
+
+ if (priv->state == STATE_DONE) {
+ ret = priv->err;
+ break;
+ }
/* send the ACK only when fifo has been nearly depleted; else,
when tftp_read() is called with small 'insize' values, it
@@ -1041,9 +1044,12 @@ static int tftp_read(struct device_d *dev, FILE *f, void
*buf, size_t insize)
if (ret == TFTP_ERR_RESEND)
tftp_send(priv);
if (ret < 0)
- return ret;
+ break;
}
+ if (ret < 0)
+ return ret;
+
return outsize;
}
--
2.37.2
[PATCH 0/3] tftp fixups
Fixups for the "tftp" branch in next. Enrico Scholz (3): fixup! tftp: implement 'windowsize' (RFC 7440) support fixup! tftp: detect out-of-memory situations tftp: make read() fail in error case fs/tftp.c | 29 ++--- 1 file changed, 22 insertions(+), 7 deletions(-) -- 2.37.2
[PATCH 2/3] fixup! tftp: detect out-of-memory situations
minor typo fix
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index a3fe7dfd590e..9a3753e50e37 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -591,7 +591,7 @@ static void tftp_put_data(struct file_priv *priv, uint16_t
block,
sz = kfifo_put(priv->fifo, pkt, len);
if (sz != len) {
- pr_err("tftp: not enough room in kfifo (only %u out of %zu
written\n",
+ pr_err("tftp: not enough room in kfifo (only %u out of %zu
written)\n",
sz, len);
priv->err = -ENOMEM;
priv->state = STATE_DONE;
--
2.37.2
[PATCH v4 13/21] tftp: reduce block size on lookup requests
Save some bytes on network traffic by reducing the server response for lookup requests. Signed-off-by: Enrico Scholz --- fs/tftp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/tftp.c b/fs/tftp.c index 7181c58d1083..16e57daaef37 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -151,7 +151,9 @@ static int tftp_send(struct file_priv *priv) '\0', /* "timeout" */ TIMEOUT, '\0', '\0', /* "blksize" */ - TFTP_MTU_SIZE); + /* use only a minimal blksize for getattr + operations, */ + priv->is_getattr ? TFTP_BLOCK_SIZE : TFTP_MTU_SIZE); pkt++; if (!priv->push) -- 2.37.2
[PATCH v4 12/21] tftp: record whether tftp file is opened for lookup operation only
Opening a tftp is done in two steps: at first `tftp_lookup()` is
called to get the filesize and then it is opened again and data are
read.
The `tftp_lookup()` call sends only a RRQ/WRQ, reads then the "tsize"
from the response and closes the transfer by sending an error datagram.
The tftp server will send a full data window.
To prevent unneeded traffic, later patches set parameters to reduce
the size of the server response.
We need knowledge about type of operation which is recorded in an
"is_getattr" attribute.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 4e31adcd60ed..7181c58d1083 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -95,6 +95,7 @@ struct file_priv {
void *buf;
int blocksize;
int block_requested;
+ bool is_getattr;
};
struct tftp_priv {
@@ -460,7 +461,7 @@ static int tftp_start_transfer(struct file_priv *priv)
}
static struct file_priv *tftp_do_open(struct device_d *dev,
- int accmode, struct dentry *dentry)
+ int accmode, struct dentry *dentry, bool is_getattr)
{
struct fs_device_d *fsdev = dev_to_fs_device(dev);
struct file_priv *priv;
@@ -489,6 +490,7 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
priv->filename = dpath(dentry, fsdev->vfsmount.mnt_root);
priv->blocksize = TFTP_BLOCK_SIZE;
priv->block_requested = -1;
+ priv->is_getattr = is_getattr;
parseopt_hu(fsdev->options, "port", );
@@ -567,7 +569,7 @@ static int tftp_open(struct device_d *dev, FILE *file,
const char *filename)
{
struct file_priv *priv;
- priv = tftp_do_open(dev, file->flags, file->dentry);
+ priv = tftp_do_open(dev, file->flags, file->dentry, false);
if (IS_ERR(priv))
return PTR_ERR(priv);
@@ -783,7 +785,7 @@ static struct dentry *tftp_lookup(struct inode *dir, struct
dentry *dentry,
struct file_priv *priv;
loff_t filesize;
- priv = tftp_do_open(>dev, O_RDONLY, dentry);
+ priv = tftp_do_open(>dev, O_RDONLY, dentry, true);
if (IS_ERR(priv))
return NULL;
--
2.37.2
[PATCH v4 16/21] tftp: implement 'windowsize' (RFC 7440) support
Results (with the reorder patch; numbers in bytes/s) on an iMX8MP are:
| windowsize | VPN | 1 Gb/s | 100 Mb/s |
||---|||
| 128| 3.869.284 | 98.643.085 | 11.434.852 |
| 64| 3.863.581 | 98.550.375 | 11.434.852 |
| 48| 3.431.580 | 94.211.680 | 11.275.010 |
| 32| 2.835.129 | 85.250.081 | 10.985.605 |
| 24| 2.344.858 | 77.787.537 | 10.765.667 |
| 16| 1.734.186 | 67.519.381 | 10.210.087 |
| 12| 1.403.340 | 61.972.576 | 9.915.612 |
| 8| 1.002.462 | 50.852.376 | 9.016.130 |
| 6| 775.573 | 42.781.558 | 8.422.297 |
| 4| 547.845 | 32.066.544 | 6.835.567 |
| 3| 412.987 | 26.526.081 | 6.322.435 |
| 2| 280.987 | 19.120.641 | 5.494.241 |
| 1| 141.699 | 10.431.516 | 2.967.224 |
(VPN = OpenVPN on ADSL 50 Mb/s).
The window size can be configured at runtime.
This commit increases barebox size by
| add/remove: 1/0 grow/shrink: 4/1 up/down: 148/-16 (132)
| Function old new delta
| tftp_send336 384 +48
| tftp_handler 880 928 +48
| tftp_init 16 60 +44
| tftp_allocate_transfer 100 104 +4
| g_tftp_window_size - 4 +4
| tftp_poll180 164 -16
| Total: Before=629980, After=630112, chg +0.02%
Setting FS_TFTP_MAX_WINDOW_SIZE to zero reduces it to
| add/remove: 1/0 grow/shrink: 3/2 up/down: 96/-52 (44)
| Function old new delta
| tftp_init 16 60 +44
| tftp_handler 880 924 +44
| tftp_allocate_transfer 100 104 +4
| g_tftp_window_size - 4 +4
| tftp_poll180 164 -16
| tftp_send336 300 -36
| Total: Before=629980, After=630024, chg +0.01%
Signed-off-by: Enrico Scholz
---
fs/Kconfig | 14 ++
fs/tftp.c | 54 +-
2 files changed, 55 insertions(+), 13 deletions(-)
diff --git a/fs/Kconfig b/fs/Kconfig
index aeba00073eed..0c4934285942 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -43,6 +43,20 @@ config FS_TFTP
prompt "tftp support"
depends on NET
+config FS_TFTP_MAX_WINDOW_SIZE
+ int
+ prompt "maximum tftp window size (RFC 7440)"
+ depends on FS_TFTP
+ default 128
+ range 1 128
+ help
+ The maximum allowed tftp "windowsize" (RFC 7440). Higher
+ value increase speed of the tftp download with the cost of
+ memory (1432 bytes per slot).
+
+ Requires tftp "windowsize" (RFC 7440) support on server side
+ to have an effect.
+
config FS_OMAP4_USBBOOT
bool
prompt "Filesystem over usb boot"
diff --git a/fs/tftp.c b/fs/tftp.c
index e62ff2a80bb5..88767223b052 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -26,9 +26,11 @@
#include
#include
#include
+#include
#include
#include
#include
+#include
#include
#include
#include
@@ -68,6 +70,7 @@
#define TFTP_BLOCK_SIZE512 /* default TFTP block size */
#define TFTP_MTU_SIZE 1432/* MTU based block size */
+#define TFTP_MAX_WINDOW_SIZE CONFIG_FS_TFTP_MAX_WINDOW_SIZE
#define TFTP_ERR_RESEND1
@@ -80,11 +83,14 @@
} while (0)
#endif
+static int g_tftp_window_size = DIV_ROUND_UP(TFTP_MAX_WINDOW_SIZE, 2);
+
struct file_priv {
struct net_connection *tftp_con;
int push;
uint16_t block;
uint16_t last_block;
+ uint16_t ack_block;
int state;
int err;
char *filename;
@@ -94,7 +100,7 @@ struct file_priv {
struct kfifo *fifo;
void *buf;
int blocksize;
- int block_requested;
+ unsigned int windowsize;
bool is_getattr;
};
@@ -125,6 +131,7 @@ static int tftp_send(struct file_priv *priv)
int len = 0;
uint16_t *s;
unsigned char *pkt = net_udp_get_payload(priv->tftp_con);
+ unsigned int window_size;
int ret;
pr_vdebug("%s: state %s\n", __func__, tftp_states[priv->state]);
@@ -132,6 +139,15 @@ static int tftp_send(struct file_priv *priv)
switch (priv->state) {
case STATE_RRQ:
case STATE_WRQ:
+ if (priv->push || priv->is_getattr)
+ /* atm, windowsize is supported only for RRQ and there
+ is no need to request a full window when we are
+ just looking up file
[PATCH v4 19/21] tftp: add selftest
Unittest for window cache functions.
Signed-off-by: Enrico Scholz
---
fs/tftp-selftest.h | 56
fs/tftp.c | 106 -
test/self/Kconfig | 7 +++
3 files changed, 168 insertions(+), 1 deletion(-)
create mode 100644 fs/tftp-selftest.h
diff --git a/fs/tftp-selftest.h b/fs/tftp-selftest.h
new file mode 100644
index ..2406ed329ecc
--- /dev/null
+++ b/fs/tftp-selftest.h
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0+
+// SPDX-FileCopyrightText: 2022 Enrico Scholz
+
+#ifndef H_BAREBOX_FS_TFTP_SELFTEST_H
+#define H_BAREBOX_FS_TFTP_SELFTEST_H
+
+#include
+
+#define _expect_fmt(_v) _Generic(_v, \
+void const *: "%p",\
+void *: "%p", \
+bool: "%d",\
+long int: "%lld", \
+long unsigned int: "%llu", \
+unsigned short: "%h", \
+signed int: "%d", \
+unsigned int: "%u")
+
+#define _expect_op(_a, _b, _op)
\
+ ({ \
+ __typeof__(_a) __a = (_a); \
+ __typeof__(_b) __b = (_b); \
+ \
+ total_tests++; \
+ \
+ if (!(__a _op __b)) { \
+ char fmt[sizeof "%s:%d: failed: %XXX %XXX\n" # _op]; \
+ strcpy(fmt, "%s:%d: failed: "); \
+ strcat(fmt, _expect_fmt(__a)); \
+ strcat(fmt, " " # _op " "); \
+ strcat(fmt, _expect_fmt(__b)); \
+ strcat(fmt, "\n"); \
+ \
+ failed_tests++; \
+ printf(fmt, __func__, __LINE__, __a, __b); \
+ } \
+ })
+
+#define _as_void(_p) ({\
+ void const *__res = (_p); \
+ __res; \
+ })
+
+#define expect_eq(_a, _b) _expect_op(_a, _b, ==);
+#define expect_ne(_a, _b) _expect_op(_a, _b, !=);
+#define expect_it(_a) _expect_op(_a, true, ==);
+
+#define expect_err(_a) _expect_op(_a, 0, <);
+#define expect_ok(_a) _expect_op(_a, 0, ==);
+
+/* _Generic() does not match 'void *' for typed pointers; cast them to raw
+ 'void *' first */
+#define expect_NULL(_a)_expect_op(_as_void(_a), NULL, ==);
+#define expect_PTR(_a) _expect_op(_as_void(_a), NULL, !=);
+
+#endif /* H_BAREBOX_FS_TFTP_SELFTEST_H */
diff --git a/fs/tftp.c b/fs/tftp.c
index b010a6be6dbb..a9cc0ff3b118 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -36,6 +36,8 @@
#include
#include
+#include "tftp-selftest.h"
+
#define TFTP_PORT 69 /* Well known TFTP port number */
/* Seconds to wait before remote server is allowed to resend a lost packet */
@@ -81,7 +83,7 @@
#define TFTP_ERR_RESEND1
-#ifdef DEBUG
+#if defined(DEBUG) || IS_ENABLED(CONFIG_SELFTEST_TFTP)
# define debug_assert(_cond) BUG_ON(!(_cond))
#else
# define debug_assert(_cond) do { \
@@ -1208,3 +1210,105 @@ static int tftp_init(void)
return register_fs_driver(_driver);
}
coredevice_initcall(tftp_init);
+
+
+BSELFTEST_GLOBALS();
+
+static int __maybe_unused tftp_window_cache_selftest(void)
+{
+ struct tftp_cache *cache = malloc(sizeof *cache);
+
+ if (!cache)
+ return -ENOMEM;
+
+ (void)skipped_tests;
+
+ expect_it( is_block_before(0, 1));
+ expect_it(!is_block_before(1, 0));
+ expect_it( is_block_before(65535, 0));
+ expect_it(!is_block_before(0, 65535));
+
+ expect_eq(get_block_delta(0, 1), 1);
+ expect_eq(get_block_delta(65535, 0), 1);
+ expect_eq(get_block_delta(65535, 1), 2);
+
+ expect_it(!in_window(0, 1, 3));
+ expect_it( in_window(1, 1, 3));
+ expect_it( in_window(2, 1, 3));
+ expect_it( in_window(3, 1, 3));
+ expect_it(!in_window(4, 1, 3));
[PATCH v4 00/21] add "windowsize" (RFC 7440) support for tftp
296 328 +32 | tftp_open 64 68 +4 | tftp_lookup 136 140 +4 | g_tftp_window_size - 4 +4 | tftp_read180 164 -16 | tftp_poll180 164 -16 | Total: Before=629556, After=630244, chg +0.11% Enrico Scholz (21): tftp: add some 'const' annotations tftp: allow to change tftp port cmd:tftp: add '-P' option to set tftp server port number tftp: do not set 'tsize' in WRQ requests tftp: assign 'priv->block' later in WRQ tftp: minor refactoring of RRQ/WRQ packet generation code tftp: replace hardcoded blksize by global constant tftp: remove sanity check of first block tftp: add debug_assert() macro tftp: allocate buffers and fifo dynamically tftp: add sanity check for OACK response tftp: record whether tftp file is opened for lookup operation only tftp: reduce block size on lookup requests tftp: refactor data processing tftp: detect out-of-memory situations tftp: implement 'windowsize' (RFC 7440) support tftp: do not use 'priv->block' for RRQ tftp: reorder tftp packets tftp: add selftest tftp: accept OACK + DATA datagrams only in certain states tftp: add some documentation about windowsize support Documentation/filesystems/tftp.rst | 38 ++ commands/tftp.c| 22 +- fs/Kconfig | 36 ++ fs/tftp-selftest.h | 56 +++ fs/tftp.c | 763 + test/self/Kconfig | 7 + 6 files changed, 824 insertions(+), 98 deletions(-) create mode 100644 fs/tftp-selftest.h --- v3 -> v4 - fix operation with non rfc 2347 servers - do not send 'tsize' in WRQ requests - add more sanity checks - add some documentation v2 -> v3 - use "port=XX" mount options instead of global 'tftp.port' variable - allocate fifo and send buffer dynamically based on block- and window size of the transfer. Do not use fixed constants anymore - rewritten cache code; use bitmap based functions with O(1) complexity instead of iterating over (small) arrays - unittest for cache functions - add information about binary sizes v1 -> v2 - fixes for non rfc7440 servers -- 2.37.2
[PATCH v4 09/21] tftp: add debug_assert() macro
Is a noop in normal cases (when compiler sees that the condition can
be evaluated without sideeffects) but allows optimizations based on
the condition.
E.g. in
| void foo(int a)
| {
| debug_assert(a == 23);
|
| if (a == 23)
| return;
|
| bar();
| }
the call to 'bar()' will be optimized away.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/fs/tftp.c b/fs/tftp.c
index 51cb1109d2ff..07de8334f202 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -70,6 +70,15 @@
#define TFTP_ERR_RESEND1
+#ifdef DEBUG
+# define debug_assert(_cond) BUG_ON(!(_cond))
+#else
+# define debug_assert(_cond) do { \
+ if (!(_cond)) \
+ __builtin_unreachable();\
+ } while (0)
+#endif
+
struct file_priv {
struct net_connection *tftp_con;
int push;
--
2.37.2
[PATCH v4 02/21] tftp: allow to change tftp port
This adds a 'port=' mount option for tftp filesystems.
Useful e.g. when working with a local, non-privileged tftp server
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index c26204ae76e6..913ca1df6e42 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -30,6 +30,7 @@
#include
#include
#include
+#include
#include
#define TFTP_PORT 69 /* Well known TFTP port number */
@@ -376,6 +377,7 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
struct file_priv *priv;
struct tftp_priv *tpriv = dev->priv;
int ret;
+ unsigned short port = TFTP_PORT;
priv = xzalloc(sizeof(*priv));
@@ -405,8 +407,9 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
goto out;
}
- priv->tftp_con = net_udp_new(tpriv->server, TFTP_PORT, tftp_handler,
- priv);
+ parseopt_hu(fsdev->options, "port", );
+
+ priv->tftp_con = net_udp_new(tpriv->server, port, tftp_handler, priv);
if (IS_ERR(priv->tftp_con)) {
ret = PTR_ERR(priv->tftp_con);
goto out1;
--
2.37.2
[PATCH v4 10/21] tftp: allocate buffers and fifo dynamically
Use the actual blocksize for allocating buffers instead of assuming an
hardcoded value.
This requires to add an additional 'START' state which is entered
after receiving (RRQ) or sending (WRQ) the OACK. Without it, the next
state would be entered and the (not allocated yet) fifo be used.
For non-rfc 2347 servers (which do not understand OACK and start with
data transfer immediately after RRQ/WRQ), additional transitions in
the state machine were implemented.
Code adds some sanity checks in the new code paths.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 194 --
1 file changed, 144 insertions(+), 50 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 07de8334f202..64a94797cda3 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -59,14 +59,15 @@
#define STATE_WRQ 2
#define STATE_RDATA3
#define STATE_WDATA4
-#define STATE_OACK 5
+/* OACK from server has been received and we can begin to sent either the ACK
+ (for RRQ) or data (for WRQ) */
+#define STATE_START5
#define STATE_WAITACK 6
#define STATE_LAST 7
#define STATE_DONE 8
#define TFTP_BLOCK_SIZE512 /* default TFTP block size */
#define TFTP_MTU_SIZE 1432/* MTU based block size */
-#define TFTP_FIFO_SIZE 4096
#define TFTP_ERR_RESEND1
@@ -111,10 +112,10 @@ static char const * const tftp_states[] = {
[STATE_WRQ] = "WRQ",
[STATE_RDATA] = "RDATA",
[STATE_WDATA] = "WDATA",
- [STATE_OACK] = "OACK",
[STATE_WAITACK] = "WAITACK",
[STATE_LAST] = "LAST",
[STATE_DONE] = "DONE",
+ [STATE_START] = "START",
};
static int tftp_send(struct file_priv *priv)
@@ -166,7 +167,6 @@ static int tftp_send(struct file_priv *priv)
case STATE_RDATA:
if (priv->block == priv->block_requested)
return 0;
- case STATE_OACK:
xp = pkt;
s = (uint16_t *)pkt;
*s++ = htons(TFTP_ACK);
@@ -261,11 +261,39 @@ static void tftp_timer_reset(struct file_priv *priv)
priv->progress_timeout = priv->resend_timeout = get_time_ns();
}
+static int tftp_allocate_transfer(struct file_priv *priv)
+{
+ debug_assert(!priv->fifo);
+ debug_assert(!priv->buf);
+
+ priv->fifo = kfifo_alloc(priv->blocksize);
+ if (!priv->fifo)
+ goto err;
+
+ if (priv->push) {
+ priv->buf = xmalloc(priv->blocksize);
+ if (!priv->buf) {
+ kfifo_free(priv->fifo);
+ priv->fifo = NULL;
+ goto err;
+ }
+ }
+
+ return 0;
+
+err:
+ priv->err = -ENOMEM;
+ priv->state = STATE_DONE;
+
+ return priv->err;
+}
+
static void tftp_recv(struct file_priv *priv,
uint8_t *pkt, unsigned len, uint16_t uh_sport)
{
uint16_t opcode;
uint16_t block;
+ int rc;
/* according to RFC1350 minimal tftp packet length is 4 bytes */
if (len < 4)
@@ -294,48 +322,64 @@ static void tftp_recv(struct file_priv *priv,
break;
}
- priv->block = block + 1;
- tftp_timer_reset(priv);
+ switch (priv->state) {
+ case STATE_WRQ:
+ priv->tftp_con->udp->uh_dport = uh_sport;
+ priv->state = STATE_START;
+ break;
- if (priv->state == STATE_LAST) {
+ case STATE_WAITACK:
+ priv->state = STATE_WDATA;
+ break;
+
+ case STATE_LAST:
priv->state = STATE_DONE;
break;
+
+ default:
+ pr_warn("ACK packet in %s state\n",
+ tftp_states[priv->state]);
+ goto ack_out;
}
- priv->tftp_con->udp->uh_dport = uh_sport;
- priv->state = STATE_WDATA;
+
+ priv->block = block + 1;
+ tftp_timer_reset(priv);
+
+ ack_out:
break;
case TFTP_OACK:
tftp_parse_oack(priv, pkt, len);
priv->tftp_con->udp->uh_dport = uh_sport;
-
- if (priv->push) {
- /* send first block */
- priv->state = STATE_WDATA;
- priv->block = 1;
- } else {
- /* send ACK */
- priv->state = STATE_OACK;
- priv->block = 0;
- tftp_send(priv);
- }
-
+ pr
[PATCH v4 01/21] tftp: add some 'const' annotations
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index d186e7983a6d..c26204ae76e6 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -94,7 +94,7 @@ static int tftp_truncate(struct device_d *dev, FILE *f,
loff_t size)
return 0;
}
-static char *tftp_states[] = {
+static char const * const tftp_states[] = {
[STATE_INVALID] = "INVALID",
[STATE_RRQ] = "RRQ",
[STATE_WRQ] = "WRQ",
--
2.37.2
[PATCH v4 06/21] tftp: minor refactoring of RRQ/WRQ packet generation code
Having 11 printf arguments with lot of them being 0, makes it difficulty to read and extend. Add some comments and use '\0' for %c. Signed-off-by: Enrico Scholz --- fs/tftp.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/fs/tftp.c b/fs/tftp.c index c1a193711793..0b2a420b6639 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -134,11 +134,11 @@ static int tftp_send(struct file_priv *priv) "%d%c" "blksize%c" "1432", - priv->filename + 1, 0, - 0, - 0, - TIMEOUT, 0, - 0); + priv->filename + 1, '\0', + '\0', /* "octet" */ + '\0', /* "timeout" */ + TIMEOUT, '\0', + '\0'); /* "blksize" */ pkt++; if (!priv->push) -- 2.37.2
[PATCH v4 05/21] tftp: assign 'priv->block' later in WRQ
Some refactoring; makes next patches cleaner.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 361661d2180e..c1a193711793 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -254,6 +254,7 @@ static void tftp_recv(struct file_priv *priv,
uint8_t *pkt, unsigned len, uint16_t uh_sport)
{
uint16_t opcode;
+ uint16_t block;
/* according to RFC1350 minimal tftp packet length is 4 bytes */
if (len < 4)
@@ -276,14 +277,13 @@ static void tftp_recv(struct file_priv *priv,
if (!priv->push)
break;
- priv->block = ntohs(*(uint16_t *)pkt);
- if (priv->block != priv->last_block) {
- pr_vdebug("ack %d != %d\n", priv->block,
priv->last_block);
+ block = ntohs(*(uint16_t *)pkt);
+ if (block != priv->last_block) {
+ pr_vdebug("ack %d != %d\n", block, priv->last_block);
break;
}
- priv->block++;
-
+ priv->block = block + 1;
tftp_timer_reset(priv);
if (priv->state == STATE_LAST) {
--
2.37.2
[PATCH v4 04/21] tftp: do not set 'tsize' in WRQ requests
The filesize is not known for push requests and barebox always sent '0'. Server might reject data because it will always exceed this length. Send this option only for RRQ requests. Signed-off-by: Enrico Scholz --- fs/tftp.c | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/fs/tftp.c b/fs/tftp.c index 913ca1df6e42..361661d2180e 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -132,18 +132,23 @@ static int tftp_send(struct file_priv *priv) "octet%c" "timeout%c" "%d%c" - "tsize%c" - "%lld%c" "blksize%c" "1432", priv->filename + 1, 0, 0, 0, TIMEOUT, 0, - 0, - priv->filesize, 0, 0); pkt++; + + if (!priv->push) + /* we do not know the filesize in WRQ requests and + 'priv->filesize' will always be zero */ + pkt += sprintf((unsigned char *)pkt, + "tsize%c%lld%c", + '\0', priv->filesize, + '\0'); + len = pkt - xp; break; -- 2.37.2
[PATCH v4 21/21] tftp: add some documentation about windowsize support
Signed-off-by: Enrico Scholz --- Documentation/filesystems/tftp.rst | 38 ++ 1 file changed, 38 insertions(+) diff --git a/Documentation/filesystems/tftp.rst b/Documentation/filesystems/tftp.rst index a292765e2511..8929213d3c4a 100644 --- a/Documentation/filesystems/tftp.rst +++ b/Documentation/filesystems/tftp.rst @@ -20,3 +20,41 @@ Example: In addition to the TFTP filesystem implementation, barebox does also have a :ref:`tftp command `. + +RFC 7440 "windowsize" support += + +barebox supports the tftp windowsize option for downloading files. It +is not implemented for uploads. + +Generally, this option greatly improves the download speed (factors +4-30 are not uncommon). But choosing a too large windowsize can have +the opposite effect. Performance depends on: + + - the network infrastructure: when the tftp server sends files with + 1Gb/s but there are components in the network (switches or the + target nic) which support only 100 Mb/s, packets will be dropped. + + The lower the internal buffers of the bottleneck components, the + lower the optimal window size. + + In practice (iMX8MP on a Netgear GS108Ev3 with a port configured to + 100 Mb/s) it had to be reduced to + + .. code-block:: console + global tftp.windowsize=26 + + for example. + + - the target network driver: datagrams from server will arive faster + than they can be processed and must be buffered internally. For + example, the `fec-imx` driver reserves place for + + .. code-block:: c + #define FEC_RBD_NUM 64 + + packets before they are dropped + + - partially the workload: copying downloaded files to ram will be + faster than burning them into flash. Latter can consume internal + buffers quicker so that windowsize might be reduced -- 2.37.2
[PATCH v4 11/21] tftp: add sanity check for OACK response
Catch bad 'blocksize' or 'windowsize' responses from the server.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 64a94797cda3..4e31adcd60ed 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -229,7 +229,7 @@ static int tftp_poll(struct file_priv *priv)
return 0;
}
-static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int
len)
+static int tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len)
{
unsigned char *opt, *val, *s;
@@ -246,7 +246,7 @@ static void tftp_parse_oack(struct file_priv *priv,
unsigned char *pkt, int len)
opt = s;
val = s + strlen(s) + 1;
if (val > s + len)
- return;
+ break;
if (!strcmp(opt, "tsize"))
priv->filesize = simple_strtoull(val, NULL, 10);
if (!strcmp(opt, "blksize"))
@@ -254,6 +254,13 @@ static void tftp_parse_oack(struct file_priv *priv,
unsigned char *pkt, int len)
pr_debug("OACK opt: %s val: %s\n", opt, val);
s = val + strlen(val) + 1;
}
+
+ if (priv->blocksize > TFTP_MTU_SIZE) {
+ pr_warn("tftp: invalid oack response\n");
+ return -EINVAL;
+ }
+
+ return 0;
}
static void tftp_timer_reset(struct file_priv *priv)
@@ -349,8 +356,14 @@ static void tftp_recv(struct file_priv *priv,
break;
case TFTP_OACK:
- tftp_parse_oack(priv, pkt, len);
priv->tftp_con->udp->uh_dport = uh_sport;
+
+ if (tftp_parse_oack(priv, pkt, len) < 0) {
+ priv->err = -EINVAL;
+ priv->state = STATE_DONE;
+ break;
+ }
+
priv->state = STATE_START;
break;
--
2.37.2
[PATCH v4 07/21] tftp: replace hardcoded blksize by global constant
Signed-off-by: Enrico Scholz --- fs/tftp.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/fs/tftp.c b/fs/tftp.c index 0b2a420b6639..264841f24407 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -65,6 +65,7 @@ #define STATE_DONE 8 #define TFTP_BLOCK_SIZE512 /* default TFTP block size */ +#define TFTP_MTU_SIZE 1432/* MTU based block size */ #define TFTP_FIFO_SIZE 4096 #define TFTP_ERR_RESEND1 @@ -133,12 +134,13 @@ static int tftp_send(struct file_priv *priv) "timeout%c" "%d%c" "blksize%c" - "1432", + "%u", priv->filename + 1, '\0', '\0', /* "octet" */ '\0', /* "timeout" */ TIMEOUT, '\0', - '\0'); /* "blksize" */ + '\0', /* "blksize" */ + TFTP_MTU_SIZE); pkt++; if (!priv->push) -- 2.37.2
[PATCH v4 15/21] tftp: detect out-of-memory situations
it should never happen due to the program logic; but detect a failed
kfifo_put() just in case...
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 5b0dcb2b75d6..e62ff2a80bb5 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -301,6 +301,8 @@ err:
static void tftp_put_data(struct file_priv *priv, uint16_t block,
void const *pkt, size_t len)
{
+ unsigned int sz;
+
if (len > priv->blocksize) {
pr_warn("tftp: oversized packet (%zu > %d) received\n",
len, priv->blocksize);
@@ -309,9 +311,14 @@ static void tftp_put_data(struct file_priv *priv, uint16_t
block,
priv->last_block = block;
- kfifo_put(priv->fifo, pkt, len);
+ sz = kfifo_put(priv->fifo, pkt, len);
- if (len < priv->blocksize) {
+ if (sz != len) {
+ pr_err("tftp: not enough room in kfifo (only %u out of %zu
written\n",
+ sz, len);
+ priv->err = -ENOMEM;
+ priv->state = STATE_DONE;
+ } else if (len < priv->blocksize) {
tftp_send(priv);
priv->err = 0;
priv->state = STATE_DONE;
--
2.37.2
[PATCH v4 08/21] tftp: remove sanity check of first block
With tftp window size support in the next patches, the first received
block might be !=1 (e.g. when it was reordered or dropped). There could
be checked whether it is in the first window, but the corresponding
sanity check can be dropped completely:
- OACK logic verifies that we speak with a tftp server (which always
sends block #1 as the first one). Diagnostic will help only with non
rfc 2743 servers (which are probably very rare resp. non existing
nowadays)
- the code some lines later handles this case already
Remove the check and simplify things.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 8
1 file changed, 8 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 264841f24407..51cb1109d2ff 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -321,14 +321,6 @@ static void tftp_recv(struct file_priv *priv,
priv->state = STATE_RDATA;
priv->tftp_con->udp->uh_dport = uh_sport;
priv->last_block = 0;
-
- if (priv->block != 1) { /* Assertion */
- pr_err("error: First block is not block 1
(%d)\n",
- priv->block);
- priv->err = -EINVAL;
- priv->state = STATE_DONE;
- break;
- }
}
if (priv->block == priv->last_block)
--
2.37.2
[PATCH v4 03/21] cmd:tftp: add '-P' option to set tftp server port number
Signed-off-by: Enrico Scholz
---
commands/tftp.c | 22 +++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/commands/tftp.c b/commands/tftp.c
index 48ff00c6217c..6ac822c9e832 100644
--- a/commands/tftp.c
+++ b/commands/tftp.c
@@ -21,15 +21,24 @@ static int do_tftpb(int argc, char *argv[])
char *source, *dest, *freep;
int opt;
int tftp_push = 0;
+ int port = -1;
int ret;
IPaddr_t ip;
char ip4_str[sizeof("255.255.255.255")];
+ char mount_opts[sizeof("port=12345")];
- while ((opt = getopt(argc, argv, "p")) > 0) {
+ while ((opt = getopt(argc, argv, "pP:")) > 0) {
switch(opt) {
case 'p':
tftp_push = 1;
break;
+ case 'P':
+ port = simple_strtoul(optarg, NULL, 0);
+ if (port <= 0 || port > 0x) {
+ pr_err("invalid port '%s'\n", optarg);
+ return COMMAND_ERROR_USAGE;
+ }
+ break;
default:
return COMMAND_ERROR_USAGE;
}
@@ -59,7 +68,13 @@ static int do_tftpb(int argc, char *argv[])
ip = net_get_serverip();
sprintf(ip4_str, "%pI4", );
- ret = mount(ip4_str, "tftp", TFTP_MOUNT_PATH, NULL);
+
+ if (port >= 0)
+ sprintf(mount_opts, "port=%u", port);
+ else
+ mount_opts[0] = '\0';
+
+ ret = mount(ip4_str, "tftp", TFTP_MOUNT_PATH, mount_opts);
if (ret)
goto err_rmdir;
@@ -84,12 +99,13 @@ BAREBOX_CMD_HELP_TEXT("server address is taken from the
environment (ethX.server
BAREBOX_CMD_HELP_TEXT("")
BAREBOX_CMD_HELP_TEXT("Options:")
BAREBOX_CMD_HELP_OPT ("-p", "push to TFTP server")
+BAREBOX_CMD_HELP_OPT ("-P PORT", "tftp server port number")
BAREBOX_CMD_HELP_END
BAREBOX_CMD_START(tftp)
.cmd= do_tftpb,
BAREBOX_CMD_DESC("load (or save) a file using TFTP")
- BAREBOX_CMD_OPTS("[-p] SOURCE [DEST]")
+ BAREBOX_CMD_OPTS("[-p] [-P ] SOURCE [DEST]")
BAREBOX_CMD_GROUP(CMD_GRP_NET)
BAREBOX_CMD_HELP(cmd_tftp_help)
BAREBOX_CMD_END
--
2.37.2
[PATCH v4 20/21] tftp: accept OACK + DATA datagrams only in certain states
These packets are valid in certain points of the transfer only and
accepting them too early or too late can corrupt internal states.
Reject them when they are unexpected.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 12
1 file changed, 12 insertions(+)
diff --git a/fs/tftp.c b/fs/tftp.c
index a9cc0ff3b118..2bffae2bf36e 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -713,6 +713,12 @@ static void tftp_recv(struct file_priv *priv,
break;
case TFTP_OACK:
+ if (priv->state != STATE_RRQ && priv->state != STATE_WRQ) {
+ pr_warn("OACK packet in %s state\n",
+ tftp_states[priv->state]);
+ break;
+ }
+
priv->tftp_con->udp->uh_dport = uh_sport;
if (tftp_parse_oack(priv, pkt, len) < 0) {
@@ -741,6 +747,12 @@ static void tftp_recv(struct file_priv *priv,
break;
}
+ if (priv->state != STATE_RDATA) {
+ pr_warn("DATA packet in %s state\n",
+ tftp_states[priv->state]);
+ break;
+ }
+
tftp_handle_data(priv, block, pkt + 2, len);
break;
--
2.37.2
[PATCH v4 14/21] tftp: refactor data processing
move block handling into dedicated function
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 37 +
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 16e57daaef37..5b0dcb2b75d6 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -298,6 +298,26 @@ err:
return priv->err;
}
+static void tftp_put_data(struct file_priv *priv, uint16_t block,
+ void const *pkt, size_t len)
+{
+ if (len > priv->blocksize) {
+ pr_warn("tftp: oversized packet (%zu > %d) received\n",
+ len, priv->blocksize);
+ return;
+ }
+
+ priv->last_block = block;
+
+ kfifo_put(priv->fifo, pkt, len);
+
+ if (len < priv->blocksize) {
+ tftp_send(priv);
+ priv->err = 0;
+ priv->state = STATE_DONE;
+ }
+}
+
static void tftp_recv(struct file_priv *priv,
uint8_t *pkt, unsigned len, uint16_t uh_sport)
{
@@ -390,23 +410,8 @@ static void tftp_recv(struct file_priv *priv,
/* Same block again; ignore it. */
break;
- if (len > priv->blocksize) {
- pr_warn("tftp: oversized packet (%u > %d) received\n",
- len, priv->blocksize);
- break;
- }
-
- priv->last_block = priv->block;
-
tftp_timer_reset(priv);
-
- kfifo_put(priv->fifo, pkt + 2, len);
-
- if (len < priv->blocksize) {
- tftp_send(priv);
- priv->err = 0;
- priv->state = STATE_DONE;
- }
+ tftp_put_data(priv, priv->block, pkt + 2, len);
break;
--
2.37.2
[PATCH v4 18/21] tftp: reorder tftp packets
With the tftp "windowsize" option, reordering of udp datagrams becomes
an issue. Depending on the network topology, this reordering occurs
several times with large tftp transfers and will heavily reduce the
transfer speed.
This patch adds a packet cache so that datagrams can be reassembled in
the correct order.
Because it increases memory usage and barebox binary size, it is an
Kconfig option.
bloat-o-meter reports with a non-zero FS_TFTP_REORDER_CACHE_SIZE
| add/remove: 3/0 grow/shrink: 4/0 up/down: 916/0 (916)
| Function old new delta
| tftp_handler 9201244+324
| tftp_put_data - 184+184
| tftp_window_cache_remove - 124+124
| tftp_window_cache_get_pos - 120+120
| tftp_allocate_transfer 104 188 +84
| tftp_do_close260 312 +52
| tftp_send384 412 +28
| Total: Before=630104, After=631020, chg +0.15%
After setting FS_TFTP_REORDER_CACHE_SIZE Kconfig option to 0, numbers
are going down to
| add/remove: 0/0 grow/shrink: 3/0 up/down: 152/0 (152)
| Function old new delta
| tftp_handler 9201012 +92
| tftp_allocate_transfer 104 136 +32
| tftp_send384 412 +28
| Total: Before=630104, After=630256, chg +0.02%
Signed-off-by: Enrico Scholz
---
fs/Kconfig | 22
fs/tftp.c | 307 +++--
2 files changed, 323 insertions(+), 6 deletions(-)
diff --git a/fs/Kconfig b/fs/Kconfig
index 0c4934285942..cf884e0646a1 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -57,6 +57,28 @@ config FS_TFTP_MAX_WINDOW_SIZE
Requires tftp "windowsize" (RFC 7440) support on server side
to have an effect.
+config FS_TFTP_REORDER_CACHE_SIZE
+ int
+ prompt "number of out-of-order tftp packets to be cached"
+ depends on FS_TFTP
+ default 16 if FS_TFTP_MAX_WINDOW_SIZE > 16
+ default 0 if FS_TFTP_MAX_WINDOW_SIZE = 1
+## TODO: it should be 'FS_TFTP_MAX_WINDOW_SIZE - 1' but this
+## is not supported by Kconfig
+ default FS_TFTP_MAX_WINDOW_SIZE
+ range 0 FS_TFTP_MAX_WINDOW_SIZE
+ help
+ UDP allows reordering of datagrams; with this option,
+ unexpected tftp packets will be cached and later
+ reassembled. This increases stability of the tftp download
+ with the cost of memory (around 1440 bytes per cache
+ element) and barebox binary size (around 700 bytes).
+
+ A value of 0 disables caching.
+
+ Requires tftp "windowsize" (RFC 7440) support on server side
+ to have an effect.
+
config FS_OMAP4_USBBOOT
bool
prompt "Filesystem over usb boot"
diff --git a/fs/tftp.c b/fs/tftp.c
index c6c7a2e3c887..b010a6be6dbb 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -72,6 +73,12 @@
#define TFTP_MTU_SIZE 1432/* MTU based block size */
#define TFTP_MAX_WINDOW_SIZE CONFIG_FS_TFTP_MAX_WINDOW_SIZE
+/* size of cache which deals with udp reordering */
+#define TFTP_WINDOW_CACHE_NUM CONFIG_FS_TFTP_REORDER_CACHE_SIZE
+
+/* marker for an emtpy 'tftp_cache' */
+#define TFTP_CACHE_NO_ID (-1)
+
#define TFTP_ERR_RESEND1
#ifdef DEBUG
@@ -85,6 +92,30 @@
static int g_tftp_window_size = DIV_ROUND_UP(TFTP_MAX_WINDOW_SIZE, 2);
+struct tftp_block {
+ uint16_t id;
+ uint16_t len;
+
+ struct list_head head;
+ uint8_t data[];
+};
+
+struct tftp_cache {
+ /* The id located at @pos or TFTP_CACHE_NO_ID when cache is empty. It
+ is possible that the corresponding bit in @used is NOT set. */
+ unsigned intid;
+ unsigned intpos;
+
+ /* bitmask */
+ unsigned long used[BITS_TO_LONGS(TFTP_WINDOW_CACHE_NUM)];
+
+ /* size of the cache; is limited by TFTP_WINDOW_CACHE_NUM and the
+ actual window size */
+ unsigned intsize;
+ unsigned intblock_len;
+ struct tftp_block *blocks[TFTP_WINDOW_CACHE_NUM];
+};
+
struct file_priv {
struct net_connection *tftp_con;
int push;
@@ -102,12 +133,222 @@ struct file_priv {
int blocksize;
unsigned int windowsize;
bool is_getattr;
+ struct tftp_cache cache;
};
struct tftp_priv {
IPaddr_t server;
};
+static inline bool is_block_before(uint16_t a, uint16_t b)
+{
+ return (int16_t)(b - a) > 0;
+}
+
+static inline uint16_t get_block_delta(uint16_t a, uint16_t b)
+{
+ debug_assert(!is_block_
[PATCH v4 17/21] tftp: do not use 'priv->block' for RRQ
attribute is not used outside tftp_recv() for RRQ.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 88767223b052..c6c7a2e3c887 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -425,7 +425,7 @@ static void tftp_recv(struct file_priv *priv,
case TFTP_DATA:
len -= 2;
- priv->block = ntohs(*(uint16_t *)pkt);
+ block = ntohs(*(uint16_t *)pkt);
if (priv->state == STATE_RRQ) {
/* first block received; entered only with non rfc
@@ -440,11 +440,11 @@ static void tftp_recv(struct file_priv *priv,
break;
}
- if (priv->block != (uint16_t)(priv->last_block + 1))
+ if (block != (uint16_t)(priv->last_block + 1))
break;
tftp_timer_reset(priv);
- tftp_put_data(priv, priv->block, pkt + 2, len);
+ tftp_put_data(priv, block, pkt + 2, len);
break;
--
2.37.2
[PATCH 7/8] tftp: fix WRQ support
"tftp: allocate buffers and fifo dynamically" broke WRQ support.
Reenable it.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 30 ++
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index c00857ecfa28..174365d6ed0a 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -673,22 +673,36 @@ static void tftp_recv(struct file_priv *priv,
if (!priv->push)
break;
- priv->block = ntohs(*(uint16_t *)pkt);
- if (priv->block != priv->last_block) {
- pr_vdebug("ack %d != %d\n", priv->block,
priv->last_block);
+ block = ntohs(*(uint16_t *)pkt);
+ if (block != priv->last_block) {
+ pr_vdebug("ack %d != %d\n", block, priv->last_block);
break;
}
- priv->block++;
+ switch (priv->state) {
+ case STATE_WRQ:
+ priv->tftp_con->udp->uh_dport = uh_sport;
+ priv->state = STATE_START;
+ break;
- tftp_timer_reset(priv);
+ case STATE_WAITACK:
+ priv->state = STATE_WDATA;
+ break;
- if (priv->state == STATE_LAST) {
+ case STATE_LAST:
priv->state = STATE_DONE;
break;
+
+ default:
+ pr_warn("ACK packet in %s state\n",
+ tftp_states[priv->state]);
+ goto ack_out;
}
- priv->tftp_con->udp->uh_dport = uh_sport;
- priv->state = STATE_WDATA;
+
+ priv->block = block + 1;
+ tftp_timer_reset(priv);
+
+ ack_out:
break;
case TFTP_OACK:
--
2.37.2
[PATCH 8/8] tftp: add some documentation about windowsize support
Signed-off-by: Enrico Scholz --- Documentation/filesystems/tftp.rst | 38 ++ 1 file changed, 38 insertions(+) diff --git a/Documentation/filesystems/tftp.rst b/Documentation/filesystems/tftp.rst index a292765e2511..8929213d3c4a 100644 --- a/Documentation/filesystems/tftp.rst +++ b/Documentation/filesystems/tftp.rst @@ -20,3 +20,41 @@ Example: In addition to the TFTP filesystem implementation, barebox does also have a :ref:`tftp command `. + +RFC 7440 "windowsize" support += + +barebox supports the tftp windowsize option for downloading files. It +is not implemented for uploads. + +Generally, this option greatly improves the download speed (factors +4-30 are not uncommon). But choosing a too large windowsize can have +the opposite effect. Performance depends on: + + - the network infrastructure: when the tftp server sends files with + 1Gb/s but there are components in the network (switches or the + target nic) which support only 100 Mb/s, packets will be dropped. + + The lower the internal buffers of the bottleneck components, the + lower the optimal window size. + + In practice (iMX8MP on a Netgear GS108Ev3 with a port configured to + 100 Mb/s) it had to be reduced to + + .. code-block:: console + global tftp.windowsize=26 + + for example. + + - the target network driver: datagrams from server will arive faster + than they can be processed and must be buffered internally. For + example, the `fec-imx` driver reserves place for + + .. code-block:: c + #define FEC_RBD_NUM 64 + + packets before they are dropped + + - partially the workload: copying downloaded files to ram will be + faster than burning them into flash. Latter can consume internal + buffers quicker so that windowsize might be reduced -- 2.37.2
[PATCH 3/8] tftp: split out allocation and cache initialization
Refactors existing code in a new function.
It also updates 'priv->state' now in error state.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 46 +++---
1 file changed, 31 insertions(+), 15 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index aaeb19590e93..610483d23c40 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -619,6 +619,31 @@ static void tftp_handle_data(struct file_priv *priv,
uint16_t block,
}
}
+static int tftp_allocate_transfer(struct file_priv *priv)
+{
+ debug_assert(!priv->fifo);
+
+ /* multiplication is safe; both operands were checked in
tftp_parse_oack()
+ and are small integers */
+ priv->fifo = kfifo_alloc(priv->blocksize * priv->windowsize);
+ if (!priv->fifo)
+ return -ENOMEM;
+
+ if (priv->push) {
+ priv->buf = xmalloc(priv->blocksize);
+ if (!priv->buf) {
+ kfifo_free(priv->fifo);
+ priv->fifo = NULL;
+ return -ENOMEM;
+ }
+ } else {
+ tftp_window_cache_init(>cache,
+ priv->blocksize, priv->windowsize);
+ }
+
+ return 0;
+}
+
static void tftp_recv(struct file_priv *priv,
uint8_t *pkt, unsigned len, uint16_t uh_sport)
{
@@ -723,22 +748,13 @@ static void tftp_handler(void *ctx, char *packet,
unsigned len)
static int tftp_start_transfer(struct file_priv *priv)
{
- /* multiplication is safe; both operands where checked in
tftp_parse_oack()
- and are small integers */
- priv->fifo = kfifo_alloc(priv->blocksize * priv->windowsize);
- if (!priv->fifo)
- return -ENOMEM;
+ int rc;
- if (priv->push) {
- priv->buf = xmalloc(priv->blocksize);
- if (!priv->buf) {
- kfifo_free(priv->fifo);
- priv->fifo = NULL;
- return -ENOMEM;
- }
- } else {
- tftp_window_cache_init(>cache,
- priv->blocksize, priv->windowsize);
+ rc = tftp_allocate_transfer(priv);
+ if (rc < 0) {
+ priv->err = rc;
+ priv->state = STATE_DONE;
+ return rc;
}
if (priv->push) {
--
2.37.2
[PATCH 1/8] tftp: make debug_assert() critical when selftest is enabled.
Run BUG_ON() when self test is enabled.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index e01aafce47b5..783413797251 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -84,7 +84,7 @@
#define TFTP_ERR_RESEND1
-#ifdef DEBUG
+#if defined(DEBUG) || IS_ENABLED(CONFIG_SELFTEST_TFTP)
# define debug_assert(_cond) BUG_ON(!(_cond))
#else
# define debug_assert(_cond) do { \
--
2.37.2
[PATCH 2/8] tftp: remove sanity check of first block
With tftp window size support, the first received block might be !=
1 (e.g. when it was reordered or dropped). There could be checked
against '!in_window(block, 1, priv->windowsize)', but the corresponding
sanity check can be dropped completely:
- OACK logic verifies that we speak with a tftp server (which always
sends block #1 as the first one)
- the code some lines later handles this case already
Remove the check and simplify things.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 8
1 file changed, 8 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 783413797251..aaeb19590e93 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -686,14 +686,6 @@ static void tftp_recv(struct file_priv *priv,
priv->tftp_con->udp->uh_dport = uh_sport;
priv->last_block = 0;
tftp_window_cache_reset(>cache);
-
- if (block != 1) { /* Assertion */
- pr_err("error: First block is not block 1
(%d)\n",
- block);
- priv->err = -EINVAL;
- priv->state = STATE_DONE;
- break;
- }
}
tftp_handle_data(priv, block, pkt + 2, len);
--
2.37.2
[PATCH 6/8] tftp: do not set 'tsize' in wrq requests
The filesize is not known for push requests and barebox always sent '0'. Server might reject data because it will always exceed this length. Send this option only for rrq requests. Signed-off-by: Enrico Scholz --- fs/tftp.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/fs/tftp.c b/fs/tftp.c index 7edea904aabe..c00857ecfa28 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -403,22 +403,26 @@ static int tftp_send(struct file_priv *priv) "octet%c" "timeout%c" "%d%c" - "tsize%c" - "%lld%c" "blksize%c" "%u", priv->filename + 1, '\0', '\0', /* "octet" */ '\0', /* "timeout" */ TIMEOUT, '\0', - '\0', /* "tsize" */ - priv->filesize, '\0', '\0', /* "blksize" */ /* use only a minimal blksize for getattr operations, */ priv->is_getattr ? TFTP_BLOCK_SIZE : TFTP_MTU_SIZE); pkt++; + if (!priv->push) + /* we do not know the filesize in WRQ requests and + 'priv->filesize' will always be zero */ + pkt += sprintf((unsigned char *)pkt, + "tsize%c%lld%c", + '\0', priv->filesize, + '\0'); + if (window_size > 1) pkt += sprintf((unsigned char *)pkt, "windowsize%c%u%c", -- 2.37.2
[PATCH 4/8] tftp: accept OACK + DATA datagrams only in certain states
These packets are valid in certain points of the transfer only and
accepting them too early or too late can corrupt internal states.
Reject them when they are unexpected.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 12
1 file changed, 12 insertions(+)
diff --git a/fs/tftp.c b/fs/tftp.c
index 610483d23c40..fb6c368b3a64 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -690,6 +690,12 @@ static void tftp_recv(struct file_priv *priv,
break;
case TFTP_OACK:
+ if (priv->state != STATE_RRQ && priv->state != STATE_WRQ) {
+ pr_warn("OACK packet in %s state\n",
+ tftp_states[priv->state]);
+ break;
+ }
+
priv->tftp_con->udp->uh_dport = uh_sport;
if (tftp_parse_oack(priv, pkt, len) < 0) {
@@ -713,6 +719,12 @@ static void tftp_recv(struct file_priv *priv,
tftp_window_cache_reset(>cache);
}
+ if (priv->state != STATE_RDATA) {
+ pr_warn("DATA packet in %s state\n",
+ tftp_states[priv->state]);
+ break;
+ }
+
tftp_handle_data(priv, block, pkt + 2, len);
break;
--
2.37.2
[PATCH 0/8] tftp fixups
The "tftp: allocate buffers and fifo dynamically" patch in the last patchset broke interaction with non rfc 2347 servers (e.g. when data transfer starts immediately after RRQ/WRQ without the OACK negotiation phase). This has been fixed for both RRQ and WRQ requests. For WRQ requests (push), the "tsize" option will not be sent anymore because it is always '0' and can confuse servers. New patches add some sanity checks which prevent modification of internal information (blocksize or port numbers) when OACK packets arrive in the middle of a transfer. Enrico Scholz (8): tftp: make debug_assert() critical when selftest is enabled. tftp: remove sanity check of first block tftp: split out allocation and cache initialization tftp: accept OACK + DATA datagrams only in certain states tftp: support non rfc 2347 servers tftp: do not set 'tsize' in wrq requests tftp: fix WRQ support tftp: add some documentation about windowsize support Documentation/filesystems/tftp.rst | 38 ++ fs/tftp.c | 189 +++-- 2 files changed, 166 insertions(+), 61 deletions(-) -- 2.37.2
[PATCH 5/8] tftp: support non rfc 2347 servers
State machine must be changed for servers without OACK support. Here,
objects for data transfer must be allocated before the first datagram
is handled and it is possible that whole transfer finishes at this
point.
Fixes "tftp: allocate buffers and fifo dynamically"
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 87 ---
1 file changed, 58 insertions(+), 29 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index fb6c368b3a64..7edea904aabe 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -64,13 +64,12 @@
#define STATE_WRQ 2
#define STATE_RDATA3
#define STATE_WDATA4
-#define STATE_OACK 5
+/* OACK from server has been received and we can begin to sent either the ACK
+ (for RRQ) or data (for WRQ) */
+#define STATE_START5
#define STATE_WAITACK 6
#define STATE_LAST 7
#define STATE_DONE 8
-/* OACK from server has been received and we can begin to sent either the ACK
- (for RRQ) or data (for WRQ) */
-#define STATE_START9
#define TFTP_BLOCK_SIZE512 /* default TFTP block size */
#define TFTP_MTU_SIZE 1432/* MTU based block size */
@@ -363,7 +362,6 @@ static char const * const tftp_states[] = {
[STATE_WRQ] = "WRQ",
[STATE_RDATA] = "RDATA",
[STATE_WDATA] = "WDATA",
- [STATE_OACK] = "OACK",
[STATE_WAITACK] = "WAITACK",
[STATE_LAST] = "LAST",
[STATE_DONE] = "DONE",
@@ -431,7 +429,6 @@ static int tftp_send(struct file_priv *priv)
break;
case STATE_RDATA:
- case STATE_OACK:
xp = pkt;
s = (uint16_t *)pkt;
*s++ = htons(TFTP_ACK);
@@ -649,6 +646,7 @@ static void tftp_recv(struct file_priv *priv,
{
uint16_t opcode;
uint16_t block;
+ int rc;
/* according to RFC1350 minimal tftp packet length is 4 bytes */
if (len < 4)
@@ -711,12 +709,20 @@ static void tftp_recv(struct file_priv *priv,
len -= 2;
block = ntohs(*(uint16_t *)pkt);
- if (priv->state == STATE_RRQ || priv->state == STATE_OACK) {
- /* first block received */
- priv->state = STATE_RDATA;
+ if (priv->state == STATE_RRQ) {
+ /* first block received; entered only with non rfc
+ 2347 (TFTP Option extension) compliant servers */
priv->tftp_con->udp->uh_dport = uh_sport;
+ priv->state = STATE_RDATA;
priv->last_block = 0;
- tftp_window_cache_reset(>cache);
+ priv->ack_block = priv->windowsize;
+
+ rc = tftp_allocate_transfer(priv);
+ if (rc < 0) {
+ priv->err = rc;
+ priv->state = STATE_DONE;
+ break;
+ }
}
if (priv->state != STATE_RDATA) {
@@ -726,7 +732,6 @@ static void tftp_recv(struct file_priv *priv,
}
tftp_handle_data(priv, block, pkt + 2, len);
-
break;
case TFTP_ERROR:
@@ -775,7 +780,7 @@ static int tftp_start_transfer(struct file_priv *priv)
priv->block = 1;
} else {
/* send ACK */
- priv->state = STATE_OACK;
+ priv->state = STATE_RDATA;
priv->last_block = 0;
tftp_send(priv);
}
@@ -828,26 +833,49 @@ static struct file_priv *tftp_do_open(struct device_d
*dev,
goto out1;
tftp_timer_reset(priv);
- while (priv->state != STATE_DONE && priv->state != STATE_START) {
- ret = tftp_poll(priv);
- if (ret == TFTP_ERR_RESEND)
- tftp_send(priv);
- if (ret < 0)
- goto out1;
- }
- if (priv->state == STATE_DONE) {
- /* this should not happen; STATE_DONE without error happens
- after completing the transfer but this has not been started
- yet */
- if (WARN_ON(priv->err == 0))
- priv->err = -EIO;
+ /* - 'ret < 0' ... error
+ - 'ret == 0' ... further tftp_poll() required
+ - 'ret == 1' ... startup finished */
+ do {
+ switch (priv->state) {
+ case STATE_DONE:
+ /* branch is entered in two situations:
+ - non rfc 2347 compliant servers finished the
+transfer by sending a small file
+ - some error occurred */
+
Re: [PATCH v3 01/18] progress: add close_progress() to display some statistics
Sascha Hauer writes:
>> +void close_progress(loff_t total)
>> +{
>
> show_progress() doesn't necessarily show the progress in bytes, it could
> also be percent, megabytes or anything else. We could maybe rename this
> to close_progress_bytes().
When I started this patch, I had an
| enum progress_summary {
| SUMMARY_NONE,
| SUMMARY_BYTES,
| }
argument in this function. But it felt too overdesigned at this time.
> Also it would be nice to print the sizes in human readable form, maybe
> size_human_readable() could be used here.
Yes; I played with human readable sizes/speeds too. But for developing,
I needed the exact size.
Support for the "'" grouping flag in printf() might be a more generic
approach.
Enrico
Re: [PATCH v3 00/18] add "windowsize" (RFC 7440) support for tftp
Sascha Hauer writes: >> The tftp "windowsize" greatly improves the performance of tftp >> transfers. This patchset adds support for it. > > There's a variant of the tftp-hpa package with RFC7440 support here: > https://github.com/ClausKlein/tftp-hpa.git thx; seems to work with it. I had to comment out the setuid() stuff to run it as an ordinary user but can start it then as | ./tftpd/tftpd --address [::]:1230 -s /var/lib/tftpboot/ -vvv -l -L -p In barebox then :/ tftp -P 1230 data-100MiB /tmp/a && sha1sum /tmp/a [] 104857600 bytes, 100631094 bytes/s Enrico
[PATCH v3 10/18] tftp: record whether tftp file is opened for lookup operation only
Opening a tftp is done in two steps: at first `tftp_lookup()` is
called to get the filesize and then it is opened again and data are
read.
The `tftp_lookup()` call sends only a RRQ/WRQ, reads then the "tsize"
from the response and closes the transfer by sending an error datagram.
The tftp server will send a full data window.
To prevent unneeded traffic, later patches set parameters to reduce
the size of the server response.
We need knowledge about type of operation which is recorded in an
"is_getattr" attribute.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 38a9e955ee92..9f881249ce07 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -87,6 +87,7 @@ struct file_priv {
void *buf;
int blocksize;
int block_requested;
+ bool is_getattr;
};
struct tftp_priv {
@@ -415,7 +416,7 @@ static int tftp_start_transfer(struct file_priv *priv)
}
static struct file_priv *tftp_do_open(struct device_d *dev,
- int accmode, struct dentry *dentry)
+ int accmode, struct dentry *dentry, bool is_getattr)
{
struct fs_device_d *fsdev = dev_to_fs_device(dev);
struct file_priv *priv;
@@ -444,6 +445,7 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
priv->filename = dpath(dentry, fsdev->vfsmount.mnt_root);
priv->blocksize = TFTP_BLOCK_SIZE;
priv->block_requested = -1;
+ priv->is_getattr = is_getattr;
parseopt_hu(fsdev->options, "port", );
@@ -494,7 +496,7 @@ static int tftp_open(struct device_d *dev, FILE *file,
const char *filename)
{
struct file_priv *priv;
- priv = tftp_do_open(dev, file->flags, file->dentry);
+ priv = tftp_do_open(dev, file->flags, file->dentry, false);
if (IS_ERR(priv))
return PTR_ERR(priv);
@@ -710,7 +712,7 @@ static struct dentry *tftp_lookup(struct inode *dir, struct
dentry *dentry,
struct file_priv *priv;
loff_t filesize;
- priv = tftp_do_open(>dev, O_RDONLY, dentry);
+ priv = tftp_do_open(>dev, O_RDONLY, dentry, true);
if (IS_ERR(priv))
return NULL;
--
2.37.1
[PATCH v3 08/18] tftp: allocate buffers and fifo dynamically
Use the actual blocksize for allocating buffers instead of assuming an
hardcoded value.
This requires to add an additional 'START' state which is entered
after receiving (RRQ) or sending (WRQ) the OACK. Without it, the next
state would be entered and the (not allocated yet) fifo be used.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 91 ---
1 file changed, 59 insertions(+), 32 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index b552a5dc2f63..0b6d57c4603f 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -63,10 +63,12 @@
#define STATE_WAITACK 6
#define STATE_LAST 7
#define STATE_DONE 8
+/* OACK from server has been received and we can begin to sent either the ACK
+ (for RRQ) or data (for WRQ) */
+#define STATE_START9
#define TFTP_BLOCK_SIZE512 /* default TFTP block size */
#define TFTP_MTU_SIZE 1432/* MTU based block size */
-#define TFTP_FIFO_SIZE 4096
#define TFTP_ERR_RESEND1
@@ -106,6 +108,7 @@ static char const * const tftp_states[] = {
[STATE_WAITACK] = "WAITACK",
[STATE_LAST] = "LAST",
[STATE_DONE] = "DONE",
+ [STATE_START] = "START",
};
static int tftp_send(struct file_priv *priv)
@@ -294,19 +297,9 @@ static void tftp_recv(struct file_priv *priv,
case TFTP_OACK:
tftp_parse_oack(priv, pkt, len);
priv->tftp_con->udp->uh_dport = uh_sport;
-
- if (priv->push) {
- /* send first block */
- priv->state = STATE_WDATA;
- priv->block = 1;
- } else {
- /* send ACK */
- priv->state = STATE_OACK;
- priv->block = 0;
- tftp_send(priv);
- }
-
+ priv->state = STATE_START;
break;
+
case TFTP_DATA:
len -= 2;
priv->block = ntohs(*(uint16_t *)pkt);
@@ -330,6 +323,12 @@ static void tftp_recv(struct file_priv *priv,
/* Same block again; ignore it. */
break;
+ if (len > priv->blocksize) {
+ pr_warn("tftp: oversized packet (%u > %d) received\n",
+ len, priv->blocksize);
+ break;
+ }
+
priv->last_block = priv->block;
tftp_timer_reset(priv);
@@ -372,6 +371,36 @@ static void tftp_handler(void *ctx, char *packet, unsigned
len)
tftp_recv(priv, pkt, net_eth_to_udplen(packet), udp->uh_sport);
}
+
+static int tftp_start_transfer(struct file_priv *priv)
+{
+ priv->fifo = kfifo_alloc(priv->blocksize);
+ if (!priv->fifo)
+ return -ENOMEM;
+
+ if (priv->push) {
+ priv->buf = xmalloc(priv->blocksize);
+ if (!priv->buf) {
+ kfifo_free(priv->fifo);
+ priv->fifo = NULL;
+ return -ENOMEM;
+ }
+ }
+
+ if (priv->push) {
+ /* send first block */
+ priv->state = STATE_WDATA;
+ priv->block = 1;
+ } else {
+ /* send ACK */
+ priv->state = STATE_OACK;
+ priv->block = 0;
+ tftp_send(priv);
+ }
+
+ return 0;
+}
+
static struct file_priv *tftp_do_open(struct device_d *dev,
int accmode, struct dentry *dentry)
{
@@ -403,47 +432,45 @@ static struct file_priv *tftp_do_open(struct device_d
*dev,
priv->blocksize = TFTP_BLOCK_SIZE;
priv->block_requested = -1;
- priv->fifo = kfifo_alloc(TFTP_FIFO_SIZE);
- if (!priv->fifo) {
- ret = -ENOMEM;
- goto out;
- }
-
parseopt_hu(fsdev->options, "port", );
priv->tftp_con = net_udp_new(tpriv->server, port, tftp_handler, priv);
if (IS_ERR(priv->tftp_con)) {
ret = PTR_ERR(priv->tftp_con);
- goto out1;
+ goto out;
}
ret = tftp_send(priv);
if (ret)
- goto out2;
+ goto out1;
tftp_timer_reset(priv);
- while (priv->state != STATE_RDATA &&
- priv->state != STATE_DONE &&
- priv->state != STATE_WDATA) {
+ while (priv->state != STATE_DONE && priv->state != STATE_START) {
ret = tftp_poll(priv);
if (ret == TFTP_ERR_RESEND)
tftp_send(priv);
if (ret < 0)
- goto out2;
+ goto out1;
}
- if (priv->state
[PATCH v3 12/18] tftp: refactor data processing
move block handling into dedicated function
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 37 +
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 2ed62b77901b..2bb9361d8929 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -260,6 +260,26 @@ static void tftp_timer_reset(struct file_priv *priv)
priv->progress_timeout = priv->resend_timeout = get_time_ns();
}
+static void tftp_put_data(struct file_priv *priv, uint16_t block,
+ void const *pkt, size_t len)
+{
+ if (len > priv->blocksize) {
+ pr_warn("tftp: oversized packet (%zu > %d) received\n",
+ len, priv->blocksize);
+ return;
+ }
+
+ priv->last_block = block;
+
+ kfifo_put(priv->fifo, pkt, len);
+
+ if (len < priv->blocksize) {
+ tftp_send(priv);
+ priv->err = 0;
+ priv->state = STATE_DONE;
+ }
+}
+
static void tftp_recv(struct file_priv *priv,
uint8_t *pkt, unsigned len, uint16_t uh_sport)
{
@@ -339,23 +359,8 @@ static void tftp_recv(struct file_priv *priv,
/* Same block again; ignore it. */
break;
- if (len > priv->blocksize) {
- pr_warn("tftp: oversized packet (%u > %d) received\n",
- len, priv->blocksize);
- break;
- }
-
- priv->last_block = priv->block;
-
tftp_timer_reset(priv);
-
- kfifo_put(priv->fifo, pkt + 2, len);
-
- if (len < priv->blocksize) {
- tftp_send(priv);
- priv->err = 0;
- priv->state = STATE_DONE;
- }
+ tftp_put_data(priv, priv->block, pkt + 2, len);
break;
--
2.37.1
[PATCH v3 06/18] tftp: minor refactoring of RRQ/WRQ packet generation code
Having 11 printf arguments with lot of them being 0, makes it difficulty to read and extend. Add some comments and use '\0' for %c. Signed-off-by: Enrico Scholz --- fs/tftp.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/fs/tftp.c b/fs/tftp.c index 913ca1df6e42..7627f3f59669 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -136,13 +136,13 @@ static int tftp_send(struct file_priv *priv) "%lld%c" "blksize%c" "1432", - priv->filename + 1, 0, - 0, - 0, - TIMEOUT, 0, - 0, - priv->filesize, 0, - 0); + priv->filename + 1, '\0', + '\0', /* "octet" */ + '\0', /* "timeout" */ + TIMEOUT, '\0', + '\0', /* "tsize" */ + priv->filesize, '\0', + '\0'); /* "blksize" */ pkt++; len = pkt - xp; break; -- 2.37.1
[PATCH v3 03/18] tftp: add some 'const' annotations
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index d186e7983a6d..c26204ae76e6 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -94,7 +94,7 @@ static int tftp_truncate(struct device_d *dev, FILE *f,
loff_t size)
return 0;
}
-static char *tftp_states[] = {
+static char const * const tftp_states[] = {
[STATE_INVALID] = "INVALID",
[STATE_RRQ] = "RRQ",
[STATE_WRQ] = "WRQ",
--
2.37.1
[PATCH v3 02/18] libfile:copy_file: show statistics in verbose mode
Signed-off-by: Enrico Scholz --- lib/libfile.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/libfile.c b/lib/libfile.c index 3b7985fbcabb..9eb06de02945 100644 --- a/lib/libfile.c +++ b/lib/libfile.c @@ -454,6 +454,9 @@ int copy_file(const char *src, const char *dst, int verbose) } } + if (verbose) + close_progress(total); + ret = 0; out: if (verbose) -- 2.37.1
[PATCH v3 00/18] add "windowsize" (RFC 7440) support for tftp
XX" mount options instead of global 'tftp.port' variable - allocate fifo and send buffer dynamically based on block- and window size of the transfer. Do not use fixed constants anymore - rewritten cache code; use bitmap based functions with O(1) complexity instead of iterating over (small) arrays - unittest for cache functions - add information about binary sizes v1 -> v2 - fixes for non rfc7440 servers --- Enrico Scholz (18): progress: add close_progress() to display some statistics libfile:copy_file: show statistics in verbose mode tftp: add some 'const' annotations tftp: allow to change tftp port cmd:tftp: add '-P' option to set tftp server port number tftp: minor refactoring of RRQ/WRQ packet generation code tftp: replace hardcoded blksize by global constant tftp: allocate buffers and fifo dynamically tftp: add sanity check for OACK response tftp: record whether tftp file is opened for lookup operation only tftp: reduce block size on lookup requests tftp: refactor data processing tftp: detect out-of-memory situations tftp: implement 'windowsize' (RFC 7440) support tftp: do not use 'priv->block' for RRQ tftp: add debug_assert() macro tftp: reorder tftp packets tftp: add selftest commands/tftp.c | 22 +- fs/Kconfig | 36 +++ fs/tftp-selftest.h | 56 fs/tftp.c | 640 +++- include/progress.h | 1 + lib/libfile.c | 3 + lib/show_progress.c | 25 ++ test/self/Kconfig | 7 + 8 files changed, 717 insertions(+), 73 deletions(-) create mode 100644 fs/tftp-selftest.h -- 2.37.1
[PATCH v3 04/18] tftp: allow to change tftp port
This adds a 'port=' mount option for tftp filesystems.
Useful e.g. when working with a local, non-privileged tftp server
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index c26204ae76e6..913ca1df6e42 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -30,6 +30,7 @@
#include
#include
#include
+#include
#include
#define TFTP_PORT 69 /* Well known TFTP port number */
@@ -376,6 +377,7 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
struct file_priv *priv;
struct tftp_priv *tpriv = dev->priv;
int ret;
+ unsigned short port = TFTP_PORT;
priv = xzalloc(sizeof(*priv));
@@ -405,8 +407,9 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
goto out;
}
- priv->tftp_con = net_udp_new(tpriv->server, TFTP_PORT, tftp_handler,
- priv);
+ parseopt_hu(fsdev->options, "port", );
+
+ priv->tftp_con = net_udp_new(tpriv->server, port, tftp_handler, priv);
if (IS_ERR(priv->tftp_con)) {
ret = PTR_ERR(priv->tftp_con);
goto out1;
--
2.37.1
[PATCH v3 01/18] progress: add close_progress() to display some statistics
With a later patch, this will produce output like
| :/ cp -v /mnt/tftp/tmp /tmp/
| [...#] 29138411 bytes, 127413 bytes/s
Signed-off-by: Enrico Scholz
---
include/progress.h | 1 +
lib/show_progress.c | 25 +
2 files changed, 26 insertions(+)
diff --git a/include/progress.h b/include/progress.h
index 7230bd3a9bd5..219d772201bf 100644
--- a/include/progress.h
+++ b/include/progress.h
@@ -16,6 +16,7 @@ void init_progression_bar(loff_t max);
* spinner is printed.
*/
void show_progress(loff_t now);
+void close_progress(loff_t total);
extern struct notifier_head progress_notifier;
diff --git a/lib/show_progress.c b/lib/show_progress.c
index 1b624bcb9af8..5b42a2d74b8f 100644
--- a/lib/show_progress.c
+++ b/lib/show_progress.c
@@ -24,6 +24,7 @@
static loff_t printed;
static loff_t progress_max;
static unsigned spin;
+static uint64_t start_tm;
void show_progress(loff_t now)
{
@@ -47,11 +48,35 @@ void show_progress(loff_t now)
}
}
+void close_progress(loff_t total)
+{
+ uint64_t now = get_time_ns();
+ unsigned long speed;
+ unsigned long delta;
+
+ if (now <= start_tm) {
+ pr_crit("tm mismatch");
+ return;
+ }
+
+ if (total < 0)
+ return;
+
+ /* convert to ms and add '1' to avoid div-by-zero */
+ delta = div_u64(now - start_tm, 100) + 1;
+
+ /* speed is bytes/s */
+ speed = div_u64(total * 1000, delta);
+
+ printf("] %llu bytes, %lu bytes/s", (unsigned long long)total, speed);
+}
+
void init_progression_bar(loff_t max)
{
printed = 0;
progress_max = max;
spin = 0;
+ start_tm = get_time_ns();
if (progress_max && progress_max != FILESIZE_MAX)
printf("\t[%*s]\r\t[", HASHES_PER_LINE, "");
else
--
2.37.1
[PATCH v3 17/18] tftp: reorder tftp packets
With the tftp "windowsize" option, reordering of udp datagrams becomes
an issue. Depending on the network topology, this reordering occurs
several times with large tftp transfers and will heavily reduce the
transfer speed.
This patch adds a packet cache so that datagrams can be reassembled in
the correct order.
Because it increases memory usage and barebox binary size, it is an
Kconfig option.
bloat-o-meter reports with a non-zero FS_TFTP_REORDER_CACHE_SIZE
| add/remove: 3/0 grow/shrink: 4/0 up/down: 920/0 (920)
| Function old new delta
| tftp_handler 8601200+340
| tftp_put_data - 184+184
| tftp_window_cache_remove - 124+124
| tftp_window_cache_get_pos - 120+120
| tftp_do_open 536 608 +72
| tftp_do_close260 312 +52
| tftp_send364 392 +28
| Total: Before=626431, After=627351, chg +0.15%
After setting FS_TFTP_REORDER_CACHE_SIZE Kconfig option to 0, numbers
are going down to
| add/remove: 0/0 grow/shrink: 3/0 up/down: 188/0 (188)
| Function old new delta
| tftp_handler 860 992+132
| tftp_send364 392 +28
| tftp_do_open 536 564 +28
| Total: Before=626431, After=626619, chg +0.03%
Signed-off-by: Enrico Scholz
---
fs/Kconfig | 22
fs/tftp.c | 307 -
2 files changed, 324 insertions(+), 5 deletions(-)
diff --git a/fs/Kconfig b/fs/Kconfig
index 0c4934285942..cf884e0646a1 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -57,6 +57,28 @@ config FS_TFTP_MAX_WINDOW_SIZE
Requires tftp "windowsize" (RFC 7440) support on server side
to have an effect.
+config FS_TFTP_REORDER_CACHE_SIZE
+ int
+ prompt "number of out-of-order tftp packets to be cached"
+ depends on FS_TFTP
+ default 16 if FS_TFTP_MAX_WINDOW_SIZE > 16
+ default 0 if FS_TFTP_MAX_WINDOW_SIZE = 1
+## TODO: it should be 'FS_TFTP_MAX_WINDOW_SIZE - 1' but this
+## is not supported by Kconfig
+ default FS_TFTP_MAX_WINDOW_SIZE
+ range 0 FS_TFTP_MAX_WINDOW_SIZE
+ help
+ UDP allows reordering of datagrams; with this option,
+ unexpected tftp packets will be cached and later
+ reassembled. This increases stability of the tftp download
+ with the cost of memory (around 1440 bytes per cache
+ element) and barebox binary size (around 700 bytes).
+
+ A value of 0 disables caching.
+
+ Requires tftp "windowsize" (RFC 7440) support on server side
+ to have an effect.
+
config FS_OMAP4_USBBOOT
bool
prompt "Filesystem over usb boot"
diff --git a/fs/tftp.c b/fs/tftp.c
index d02ca750cd6d..8a2c2299a52f 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -73,6 +74,12 @@
#define TFTP_MTU_SIZE 1432/* MTU based block size */
#define TFTP_MAX_WINDOW_SIZE CONFIG_FS_TFTP_MAX_WINDOW_SIZE
+/* size of cache which deals with udp reordering */
+#define TFTP_WINDOW_CACHE_NUM CONFIG_FS_TFTP_REORDER_CACHE_SIZE
+
+/* marker for an emtpy 'tftp_cache' */
+#define TFTP_CACHE_NO_ID (-1)
+
#define TFTP_ERR_RESEND1
#ifdef DEBUG
@@ -86,6 +93,30 @@
static int g_tftp_window_size = DIV_ROUND_UP(TFTP_MAX_WINDOW_SIZE, 2);
+struct tftp_block {
+ uint16_t id;
+ uint16_t len;
+
+ struct list_head head;
+ uint8_t data[];
+};
+
+struct tftp_cache {
+ /* The id located at @pos or TFTP_CACHE_NO_ID when cache is empty. It
+ is possible that the corresponding bit in @used is NOT set. */
+ unsigned intid;
+ unsigned intpos;
+
+ /* bitmask */
+ unsigned long used[BITS_TO_LONGS(TFTP_WINDOW_CACHE_NUM)];
+
+ /* size of the cache; is limited by TFTP_WINDOW_CACHE_NUM and the
+ actual window size */
+ unsigned intsize;
+ unsigned intblock_len;
+ struct tftp_block *blocks[TFTP_WINDOW_CACHE_NUM];
+};
+
struct file_priv {
struct net_connection *tftp_con;
int push;
@@ -103,12 +134,222 @@ struct file_priv {
int blocksize;
unsigned int windowsize;
bool is_getattr;
+ struct tftp_cache cache;
};
struct tftp_priv {
IPaddr_t server;
};
+static inline bool is_block_before(uint16_t a, uint16_t b)
+{
+ return (int16_t)(b - a) > 0;
+}
+
+static inline uint16_t get_block_delta(uint16_t a, uint16_t b)
+{
+ debug_assert(!is_block_
[PATCH v3 14/18] tftp: implement 'windowsize' (RFC 7440) support
Results (with the reorder patch; numbers in bytes/s) on an iMX8MP are:
| windowsize | VPN | 1 Gb/s | 100 Mb/s |
||---|||
| 128| 3.869.284 | 98.643.085 | 11.434.852 |
| 64| 3.863.581 | 98.550.375 | 11.434.852 |
| 48| 3.431.580 | 94.211.680 | 11.275.010 |
| 32| 2.835.129 | 85.250.081 | 10.985.605 |
| 24| 2.344.858 | 77.787.537 | 10.765.667 |
| 16| 1.734.186 | 67.519.381 | 10.210.087 |
| 12| 1.403.340 | 61.972.576 | 9.915.612 |
| 8| 1.002.462 | 50.852.376 | 9.016.130 |
| 6| 775.573 | 42.781.558 | 8.422.297 |
| 4| 547.845 | 32.066.544 | 6.835.567 |
| 3| 412.987 | 26.526.081 | 6.322.435 |
| 2| 280.987 | 19.120.641 | 5.494.241 |
| 1| 141.699 | 10.431.516 | 2.967.224 |
(VPN = OpenVPN on ADSL 50 Mb/s).
The window size can be configured at runtime.
This commit increases barebox size by
| add/remove: 1/0 grow/shrink: 4/1 up/down: 144/-16 (128)
| Function old new delta
| tftp_send316 364 +48
| tftp_init 16 60 +44
| tftp_handler 824 868 +44
| tftp_do_open 532 536 +4
| g_tftp_window_size - 4 +4
| tftp_poll180 164 -16
| Total: Before=626311, After=626439, chg +0.02%
Setting FS_TFTP_MAX_WINDOW_SIZE to zero reduces it to
| add/remove: 1/0 grow/shrink: 3/2 up/down: 92/-52 (40)
| Function old new delta
| tftp_init 16 60 +44
| tftp_handler 824 864 +40
| tftp_do_open 532 536 +4
| g_tftp_window_size - 4 +4
| tftp_poll180 164 -16
| tftp_send316 280 -36
| Total: Before=626311, After=626351, chg +0.01%
Signed-off-by: Enrico Scholz
---
fs/Kconfig | 14 ++
fs/tftp.c | 54 +-
2 files changed, 55 insertions(+), 13 deletions(-)
diff --git a/fs/Kconfig b/fs/Kconfig
index aeba00073eed..0c4934285942 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -43,6 +43,20 @@ config FS_TFTP
prompt "tftp support"
depends on NET
+config FS_TFTP_MAX_WINDOW_SIZE
+ int
+ prompt "maximum tftp window size (RFC 7440)"
+ depends on FS_TFTP
+ default 128
+ range 1 128
+ help
+ The maximum allowed tftp "windowsize" (RFC 7440). Higher
+ value increase speed of the tftp download with the cost of
+ memory (1432 bytes per slot).
+
+ Requires tftp "windowsize" (RFC 7440) support on server side
+ to have an effect.
+
config FS_OMAP4_USBBOOT
bool
prompt "Filesystem over usb boot"
diff --git a/fs/tftp.c b/fs/tftp.c
index 5c3e56a6cb22..4c3980404dc1 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -26,9 +26,11 @@
#include
#include
#include
+#include
#include
#include
#include
+#include
#include
#include
#include
@@ -69,14 +71,18 @@
#define TFTP_BLOCK_SIZE512 /* default TFTP block size */
#define TFTP_MTU_SIZE 1432/* MTU based block size */
+#define TFTP_MAX_WINDOW_SIZE CONFIG_FS_TFTP_MAX_WINDOW_SIZE
#define TFTP_ERR_RESEND1
+static int g_tftp_window_size = DIV_ROUND_UP(TFTP_MAX_WINDOW_SIZE, 2);
+
struct file_priv {
struct net_connection *tftp_con;
int push;
uint16_t block;
uint16_t last_block;
+ uint16_t ack_block;
int state;
int err;
char *filename;
@@ -86,7 +92,7 @@ struct file_priv {
struct kfifo *fifo;
void *buf;
int blocksize;
- int block_requested;
+ unsigned int windowsize;
bool is_getattr;
};
@@ -118,6 +124,7 @@ static int tftp_send(struct file_priv *priv)
int len = 0;
uint16_t *s;
unsigned char *pkt = net_udp_get_payload(priv->tftp_con);
+ unsigned int window_size;
int ret;
pr_vdebug("%s: state %s\n", __func__, tftp_states[priv->state]);
@@ -125,6 +132,15 @@ static int tftp_send(struct file_priv *priv)
switch (priv->state) {
case STATE_RRQ:
case STATE_WRQ:
+ if (priv->push || priv->is_getattr)
+ /* atm, windowsize is supported only for RRQ and there
+ is no need to request a full window when we are
+ just looking up file attributes */
+ window_size = 1;
+ else
+
[PATCH v3 18/18] tftp: add selftest
Unittest for window cache functions.
Signed-off-by: Enrico Scholz
---
fs/tftp-selftest.h | 56
fs/tftp.c | 104 +
test/self/Kconfig | 7 +++
3 files changed, 167 insertions(+)
create mode 100644 fs/tftp-selftest.h
diff --git a/fs/tftp-selftest.h b/fs/tftp-selftest.h
new file mode 100644
index ..2406ed329ecc
--- /dev/null
+++ b/fs/tftp-selftest.h
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0+
+// SPDX-FileCopyrightText: 2022 Enrico Scholz
+
+#ifndef H_BAREBOX_FS_TFTP_SELFTEST_H
+#define H_BAREBOX_FS_TFTP_SELFTEST_H
+
+#include
+
+#define _expect_fmt(_v) _Generic(_v, \
+void const *: "%p",\
+void *: "%p", \
+bool: "%d",\
+long int: "%lld", \
+long unsigned int: "%llu", \
+unsigned short: "%h", \
+signed int: "%d", \
+unsigned int: "%u")
+
+#define _expect_op(_a, _b, _op)
\
+ ({ \
+ __typeof__(_a) __a = (_a); \
+ __typeof__(_b) __b = (_b); \
+ \
+ total_tests++; \
+ \
+ if (!(__a _op __b)) { \
+ char fmt[sizeof "%s:%d: failed: %XXX %XXX\n" # _op]; \
+ strcpy(fmt, "%s:%d: failed: "); \
+ strcat(fmt, _expect_fmt(__a)); \
+ strcat(fmt, " " # _op " "); \
+ strcat(fmt, _expect_fmt(__b)); \
+ strcat(fmt, "\n"); \
+ \
+ failed_tests++; \
+ printf(fmt, __func__, __LINE__, __a, __b); \
+ } \
+ })
+
+#define _as_void(_p) ({\
+ void const *__res = (_p); \
+ __res; \
+ })
+
+#define expect_eq(_a, _b) _expect_op(_a, _b, ==);
+#define expect_ne(_a, _b) _expect_op(_a, _b, !=);
+#define expect_it(_a) _expect_op(_a, true, ==);
+
+#define expect_err(_a) _expect_op(_a, 0, <);
+#define expect_ok(_a) _expect_op(_a, 0, ==);
+
+/* _Generic() does not match 'void *' for typed pointers; cast them to raw
+ 'void *' first */
+#define expect_NULL(_a)_expect_op(_as_void(_a), NULL, ==);
+#define expect_PTR(_a) _expect_op(_as_void(_a), NULL, !=);
+
+#endif /* H_BAREBOX_FS_TFTP_SELFTEST_H */
diff --git a/fs/tftp.c b/fs/tftp.c
index 8a2c2299a52f..e01aafce47b5 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -36,6 +36,8 @@
#include
#include
+#include "tftp-selftest.h"
+
#define TFTP_PORT 69 /* Well known TFTP port number */
/* Seconds to wait before remote server is allowed to resend a lost packet */
@@ -1147,3 +1149,105 @@ static int tftp_init(void)
return register_fs_driver(_driver);
}
coredevice_initcall(tftp_init);
+
+
+BSELFTEST_GLOBALS();
+
+static int __maybe_unused tftp_window_cache_selftest(void)
+{
+ struct tftp_cache *cache = malloc(sizeof *cache);
+
+ if (!cache)
+ return -ENOMEM;
+
+ (void)skipped_tests;
+
+ expect_it( is_block_before(0, 1));
+ expect_it(!is_block_before(1, 0));
+ expect_it( is_block_before(65535, 0));
+ expect_it(!is_block_before(0, 65535));
+
+ expect_eq(get_block_delta(0, 1), 1);
+ expect_eq(get_block_delta(65535, 0), 1);
+ expect_eq(get_block_delta(65535, 1), 2);
+
+ expect_it(!in_window(0, 1, 3));
+ expect_it( in_window(1, 1, 3));
+ expect_it( in_window(2, 1, 3));
+ expect_it( in_window(3, 1, 3));
+ expect_it(!in_window(4, 1, 3));
+
+ expect_it(!in_window(65534, 65535, 1));
+ expect_it( in_window(65535, 65535, 1));
+ expect_it( in_window(0, 65535, 1));
+ expect_it( in_window(1, 65535, 1));
+ expect_it(!in_window(2, 65535, 1));
+
[PATCH v3 09/18] tftp: add sanity check for OACK response
Catch bad 'blocksize' or 'windowsize' responses from the server.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 0b6d57c4603f..38a9e955ee92 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -218,7 +218,7 @@ static int tftp_poll(struct file_priv *priv)
return 0;
}
-static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int
len)
+static int tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len)
{
unsigned char *opt, *val, *s;
@@ -235,7 +235,7 @@ static void tftp_parse_oack(struct file_priv *priv,
unsigned char *pkt, int len)
opt = s;
val = s + strlen(s) + 1;
if (val > s + len)
- return;
+ break;
if (!strcmp(opt, "tsize"))
priv->filesize = simple_strtoull(val, NULL, 10);
if (!strcmp(opt, "blksize"))
@@ -243,6 +243,13 @@ static void tftp_parse_oack(struct file_priv *priv,
unsigned char *pkt, int len)
pr_debug("OACK opt: %s val: %s\n", opt, val);
s = val + strlen(val) + 1;
}
+
+ if (priv->blocksize > TFTP_MTU_SIZE) {
+ pr_warn("tftp: invalid oack response\n");
+ return -EINVAL;
+ }
+
+ return 0;
}
static void tftp_timer_reset(struct file_priv *priv)
@@ -295,8 +302,14 @@ static void tftp_recv(struct file_priv *priv,
break;
case TFTP_OACK:
- tftp_parse_oack(priv, pkt, len);
priv->tftp_con->udp->uh_dport = uh_sport;
+
+ if (tftp_parse_oack(priv, pkt, len) < 0) {
+ priv->err = -EINVAL;
+ priv->state = STATE_DONE;
+ break;
+ }
+
priv->state = STATE_START;
break;
--
2.37.1
[PATCH v3 15/18] tftp: do not use 'priv->block' for RRQ
attribute is not used outside tftp_recv() for RRQ.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 4c3980404dc1..c96c8917633a 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -316,6 +316,7 @@ static void tftp_recv(struct file_priv *priv,
uint8_t *pkt, unsigned len, uint16_t uh_sport)
{
uint16_t opcode;
+ uint16_t block;
/* according to RFC1350 minimal tftp packet length is 4 bytes */
if (len < 4)
@@ -370,7 +371,7 @@ static void tftp_recv(struct file_priv *priv,
case TFTP_DATA:
len -= 2;
- priv->block = ntohs(*(uint16_t *)pkt);
+ block = ntohs(*(uint16_t *)pkt);
if (priv->state == STATE_RRQ || priv->state == STATE_OACK) {
/* first block received */
@@ -378,20 +379,20 @@ static void tftp_recv(struct file_priv *priv,
priv->tftp_con->udp->uh_dport = uh_sport;
priv->last_block = 0;
- if (priv->block != 1) { /* Assertion */
+ if (block != 1) { /* Assertion */
pr_err("error: First block is not block 1
(%d)\n",
- priv->block);
+ block);
priv->err = -EINVAL;
priv->state = STATE_DONE;
break;
}
}
- if (priv->block != (uint16_t)(priv->last_block + 1))
+ if (block != (uint16_t)(priv->last_block + 1))
break;
tftp_timer_reset(priv);
- tftp_put_data(priv, priv->block, pkt + 2, len);
+ tftp_put_data(priv, block, pkt + 2, len);
break;
--
2.37.1
[PATCH v3 07/18] tftp: replace hardcoded blksize by global constant
Signed-off-by: Enrico Scholz --- fs/tftp.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/fs/tftp.c b/fs/tftp.c index 7627f3f59669..b552a5dc2f63 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -65,6 +65,7 @@ #define STATE_DONE 8 #define TFTP_BLOCK_SIZE512 /* default TFTP block size */ +#define TFTP_MTU_SIZE 1432/* MTU based block size */ #define TFTP_FIFO_SIZE 4096 #define TFTP_ERR_RESEND1 @@ -135,14 +136,15 @@ static int tftp_send(struct file_priv *priv) "tsize%c" "%lld%c" "blksize%c" - "1432", + "%u", priv->filename + 1, '\0', '\0', /* "octet" */ '\0', /* "timeout" */ TIMEOUT, '\0', '\0', /* "tsize" */ priv->filesize, '\0', - '\0'); /* "blksize" */ + '\0', /* "blksize" */ + TFTP_MTU_SIZE); pkt++; len = pkt - xp; break; -- 2.37.1
[PATCH v3 11/18] tftp: reduce block size on lookup requests
Save some bytes on network traffic by reducing the server response for lookup requests. Signed-off-by: Enrico Scholz --- fs/tftp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/tftp.c b/fs/tftp.c index 9f881249ce07..2ed62b77901b 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -148,7 +148,9 @@ static int tftp_send(struct file_priv *priv) '\0', /* "tsize" */ priv->filesize, '\0', '\0', /* "blksize" */ - TFTP_MTU_SIZE); + /* use only a minimal blksize for getattr + operations, */ + priv->is_getattr ? TFTP_BLOCK_SIZE : TFTP_MTU_SIZE); pkt++; len = pkt - xp; break; -- 2.37.1
[PATCH v3 13/18] tftp: detect out-of-memory situations
it should never happen due to the program logic; but detect a failed
kfifo_put() just in case...
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 2bb9361d8929..5c3e56a6cb22 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -263,6 +263,8 @@ static void tftp_timer_reset(struct file_priv *priv)
static void tftp_put_data(struct file_priv *priv, uint16_t block,
void const *pkt, size_t len)
{
+ unsigned int sz;
+
if (len > priv->blocksize) {
pr_warn("tftp: oversized packet (%zu > %d) received\n",
len, priv->blocksize);
@@ -271,9 +273,14 @@ static void tftp_put_data(struct file_priv *priv, uint16_t
block,
priv->last_block = block;
- kfifo_put(priv->fifo, pkt, len);
+ sz = kfifo_put(priv->fifo, pkt, len);
- if (len < priv->blocksize) {
+ if (sz != len) {
+ pr_err("tftp: not enough room in kfifo (only %u out of %zu
written\n",
+ sz, len);
+ priv->err = -ENOMEM;
+ priv->state = STATE_DONE;
+ } else if (len < priv->blocksize) {
tftp_send(priv);
priv->err = 0;
priv->state = STATE_DONE;
--
2.37.1
[PATCH v3 05/18] cmd:tftp: add '-P' option to set tftp server port number
Signed-off-by: Enrico Scholz
---
commands/tftp.c | 22 +++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/commands/tftp.c b/commands/tftp.c
index 48ff00c6217c..6ac822c9e832 100644
--- a/commands/tftp.c
+++ b/commands/tftp.c
@@ -21,15 +21,24 @@ static int do_tftpb(int argc, char *argv[])
char *source, *dest, *freep;
int opt;
int tftp_push = 0;
+ int port = -1;
int ret;
IPaddr_t ip;
char ip4_str[sizeof("255.255.255.255")];
+ char mount_opts[sizeof("port=12345")];
- while ((opt = getopt(argc, argv, "p")) > 0) {
+ while ((opt = getopt(argc, argv, "pP:")) > 0) {
switch(opt) {
case 'p':
tftp_push = 1;
break;
+ case 'P':
+ port = simple_strtoul(optarg, NULL, 0);
+ if (port <= 0 || port > 0x) {
+ pr_err("invalid port '%s'\n", optarg);
+ return COMMAND_ERROR_USAGE;
+ }
+ break;
default:
return COMMAND_ERROR_USAGE;
}
@@ -59,7 +68,13 @@ static int do_tftpb(int argc, char *argv[])
ip = net_get_serverip();
sprintf(ip4_str, "%pI4", );
- ret = mount(ip4_str, "tftp", TFTP_MOUNT_PATH, NULL);
+
+ if (port >= 0)
+ sprintf(mount_opts, "port=%u", port);
+ else
+ mount_opts[0] = '\0';
+
+ ret = mount(ip4_str, "tftp", TFTP_MOUNT_PATH, mount_opts);
if (ret)
goto err_rmdir;
@@ -84,12 +99,13 @@ BAREBOX_CMD_HELP_TEXT("server address is taken from the
environment (ethX.server
BAREBOX_CMD_HELP_TEXT("")
BAREBOX_CMD_HELP_TEXT("Options:")
BAREBOX_CMD_HELP_OPT ("-p", "push to TFTP server")
+BAREBOX_CMD_HELP_OPT ("-P PORT", "tftp server port number")
BAREBOX_CMD_HELP_END
BAREBOX_CMD_START(tftp)
.cmd= do_tftpb,
BAREBOX_CMD_DESC("load (or save) a file using TFTP")
- BAREBOX_CMD_OPTS("[-p] SOURCE [DEST]")
+ BAREBOX_CMD_OPTS("[-p] [-P ] SOURCE [DEST]")
BAREBOX_CMD_GROUP(CMD_GRP_NET)
BAREBOX_CMD_HELP(cmd_tftp_help)
BAREBOX_CMD_END
--
2.37.1
[PATCH v3 16/18] tftp: add debug_assert() macro
Is a noop in normal cases (when compiler sees that condition can be
evaluated without sideeffects) but allows optimizations based on the
condition.
E.g. in
| void foo(int a)
| {
| debug_assert(a == 23);
|
| if (a == 23)
| return;
|
| bar();
| }
the call to 'bar()' will be optimized away.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/fs/tftp.c b/fs/tftp.c
index c96c8917633a..d02ca750cd6d 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -75,6 +75,15 @@
#define TFTP_ERR_RESEND1
+#ifdef DEBUG
+# define debug_assert(_cond) BUG_ON(!(_cond))
+#else
+# define debug_assert(_cond) do { \
+ if (!(_cond)) \
+ __builtin_unreachable();\
+ } while (0)
+#endif
+
static int g_tftp_window_size = DIV_ROUND_UP(TFTP_MAX_WINDOW_SIZE, 2);
struct file_priv {
--
2.37.1
Re: [PATCH 00/13] add "windowsize" (RFC 7440) support for tftp
Sascha Hauer writes: >> The tftp "windowsize" greatly improves the performance of tftp >> transfers. This patchset adds support for it. > > TCP for the poor, nice ;) > > Very nice stuff indeed. I gave it a quick test and TFTP works as before, > that's good to start with, but to gain speed improvements I need a > RFC7440 capable server on the other end. Which server did you test it > with? Have any RFC7440 capable servers made it to the distributions > already? https://github.com/ensc/r-tftpd ;) yes... it is problem to implement both server and client side. But the window size feature also works with PXE clients and u-boot, so I think it is correct. I started to implement it with transparent proxying support in mind (e.g. translate tftp requests to http ones, or fetch cached images with 'rsync'). But when I saw the effects of windows size support, I focused on that. Enrico
Re: [PATCH v2 09/13] tftp: implement 'windowsize' (RFC 7440) support
Sascha Hauer writes: >> +config FS_TFTP_MAX_WINDOW_SIZE >> +int >> +prompt "maximum tftp window size (RFC 7440)" >> +depends on FS_TFTP >> +default 128 >> +range 1 128 >> +help >> + The maximum allowed tftp "windowsize" (RFC 7440). Higher >> + value increase speed of the tftp download with the cost of >> + memory (1432 bytes per slot). >> + >> + Requires tftp "windowsize" (RFC 7440) support on server side >> + to have an effect. > > Can we agree on some sane default here and drop this from Kconfig? I think, it is difficult to find a sane default. Processors like iMX8 can deal with large window sizes, but low end ones like iMX6ULL can not handle data fast enough and require smaller sizes (around max 30). Workloads like writing to slow memory (cp /mnt/tftp/... /dev/nand) might require smaller sizes too. Or networks with high drop rates. That's why, I think it should be possible to configure it in some _defconfig. >> +/* calculate fifo so that it can hold the complete window plus the incoming >> + packet. Add some extra space just in case... */ >> +#define TFTP_FIFO_SIZE (TFTP_MTU_SIZE * TFTP_MAX_WINDOW_SIZE + >> 2048) > > Memory should be allocated according to the actual window size, not to > the maximum window size. ok; I will try it. Moving 'kfifo_alloc()' should be possible in | static struct file_priv *tftp_do_open(struct device_d *dev, | int accmode, struct dentry *dentry, bool is_getattr) | | priv->fifo = kfifo_alloc(TFTP_FIFO_SIZE); | | ret = tftp_poll(priv); > Otherwise I don't see a reason to decrease the window size using > global.tftp.windowsize when the memory allocated is always the same. As I said above: some workloads or networks with a high packet drop rate work better with lower window sizes. Enrco
[PATCH v2 13/13] tftp: add sanity check for OACK response
Catch bad 'blocksize' or 'windowsize' responses from the server.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 19 +++
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index cfeb94b21..93b0606d3 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -292,7 +292,7 @@ static int tftp_poll(struct file_priv *priv)
return 0;
}
-static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int
len)
+static int tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len)
{
unsigned char *opt, *val, *s;
@@ -309,7 +309,7 @@ static void tftp_parse_oack(struct file_priv *priv,
unsigned char *pkt, int len)
opt = s;
val = s + strlen(s) + 1;
if (val > s + len)
- return;
+ break;
if (!strcmp(opt, "tsize"))
priv->filesize = simple_strtoull(val, NULL, 10);
if (!strcmp(opt, "blksize"))
@@ -319,6 +319,15 @@ static void tftp_parse_oack(struct file_priv *priv,
unsigned char *pkt, int len)
pr_debug("OACK opt: %s val: %s\n", opt, val);
s = val + strlen(val) + 1;
}
+
+ if (priv->blocksize > TFTP_MTU_SIZE ||
+ priv->windowsize > TFTP_MAX_WINDOW_SIZE ||
+ priv->windowsize == 0) {
+ pr_warn("tftp: invalid oack response\n");
+ return -EINVAL;
+ }
+
+ return 0;
}
static void tftp_timer_reset(struct file_priv *priv)
@@ -500,10 +509,12 @@ static void tftp_recv(struct file_priv *priv,
break;
case TFTP_OACK:
- tftp_parse_oack(priv, pkt, len);
priv->tftp_con->udp->uh_dport = uh_sport;
- if (priv->push) {
+ if (tftp_parse_oack(priv, pkt, len) < 0) {
+ priv->err = -EINVAL;
+ priv->state = STATE_DONE;
+ } else if (priv->push) {
/* send first block */
priv->state = STATE_WDATA;
priv->block = 1;
--
2.37.1
[PATCH v2 09/13] tftp: implement 'windowsize' (RFC 7440) support
Results (with the reorder patch; numbers in bytes/s) on an iMX8MP are:
| windowsize | VPN | 1 Gb/s | 100 Mb/s |
||---|||
| 128| 3.869.284 | 98.643.085 | 11.434.852 |
| 64| 3.863.581 | 98.550.375 | 11.434.852 |
| 48| 3.431.580 | 94.211.680 | 11.275.010 |
| 32| 2.835.129 | 85.250.081 | 10.985.605 |
| 24| 2.344.858 | 77.787.537 | 10.765.667 |
| 16| 1.734.186 | 67.519.381 | 10.210.087 |
| 12| 1.403.340 | 61.972.576 | 9.915.612 |
| 8| 1.002.462 | 50.852.376 | 9.016.130 |
| 6| 775.573 | 42.781.558 | 8.422.297 |
| 4| 547.845 | 32.066.544 | 6.835.567 |
| 3| 412.987 | 26.526.081 | 6.322.435 |
| 2| 280.987 | 19.120.641 | 5.494.241 |
| 1| 141.699 | 10.431.516 | 2.967.224 |
(VPN = OpenVPN on ADSL 50 Mb/s).
The window size can be configured at runtime.
Signed-off-by: Enrico Scholz
---
fs/Kconfig | 14 ++
fs/tftp.c | 51 ---
2 files changed, 54 insertions(+), 11 deletions(-)
diff --git a/fs/Kconfig b/fs/Kconfig
index aeba00073..0c4934285 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -43,6 +43,20 @@ config FS_TFTP
prompt "tftp support"
depends on NET
+config FS_TFTP_MAX_WINDOW_SIZE
+ int
+ prompt "maximum tftp window size (RFC 7440)"
+ depends on FS_TFTP
+ default 128
+ range 1 128
+ help
+ The maximum allowed tftp "windowsize" (RFC 7440). Higher
+ value increase speed of the tftp download with the cost of
+ memory (1432 bytes per slot).
+
+ Requires tftp "windowsize" (RFC 7440) support on server side
+ to have an effect.
+
config FS_OMAP4_USBBOOT
bool
prompt "Filesystem over usb boot"
diff --git a/fs/tftp.c b/fs/tftp.c
index ef2ce0200..b8950f250 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -29,7 +29,9 @@
#include
#include
#include
+#include
#include
+#include
#include
#define TFTP_PORT 69 /* Well known TFTP port number */
@@ -65,15 +67,22 @@
#define TFTP_BLOCK_SIZE512 /* default TFTP block size */
#define TFTP_MTU_SIZE 1432/* MTU based block size */
-#define TFTP_FIFO_SIZE 4096
+#define TFTP_MAX_WINDOW_SIZE CONFIG_FS_TFTP_MAX_WINDOW_SIZE
+
+/* calculate fifo so that it can hold the complete window plus the incoming
+ packet. Add some extra space just in case... */
+#define TFTP_FIFO_SIZE (TFTP_MTU_SIZE * TFTP_MAX_WINDOW_SIZE + 2048)
#define TFTP_ERR_RESEND1
+static int g_tftp_window_size = DIV_ROUND_UP(TFTP_MAX_WINDOW_SIZE, 2);
+
struct file_priv {
struct net_connection *tftp_con;
int push;
uint16_t block;
uint16_t last_block;
+ uint16_t ack_block;
int state;
int err;
char *filename;
@@ -83,7 +92,7 @@ struct file_priv {
struct kfifo *fifo;
void *buf;
int blocksize;
- int block_requested;
+ unsigned int windowsize;
bool is_getattr;
};
@@ -114,6 +123,7 @@ static int tftp_send(struct file_priv *priv)
int len = 0;
uint16_t *s;
unsigned char *pkt = net_udp_get_payload(priv->tftp_con);
+ unsigned int window_size;
int ret;
pr_vdebug("%s: state %s\n", __func__, tftp_states[priv->state]);
@@ -121,6 +131,15 @@ static int tftp_send(struct file_priv *priv)
switch (priv->state) {
case STATE_RRQ:
case STATE_WRQ:
+ if (priv->push || priv->is_getattr)
+ /* atm, windowsize is suported only for RRQ and there
+ is no need to request a full window when we are
+ just looking up file attributes */
+ window_size = 1;
+ else
+ window_size = min_t(unsigned int, g_tftp_window_size,
+ TFTP_MAX_WINDOW_SIZE);
+
xp = pkt;
s = (uint16_t *)pkt;
if (priv->state == STATE_RRQ)
@@ -148,18 +167,24 @@ static int tftp_send(struct file_priv *priv)
operations, */
priv->is_getattr ? TFTP_BLOCK_SIZE :
TFTP_MTU_SIZE);
pkt++;
+
+ if (window_size > 1)
+ pkt += sprintf((unsigned char *)pkt,
+ "windowsize%c%u%c",
+ '\0', window_size,
+ '\0');
+
len = pkt - xp;
break;
case STATE_RDATA:
- if (priv->block == priv->block_requested)
- return 0;
case STATE_OACK:
xp =
[PATCH 10/13] tftp: do not use 'priv->block' for RRQ
attribute is not used outside tftp_recv() for RRQ.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 7f836d36b7df..f85136f03e22 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -298,6 +298,7 @@ static void tftp_recv(struct file_priv *priv,
uint8_t *pkt, unsigned len, uint16_t uh_sport)
{
uint16_t opcode;
+ uint16_t block;
/* according to RFC1350 minimal tftp packet length is 4 bytes */
if (len < 4)
@@ -349,7 +350,6 @@ static void tftp_recv(struct file_priv *priv,
} else {
/* send ACK */
priv->state = STATE_OACK;
- priv->block = 0;
priv->last_block = 0;
tftp_send(priv);
}
@@ -357,7 +357,7 @@ static void tftp_recv(struct file_priv *priv,
break;
case TFTP_DATA:
len -= 2;
- priv->block = ntohs(*(uint16_t *)pkt);
+ block = ntohs(*(uint16_t *)pkt);
if (priv->state == STATE_RRQ || priv->state == STATE_OACK) {
/* first block received */
@@ -365,21 +365,21 @@ static void tftp_recv(struct file_priv *priv,
priv->tftp_con->udp->uh_dport = uh_sport;
priv->last_block = 0;
- if (priv->block != 1) { /* Assertion */
+ if (block != 1) { /* Assertion */
pr_err("error: First block is not block 1
(%d)\n",
- priv->block);
+ block);
priv->err = -EINVAL;
priv->state = STATE_DONE;
break;
}
}
- if (priv->block != (uint16_t)(priv->last_block + 1)) {
+ if (block != (uint16_t)(priv->last_block + 1)) {
break;
}
tftp_timer_reset(priv);
- tftp_put_data(priv, priv->block, pkt + 2, len);
+ tftp_put_data(priv, block, pkt + 2, len);
break;
--
2.36.1
[PATCH 07/13] tftp: refactor data processing
move block handling into dedicated function
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 01d3beff14bf..81141626ab91 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -249,6 +249,20 @@ static void tftp_timer_reset(struct file_priv *priv)
priv->progress_timeout = priv->resend_timeout = get_time_ns();
}
+static void tftp_put_data(struct file_priv *priv, uint16_t block,
+ void const *pkt, size_t len)
+{
+ priv->last_block = block;
+
+ kfifo_put(priv->fifo, pkt, len);
+
+ if (len < priv->blocksize) {
+ tftp_send(priv);
+ priv->err = 0;
+ priv->state = STATE_DONE;
+ }
+}
+
static void tftp_recv(struct file_priv *priv,
uint8_t *pkt, unsigned len, uint16_t uh_sport)
{
@@ -332,17 +346,8 @@ static void tftp_recv(struct file_priv *priv,
/* Same block again; ignore it. */
break;
- priv->last_block = priv->block;
-
tftp_timer_reset(priv);
-
- kfifo_put(priv->fifo, pkt + 2, len);
-
- if (len < priv->blocksize) {
- tftp_send(priv);
- priv->err = 0;
- priv->state = STATE_DONE;
- }
+ tftp_put_data(priv, priv->block, pkt + 2, len);
break;
--
2.36.1
[PATCH 12/13] tftp: allow to change tftp port
useful e.g. when working with a local, non-privileged tftp server
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 2c2ff081be51..400209c26023 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -78,6 +78,7 @@
#define TFTP_ERR_RESEND1
static int g_tftp_window_size = TFTP_MAX_WINDOW_SIZE / 1;
+static int g_tftp_port = TFTP_PORT;
struct tftp_block {
uint16_t id;
@@ -600,7 +601,7 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
goto out;
}
- priv->tftp_con = net_udp_new(tpriv->server, TFTP_PORT, tftp_handler,
+ priv->tftp_con = net_udp_new(tpriv->server, g_tftp_port, tftp_handler,
priv);
if (IS_ERR(priv->tftp_con)) {
ret = PTR_ERR(priv->tftp_con);
@@ -944,6 +945,7 @@ static struct fs_driver_d tftp_driver = {
static int tftp_init(void)
{
globalvar_add_simple_int("tftp.windowsize", _tftp_window_size, "%u");
+ globalvar_add_simple_int("tftp.port", _tftp_port, "%u");
return register_fs_driver(_driver);
}
--
2.36.1
[PATCH 08/13] tftp: detect out-of-memory situations
it should never happen due to the program logic; but detect a failed
kfifo_put() just in case...
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 81141626ab91..ef2ce0200b38 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -252,11 +252,18 @@ static void tftp_timer_reset(struct file_priv *priv)
static void tftp_put_data(struct file_priv *priv, uint16_t block,
void const *pkt, size_t len)
{
+ unsigned int sz;
+
priv->last_block = block;
- kfifo_put(priv->fifo, pkt, len);
+ sz = kfifo_put(priv->fifo, pkt, len);
- if (len < priv->blocksize) {
+ if (sz != len) {
+ pr_err("tftp: not enough room in kfifo (only %u out of %zu
written\n",
+ sz, len);
+ priv->err = -ENOMEM;
+ priv->state = STATE_DONE;
+ } else if (len < priv->blocksize) {
tftp_send(priv);
priv->err = 0;
priv->state = STATE_DONE;
--
2.36.1
[PATCH 01/13] progress: add close_progress() to display some statistics
With a later patch, this will produce output like
| :/ cp -v /mnt/tftp/tmp /tmp/
| [...#] 29138411 bytes, 127413 bytes/s
Signed-off-by: Enrico Scholz
---
include/progress.h | 1 +
lib/show_progress.c | 25 +
2 files changed, 26 insertions(+)
diff --git a/include/progress.h b/include/progress.h
index 7230bd3a9bd5..219d772201bf 100644
--- a/include/progress.h
+++ b/include/progress.h
@@ -16,6 +16,7 @@ void init_progression_bar(loff_t max);
* spinner is printed.
*/
void show_progress(loff_t now);
+void close_progress(loff_t total);
extern struct notifier_head progress_notifier;
diff --git a/lib/show_progress.c b/lib/show_progress.c
index 1b624bcb9af8..5b42a2d74b8f 100644
--- a/lib/show_progress.c
+++ b/lib/show_progress.c
@@ -24,6 +24,7 @@
static loff_t printed;
static loff_t progress_max;
static unsigned spin;
+static uint64_t start_tm;
void show_progress(loff_t now)
{
@@ -47,11 +48,35 @@ void show_progress(loff_t now)
}
}
+void close_progress(loff_t total)
+{
+ uint64_t now = get_time_ns();
+ unsigned long speed;
+ unsigned long delta;
+
+ if (now <= start_tm) {
+ pr_crit("tm mismatch");
+ return;
+ }
+
+ if (total < 0)
+ return;
+
+ /* convert to ms and add '1' to avoid div-by-zero */
+ delta = div_u64(now - start_tm, 100) + 1;
+
+ /* speed is bytes/s */
+ speed = div_u64(total * 1000, delta);
+
+ printf("] %llu bytes, %lu bytes/s", (unsigned long long)total, speed);
+}
+
void init_progression_bar(loff_t max)
{
printed = 0;
progress_max = max;
spin = 0;
+ start_tm = get_time_ns();
if (progress_max && progress_max != FILESIZE_MAX)
printf("\t[%*s]\r\t[", HASHES_PER_LINE, "");
else
--
2.36.1
[PATCH 03/13] tftp: minor refactoring of RRQ/WRQ packet generation code
Having 11 printf arguments with lot of them being 0, makes it difficulty to read and extend. Add some comments and use '\0' for %c. Signed-off-by: Enrico Scholz --- fs/tftp.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/fs/tftp.c b/fs/tftp.c index d186e7983a6d..13334baaf892 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -135,13 +135,13 @@ static int tftp_send(struct file_priv *priv) "%lld%c" "blksize%c" "1432", - priv->filename + 1, 0, - 0, - 0, - TIMEOUT, 0, - 0, - priv->filesize, 0, - 0); + priv->filename + 1, '\0', + '\0', /* "octet" */ + '\0', /* "timeout" */ + TIMEOUT, '\0', + '\0', /* "tsize" */ + priv->filesize, '\0', + '\0'); /* "blksize" */ pkt++; len = pkt - xp; break; -- 2.36.1
[PATCH 02/13] libfile:copy_file: show statistics in verbose mode
Signed-off-by: Enrico Scholz --- lib/libfile.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/libfile.c b/lib/libfile.c index 3b7985fbcabb..9eb06de02945 100644 --- a/lib/libfile.c +++ b/lib/libfile.c @@ -454,6 +454,9 @@ int copy_file(const char *src, const char *dst, int verbose) } } + if (verbose) + close_progress(total); + ret = 0; out: if (verbose) -- 2.36.1
[PATCH 05/13] tftp: record whether tftp file is opened for lookup operation only
Opening a tftp is done in two steps: at first `tftp_lookup()` is
called to get the filesize and then it is opened again and data are
read.
The `tftp_lookup()` call sends only a RRQ/WRQ, reads then the "tsize"
from the response and closes the transfer by sending an error datagram.
The tftp server will send a full data window.
To prevent unneeded traffic, later patches set parameters to reduce
the size of the server response.
We need knowledge about type of operation which is recorded in an
"is_getattr" attribute.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 7ad03ae5cdc6..ab1af0e9084c 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -84,6 +84,7 @@ struct file_priv {
void *buf;
int blocksize;
int block_requested;
+ bool is_getattr;
};
struct tftp_priv {
@@ -372,7 +373,7 @@ static void tftp_handler(void *ctx, char *packet, unsigned
len)
}
static struct file_priv *tftp_do_open(struct device_d *dev,
- int accmode, struct dentry *dentry)
+ int accmode, struct dentry *dentry, bool is_getattr)
{
struct fs_device_d *fsdev = dev_to_fs_device(dev);
struct file_priv *priv;
@@ -400,6 +401,7 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
priv->filename = dpath(dentry, fsdev->vfsmount.mnt_root);
priv->blocksize = TFTP_BLOCK_SIZE;
priv->block_requested = -1;
+ priv->is_getattr = is_getattr;
priv->fifo = kfifo_alloc(TFTP_FIFO_SIZE);
if (!priv->fifo) {
@@ -451,7 +453,7 @@ static int tftp_open(struct device_d *dev, FILE *file,
const char *filename)
{
struct file_priv *priv;
- priv = tftp_do_open(dev, file->flags, file->dentry);
+ priv = tftp_do_open(dev, file->flags, file->dentry, false);
if (IS_ERR(priv))
return PTR_ERR(priv);
@@ -667,7 +669,7 @@ static struct dentry *tftp_lookup(struct inode *dir, struct
dentry *dentry,
struct file_priv *priv;
loff_t filesize;
- priv = tftp_do_open(>dev, O_RDONLY, dentry);
+ priv = tftp_do_open(>dev, O_RDONLY, dentry, true);
if (IS_ERR(priv))
return NULL;
--
2.36.1
[PATCH 09/13] tftp: implement 'windowsize' (RFC 7440) support
Results (with the reorder patch; numbers in bytes/s) on an iMX8MP are:
| windowsize | VPN | 1 Gb/s | 100 Mb/s |
||---|||
| 128| 3.869.284 | 98.643.085 | 11.434.852 |
| 64| 3.863.581 | 98.550.375 | 11.434.852 |
| 48| 3.431.580 | 94.211.680 | 11.275.010 |
| 32| 2.835.129 | 85.250.081 | 10.985.605 |
| 24| 2.344.858 | 77.787.537 | 10.765.667 |
| 16| 1.734.186 | 67.519.381 | 10.210.087 |
| 12| 1.403.340 | 61.972.576 | 9.915.612 |
| 8| 1.002.462 | 50.852.376 | 9.016.130 |
| 6| 775.573 | 42.781.558 | 8.422.297 |
| 4| 547.845 | 32.066.544 | 6.835.567 |
| 3| 412.987 | 26.526.081 | 6.322.435 |
| 2| 280.987 | 19.120.641 | 5.494.241 |
| 1| 141.699 | 10.431.516 | 2.967.224 |
(VPN = OpenVPN on ADSL 50 Mb/s).
The window size can be configured at runtime.
Signed-off-by: Enrico Scholz
---
fs/Kconfig | 14 ++
fs/tftp.c | 54 --
2 files changed, 54 insertions(+), 14 deletions(-)
diff --git a/fs/Kconfig b/fs/Kconfig
index aeba00073eed..0c4934285942 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -43,6 +43,20 @@ config FS_TFTP
prompt "tftp support"
depends on NET
+config FS_TFTP_MAX_WINDOW_SIZE
+ int
+ prompt "maximum tftp window size (RFC 7440)"
+ depends on FS_TFTP
+ default 128
+ range 1 128
+ help
+ The maximum allowed tftp "windowsize" (RFC 7440). Higher
+ value increase speed of the tftp download with the cost of
+ memory (1432 bytes per slot).
+
+ Requires tftp "windowsize" (RFC 7440) support on server side
+ to have an effect.
+
config FS_OMAP4_USBBOOT
bool
prompt "Filesystem over usb boot"
diff --git a/fs/tftp.c b/fs/tftp.c
index ef2ce0200b38..7f836d36b7df 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -30,6 +30,7 @@
#include
#include
#include
+#include
#include
#define TFTP_PORT 69 /* Well known TFTP port number */
@@ -65,15 +66,22 @@
#define TFTP_BLOCK_SIZE512 /* default TFTP block size */
#define TFTP_MTU_SIZE 1432/* MTU based block size */
-#define TFTP_FIFO_SIZE 4096
+#define TFTP_MAX_WINDOW_SIZE CONFIG_FS_TFTP_MAX_WINDOW_SIZE
+
+/* calculate fifo so that it can hold the complete window plus the incoming
+ packet. Add some extra space just in case... */
+#define TFTP_FIFO_SIZE (TFTP_MTU_SIZE * TFTP_MAX_WINDOW_SIZE + 2048)
#define TFTP_ERR_RESEND1
+static int g_tftp_window_size = TFTP_MAX_WINDOW_SIZE / 1;
+
struct file_priv {
struct net_connection *tftp_con;
int push;
uint16_t block;
uint16_t last_block;
+ uint16_t ack_block;
int state;
int err;
char *filename;
@@ -83,7 +91,7 @@ struct file_priv {
struct kfifo *fifo;
void *buf;
int blocksize;
- int block_requested;
+ unsigned int windowsize;
bool is_getattr;
};
@@ -114,6 +122,7 @@ static int tftp_send(struct file_priv *priv)
int len = 0;
uint16_t *s;
unsigned char *pkt = net_udp_get_payload(priv->tftp_con);
+ unsigned int window_size;
int ret;
pr_vdebug("%s: state %s\n", __func__, tftp_states[priv->state]);
@@ -121,6 +130,15 @@ static int tftp_send(struct file_priv *priv)
switch (priv->state) {
case STATE_RRQ:
case STATE_WRQ:
+ if (priv->push || priv->is_getattr)
+ /* atm, windowsize is suported only for RRQ and there
+ is no need to request a full window when we are
+ just looking up file attributes */
+ window_size = 1;
+ else
+ window_size = min_t(unsigned int, g_tftp_window_size,
+ TFTP_MAX_WINDOW_SIZE);
+
xp = pkt;
s = (uint16_t *)pkt;
if (priv->state == STATE_RRQ)
@@ -136,30 +154,35 @@ static int tftp_send(struct file_priv *priv)
"tsize%c"
"%lld%c"
"blksize%c"
- "%u",
+ "%u%c"
+ "windowsize%c"
+ "%u",
priv->filename + 1, '\0',
'\0', /* "octet" */
'\0', /* "timeout" */
TIMEOUT, '\0',
'\0', /* "tsize" */
[PATCH 11/13] tftp: reorder tftp packets
With the tftp "windowsize" option, reordering of udp datagrams becomes
an issue. Depending on the network topology, this reordering occurs
several times with large tftp transfers and will heavily reduce the
transfer speed.
This patch adds a packet cache so that datagrams can be reassembled in
the correct order.
Because it increases memory usage, it is an Kconfig option.
Signed-off-by: Enrico Scholz
---
fs/Kconfig | 22 +++
fs/tftp.c | 165 +++--
2 files changed, 181 insertions(+), 6 deletions(-)
diff --git a/fs/Kconfig b/fs/Kconfig
index 0c4934285942..02aa25d6abf7 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -57,6 +57,28 @@ config FS_TFTP_MAX_WINDOW_SIZE
Requires tftp "windowsize" (RFC 7440) support on server side
to have an effect.
+config FS_TFTP_REORDER_CACHE_SIZE
+ int
+ prompt "number of out-of-order tftp packets to be cached"
+ depends on FS_TFTP
+ default 16 if FS_TFTP_MAX_WINDOW_SIZE > 16
+ default 0 if FS_TFTP_MAX_WINDOW_SIZE = 1
+## TODO: it should be 'FS_TFTP_MAX_WINDOW_SIZE - 1' but this
+## is not supported by Kconfig
+ default FS_TFTP_MAX_WINDOW_SIZE
+ range 0 FS_TFTP_MAX_WINDOW_SIZE
+ help
+ UDP allows reordering of datagrams; with this option,
+ unexpected tftp packets will be cached and later
+ reassembled. This increases stability of the tftp download
+ with the cost of memory (around 1440 bytes per cache
+ element).
+
+ A value of 0 disables caching.
+
+ Requires tftp "windowsize" (RFC 7440) support on server side
+ to have an effect.
+
config FS_OMAP4_USBBOOT
bool
prompt "Filesystem over usb boot"
diff --git a/fs/tftp.c b/fs/tftp.c
index f85136f03e22..2c2ff081be51 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -68,6 +68,9 @@
#define TFTP_MTU_SIZE 1432/* MTU based block size */
#define TFTP_MAX_WINDOW_SIZE CONFIG_FS_TFTP_MAX_WINDOW_SIZE
+/* size of cache which deals with udp reordering */
+#define TFTP_WINDOW_CACHE_NUM CONFIG_FS_TFTP_REORDER_CACHE_SIZE
+
/* calculate fifo so that it can hold the complete window plus the incoming
packet. Add some extra space just in case... */
#define TFTP_FIFO_SIZE (TFTP_MTU_SIZE * TFTP_MAX_WINDOW_SIZE + 2048)
@@ -76,6 +79,15 @@
static int g_tftp_window_size = TFTP_MAX_WINDOW_SIZE / 1;
+struct tftp_block {
+ uint16_t id;
+ uint8_t data[TFTP_MTU_SIZE];
+
+ /* len will not exceed TFTP_MTU_SIZE; 14 bits should suffice... */
+ uint16_t len:14;
+ bool valid:1;
+};
+
struct file_priv {
struct net_connection *tftp_con;
int push;
@@ -93,12 +105,49 @@ struct file_priv {
int blocksize;
unsigned int windowsize;
bool is_getattr;
+#if TFTP_WINDOW_CACHE_NUM > 0
+ struct tftp_block window_cache[TFTP_WINDOW_CACHE_NUM];
+#endif
};
struct tftp_priv {
IPaddr_t server;
};
+inline static size_t tftp_window_cache_size(struct file_priv const *priv)
+{
+#if TFTP_WINDOW_CACHE_NUM > 0
+ return min_t(unsigned int, priv->windowsize - 1,
+ARRAY_SIZE(priv->window_cache));
+#else
+ return 0;
+#endif
+}
+
+inline static struct tftp_block *tftp_window_cache_get(struct file_priv *priv,
+ size_t idx)
+{
+#if TFTP_WINDOW_CACHE_NUM > 0
+ return >window_cache[idx];
+#else
+ return NULL;
+#endif
+}
+
+static void tftp_window_cache_clear(struct file_priv *priv)
+{
+ size_t i;
+
+ for (i = 0; i < tftp_window_cache_size(priv); ++i) {
+ struct tftp_block *block = tftp_window_cache_get(priv, i);
+
+ if (block->valid)
+ pr_debug("cached item #%d still valid\n", block->id);
+
+ block->valid = false;
+ }
+}
+
static int tftp_truncate(struct device_d *dev, FILE *f, loff_t size)
{
return 0;
@@ -185,6 +234,7 @@ static int tftp_send(struct file_priv *priv)
priv->ack_block += priv->windowsize;
pkt = (unsigned char *)s;
len = pkt - xp;
+ tftp_window_cache_clear(priv);
break;
}
@@ -294,6 +344,113 @@ static void tftp_put_data(struct file_priv *priv,
uint16_t block,
}
}
+static bool in_window(uint16_t block, uint16_t start, uint16_t end)
+{
+ return ((start <= block && block <= end) ||
+ (block <= end && end <= start));
+}
+
+static int tftp_window_cache_insert(struct file_priv *priv, uint16_t id,
+ void const *data, size_t len)
+{
+ size_t i;
+
+ if (len > sizeof tftp_window_cache_get(priv, 0)->data) {
+ pr_err("datagram too large\n");
+
[PATCH 00/13] add "windowsize" (RFC 7440) support for tftp
The tftp "windowsize" greatly improves the performance of tftp transfers. This patchset adds support for it. The first two patches are a little bit unrelated and enhance the 'cp -v' output by giving information about the transfer speed. They can be dropped if they are unwanted. I tested the function with an iMX8MP platform in three environments: - at home over OpenVPN on an ADSL 50 line --> 27x speedup - 1 Gb/s connection --> 9x speedup - connection over 100 Mb/s switch --> 4x speedup In the test, I downloaded variable sized files which were filled from /dev/urandom. E.g. | :/ global tftp.windowsize=128 | :/ cp -v /mnt/tftp/data-100MiB /tmp/data && sha1sum /tmp/data | [] 104857600 bytes, 98550375 bytes/s For slow connection speed, smaller files (1MiB, 4 MiB + 20 MiB) were used. The numbers (bytes/s) are | windowsize | VPN | 1 Gb/s | 100 Mb/s | ||---||| | 128| 3.869.284 | 98.643.085 | 11.434.852 | | 64| 3.863.581 | 98.550.375 | 11.434.852 | | 48| 3.431.580 | 94.211.680 | 11.275.010 | | 32| 2.835.129 | 85.250.081 | 10.985.605 | | 24| 2.344.858 | 77.787.537 | 10.765.667 | | 16| 1.734.186 | 67.519.381 | 10.210.087 | | 12| 1.403.340 | 61.972.576 | 9.915.612 | | 8| 1.002.462 | 50.852.376 | 9.016.130 | | 6| 775.573 | 42.781.558 | 8.422.297 | | 4| 547.845 | 32.066.544 | 6.835.567 | | 3| 412.987 | 26.526.081 | 6.322.435 | | 2| 280.987 | 19.120.641 | 5.494.241 | | 1| 141.699 | 10.431.516 | 2.967.224 | ||---||| | unpatched | 140.587 | 10.553.301 | 2.978.063 | Enrico Scholz (13): progress: add close_progress() to display some statistics libfile:copy_file: show statistics in verbose mode tftp: minor refactoring of RRQ/WRQ packet generation code tftp: replace hardcoded blksize by global constant tftp: record whether tftp file is opened for lookup operation only tftp: reduce block size on lookup requests tftp: refactor data processing tftp: detect out-of-memory situations tftp: implement 'windowsize' (RFC 7440) support tftp: do not use 'priv->block' for RRQ tftp: reorder tftp packets tftp: allow to change tftp port tftp: add sanity check for OACK response fs/Kconfig | 36 ++ fs/tftp.c | 298 +--- include/progress.h | 1 + lib/libfile.c | 3 + lib/show_progress.c | 25 5 files changed, 319 insertions(+), 44 deletions(-) -- 2.36.1
[PATCH 06/13] tftp: reduce block size on lookup requests
Save some bytes on network traffic by reducing the server response for lookup requests. Signed-off-by: Enrico Scholz --- fs/tftp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/tftp.c b/fs/tftp.c index ab1af0e9084c..01d3beff14bf 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -144,7 +144,9 @@ static int tftp_send(struct file_priv *priv) '\0', /* "tsize" */ priv->filesize, '\0', '\0',/* "blksize" */ - TFTP_MTU_SIZE); + /* use only a minimal blksize for getattr + operations, */ + priv->is_getattr ? TFTP_BLOCK_SIZE : TFTP_MTU_SIZE); pkt++; len = pkt - xp; break; -- 2.36.1
[PATCH 13/13] tftp: add sanity check for OACK response
Catch bad 'blocksize' or 'windowsize' responses from the server.
Signed-off-by: Enrico Scholz
---
fs/tftp.c | 19 +++
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/fs/tftp.c b/fs/tftp.c
index 400209c26023..c6491b537481 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -290,7 +290,7 @@ static int tftp_poll(struct file_priv *priv)
return 0;
}
-static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int
len)
+static int tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len)
{
unsigned char *opt, *val, *s;
@@ -307,7 +307,7 @@ static void tftp_parse_oack(struct file_priv *priv,
unsigned char *pkt, int len)
opt = s;
val = s + strlen(s) + 1;
if (val > s + len)
- return;
+ break;
if (!strcmp(opt, "tsize"))
priv->filesize = simple_strtoull(val, NULL, 10);
if (!strcmp(opt, "blksize"))
@@ -317,6 +317,15 @@ static void tftp_parse_oack(struct file_priv *priv,
unsigned char *pkt, int len)
pr_debug("OACK opt: %s val: %s\n", opt, val);
s = val + strlen(val) + 1;
}
+
+ if (priv->blocksize > TFTP_MTU_SIZE ||
+ priv->windowsize > TFTP_MAX_WINDOW_SIZE ||
+ priv->windowsize == 0) {
+ pr_warn("tftp: invalid oack response");
+ return -EINVAL;
+ }
+
+ return 0;
}
static void tftp_timer_reset(struct file_priv *priv)
@@ -498,10 +507,12 @@ static void tftp_recv(struct file_priv *priv,
break;
case TFTP_OACK:
- tftp_parse_oack(priv, pkt, len);
priv->tftp_con->udp->uh_dport = uh_sport;
- if (priv->push) {
+ if (tftp_parse_oack(priv, pkt, len) < 0) {
+ priv->err = -EINVAL;
+ priv->state = STATE_DONE;
+ } else if (priv->push) {
/* send first block */
priv->state = STATE_WDATA;
priv->block = 1;
--
2.36.1
[PATCH 04/13] tftp: replace hardcoded blksize by global constant
Signed-off-by: Enrico Scholz --- fs/tftp.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/fs/tftp.c b/fs/tftp.c index 13334baaf892..7ad03ae5cdc6 100644 --- a/fs/tftp.c +++ b/fs/tftp.c @@ -64,6 +64,7 @@ #define STATE_DONE 8 #define TFTP_BLOCK_SIZE512 /* default TFTP block size */ +#define TFTP_MTU_SIZE 1432/* MTU based block size */ #define TFTP_FIFO_SIZE 4096 #define TFTP_ERR_RESEND1 @@ -134,14 +135,15 @@ static int tftp_send(struct file_priv *priv) "tsize%c" "%lld%c" "blksize%c" - "1432", + "%u", priv->filename + 1, '\0', '\0', /* "octet" */ '\0', /* "timeout" */ TIMEOUT, '\0', '\0', /* "tsize" */ priv->filesize, '\0', - '\0'); /* "blksize" */ + '\0',/* "blksize" */ + TFTP_MTU_SIZE); pkt++; len = pkt - xp; break; -- 2.36.1
[PATCH] usb:chipidea-imx: honor "phys" dtree property
Recent kernel devicetrees changed the "fsl,usbphy" property name to
"phys" on some platforms (e.g. iMX8) but the old one was kept on other
platforms (e.g. iMX6).
Check them both in the usb driver by preferring "phys".
NOTE: this fixes a hard lockup when booting NXP linux kernels on iMX8.
Signed-off-by: Enrico Scholz
---
drivers/usb/imx/chipidea-imx.c | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
index f81477885884..bf8b6f1eb88a 100644
--- a/drivers/usb/imx/chipidea-imx.c
+++ b/drivers/usb/imx/chipidea-imx.c
@@ -218,6 +218,7 @@ static int imx_chipidea_probe(struct device_d *dev)
struct resource *iores;
struct imx_chipidea_data *imx_data;
struct imxusb_platformdata *pdata = dev->platform_data;
+ char const *phynode_name;
int ret;
void __iomem *base;
struct imx_chipidea *ci;
@@ -262,8 +263,19 @@ static int imx_chipidea_probe(struct device_d *dev)
if (!IS_ERR(ci->clk))
clk_enable(ci->clk);
- if (of_property_read_bool(dev->device_node, "fsl,usbphy")) {
- ci->phy = of_phy_get_by_phandle(dev, "fsl,usbphy", 0);
+ /* Device trees are using both "phys" and "fsl,usbphy". Prefer the
+* more modern former one but fall back to the old one.
+*
+* Code should be removed when all devicetrees are using "phys" */
+ if (of_property_read_bool(dev->device_node, "phys"))
+ phynode_name = "phys";
+ else if (of_property_read_bool(dev->device_node, "fsl,usbphy"))
+ phynode_name = "fsl,usbphy";
+ else
+ phynode_name = NULL;
+
+ if (phynode_name) {
+ ci->phy = of_phy_get_by_phandle(dev, phynode_name, 0);
if (IS_ERR(ci->phy)) {
dev_err(dev, "Cannot get phy: %pe\n", ci->phy);
return PTR_ERR(ci->phy);
--
2.33.1
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
[PATCH] dt:overlay: fix operation with multiple overlays
When applying multiple devicetree overlays the 'phandle' attribute
must be updated too. Else, every overlay will be adjusted to start
with the same base which causes duplicate phandles.
Signed-off-by: Enrico Scholz
---
drivers/of/base.c| 1 +
drivers/of/overlay.c | 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index edb0a8e71a0f..c88803d8ab4b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2385,6 +2385,7 @@ struct device_node *of_copy_node(struct device_node
*parent, const struct device
struct property *pp;
np = of_new_node(parent, other->name);
+ np->phandle = other->phandle;
list_for_each_entry(pp, >properties, list)
of_new_property(np, pp->name, pp->value, pp->length);
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 92d20b9a247e..79f8b6dfdbcf 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -66,8 +66,10 @@ static int of_overlay_apply(struct device_node *target,
for_each_child_of_node(overlay, child) {
target_child = of_get_child_by_name(target, child->name);
- if (!target_child)
+ if (!target_child) {
target_child = of_new_node(target, child->name);
+ target_child->phandle = child->phandle;
+ }
if (!target_child)
return -ENOMEM;
--
2.29.2
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
[PATCH] imx8:pinfunc: fix definition
Signed-off-by: Enrico Scholz --- dts/src/arm64/freescale/imx8mm-pinfunc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dts/src/arm64/freescale/imx8mm-pinfunc.h b/dts/src/arm64/freescale/imx8mm-pinfunc.h index 5ccc4cc91959..a003e6af3353 100644 --- a/dts/src/arm64/freescale/imx8mm-pinfunc.h +++ b/dts/src/arm64/freescale/imx8mm-pinfunc.h @@ -124,7 +124,7 @@ #define MX8MM_IOMUXC_SD1_CMD_USDHC1_CMD 0x0A4 0x30C 0x000 0x0 0x0 #define MX8MM_IOMUXC_SD1_CMD_GPIO2_IO1 0x0A4 0x30C 0x000 0x5 0x0 #define MX8MM_IOMUXC_SD1_DATA0_USDHC1_DATA0 0x0A8 0x310 0x000 0x0 0x0 -#define MX8MM_IOMUXC_SD1_DATA0_GPIO2_IO2 0x0A8 0x31 0x000 0x5 0x0 +#define MX8MM_IOMUXC_SD1_DATA0_GPIO2_IO2 0x0A8 0x310 0x000 0x5 0x0 #define MX8MM_IOMUXC_SD1_DATA1_USDHC1_DATA1 0x0AC 0x314 0x000 0x0 0x0 #define MX8MM_IOMUXC_SD1_DATA1_GPIO2_IO3 0x0AC 0x314 0x000 0x5 0x0 #define MX8MM_IOMUXC_SD1_DATA2_USDHC1_DATA2 0x0B0 0x318 0x000 0x0 0x0 -- 2.29.2 ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
[PATCH] nvmem: avoid false positive in of_nvmem_find() and simplify it
of_nvmem_find() compared only the node names which can return the
wrong device in setups like
| {
| eeprom@50 {
| };
| };
| {
| eeprom@50 {
| };
| };
Instead of checking the complete path ('full_name' attribute), the
patch compares the device_node pointers directly.
This is done in other places (e.g. of_find_i2c_adapter_by_node()) and
in the linux kernel too.
Signed-off-by: Enrico Scholz
---
drivers/nvmem/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 06e1414769aa..02d0af6e1de0 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -129,7 +129,7 @@ static struct nvmem_device *of_nvmem_find(struct
device_node *nvmem_np)
return NULL;
list_for_each_entry(dev, _devs, node)
- if (dev->dev.device_node->name &&
!strcmp(dev->dev.device_node->name, nvmem_np->name))
+ if (dev->dev.device_node == nvmem_np)
return dev;
return NULL;
--
2.29.2
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
[PATCH] net:fec: fixed unaligned access and stack corruption
on 64 bit architectures, the 'enum fec_type' might not be aligned and large enough to hold a pointer. When running barebox without MMU, this will crash like | i.MX8MM unique ID: dab4b7491a2c4209 | DABT (current EL) exception (ESR 0x9661) at 0xfffefeb4 | elr: ffe14c28 lr : ffe196e0 | x0 : 0002 x1 : fffefeb4 | x2 : ffe91370 x3 : bfe1b6e8 | x4 : x5 : 1100 | ... | Call trace: | [] (dev_get_drvdata+0xc/0x30) from [] (device_probe+0x54/0xd0) | [] (device_probe+0x54/0xd0) from [] (match+0x48/0x58) | [] (match+0x48/0x58) from [] (register_driver+0xc0/0xd0) | [] (register_driver+0xc0/0xd0) from [] (start_barebox+0x64/0x90) Signed-off-by: Enrico Scholz --- drivers/net/fec_imx.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c index 772f930f0d08..30ee7841faba 100644 --- a/drivers/net/fec_imx.c +++ b/drivers/net/fec_imx.c @@ -739,14 +739,17 @@ static int fec_probe(struct device_d *dev) void *base; int ret; enum fec_type type; + void const *type_v; int phy_reset; u32 msec = 1, phy_post_delay = 0; u32 reg; - ret = dev_get_drvdata(dev, (const void **)); + ret = dev_get_drvdata(dev, _v); if (ret) return ret; + type = (uintptr_t)(type_v); + fec = xzalloc(sizeof(*fec)); fec->type = type; fec->dev = dev; -- 2.25.4 ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
[PATCH 2/2] ARM: MMU: fixed calculation of number of PTEs
barebox uses 4KiB pages so that number of PTEs is 'size >> 12', not
'size >> 10'.
Thie 'size >> 10' limit is not an immediate problem because it allocates
too much PTEs only which are not used. But it can overflow an integer
multiplication ('i * PAGE_SIZE') which causes undefined behaviour with
gcc5.
Signed-off-by: Enrico Scholz <enrico.sch...@sigma-chemnitz.de>
---
arch/arm/cpu/mmu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/cpu/mmu.c b/arch/arm/cpu/mmu.c
index 8ceb450..014bba2 100644
--- a/arch/arm/cpu/mmu.c
+++ b/arch/arm/cpu/mmu.c
@@ -213,7 +213,7 @@ static int arm_mmu_remap_sdram(struct memory_bank *bank)
unsigned long phys = (unsigned long)bank->start;
unsigned long ttb_start = phys >> 20;
unsigned long ttb_end = (phys >> 20) + (bank->size >> 20);
- unsigned long num_ptes = bank->size >> 10;
+ unsigned long num_ptes = bank->size >> 12;
int i, pte;
u32 *ptes;
--
2.4.3
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
ARM: gcc5 causes undefined behaviour in mmu_init()
Hi,
a barebox built with gcc5 hangs in tlb_invalidate() very early. This
seems to be caused by an integer overflow:
--- arch/arm/cpu/mmu.c:
static int arm_mmu_remap_sdram(struct memory_bank *bank)
{
unsigned long num_ptes = bank->size >> 10;
int i;
for (i = 0; i < num_ptes; i++) {
ptes[i] = (phys + i * PAGE_SIZE) | PTE_TYPE_SMALL |
pte_flags_cached;
}
For 1GiB RAM, 'num_ptes' is 1MiB and due to integer promotion, the 'i *
PAGE_SIZE' overflows and causes undefined behavior.
A trivial fix is to make 'i' an unsigned int or long.
But I wonder whether calculation of 'num_ptes' is really correct. Does
barebox really use a 1KiB pagesize for PTEs or should the '>> 10' be a
'>> 12'? When it is really 1KiB, the mapping for memory sizes > 1GiB
seem to be ambiguous then.
For reference; in broken case ('int i'), gcc5 generates:
| 4badc: f7b4 fe7e bl 7dc
| 4bae0: f8d9 3000 ldr.w r3, [r9]
| 4bae4: f1aa 0104 sub.w r1, sl, #4
| 4bae8: f043 0202 orr.w r2, r3, #2
| 4baec: 9b03ldr r3, [sp, #12]
| 4baee: eb04 3303 add.w r3, r4, r3, lsl #12
|
| 4baf2: 42a3cmp r3, r4
| 4baf4: d006beq.n 4bb04
| 4baf6: ea42 0004 orr.w r0, r2, r4
| 4bafa: f504 5480 add.w r4, r4, #4096 ; 0x1000
| 4bafe: f841 0f04 str.w r0, [r1, #4]!
| 4bb02: e7f6b.n 4baf2
Building with 'unsigned long i' generates:
| 4bad8: f7b4 fe80 bl 7dc
| 4badc: 683bldr r3, [r7, #0]
| 4bade: f043 0202 orr.w r2, r3, #2
| 4bae2: 9b05ldr r3, [sp, #20]
|
| 4bae4: 9904ldr r1, [sp, #16]
| 4bae6: 4299cmp r1, r3
| 4bae8: d006beq.n 4baf8
| 4baea: eb0a 3103 add.w r1, sl, r3, lsl #12
| 4baee: 4311orrsr1, r2
| 4baf0: f849 1023 str.w r1, [r9, r3, lsl #2]
| 4baf4: 3301addsr3, #1
| 4baf6: e7f5b.n 4bae4
Enrico
--
SIGMA Chemnitz GmbH Registergericht: Amtsgericht Chemnitz HRB 1750
Am Erlenwald 13 Geschaeftsfuehrer: Grit Freitag, Frank Pyritz
09128 Chemnitz
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
