[PATCH 1/3] dfu: use pointer to pass length to dfu_write()

[PATCH 1/3] dfu: use pointer to pass length to dfu_write()

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

This doesn't include any functional changes, but allows us to use the
size parameter to report the actual written bytes back to the caller.

This is useful in cases of unaligned writes to serial flash where the
first write covers only a part of the buffer (until the next sector
boundary) and the remaining bytes need to be written in a further call
to dfu_write().

While at it, we also adjust the data type of the 'write' and 'left'
variable in dfu_write_from_mem_addr() to match the values passed.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/dfu/dfu.c | 23 ++++++++++++-----------
 include/dfu.h     |  2 +-
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
index ff1859d946..5b2659ee7d 100644
--- a/drivers/dfu/dfu.c
+++ b/drivers/dfu/dfu.c
@@ -334,12 +334,12 @@ int dfu_flush(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
 	return ret;
 }
 
-int dfu_write(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
+int dfu_write(struct dfu_entity *dfu, void *buf, int *size, int blk_seq_num)
 {
 	int ret;
 
 	debug("%s: name: %s buf: 0x%p size: 0x%x p_num: 0x%x offset: 0x%llx bufoffset: 0x%lx\n",
-	      __func__, dfu->name, buf, size, blk_seq_num, dfu->offset,
+	      __func__, dfu->name, buf, *size, blk_seq_num, dfu->offset,
 	      (unsigned long)(dfu->i_buf - dfu->i_buf_start));
 
 	ret = dfu_transaction_initiate(dfu, false);
@@ -371,7 +371,7 @@ int dfu_write(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
 	dfu->i_blk_seq_num = (dfu->i_blk_seq_num + 1) & 0xffff;
 
 	/* flush buffer if overflow */
-	if ((dfu->i_buf + size) > dfu->i_buf_end) {
+	if ((dfu->i_buf + *size) > dfu->i_buf_end) {
 		ret = dfu_write_buffer_drain(dfu);
 		if (ret) {
 			dfu_transaction_cleanup(dfu);
@@ -381,19 +381,19 @@ int dfu_write(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
 	}
 
 	/* we should be in buffer now (if not then size too large) */
-	if ((dfu->i_buf + size) > dfu->i_buf_end) {
+	if ((dfu->i_buf + *size) > dfu->i_buf_end) {
 		pr_err("Buffer overflow! (0x%p + 0x%x > 0x%p)\n", dfu->i_buf,
-		      size, dfu->i_buf_end);
+		      *size, dfu->i_buf_end);
 		dfu_transaction_cleanup(dfu);
 		dfu_error_callback(dfu, "Buffer overflow");
 		return -1;
 	}
 
-	memcpy(dfu->i_buf, buf, size);
-	dfu->i_buf += size;
+	memcpy(dfu->i_buf, buf, *size);
+	dfu->i_buf += *size;
 
 	/* if end or if buffer full flush */
-	if (size == 0 || (dfu->i_buf + size) > dfu->i_buf_end) {
+	if (*size == 0 || (dfu->i_buf + *size) > dfu->i_buf_end) {
 		ret = dfu_write_buffer_drain(dfu);
 		if (ret) {
 			dfu_transaction_cleanup(dfu);
@@ -704,7 +704,8 @@ int dfu_get_alt(char *name)
 
 int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int size)
 {
-	unsigned long dfu_buf_size, write, left = size;
+	unsigned int write, left = size;
+	unsigned long dfu_buf_size;
 	int i, ret = 0;
 	void *dp = buf;
 
@@ -720,9 +721,9 @@ int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int size)
 	for (i = 0; left > 0; i++) {
 		write = min(dfu_buf_size, left);
 
-		debug("%s: dp: 0x%p left: %lu write: %lu\n", __func__,
+		debug("%s: dp: 0x%p left: %lu write: %u\n", __func__,
 		      dp, left, write);
-		ret = dfu_write(dfu, dp, write, i);
+		ret = dfu_write(dfu, dp, &write, i);
 		if (ret) {
 			pr_err("DFU write failed\n");
 			return ret;
diff --git a/include/dfu.h b/include/dfu.h
index f6868982df..2df55e507e 100644
--- a/include/dfu.h
+++ b/include/dfu.h
@@ -335,7 +335,7 @@ int dfu_read(struct dfu_entity *de, void *buf, int size, int blk_seq_num);
  * @blk_seq_num:	block sequence number
  * Return:		0 for success, -1 for error
  */
-int dfu_write(struct dfu_entity *de, void *buf, int size, int blk_seq_num);
+int dfu_write(struct dfu_entity *de, void *buf, int *size, int blk_seq_num);
 
 /**
  * dfu_flush() - flush to dfu entity
-- 
2.33.0

[PATCH 2/3] dfu: dfu_fs: Simplify find_sector()

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

This doesn't include any functional changes, but simplifies the
existing code a bit and prepares for the following patch that
implements unaligned writes.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/dfu/dfu_sf.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c
index 7e64ab772f..c7ed674092 100644
--- a/drivers/dfu/dfu_sf.c
+++ b/drivers/dfu/dfu_sf.c
@@ -28,25 +28,24 @@ static int dfu_read_medium_sf(struct dfu_entity *dfu, u64 offset, void *buf,
 		*len, buf);
 }
 
-static u64 find_sector(struct dfu_entity *dfu, u64 start, u64 offset)
+static u64 find_sector(struct dfu_entity *dfu, u64 offset)
 {
-	return (lldiv((start + offset), dfu->data.sf.dev->sector_size)) *
+	return (lldiv(offset, dfu->data.sf.dev->sector_size)) *
 		dfu->data.sf.dev->sector_size;
 }
 
 static int dfu_write_medium_sf(struct dfu_entity *dfu,
 			       u64 offset, void *buf, long *len)
 {
+	u64 start = dfu->data.sf.start + offset;
 	int ret;
 
-	ret = spi_flash_erase(dfu->data.sf.dev,
-			      find_sector(dfu, dfu->data.sf.start, offset),
+	ret = spi_flash_erase(dfu->data.sf.dev, find_sector(dfu, start),
 			      dfu->data.sf.dev->sector_size);
 	if (ret)
 		return ret;
 
-	ret = spi_flash_write(dfu->data.sf.dev, dfu->data.sf.start + offset,
-			      *len, buf);
+	ret = spi_flash_write(dfu->data.sf.dev, start, *len, buf);
 	if (ret)
 		return ret;
 
@@ -61,7 +60,7 @@ static int dfu_flush_medium_sf(struct dfu_entity *dfu)
 		return 0;
 
 	/* in case of ubi partition, erase rest of the partition */
-	off = find_sector(dfu, dfu->data.sf.start, dfu->offset);
+	off = find_sector(dfu, dfu->data.sf.start + dfu->offset);
 	/* last write ended with unaligned length jump to next */
 	if (off != dfu->data.sf.start + dfu->offset)
 		off += dfu->data.sf.dev->sector_size;
-- 
2.33.0

[PATCH 3/3] dfu: dfu_sf: Support unaligned offsets for writing to serial flash

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

Currently using an offset that is not aligned to the erase block size
of the flash results in incorrect data on the device. To support this
use case, we prevent the first cycle to write beyond the erase block
border and return the actual bytes written to the calling functions.
That way the subsequent write cycles will be aligned and the correct
position in the source buffer can be tracked.

This was tested on a SPI NOR with 4k erase blocks and with:
dfu_alt_info=sf 0:0=flash-bin raw 0x400 0x1f0000

Please note, that this implementation doesn't preserve the data in the
first erase block (before the start offset), but neither does the
previous implementation do that for the last erase block when the end
of the data is not aligned. So this seems like an acceptable limitation.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/dfu/dfu.c    |  9 ++++++++-
 drivers/dfu/dfu_sf.c | 12 ++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
index 5b2659ee7d..c43db77823 100644
--- a/drivers/dfu/dfu.c
+++ b/drivers/dfu/dfu.c
@@ -242,11 +242,12 @@ static char *dfu_get_hash_algo(void)
 
 static int dfu_write_buffer_drain(struct dfu_entity *dfu)
 {
-	long w_size;
+	long w_size, w_size_req;
 	int ret;
 
 	/* flush size? */
 	w_size = dfu->i_buf - dfu->i_buf_start;
+	w_size_req = w_size;
 	if (w_size == 0)
 		return 0;
 
@@ -264,6 +265,9 @@ static int dfu_write_buffer_drain(struct dfu_entity *dfu)
 	/* update offset */
 	dfu->offset += w_size;
 
+	/* store remaining bytes */
+	dfu->b_left = w_size_req - w_size;
+
 	puts("#");
 
 	return ret;
@@ -402,6 +406,9 @@ int dfu_write(struct dfu_entity *dfu, void *buf, int *size, int blk_seq_num)
 		}
 	}
 
+	/* if not all bytes have been written, adjust size */
+	*size -= dfu->b_left;
+
 	return 0;
 }
 
diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c
index c7ed674092..814df6ef88 100644
--- a/drivers/dfu/dfu_sf.c
+++ b/drivers/dfu/dfu_sf.c
@@ -38,13 +38,25 @@ static int dfu_write_medium_sf(struct dfu_entity *dfu,
 			       u64 offset, void *buf, long *len)
 {
 	u64 start = dfu->data.sf.start + offset;
+	u32 sector_offset;
+	u64 tmp = start;
 	int ret;
 
+	/* Calculate the offset into the sector for unaligned writes */
+	sector_offset = do_div(tmp, dfu->data.sf.dev->sector_size);
+
 	ret = spi_flash_erase(dfu->data.sf.dev, find_sector(dfu, start),
 			      dfu->data.sf.dev->sector_size);
 	if (ret)
 		return ret;
 
+	/*
+	 * In case of an unaligned start address, only write until the
+	 * next sector boundary
+	 */
+	if (sector_offset)
+		*len = dfu->data.sf.dev->sector_size - sector_offset;
+
 	ret = spi_flash_write(dfu->data.sf.dev, start, *len, buf);
 	if (ret)
 		return ret;
-- 
2.33.0

Re: [PATCH 1/3] dfu: use pointer to pass length to dfu_write()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3210 bytes --]

On Thu, Sep 30, 2021 at 06:22:06PM +0200, Frieder Schrempf wrote:

> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> This doesn't include any functional changes, but allows us to use the
> size parameter to report the actual written bytes back to the caller.
> 
> This is useful in cases of unaligned writes to serial flash where the
> first write covers only a part of the buffer (until the next sector
> boundary) and the remaining bytes need to be written in a further call
> to dfu_write().
> 
> While at it, we also adjust the data type of the 'write' and 'left'
> variable in dfu_write_from_mem_addr() to match the values passed.
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>

This causes:

   aarch64:  +   qemu_arm64
+(qemu_arm64) In file included from include/linux/bitops.h:22,
+(qemu_arm64)                  from include/log.h:15,
+(qemu_arm64)                  from include/linux/printk.h:4,
+(qemu_arm64)                  from include/common.h:20,
+(qemu_arm64)                  from drivers/dfu/dfu.c:9:
+(qemu_arm64) drivers/dfu/dfu.c: In function 'dfu_write_from_mem_addr':
+(qemu_arm64) include/linux/kernel.h:184:24: error: comparison of distinct pointer types lacks a cast [-Werror]
+(qemu_arm64)   184 |         (void) (&_min1 == &_min2);              \
+(qemu_arm64)       |                        ^~
+(qemu_arm64) drivers/dfu/dfu.c:748:25: note: in expansion of macro 'min'
+(qemu_arm64)   748 |                 write = min(dfu_buf_size, left);
+(qemu_arm64)       |                         ^~~
+(qemu_arm64) In file included from include/linux/printk.h:4,
+(qemu_arm64) drivers/dfu/dfu.c:750:23: error: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'unsigned int' [-Werror=format=]
+(qemu_arm64)   750 |                 debug("%s: dp: 0x%p left: %lu write: %u\n", __func__,
+(qemu_arm64)       |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+(qemu_arm64) include/log.h:163:21: note: in definition of macro 'pr_fmt'
+(qemu_arm64)   163 | #define pr_fmt(fmt) fmt
+(qemu_arm64)       |                     ^~~
+(qemu_arm64) include/log.h:283:9: note: in expansion of macro 'debug_cond'
+(qemu_arm64)   283 |         debug_cond(_DEBUG, fmt, ##args)
+(qemu_arm64)       |         ^~~~~~~~~~
+(qemu_arm64) drivers/dfu/dfu.c:750:17: note: in expansion of macro 'debug'
+(qemu_arm64)       |                 ^~~~~
+(qemu_arm64) drivers/dfu/dfu.c:750:45: note: format string is defined here
+(qemu_arm64)       |                                           ~~^
+(qemu_arm64)       |                                             |
+(qemu_arm64)       |                                             long unsigned int
+(qemu_arm64)       |                                           %u
+(qemu_arm64) cc1: all warnings being treated as errors
+(qemu_arm64) make[3]: *** [scripts/Makefile.build:253: drivers/dfu/dfu.o] Error 1
+(qemu_arm64) make[2]: *** [scripts/Makefile.build:394: drivers/dfu] Error 2
+(qemu_arm64) make[1]: *** [Makefile:1889: drivers] Error 2
+(qemu_arm64) make: *** [Makefile:177: sub-make] Error 2

Please rework and repost, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]