[U-Boot] [PATCH v3 0/6] Raspberry Pi4: add support for DFU over USB

[U-Boot] [PATCH v3 0/6] Raspberry Pi4: add support for DFU over USB

[Marek Szyprowski]

Hi All!

This patchset enables support for DFU over USB protocol on Raspberry Pi4
board. The board has DWC2 UDC controller connected to the USB-C power
connector. Enabling DFU on it, make the u-boot development much more
convenient, as one no longer needs to swap SD-card between RPi4 board and
host machine to update the u-boot binary.

Patches are based on current 'master' u-boot branch. They were tested on
the 2019-07-10-raspbian-buster-lite.img sd-card image with the following
lines added to config.txt:
dtoverlay=dwc2,dr_mode=peripheral
dtparam=i2c_arm=on
dtparam=spi=on
enable_uart=1
uart_2ndstage=1
kernel=u-boot.bin

To enable DFU, one has to enter follwing command:
# dfu 0 mmc 0

During the development of this feature I've encountered a serious bugs
in FAT write code. Over-writing discontiguous files always caused serious
filesystem corruption. This was especially anoying, because the system
environment is kept on FAT volume in uboot.env file, so 'saveenv'
basically corrupted the boot partiting on the second call. Another bunch
of the issues in the FAT write code has been revealed while removing
predefined file size limit in DFU MMC code and then running sandbox
tests.

I hope that my fixes for FAT code will be helpful for non-RPi users too.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


Changelog:

v3:
- fixed one more FAT issue revealed by sandbox tests

v3: (patch 6/6 posted separately): https://patchwork.ozlabs.org/patch/1195645/
- fixed non-RPi4 builds (missing #else ENV_DFU_SETTINGS def)
- removed config.txt entity (not needed in uboot-based boot)
- switched arm64 kernel filename to 'Image'

v2: https://patchwork.ozlabs.org/cover/1166589/
- added changes to rpi_4_defconfig too (arm64 version)
- extended DFU entity list by confix.txt, cmdline.txt and Image.gz
- fixed missing '\0' at the end of dfu_alt_info env
- added reviewed-by tags
- added patches for DFU MMC to remove file size limit
- added patch for fat write to fix issues on non-zero file offset
  (revealed by previous patch)

v1: https://patchwork.ozlabs.org/cover/1162770/
- initial version


Patch summary:

Marek Szyprowski (6):
  fat: write: fix broken write to fragmented files
  fat: write: fix broken write at non-zero file offset
  dfu: mmc: rearrange the code
  dfu: mmc: remove file size limit for io operations
  usb: dwc2_udc_otg: add bcm2835 SoC (Raspberry Pi4) support
  config: enable DFU over USB on Raspberry Pi4 boards

 configs/rpi_4_32b_defconfig                | 11 +++
 configs/rpi_4_defconfig                    | 11 +++
 drivers/dfu/dfu_mmc.c                      | 93 +++++++++++++---------
 drivers/usb/gadget/dwc2_udc_otg.c          |  2 +
 drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 12 +--
 fs/fat/fat_write.c                         | 19 +++--
 include/configs/rpi.h                      | 20 +++++
 7 files changed, 116 insertions(+), 52 deletions(-)

-- 
2.17.1

[U-Boot] [PATCH v3 1/6] fat: write: fix broken write to fragmented files

[Marek Szyprowski]

The code for handing file overwrite incorrectly assumed that the file on
disk is always contiguous. This resulted in corrupting disk structure
every time when write to existing fragmented file happened. Fix this
by adding proper check for cluster discontinuity and adjust chunk size
on each partial write.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
Reviewed-by: Lukasz Majewski <lukma@denx.de>
---
 fs/fat/fat_write.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 729cf39630..6cfa5b4565 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -794,6 +794,8 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
 
 			newclust = get_fatent(mydata, endclust);
 
+			if ((newclust - 1) != endclust)
+				break;
 			if (IS_LAST_CLUST(newclust, mydata->fatsize))
 				break;
 			if (CHECK_CLUST(newclust, mydata->fatsize)) {
@@ -811,7 +813,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
 			offset = 0;
 		else
 			offset = pos - cur_pos;
-		wsize = min(cur_pos + actsize, filesize) - pos;
+		wsize = min_t(unsigned long long, actsize, filesize - cur_pos);
 		if (get_set_cluster(mydata, curclust, offset,
 				    buffer, wsize, &actsize)) {
 			printf("Error get-and-setting cluster\n");
@@ -824,8 +826,6 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
 		if (filesize <= cur_pos)
 			break;
 
-		/* CHECK: newclust = get_fatent(mydata, endclust); */
-
 		if (IS_LAST_CLUST(newclust, mydata->fatsize))
 			/* no more clusters */
 			break;
-- 
2.17.1

[U-Boot] [PATCH v3 2/6] fat: write: fix broken write at non-zero file offset

[Marek Szyprowski]

Handling of the start file offset was broken in the current code. Although
the code skipped the needed clusters, it then tried to continue write with
current cluster set to EOF, what caused assertion. It also lacked adjusting
filesize in case of writing at the end of file and adjusting in-cluster
offset for partial overwrite.

This patch fixes all those issues.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 fs/fat/fat_write.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 6cfa5b4565..7fb373589d 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -756,14 +756,12 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
 	/* go to cluster at pos */
 	cur_pos = bytesperclust;
 	while (1) {
+		newclust = get_fatent(mydata, curclust);
 		if (pos <= cur_pos)
 			break;
-		if (IS_LAST_CLUST(curclust, mydata->fatsize))
+		if (IS_LAST_CLUST(newclust, mydata->fatsize))
 			break;
-
-		newclust = get_fatent(mydata, curclust);
-		if (!IS_LAST_CLUST(newclust, mydata->fatsize) &&
-		    CHECK_CLUST(newclust, mydata->fatsize)) {
+		if (CHECK_CLUST(newclust, mydata->fatsize)) {
 			debug("curclust: 0x%x\n", curclust);
 			debug("Invalid FAT entry\n");
 			return -1;
@@ -772,8 +770,8 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
 		cur_pos += bytesperclust;
 		curclust = newclust;
 	}
-	if (IS_LAST_CLUST(curclust, mydata->fatsize)) {
-		assert(pos == cur_pos);
+	if (pos == cur_pos && IS_LAST_CLUST(newclust, mydata->fatsize)) {
+		filesize -= pos;
 		goto set_clusters;
 	}
 
@@ -814,6 +812,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
 		else
 			offset = pos - cur_pos;
 		wsize = min_t(unsigned long long, actsize, filesize - cur_pos);
+		wsize -= offset;
 		if (get_set_cluster(mydata, curclust, offset,
 				    buffer, wsize, &actsize)) {
 			printf("Error get-and-setting cluster\n");
-- 
2.17.1

[U-Boot] [PATCH v3 3/6] dfu: mmc: rearrange the code

[Marek Szyprowski]

Rename functions for bufferred file io operations to make them easier to
understand. Also add missing file offset argument to them (currently
unused). All this is a preparation to remove predefined file size limit
(CONFIG_SYS_DFU_MAX_FILE_SIZE) for DFU read/write operations.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Acked-by: Lukasz Majewski <lukma@denx.de>
---
 drivers/dfu/dfu_mmc.c | 61 ++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 27 deletions(-)

diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
index 5b551f6ae1..e52c02be10 100644
--- a/drivers/dfu/dfu_mmc.c
+++ b/drivers/dfu/dfu_mmc.c
@@ -91,22 +91,8 @@ static int mmc_block_op(enum dfu_op op, struct dfu_entity *dfu,
 	return 0;
 }
 
-static int mmc_file_buffer(struct dfu_entity *dfu, void *buf, long *len)
-{
-	if (dfu_file_buf_len + *len > CONFIG_SYS_DFU_MAX_FILE_SIZE) {
-		dfu_file_buf_len = 0;
-		return -EINVAL;
-	}
-
-	/* Add to the current buffer. */
-	memcpy(dfu_file_buf + dfu_file_buf_len, buf, *len);
-	dfu_file_buf_len += *len;
-
-	return 0;
-}
-
 static int mmc_file_op(enum dfu_op op, struct dfu_entity *dfu,
-			void *buf, u64 *len)
+			u64 offset, void *buf, u64 *len)
 {
 	char dev_part_str[8];
 	int ret;
@@ -137,7 +123,7 @@ static int mmc_file_op(enum dfu_op op, struct dfu_entity *dfu,
 
 	switch (op) {
 	case DFU_OP_READ:
-		ret = fs_read(dfu->name, (size_t)buf, 0, 0, &size);
+		ret = fs_read(dfu->name, (size_t)buf, offset, 0, &size);
 		if (ret) {
 			puts("dfu: fs_read error!\n");
 			return ret;
@@ -145,7 +131,7 @@ static int mmc_file_op(enum dfu_op op, struct dfu_entity *dfu,
 		*len = size;
 		break;
 	case DFU_OP_WRITE:
-		ret = fs_write(dfu->name, (size_t)buf, 0, *len, &size);
+		ret = fs_write(dfu->name, (size_t)buf, offset, *len, &size);
 		if (ret) {
 			puts("dfu: fs_write error!\n");
 			return ret;
@@ -166,6 +152,30 @@ static int mmc_file_op(enum dfu_op op, struct dfu_entity *dfu,
 	return ret;
 }
 
+static int mmc_file_buf_write(struct dfu_entity *dfu, u64 offset, void *buf, long *len)
+{
+	if (dfu_file_buf_len + *len > CONFIG_SYS_DFU_MAX_FILE_SIZE) {
+		dfu_file_buf_len = 0;
+		return -EINVAL;
+	}
+
+	/* Add to the current buffer. */
+	memcpy(dfu_file_buf + dfu_file_buf_len, buf, *len);
+	dfu_file_buf_len += *len;
+
+	return 0;
+}
+
+static int mmc_file_buf_write_finish(struct dfu_entity *dfu)
+{
+	int ret = mmc_file_op(DFU_OP_WRITE, dfu, 0, dfu_file_buf,
+			&dfu_file_buf_len);
+
+	/* Now that we're done */
+	dfu_file_buf_len = 0;
+	return ret;
+}
+
 int dfu_write_medium_mmc(struct dfu_entity *dfu,
 		u64 offset, void *buf, long *len)
 {
@@ -177,7 +187,7 @@ int dfu_write_medium_mmc(struct dfu_entity *dfu,
 		break;
 	case DFU_FS_FAT:
 	case DFU_FS_EXT4:
-		ret = mmc_file_buffer(dfu, buf, len);
+		ret = mmc_file_buf_write(dfu, offset, buf, len);
 		break;
 	default:
 		printf("%s: Layout (%s) not (yet) supported!\n", __func__,
@@ -193,11 +203,7 @@ int dfu_flush_medium_mmc(struct dfu_entity *dfu)
 
 	if (dfu->layout != DFU_RAW_ADDR) {
 		/* Do stuff here. */
-		ret = mmc_file_op(DFU_OP_WRITE, dfu, dfu_file_buf,
-				&dfu_file_buf_len);
-
-		/* Now that we're done */
-		dfu_file_buf_len = 0;
+		ret = mmc_file_buf_write_finish(dfu);
 	}
 
 	return ret;
@@ -214,7 +220,7 @@ int dfu_get_medium_size_mmc(struct dfu_entity *dfu, u64 *size)
 	case DFU_FS_FAT:
 	case DFU_FS_EXT4:
 		dfu_file_buf_filled = -1;
-		ret = mmc_file_op(DFU_OP_SIZE, dfu, NULL, size);
+		ret = mmc_file_op(DFU_OP_SIZE, dfu, 0, NULL, size);
 		if (ret < 0)
 			return ret;
 		if (*size > CONFIG_SYS_DFU_MAX_FILE_SIZE)
@@ -227,14 +233,15 @@ int dfu_get_medium_size_mmc(struct dfu_entity *dfu, u64 *size)
 	}
 }
 
-static int mmc_file_unbuffer(struct dfu_entity *dfu, u64 offset, void *buf,
+
+static int mmc_file_buf_read(struct dfu_entity *dfu, u64 offset, void *buf,
 			     long *len)
 {
 	int ret;
 	u64 file_len;
 
 	if (dfu_file_buf_filled == -1) {
-		ret = mmc_file_op(DFU_OP_READ, dfu, dfu_file_buf, &file_len);
+		ret = mmc_file_op(DFU_OP_READ, dfu, 0, dfu_file_buf, &file_len);
 		if (ret < 0)
 			return ret;
 		dfu_file_buf_filled = file_len;
@@ -259,7 +266,7 @@ int dfu_read_medium_mmc(struct dfu_entity *dfu, u64 offset, void *buf,
 		break;
 	case DFU_FS_FAT:
 	case DFU_FS_EXT4:
-		ret = mmc_file_unbuffer(dfu, offset, buf, len);
+		ret = mmc_file_buf_read(dfu, offset, buf, len);
 		break;
 	default:
 		printf("%s: Layout (%s) not (yet) supported!\n", __func__,
-- 
2.17.1

[U-Boot] [PATCH v3 4/6] dfu: mmc: remove file size limit for io operations

[Marek Szyprowski]

Add support for operations on files larger than
CONFIG_SYS_DFU_MAX_FILE_SIZE. The buffered io mechanism is still used for
aggregating io requests, so for files up to CONFIG_SYS_DFU_MAX_FILE_SIZE
nothing is changed and they will be handled in a single filesystem call.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Acked-by: Lukasz Majewski <lukma@denx.de>
---
 drivers/dfu/dfu_mmc.c | 46 ++++++++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
index e52c02be10..0d495a785b 100644
--- a/drivers/dfu/dfu_mmc.c
+++ b/drivers/dfu/dfu_mmc.c
@@ -17,7 +17,7 @@
 
 static unsigned char *dfu_file_buf;
 static u64 dfu_file_buf_len;
-static long dfu_file_buf_filled;
+static u64 dfu_file_buf_offset;
 
 static int mmc_block_op(enum dfu_op op, struct dfu_entity *dfu,
 			u64 offset, void *buf, long *len)
@@ -123,7 +123,7 @@ static int mmc_file_op(enum dfu_op op, struct dfu_entity *dfu,
 
 	switch (op) {
 	case DFU_OP_READ:
-		ret = fs_read(dfu->name, (size_t)buf, offset, 0, &size);
+		ret = fs_read(dfu->name, (size_t)buf, offset, *len, &size);
 		if (ret) {
 			puts("dfu: fs_read error!\n");
 			return ret;
@@ -154,25 +154,38 @@ static int mmc_file_op(enum dfu_op op, struct dfu_entity *dfu,
 
 static int mmc_file_buf_write(struct dfu_entity *dfu, u64 offset, void *buf, long *len)
 {
-	if (dfu_file_buf_len + *len > CONFIG_SYS_DFU_MAX_FILE_SIZE) {
+	int ret = 0;
+
+	if (offset == 0) {
 		dfu_file_buf_len = 0;
-		return -EINVAL;
+		dfu_file_buf_offset = 0;
 	}
 
 	/* Add to the current buffer. */
+	if (dfu_file_buf_len + *len > CONFIG_SYS_DFU_MAX_FILE_SIZE)
+		*len = CONFIG_SYS_DFU_MAX_FILE_SIZE - dfu_file_buf_len;
 	memcpy(dfu_file_buf + dfu_file_buf_len, buf, *len);
 	dfu_file_buf_len += *len;
 
-	return 0;
+	if (dfu_file_buf_len == CONFIG_SYS_DFU_MAX_FILE_SIZE) {
+		ret = mmc_file_op(DFU_OP_WRITE, dfu, dfu_file_buf_offset,
+				  dfu_file_buf, &dfu_file_buf_len);
+		dfu_file_buf_offset += dfu_file_buf_len;
+		dfu_file_buf_len = 0;
+	}
+
+	return ret;
 }
 
 static int mmc_file_buf_write_finish(struct dfu_entity *dfu)
 {
-	int ret = mmc_file_op(DFU_OP_WRITE, dfu, 0, dfu_file_buf,
-			&dfu_file_buf_len);
+	int ret = mmc_file_op(DFU_OP_WRITE, dfu, dfu_file_buf_offset,
+			dfu_file_buf, &dfu_file_buf_len);
 
 	/* Now that we're done */
 	dfu_file_buf_len = 0;
+	dfu_file_buf_offset = 0;
+
 	return ret;
 }
 
@@ -219,12 +232,9 @@ int dfu_get_medium_size_mmc(struct dfu_entity *dfu, u64 *size)
 		return 0;
 	case DFU_FS_FAT:
 	case DFU_FS_EXT4:
-		dfu_file_buf_filled = -1;
 		ret = mmc_file_op(DFU_OP_SIZE, dfu, 0, NULL, size);
 		if (ret < 0)
 			return ret;
-		if (*size > CONFIG_SYS_DFU_MAX_FILE_SIZE)
-			return -1;
 		return 0;
 	default:
 		printf("%s: Layout (%s) not (yet) supported!\n", __func__,
@@ -238,19 +248,23 @@ static int mmc_file_buf_read(struct dfu_entity *dfu, u64 offset, void *buf,
 			     long *len)
 {
 	int ret;
-	u64 file_len;
 
-	if (dfu_file_buf_filled == -1) {
-		ret = mmc_file_op(DFU_OP_READ, dfu, 0, dfu_file_buf, &file_len);
+	if (offset == 0 || offset >= dfu_file_buf_offset + dfu_file_buf_len ||
+	    offset + *len < dfu_file_buf_offset) {
+		u64 file_len = CONFIG_SYS_DFU_MAX_FILE_SIZE;
+
+		ret = mmc_file_op(DFU_OP_READ, dfu, offset, dfu_file_buf,
+				  &file_len);
 		if (ret < 0)
 			return ret;
-		dfu_file_buf_filled = file_len;
+		dfu_file_buf_len = file_len;
+		dfu_file_buf_offset = offset;
 	}
-	if (offset + *len > dfu_file_buf_filled)
+	if (offset + *len > dfu_file_buf_offset + dfu_file_buf_len)
 		return -EINVAL;
 
 	/* Add to the current buffer. */
-	memcpy(buf, dfu_file_buf + offset, *len);
+	memcpy(buf, dfu_file_buf + offset - dfu_file_buf_offset, *len);
 
 	return 0;
 }
-- 
2.17.1

[U-Boot] [PATCH v3 5/6] usb: dwc2_udc_otg: add bcm2835 SoC (Raspberry Pi4) support

[Marek Szyprowski]

Broadcom 2835 SoC requires special conversion of physical memory addresses
for DMA purpose, so add needed wrappers to dwc2_udc_otg driver. Also extend
the list of compatible devices with 'brcm,bcm2835-usb' entry. This allows
to use USB gadget drivers (i.e. DFU) on Raspberry Pi4 boards.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Lukasz Majewski <lukma@denx.de>
---
 drivers/usb/gadget/dwc2_udc_otg.c          |  2 ++
 drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 12 ++++++------
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/dwc2_udc_otg.c b/drivers/usb/gadget/dwc2_udc_otg.c
index 35f4147840..49f342eb21 100644
--- a/drivers/usb/gadget/dwc2_udc_otg.c
+++ b/drivers/usb/gadget/dwc2_udc_otg.c
@@ -31,6 +31,7 @@
 #include <linux/usb/otg.h>
 #include <linux/usb/gadget.h>
 
+#include <phys2bus.h>
 #include <asm/byteorder.h>
 #include <asm/unaligned.h>
 #include <asm/io.h>
@@ -1213,6 +1214,7 @@ static int dwc2_udc_otg_remove(struct udevice *dev)
 
 static const struct udevice_id dwc2_udc_otg_ids[] = {
 	{ .compatible = "snps,dwc2" },
+	{ .compatible = "brcm,bcm2835-usb" },
 	{ .compatible = "st,stm32mp1-hsotg",
 	  .data = (ulong)dwc2_set_stm32mp1_hsotg_params },
 	{},
diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
index 7eb632d3b1..5e695b4ff2 100644
--- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
+++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
@@ -28,7 +28,7 @@ static inline void dwc2_udc_ep0_zlp(struct dwc2_udc *dev)
 {
 	u32 ep_ctrl;
 
-	writel(usb_ctrl_dma_addr, &reg->in_endp[EP0_CON].diepdma);
+	writel(phys_to_bus((unsigned long)usb_ctrl_dma_addr), &reg->in_endp[EP0_CON].diepdma);
 	writel(DIEPT_SIZ_PKT_CNT(1), &reg->in_endp[EP0_CON].dieptsiz);
 
 	ep_ctrl = readl(&reg->in_endp[EP0_CON].diepctl);
@@ -49,7 +49,7 @@ static void dwc2_udc_pre_setup(void)
 
 	writel(DOEPT_SIZ_PKT_CNT(1) | sizeof(struct usb_ctrlrequest),
 	       &reg->out_endp[EP0_CON].doeptsiz);
-	writel(usb_ctrl_dma_addr, &reg->out_endp[EP0_CON].doepdma);
+	writel(phys_to_bus((unsigned long)usb_ctrl_dma_addr), &reg->out_endp[EP0_CON].doepdma);
 
 	ep_ctrl = readl(&reg->out_endp[EP0_CON].doepctl);
 	writel(ep_ctrl|DEPCTL_EPENA, &reg->out_endp[EP0_CON].doepctl);
@@ -75,7 +75,7 @@ static inline void dwc2_ep0_complete_out(void)
 
 	writel(DOEPT_SIZ_PKT_CNT(1) | sizeof(struct usb_ctrlrequest),
 	       &reg->out_endp[EP0_CON].doeptsiz);
-	writel(usb_ctrl_dma_addr, &reg->out_endp[EP0_CON].doepdma);
+	writel(phys_to_bus((unsigned long)usb_ctrl_dma_addr), &reg->out_endp[EP0_CON].doepdma);
 
 	ep_ctrl = readl(&reg->out_endp[EP0_CON].doepctl);
 	writel(ep_ctrl|DEPCTL_EPENA|DEPCTL_CNAK,
@@ -113,7 +113,7 @@ static int setdma_rx(struct dwc2_ep *ep, struct dwc2_request *req)
 				(unsigned long) ep->dma_buf +
 				ROUND(ep->len, CONFIG_SYS_CACHELINE_SIZE));
 
-	writel((unsigned long) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
+	writel(phys_to_bus((unsigned long)ep->dma_buf), &reg->out_endp[ep_num].doepdma);
 	writel(DOEPT_SIZ_PKT_CNT(pktcnt) | DOEPT_SIZ_XFER_SIZE(length),
 	       &reg->out_endp[ep_num].doeptsiz);
 	writel(DEPCTL_EPENA|DEPCTL_CNAK|ctrl, &reg->out_endp[ep_num].doepctl);
@@ -161,7 +161,7 @@ static int setdma_tx(struct dwc2_ep *ep, struct dwc2_request *req)
 	while (readl(&reg->grstctl) & TX_FIFO_FLUSH)
 		;
 
-	writel((unsigned long) ep->dma_buf, &reg->in_endp[ep_num].diepdma);
+	writel(phys_to_bus((unsigned long)ep->dma_buf), &reg->in_endp[ep_num].diepdma);
 	writel(DIEPT_SIZ_PKT_CNT(pktcnt) | DIEPT_SIZ_XFER_SIZE(length),
 	       &reg->in_endp[ep_num].dieptsiz);
 
@@ -921,7 +921,7 @@ static int dwc2_udc_get_status(struct dwc2_udc *dev,
 			   (unsigned long) usb_ctrl +
 			   ROUND(sizeof(g_status), CONFIG_SYS_CACHELINE_SIZE));
 
-	writel(usb_ctrl_dma_addr, &reg->in_endp[EP0_CON].diepdma);
+	writel(phys_to_bus(usb_ctrl_dma_addr), &reg->in_endp[EP0_CON].diepdma);
 	writel(DIEPT_SIZ_PKT_CNT(1) | DIEPT_SIZ_XFER_SIZE(2),
 	       &reg->in_endp[EP0_CON].dieptsiz);
 
-- 
2.17.1

[U-Boot] [PATCH v3 6/6] config: enable DFU over USB on Raspberry Pi4 boards

[Marek Szyprowski]

Enable support for DFU over USB. This requires to enable USB gadget,
DWC2 UDC OTG driver and DFU command. DFU entities are defined for the
following firmware objects: u-boot.bin, uboot.env, config.txt, and
zImage/Image.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Lukasz Majewski <lukma@denx.de>
---
 configs/rpi_4_32b_defconfig | 11 +++++++++++
 configs/rpi_4_defconfig     | 11 +++++++++++
 include/configs/rpi.h       | 20 ++++++++++++++++++++
 3 files changed, 42 insertions(+)

diff --git a/configs/rpi_4_32b_defconfig b/configs/rpi_4_32b_defconfig
index dc696906fd..a0ba8782bc 100644
--- a/configs/rpi_4_32b_defconfig
+++ b/configs/rpi_4_32b_defconfig
@@ -11,6 +11,7 @@ CONFIG_MISC_INIT_R=y
 # CONFIG_DISPLAY_CPUINFO is not set
 # CONFIG_DISPLAY_BOARDINFO is not set
 CONFIG_SYS_PROMPT="U-Boot> "
+CONFIG_CMD_DFU=y
 # CONFIG_CMD_FLASH is not set
 CONFIG_CMD_GPIO=y
 CONFIG_CMD_MMC=y
@@ -19,6 +20,7 @@ CONFIG_OF_BOARD=y
 CONFIG_ENV_FAT_INTERFACE="mmc"
 CONFIG_ENV_FAT_DEVICE_AND_PART="0:1"
 CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG=y
+CONFIG_DFU_MMC=y
 CONFIG_DM_KEYBOARD=y
 CONFIG_DM_MMC=y
 CONFIG_MMC_SDHCI=y
@@ -26,6 +28,15 @@ CONFIG_MMC_SDHCI_BCM2835=y
 CONFIG_PINCTRL=y
 # CONFIG_PINCTRL_GENERIC is not set
 # CONFIG_REQUIRE_SERIAL_CONSOLE is not set
+CONFIG_USB=y
+CONFIG_DM_USB=y
+CONFIG_DM_USB_GADGET=y
+CONFIG_USB_GADGET=y
+CONFIG_USB_GADGET_MANUFACTURER="FSL"
+CONFIG_USB_GADGET_VENDOR_NUM=0x0525
+CONFIG_USB_GADGET_PRODUCT_NUM=0xa4a5
+CONFIG_USB_GADGET_DWC2_OTG=y
+CONFIG_USB_GADGET_DOWNLOAD=y
 CONFIG_DM_VIDEO=y
 CONFIG_SYS_WHITE_ON_BLACK=y
 CONFIG_CONSOLE_SCROLL_LINES=10
diff --git a/configs/rpi_4_defconfig b/configs/rpi_4_defconfig
index 2954e17ac3..2fcd56ebf3 100644
--- a/configs/rpi_4_defconfig
+++ b/configs/rpi_4_defconfig
@@ -11,6 +11,7 @@ CONFIG_MISC_INIT_R=y
 # CONFIG_DISPLAY_CPUINFO is not set
 # CONFIG_DISPLAY_BOARDINFO is not set
 CONFIG_SYS_PROMPT="U-Boot> "
+CONFIG_CMD_DFU=y
 # CONFIG_CMD_FLASH is not set
 CONFIG_CMD_GPIO=y
 CONFIG_CMD_MMC=y
@@ -19,6 +20,7 @@ CONFIG_OF_BOARD=y
 CONFIG_ENV_FAT_INTERFACE="mmc"
 CONFIG_ENV_FAT_DEVICE_AND_PART="0:1"
 CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG=y
+CONFIG_DFU_MMC=y
 CONFIG_DM_KEYBOARD=y
 CONFIG_DM_MMC=y
 CONFIG_MMC_SDHCI=y
@@ -26,6 +28,15 @@ CONFIG_MMC_SDHCI_BCM2835=y
 CONFIG_PINCTRL=y
 # CONFIG_PINCTRL_GENERIC is not set
 # CONFIG_REQUIRE_SERIAL_CONSOLE is not set
+CONFIG_USB=y
+CONFIG_DM_USB=y
+CONFIG_DM_USB_GADGET=y
+CONFIG_USB_GADGET=y
+CONFIG_USB_GADGET_MANUFACTURER="FSL"
+CONFIG_USB_GADGET_VENDOR_NUM=0x0525
+CONFIG_USB_GADGET_PRODUCT_NUM=0xa4a5
+CONFIG_USB_GADGET_DWC2_OTG=y
+CONFIG_USB_GADGET_DOWNLOAD=y
 CONFIG_DM_VIDEO=y
 CONFIG_SYS_WHITE_ON_BLACK=y
 CONFIG_CONSOLE_SCROLL_LINES=10
diff --git a/include/configs/rpi.h b/include/configs/rpi.h
index 77d2d5458a..6723c7cc92 100644
--- a/include/configs/rpi.h
+++ b/include/configs/rpi.h
@@ -70,6 +70,25 @@
 #define CONFIG_TFTP_TSIZE
 #endif
 
+/* DFU over USB/UDC */
+#ifdef CONFIG_CMD_DFU
+#define CONFIG_SYS_DFU_DATA_BUF_SIZE	SZ_1M
+#define CONFIG_SYS_DFU_MAX_FILE_SIZE	SZ_2M
+
+#ifdef CONFIG_ARM64
+#define KERNEL_FILENAME		"Image"
+#else
+#define KERNEL_FILENAME		"zImage"
+#endif
+
+#define ENV_DFU_SETTINGS \
+	"dfu_alt_info=u-boot.bin fat 0 1;uboot.env fat 0 1;" \
+		      "config.txt fat 0 1;" \
+		      KERNEL_FILENAME " fat 0 1\0"
+#else
+#define ENV_DFU_SETTINGS ""
+#endif
+
 /* Console configuration */
 #define CONFIG_SYS_CBSIZE		1024
 
@@ -185,6 +204,7 @@
 #define CONFIG_EXTRA_ENV_SETTINGS \
 	"dhcpuboot=usb start; dhcp u-boot.uimg; bootm\0" \
 	ENV_DEVICE_SETTINGS \
+	ENV_DFU_SETTINGS \
 	ENV_MEM_LAYOUT_SETTINGS \
 	BOOTENV
 
-- 
2.17.1

[U-Boot] [PATCH v3 1/6] fat: write: fix broken write to fragmented files

[Tom Rini]

On Tue, Nov 26, 2019 at 09:15:07AM +0100, Marek Szyprowski wrote:

> The code for handing file overwrite incorrectly assumed that the file on
> disk is always contiguous. This resulted in corrupting disk structure
> every time when write to existing fragmented file happened. Fix this
> by adding proper check for cluster discontinuity and adjust chunk size
> on each partial write.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> ---
>  fs/fat/fat_write.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> index 729cf39630..6cfa5b4565 100644
> --- a/fs/fat/fat_write.c
> +++ b/fs/fat/fat_write.c
> @@ -794,6 +794,8 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>  
>  			newclust = get_fatent(mydata, endclust);
>  
> +			if ((newclust - 1) != endclust)
> +				break;
>  			if (IS_LAST_CLUST(newclust, mydata->fatsize))
>  				break;
>  			if (CHECK_CLUST(newclust, mydata->fatsize)) {
> @@ -811,7 +813,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>  			offset = 0;
>  		else
>  			offset = pos - cur_pos;
> -		wsize = min(cur_pos + actsize, filesize) - pos;
> +		wsize = min_t(unsigned long long, actsize, filesize - cur_pos);
>  		if (get_set_cluster(mydata, curclust, offset,
>  				    buffer, wsize, &actsize)) {
>  			printf("Error get-and-setting cluster\n");
> @@ -824,8 +826,6 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>  		if (filesize <= cur_pos)
>  			break;
>  
> -		/* CHECK: newclust = get_fatent(mydata, endclust); */
> -
>  		if (IS_LAST_CLUST(newclust, mydata->fatsize))
>  			/* no more clusters */
>  			break;

Adding in Heinrich and Akashi-san for more review on this, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191126/5e0b690a/attachment.sig>

[U-Boot] [PATCH v3 2/6] fat: write: fix broken write at non-zero file offset

[Tom Rini]

On Tue, Nov 26, 2019 at 09:15:08AM +0100, Marek Szyprowski wrote:

> Handling of the start file offset was broken in the current code. Although
> the code skipped the needed clusters, it then tried to continue write with
> current cluster set to EOF, what caused assertion. It also lacked adjusting
> filesize in case of writing at the end of file and adjusting in-cluster
> offset for partial overwrite.
> 
> This patch fixes all those issues.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  fs/fat/fat_write.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> index 6cfa5b4565..7fb373589d 100644
> --- a/fs/fat/fat_write.c
> +++ b/fs/fat/fat_write.c
> @@ -756,14 +756,12 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>  	/* go to cluster at pos */
>  	cur_pos = bytesperclust;
>  	while (1) {
> +		newclust = get_fatent(mydata, curclust);
>  		if (pos <= cur_pos)
>  			break;
> -		if (IS_LAST_CLUST(curclust, mydata->fatsize))
> +		if (IS_LAST_CLUST(newclust, mydata->fatsize))
>  			break;
> -
> -		newclust = get_fatent(mydata, curclust);
> -		if (!IS_LAST_CLUST(newclust, mydata->fatsize) &&
> -		    CHECK_CLUST(newclust, mydata->fatsize)) {
> +		if (CHECK_CLUST(newclust, mydata->fatsize)) {
>  			debug("curclust: 0x%x\n", curclust);
>  			debug("Invalid FAT entry\n");
>  			return -1;
> @@ -772,8 +770,8 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>  		cur_pos += bytesperclust;
>  		curclust = newclust;
>  	}
> -	if (IS_LAST_CLUST(curclust, mydata->fatsize)) {
> -		assert(pos == cur_pos);
> +	if (pos == cur_pos && IS_LAST_CLUST(newclust, mydata->fatsize)) {
> +		filesize -= pos;
>  		goto set_clusters;
>  	}
>  
> @@ -814,6 +812,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>  		else
>  			offset = pos - cur_pos;
>  		wsize = min_t(unsigned long long, actsize, filesize - cur_pos);
> +		wsize -= offset;
>  		if (get_set_cluster(mydata, curclust, offset,
>  				    buffer, wsize, &actsize)) {
>  			printf("Error get-and-setting cluster\n");

Adding in Heinrich and Akashi-san for more review on this, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191126/d3dbec99/attachment.sig>

[U-Boot] [PATCH v3 1/6] fat: write: fix broken write to fragmented files

[AKASHI Takahiro]

Thank you for the heads-up.

On Tue, Nov 26, 2019 at 11:57:29AM -0500, Tom Rini wrote:
> On Tue, Nov 26, 2019 at 09:15:07AM +0100, Marek Szyprowski wrote:
> 
> > The code for handing file overwrite incorrectly assumed that the file on
> > disk is always contiguous. This resulted in corrupting disk structure
> > every time when write to existing fragmented file happened. Fix this
> > by adding proper check for cluster discontinuity and adjust chunk size
> > on each partial write.
> > 
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> > Reviewed-by: Lukasz Majewski <lukma@denx.de>
> > ---
> >  fs/fat/fat_write.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> > index 729cf39630..6cfa5b4565 100644
> > --- a/fs/fat/fat_write.c
> > +++ b/fs/fat/fat_write.c
> > @@ -794,6 +794,8 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
> >  
> >  			newclust = get_fatent(mydata, endclust);
> >  
> > +			if ((newclust - 1) != endclust)

"newclust != (endclust + 1)" would be more intuitive?
But it's just my preference.

> > +				break;
> >  			if (IS_LAST_CLUST(newclust, mydata->fatsize))
> >  				break;
> >  			if (CHECK_CLUST(newclust, mydata->fatsize)) {
> > @@ -811,7 +813,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
> >  			offset = 0;
> >  		else
> >  			offset = pos - cur_pos;
> > -		wsize = min(cur_pos + actsize, filesize) - pos;
> > +		wsize = min_t(unsigned long long, actsize, filesize - cur_pos);

This hunk is not directly related to the issue, is it?

> >  		if (get_set_cluster(mydata, curclust, offset,
> >  				    buffer, wsize, &actsize)) {
> >  			printf("Error get-and-setting cluster\n");
> > @@ -824,8 +826,6 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
> >  		if (filesize <= cur_pos)
> >  			break;
> >  
> > -		/* CHECK: newclust = get_fatent(mydata, endclust); */
> > -
> >  		if (IS_LAST_CLUST(newclust, mydata->fatsize))
> >  			/* no more clusters */
> >  			break;
> 
> Adding in Heinrich and Akashi-san for more review on this, thanks!

Otherwise, it looks good.
Reviewed-by: AKASHI Takahiro <takahiro.akashi@linaro.org>


> -- 
> Tom

[U-Boot] [PATCH v3 2/6] fat: write: fix broken write at non-zero file offset

[AKASHI Takahiro]

# I still need to understand the issues reported here.

On Tue, Nov 26, 2019 at 11:57:34AM -0500, Tom Rini wrote:
> On Tue, Nov 26, 2019 at 09:15:08AM +0100, Marek Szyprowski wrote:
> 
> > Handling of the start file offset was broken in the current code. Although
> > the code skipped the needed clusters, it then tried to continue write with
> > current cluster set to EOF, what caused assertion. It also lacked adjusting
> > filesize in case of writing at the end of file and adjusting in-cluster
> > offset for partial overwrite.
> > 
> > This patch fixes all those issues.

If those issues are logically independent from each other,
it would be nice to split this patch into small ones.

I would like to expect you to add more test cases, especially
against corner cases that you mentioned above, to test/py/tests/est_fs
as I did in test_ext.py.
Or at least please add more assertion checks.

> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> >  fs/fat/fat_write.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> > index 6cfa5b4565..7fb373589d 100644
> > --- a/fs/fat/fat_write.c
> > +++ b/fs/fat/fat_write.c
> > @@ -756,14 +756,12 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
> >  	/* go to cluster at pos */
> >  	cur_pos = bytesperclust;
> >  	while (1) {
> > +		newclust = get_fatent(mydata, curclust);
> >  		if (pos <= cur_pos)

I think that we should change this condition as
        if (pos < cur_pos)
                break;
then modify the following code accordingly as well.

In this way, 'curclust' points to [cur_pos - bytesperclust, cur_pos)
and 'pos' is ensured to be in the middle after this 'while' unless
        (pos == cur_pos) && IS_LAST_CLUST(curclust,...).

Then the code will be expected to look better understandable.

Thanks,
-Takahiro Akashi


> >  			break;
> > -		if (IS_LAST_CLUST(curclust, mydata->fatsize))
> > +		if (IS_LAST_CLUST(newclust, mydata->fatsize))
> >  			break;
> > -
> > -		newclust = get_fatent(mydata, curclust);
> > -		if (!IS_LAST_CLUST(newclust, mydata->fatsize) &&
> > -		    CHECK_CLUST(newclust, mydata->fatsize)) {
> > +		if (CHECK_CLUST(newclust, mydata->fatsize)) {
> >  			debug("curclust: 0x%x\n", curclust);
> >  			debug("Invalid FAT entry\n");
> >  			return -1;
> > @@ -772,8 +770,8 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
> >  		cur_pos += bytesperclust;
> >  		curclust = newclust;
> >  	}
> > -	if (IS_LAST_CLUST(curclust, mydata->fatsize)) {
> > -		assert(pos == cur_pos);
> > +	if (pos == cur_pos && IS_LAST_CLUST(newclust, mydata->fatsize)) {
> > +		filesize -= pos;
> >  		goto set_clusters;
> >  	}
> >  
> > @@ -814,6 +812,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
> >  		else
> >  			offset = pos - cur_pos;
> >  		wsize = min_t(unsigned long long, actsize, filesize - cur_pos);
> > +		wsize -= offset;
> >  		if (get_set_cluster(mydata, curclust, offset,
> >  				    buffer, wsize, &actsize)) {
> >  			printf("Error get-and-setting cluster\n");
> 
> Adding in Heinrich and Akashi-san for more review on this, thanks!
> 
> -- 
> Tom

[U-Boot] [PATCH v3 1/6] fat: write: fix broken write to fragmented files

[Marek Szyprowski]

Hi,

On 27.11.2019 03:26, AKASHI Takahiro wrote:
> Thank you for the heads-up.
>
> On Tue, Nov 26, 2019 at 11:57:29AM -0500, Tom Rini wrote:
>> On Tue, Nov 26, 2019 at 09:15:07AM +0100, Marek Szyprowski wrote:
>>
>>> The code for handing file overwrite incorrectly assumed that the file on
>>> disk is always contiguous. This resulted in corrupting disk structure
>>> every time when write to existing fragmented file happened. Fix this
>>> by adding proper check for cluster discontinuity and adjust chunk size
>>> on each partial write.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
>>> Reviewed-by: Lukasz Majewski <lukma@denx.de>
>>> ---
>>>   fs/fat/fat_write.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
>>> index 729cf39630..6cfa5b4565 100644
>>> --- a/fs/fat/fat_write.c
>>> +++ b/fs/fat/fat_write.c
>>> @@ -794,6 +794,8 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>>>   
>>>   			newclust = get_fatent(mydata, endclust);
>>>   
>>> +			if ((newclust - 1) != endclust)
> "newclust != (endclust + 1)" would be more intuitive?
> But it's just my preference.

Indeed.

>>> +				break;
>>>   			if (IS_LAST_CLUST(newclust, mydata->fatsize))
>>>   				break;
>>>   			if (CHECK_CLUST(newclust, mydata->fatsize)) {
>>> @@ -811,7 +813,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>>>   			offset = 0;
>>>   		else
>>>   			offset = pos - cur_pos;
>>> -		wsize = min(cur_pos + actsize, filesize) - pos;
>>> +		wsize = min_t(unsigned long long, actsize, filesize - cur_pos);
> This hunk is not directly related to the issue, is it?

It is partially related. I remember that it was not calculated correctly 
for the fragmented files and then discovered that there was one more 
case in which the current formula failed.

>>>   		if (get_set_cluster(mydata, curclust, offset,
>>>   				    buffer, wsize, &actsize)) {
>>>   			printf("Error get-and-setting cluster\n");
>>> @@ -824,8 +826,6 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>>>   		if (filesize <= cur_pos)
>>>   			break;
>>>   
>>> -		/* CHECK: newclust = get_fatent(mydata, endclust); */
>>> -
>>>   		if (IS_LAST_CLUST(newclust, mydata->fatsize))
>>>   			/* no more clusters */
>>>   			break;
>> Adding in Heinrich and Akashi-san for more review on this, thanks!
> Otherwise, it looks good.
> Reviewed-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[U-Boot] [PATCH v3 2/6] fat: write: fix broken write at non-zero file offset

[Marek Szyprowski]

Hi

On 27.11.2019 04:13, AKASHI Takahiro wrote:
> # I still need to understand the issues reported here.
>
> On Tue, Nov 26, 2019 at 11:57:34AM -0500, Tom Rini wrote:
>> On Tue, Nov 26, 2019 at 09:15:08AM +0100, Marek Szyprowski wrote:
>>
>>> Handling of the start file offset was broken in the current code. Although
>>> the code skipped the needed clusters, it then tried to continue write with
>>> current cluster set to EOF, what caused assertion. It also lacked adjusting
>>> filesize in case of writing at the end of file and adjusting in-cluster
>>> offset for partial overwrite.
>>>
>>> This patch fixes all those issues.
> If those issues are logically independent from each other,
> it would be nice to split this patch into small ones.
>
> I would like to expect you to add more test cases, especially
> against corner cases that you mentioned above, to test/py/tests/est_fs
> as I did in test_ext.py.
> Or at least please add more assertion checks.

Okay, I will try to prepare some tests which show bugs fixed by this 
patch. I'm not sure I will manage to split this patch into patches 
fixing each single issue I've observed, because at least some of them 
were related.

I'm not familiar with py_test&co, but I will try to prepare some simple 
scripts for sandbox to reproduce the observed issues.

>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>>   fs/fat/fat_write.c | 13 ++++++-------
>>>   1 file changed, 6 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
>>> index 6cfa5b4565..7fb373589d 100644
>>> --- a/fs/fat/fat_write.c
>>> +++ b/fs/fat/fat_write.c
>>> @@ -756,14 +756,12 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>>>   	/* go to cluster at pos */
>>>   	cur_pos = bytesperclust;
>>>   	while (1) {
>>> +		newclust = get_fatent(mydata, curclust);
>>>   		if (pos <= cur_pos)
> I think that we should change this condition as
>          if (pos < cur_pos)
>                  break;
> then modify the following code accordingly as well.
>
> In this way, 'curclust' points to [cur_pos - bytesperclust, cur_pos)
> and 'pos' is ensured to be in the middle after this 'while' unless
>          (pos == cur_pos) && IS_LAST_CLUST(curclust,...).
>
> Then the code will be expected to look better understandable.
>
> Thanks,
> -Takahiro Akashi
>
>
>>>   			break;
>>> -		if (IS_LAST_CLUST(curclust, mydata->fatsize))
>>> +		if (IS_LAST_CLUST(newclust, mydata->fatsize))
>>>   			break;
>>> -
>>> -		newclust = get_fatent(mydata, curclust);
>>> -		if (!IS_LAST_CLUST(newclust, mydata->fatsize) &&
>>> -		    CHECK_CLUST(newclust, mydata->fatsize)) {
>>> +		if (CHECK_CLUST(newclust, mydata->fatsize)) {
>>>   			debug("curclust: 0x%x\n", curclust);
>>>   			debug("Invalid FAT entry\n");
>>>   			return -1;
>>> @@ -772,8 +770,8 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>>>   		cur_pos += bytesperclust;
>>>   		curclust = newclust;
>>>   	}
>>> -	if (IS_LAST_CLUST(curclust, mydata->fatsize)) {
>>> -		assert(pos == cur_pos);
>>> +	if (pos == cur_pos && IS_LAST_CLUST(newclust, mydata->fatsize)) {
>>> +		filesize -= pos;
>>>   		goto set_clusters;
>>>   	}
>>>   
>>> @@ -814,6 +812,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>>>   		else
>>>   			offset = pos - cur_pos;
>>>   		wsize = min_t(unsigned long long, actsize, filesize - cur_pos);
>>> +		wsize -= offset;
>>>   		if (get_set_cluster(mydata, curclust, offset,
>>>   				    buffer, wsize, &actsize)) {
>>>   			printf("Error get-and-setting cluster\n");
>> Adding in Heinrich and Akashi-san for more review on this, thanks!

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland