| Message ID | 1362666344-21856-1-git-send-email-trini@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
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 -----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-----
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, 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>
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);
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(-)