* [U-Boot] [PATCH] dfu:mmc: When doing block operations, operate on the given length
@ 2013-03-07 14:25 Tom Rini
2013-03-07 23:19 ` Michael Cashwell
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Tom Rini @ 2013-03-07 14:25 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [U-Boot] [PATCH] dfu:mmc: When doing block operations, operate on the given length
2013-03-07 14:25 [U-Boot] [PATCH] dfu:mmc: When doing block operations, operate on the given length Tom Rini
@ 2013-03-07 23:19 ` Michael Cashwell
2013-03-07 23:33 ` Tom Rini
2013-03-08 15:06 ` Tom Rini
2013-03-08 15:41 ` Lukasz Majewski
2 siblings, 1 reply; 6+ messages in thread
From: Michael Cashwell @ 2013-03-07 23:19 UTC (permalink / raw)
To: u-boot
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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
^ permalink raw reply [flat|nested] 6+ messages in thread* [U-Boot] [PATCH] dfu:mmc: When doing block operations, operate on the given length
2013-03-07 23:19 ` Michael Cashwell
@ 2013-03-07 23:33 ` Tom Rini
2013-03-08 0:14 ` Michael Cashwell
0 siblings, 1 reply; 6+ messages in thread
From: Tom Rini @ 2013-03-07 23:33 UTC (permalink / raw)
To: 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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJROSO1AAoJENk4IS6UOR1Wg5EP/RBY4QFlNrj47S2KLtQFA1sl
5EEN25yC0LGFPwZ118GwXLinRzgE89ALbMeXq3dPdljlyIFLD4LFjM/7mmpB80sI
GR5xcml89cFnDq4+7lhrFeqNW3jqg68soQSvVjaxdYe9sPvkDLzuNwQ77WeJFtal
7fdNOvl1ZrSHoeDQuCjqFYHuDRz+4oso5fKZcDPdhKL4mrqWhmRfrZ7RJX2iKsuC
aySnkIfh/I4dtANLvQTZta3Nqidrb4PX8kE11XNrKcTKu4yLxq/Q+sHOWlXMbqfy
oW3O0zxQD3cVedPO8T2G14gQwonG61R+XCyBlrxJqtL+ZPlzKZxJWNxiP1Sa5HUM
Axz0vDjwfB84jsK+dzJspR0UTZHZdraoWnCYOnXF2bGxxCrCAPUSbS0A1BCpZfuT
A5ayIPUQRnLgITrBip+DsYn5sHAXxRFeE6OHP3mR2PFW87ioT9iK1xFU8jVchybU
73bckJZorarUBEhSDBgaC1DScS5gF/8nPqfFiRZX/ur70opq946hMWNZNzN5kOYs
haU1r88k98jm6ktW2uFvQVxzI5LiXdrPpbCYCf1vF8+VLmTGLAezLTk4Oce+8Q4t
/A+wPKu6Xm03O87uW10kqmitAhkmbm9deFRt78oBA3ChqqR0EopuYH6FQhyboyvH
ZMjtUebCZUAJRlgb0L4F
=WWas
-----END PGP SIGNATURE-----
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] dfu:mmc: When doing block operations, operate on the given length
2013-03-07 23:33 ` Tom Rini
@ 2013-03-08 0:14 ` Michael Cashwell
0 siblings, 0 replies; 6+ messages in thread
From: Michael Cashwell @ 2013-03-08 0:14 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] dfu:mmc: When doing block operations, operate on the given length
2013-03-07 14:25 [U-Boot] [PATCH] dfu:mmc: When doing block operations, operate on the given length Tom Rini
2013-03-07 23:19 ` Michael Cashwell
@ 2013-03-08 15:06 ` Tom Rini
2013-03-08 15:41 ` Lukasz Majewski
2 siblings, 0 replies; 6+ messages in thread
From: Tom Rini @ 2013-03-08 15:06 UTC (permalink / raw)
To: u-boot
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.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130308/153274a5/attachment.pgp>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] dfu:mmc: When doing block operations, operate on the given length
2013-03-07 14:25 [U-Boot] [PATCH] dfu:mmc: When doing block operations, operate on the given length Tom Rini
2013-03-07 23:19 ` Michael Cashwell
2013-03-08 15:06 ` Tom Rini
@ 2013-03-08 15:41 ` Lukasz Majewski
2 siblings, 0 replies; 6+ messages in thread
From: Lukasz Majewski @ 2013-03-08 15:41 UTC (permalink / raw)
To: u-boot
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, 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);
Acked-by: Lukasz Majewski <l.majewski@samsung.com>
Tested-at: TRATS (Exynos4210)
Tested-by: Lukasz Majewski <l.majewski@samsung.com>
--
Best regards,
Lukasz Majewski
Samsung R&D Poland (SRPOL) | Linux Platform Group
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-03-08 15:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-07 14:25 [U-Boot] [PATCH] dfu:mmc: When doing block operations, operate on the given length Tom Rini
2013-03-07 23:19 ` Michael Cashwell
2013-03-07 23:33 ` Tom Rini
2013-03-08 0:14 ` Michael Cashwell
2013-03-08 15:06 ` Tom Rini
2013-03-08 15:41 ` Lukasz Majewski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.