[U-Boot] [PATCH] dfu:mmc: When doing block operations, operate on the given length
When working on RAW partitions, it's possible that the whole area is larger than DDR. So what we need to do is make sure that the length we are given is aligned with the LBA block size, then pass that length in as our count of LBA blocks to operate on. In doing this, we no longer need to modify *len on read operations.
Cc: Lukasz Majewski l.majewski@samsung.com Signed-off-by: Tom Rini trini@ti.com --- drivers/dfu/dfu_mmc.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c index 083d745..0bed405 100644 --- a/drivers/dfu/dfu_mmc.c +++ b/drivers/dfu/dfu_mmc.c @@ -34,14 +34,21 @@ static int mmc_block_op(enum dfu_mmc_op op, struct dfu_entity *dfu, { char cmd_buf[DFU_CMD_BUF_SIZE];
- sprintf(cmd_buf, "mmc %s 0x%x %x %x", - op == DFU_OP_READ ? "read" : "write", - (unsigned int) buf, - dfu->data.mmc.lba_start, - dfu->data.mmc.lba_size); + /* + * We must ensure that we read in lba_blk_size chunks, so ALIGN + * this value. + */ + *len = ALIGN(*len, dfu->data.mmc.lba_blk_size); + + if (*len > (dfu->data.mmc.lba_size * dfu->data.mmc.lba_blk_size)) { + puts("Request would exceed designated area!\n"); + return -EINVAL; + }
- if (op == DFU_OP_READ) - *len = dfu->data.mmc.lba_blk_size * dfu->data.mmc.lba_size; + sprintf(cmd_buf, "mmc %s 0x%x %x %lx", + op == DFU_OP_READ ? "read" : "write", + (unsigned int) buf, dfu->data.mmc.lba_start, + *len / dfu->data.mmc.lba_blk_size);
debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf); return run_command(cmd_buf, 0);
I was just bitten in this area today but in a different way.
Larger than DDR? Any update larger than the default 4MB dfu_buf[] fails too. (As a hack I redefined dfu_buf[] to be a cast of I think CONFIG_SYS_SDRAM_BASE.) But I'd love to hear more about being able to handle updates larger than DDR.
But on the code below, (both before and after the patch) the amount written is the size of the mmc partition. Why write more data than was received from the host? Why isn't the incoming len value used?
Lastly, I encountered a zero dfu->data.mmc.lba_blk_size. This gets initialized in the mmc_init() path from the card meta data. But if you just do "dfu mmc 0" right after booting that won't have been done. The MMC controller is ready but the card structures have not been read.
I have a hack there to do do the a "mmc 0 rescan" command subordinate to the dfu command but that feels gross. There has to be a better way.
Thoughts? Have you not seen dfu->data.mmc.lba_blk_size be zero?
-Mike
On Mar 7, 2013, at 9:25 AM, Tom Rini trini@ti.com wrote:
When working on RAW partitions, it's possible that the whole area is larger than DDR. So what we need to do is make sure that the length we are given is aligned with the LBA block size, then pass that length in as our count of LBA blocks to operate on. In doing this, we no longer need to modify *len on read operations.
Cc: Lukasz Majewski l.majewski@samsung.com Signed-off-by: Tom Rini trini@ti.com
drivers/dfu/dfu_mmc.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c index 083d745..0bed405 100644 --- a/drivers/dfu/dfu_mmc.c +++ b/drivers/dfu/dfu_mmc.c @@ -34,14 +34,21 @@ static int mmc_block_op(enum dfu_mmc_op op, struct dfu_entity *dfu, { char cmd_buf[DFU_CMD_BUF_SIZE];
- sprintf(cmd_buf, "mmc %s 0x%x %x %x",
op == DFU_OP_READ ? "read" : "write",(unsigned int) buf,dfu->data.mmc.lba_start,dfu->data.mmc.lba_size);
- /*
* We must ensure that we read in lba_blk_size chunks, so ALIGN* this value.*/- *len = ALIGN(*len, dfu->data.mmc.lba_blk_size);
- if (*len > (dfu->data.mmc.lba_size * dfu->data.mmc.lba_blk_size)) {
puts("Request would exceed designated area!\n");return -EINVAL;- }
- if (op == DFU_OP_READ)
*len = dfu->data.mmc.lba_blk_size * dfu->data.mmc.lba_size;
sprintf(cmd_buf, "mmc %s 0x%x %x %lx",
op == DFU_OP_READ ? "read" : "write",(unsigned int) buf, dfu->data.mmc.lba_start,*len / dfu->data.mmc.lba_blk_size);debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf); return run_command(cmd_buf, 0);
-- 1.7.9.5
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 03/07/2013 06:19 PM, Michael Cashwell wrote:
I was just bitten in this area today but in a different way.
Larger than DDR? Any update larger than the default 4MB dfu_buf[] fails too. (As a hack I redefined dfu_buf[] to be a cast of I think CONFIG_SYS_SDRAM_BASE.) But I'd love to hear more about being able to handle updates larger than DDR.
Yes, as another problem, we can only write in chunks of DFU_DATA_BUF_SIZE. And for files, since we like the infrastructure to seek in existing files, filesystem writes need to be whole, and raw writes can be chunks.
I've taken a patch from Pantelis Antoniou that solved half of this problem and made it buffer filesystem writes and chunk raw writes. It seems to be working even but I just finished it and need to test it in a few more places before posting.
But on the code below, (both before and after the patch) the amount written is the size of the mmc partition. Why write more data than was received from the host? Why isn't the incoming len value used?
Er, that's not right. It was before but *len is the length we've been given. We must make it lba_blk_size aligned, but that's typically small (512 bytes), not the whole partition like it was before :)
Lastly, I encountered a zero dfu->data.mmc.lba_blk_size. This gets initialized in the mmc_init() path from the card meta data. But if you just do "dfu mmc 0" right after booting that won't have been done. The MMC controller is ready but the card structures have not been read.
What platform are you looking on? I'll go and re-produce this on mine tomorrow, but I'd have sworn I had done dfu mmc 0 prior to any mmc rescan/etc.
- -- Tom
On Mar 7, 2013, at 6:33 PM, Tom Rini trini@ti.com wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 03/07/2013 06:19 PM, Michael Cashwell wrote:
I was just bitten in this area today but in a different way.
Larger than DDR? Any update larger than the default 4MB dfu_buf[] fails too. (As a hack I redefined dfu_buf[] to be a cast of I think CONFIG_SYS_SDRAM_BASE.) But I'd love to hear more about being able to handle updates larger than DDR.
Yes, as another problem, we can only write in chunks of DFU_DATA_BUF_SIZE.
I guess what I'm not seeing is how it can chunk at all. Without being able to seek how (with dfu-util driving things) can it not just overwrite the start of the partition over and over?
I suspect I'm missing a bunch of patches. I'm just on denx mainline.
The only place DFU_DATA_BUF_SIZE is used is in the definition of dfu_buf[]. I just put it at the base of SDRAM. (I've moved u-boot to the high end for other reasons so that would work for me.)
But on the code below, (both before and after the patch) the amount written is the size of the mmc partition. Why write more data than was received from the host? Why isn't the incoming len value used?
Er, that's not right. It was before but *len is the length we've been given. We must make it lba_blk_size aligned, but that's typically small (512 bytes), not the whole partition like it was before :)
Whoops! I hadn't looked at your patch closely enough. I was still recovering from the shock of what the code did originally!
I went with a (*len + dfu->data.mmc.lba_blk_size - 1) / dfu->data.mmc.lba_blk_size approach rather than ALIGN but the result is the same.
I found the dfu->data.mmc.lba_blk_size being zero because the division threw a signal #8.
Lastly, I encountered a zero dfu->data.mmc.lba_blk_size. This gets initialized in the mmc_init() path from the card meta data. But if you just do "dfu mmc 0" right after booting that won't have been done. The MMC controller is ready but the card structures have not been read.
What platform are you looking on? I'll go and re-produce this on mine tomorrow, but I'd have sworn I had done dfu mmc 0 prior to any mmc rescan/etc.
Pandaboard ES1.1 (which is OMAP4460 with 1GB SDRAM) with a vastly different config.h. :-)
Please do let me know. If you can do "dfu mmc 0" as the first command and it works I'd love to know why your card geometry is being read but mine isn't.
(Hmmm... My environment is nowhere. Is your's (or something else) being read from the MMC card? That would do it. But dfu shouldn't rely on that.)
Thanks for the info and assistance!
-Mike
On Thu, Mar 07, 2013 at 09:25:44AM -0500, Tom Rini wrote:
When working on RAW partitions, it's possible that the whole area is larger than DDR. So what we need to do is make sure that the length we are given is aligned with the LBA block size, then pass that length in as our count of LBA blocks to operate on. In doing this, we no longer need to modify *len on read operations.
Cc: Lukasz Majewski l.majewski@samsung.com Signed-off-by: Tom Rini trini@ti.com
As I dig at and fix the other problem (which Michael points out, > 4MiB files crash), this patch is more for comment / confirmation that this change is fine on trats as well. These same concepts are part of my patch now that updates Pantelis' patch, without breaking file writes.
Hi Tom,
When working on RAW partitions, it's possible that the whole area is larger than DDR. So what we need to do is make sure that the length we are given is aligned with the LBA block size, then pass that length in as our count of LBA blocks to operate on. In doing this, we no longer need to modify *len on read operations.
Cc: Lukasz Majewski l.majewski@samsung.com Signed-off-by: Tom Rini trini@ti.com
drivers/dfu/dfu_mmc.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c index 083d745..0bed405 100644 --- a/drivers/dfu/dfu_mmc.c +++ b/drivers/dfu/dfu_mmc.c @@ -34,14 +34,21 @@ static int mmc_block_op(enum dfu_mmc_op op, struct dfu_entity *dfu, { char cmd_buf[DFU_CMD_BUF_SIZE];
- sprintf(cmd_buf, "mmc %s 0x%x %x %x",
op == DFU_OP_READ ? "read" : "write",(unsigned int) buf,dfu->data.mmc.lba_start,dfu->data.mmc.lba_size);
- /*
* We must ensure that we read in lba_blk_size chunks, soALIGN
* this value.*/- *len = ALIGN(*len, dfu->data.mmc.lba_blk_size);
- if (*len > (dfu->data.mmc.lba_size *
dfu->data.mmc.lba_blk_size)) {
puts("Request would exceed designated area!\n");return -EINVAL;- }
- if (op == DFU_OP_READ)
*len = dfu->data.mmc.lba_blk_size *dfu->data.mmc.lba_size;
sprintf(cmd_buf, "mmc %s 0x%x %x %lx",
op == DFU_OP_READ ? "read" : "write",(unsigned int) buf, dfu->data.mmc.lba_start,*len / dfu->data.mmc.lba_blk_size);debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf); return run_command(cmd_buf, 0);
Acked-by: Lukasz Majewski l.majewski@samsung.com
Tested-at: TRATS (Exynos4210) Tested-by: Lukasz Majewski l.majewski@samsung.com
participants (3)
-
Lukasz Majewski -
Michael Cashwell -
Tom Rini