Ticket #1843 (closed defect: fixed)

Opened 11 years ago

Last modified 10 years ago

u-boot's DFU upload garbles data at block boundary

Reported by: wiml Owned by: laforge
Priority: normal Milestone:
Component: kernel Version: GTA02v6
Severity: major Keywords: dfu
Cc: Blocked By:
Blocking: #676 Estimated Completion (week):
HasPatchForReview: yes PatchReviewResult:


I was trying to verify my downloaded kernel by re-uploading it again to compare. The uploaded version differed in an interesting way: at offset 0x01f000, exactly 0x20000 bytes were missing (so that the uploaded file contained at 0x01f000 the data that was at 0x03f000 in the original file). This does not seem to be a dfu-util bug, but a u-boot bug.

Looking at the u-boot source code, I think the problem is in handle_upload() in usbdfu.c, where it handles the wraparound at the end of the NAND block. There are three bugs here:

  • when it copies a new blockful of data, it copies it into the same buffer that urb->buffer was already pointing to, thus overwriting the beginning of the last buffer before it is sent back to the requestor
  • it then calls memcpy() to copy the beginning of the newly-read block to after the end of the buffer that urb->buffer is pointing to. If ds->nand->erasesize is the same as the _buf[] array, then this will write past the end of the _buf[] array and smash some other item in RAM.
  • if a requested buffer exactly reaches the end of the block that's been buffered in ds->buf, then handle_upload() will read a new NAND block into the buffer even though it is not needed. (The test for (len > remain) should probably go before the test for ds->ptr, not after?)

I would submit a patch, but I don't have a build environment set up for uboot (or any jtag hardware for recovery from bad flashing, for that matter) --- this is just from reading the uboot-dfu.patch file in svn.

I also wonder if this is related to ticket #676, although the comments in that bug indicate corruption starting at 0x3001, not 0x1f000, so it may be unrelated.


uboot-dfu-upload.patch (2.5 KB) - added by laforge 11 years ago.
proposed patch (tested, works for me, needs more testing)

Change History

comment:1 Changed 11 years ago by roh

  • Keywords dfu added
  • Owner changed from openmoko-devel to openmoko-kernel
  • Component changed from unknown to System Software

comment:2 Changed 11 years ago by laforge

  • Owner changed from openmoko-kernel to laforge
  • Status changed from new to assigned

comment:3 Changed 11 years ago by laforge

  • Blocking 676 added

thanks for the detailed analyisis, it really describes well why many people have been experiencing various upload related problems (and it was never working, at least not on GTA02 where the erazeblock size is much bigger than on GTA01).

I'm right now working on altering the code. It's not as straight-forward as you may think. We are under tight constraints by the DFU spec, i.e. whatever amount of bytes the host asks us for, we need to deliver it. If we'd return less than what was asked for, this would implicitly mean
the end of a transmission. I also do not want to introduce a second buffer (for the URB), since
that would mean we always need to memcpy() and it would further increase the memory requirements
of u-boot.

The current code structure is a result from trying to implement UPLOAD based on the infrastructure that DOWNLOAD provided. For download it makes sense to think in terms of nand
erase blocks. For upload it actually makes much more sense to think in terms of whatever amount
of bytes the host requests.

I'll try to rework the logic accordingly.

Changed 11 years ago by laforge

proposed patch (tested, works for me, needs more testing)

comment:4 Changed 10 years ago by john_lee

  • HasPatchForReview set

comment:5 Changed 10 years ago by PaulFertser

  • Status changed from assigned to closed
  • Resolution set to fixed

Looks like it was fixed since nobody who was using recent u-boot complained. Please reopen if still reproducible.

Note: See TracTickets for help on using tickets.