On Tue, 04 Aug 2015 15:10:41 +0200 Bernhard Nortmann <[email protected]> wrote:
> Am 04.08.2015 10:33, schrieb Siarhei Siamashka: > > Hi, > > > > If anyone wonders, that's a sunxi-tools patch. To avoid confusion in > > the future, it's best to have this information in the subject line. > > Yes, sorry - I forgot to add the "sunxi-tools" prefix, and also didn't > CC you as the maintainer. > Thanks for clarifying this. I'm not a maintainer of sunxi-tools. I think that just the usual rules apply. Every patch author needs to find at least one reviewer and get an ack from him. > > Maybe it would be better to rework this code in order not assign a > > special magical meaning of the negative 'len' value? > > > > Either by passing the file name instead of the buffer pointer > > and buffer len pair to the 'aw_fel_write_uboot_image' function. > > Or by doing the check for missing u-boot part outside of the > > 'aw_fel_write_uboot_image'. > > > > In the former case, we can also support fel booting via extracting > > U-Boot from the SD card images (by checking the eGON signature both > > in the beginning of the file and at a certain offset). And because > > SD card images tend to be rather large (up to several gigabytes), > > reading the whole file to RAM is not a very good idea. > Agreed. My intention for this 'magic' was to keep the call in > aw_fel_process_spl_and_uboot() down to a very simple > aw_fel_write_uboot_image(usb, buf + SPL_LEN_LIMIT, size - SPL_LEN_LIMIT). > > But it's fair enough to simply test (size >= SPL_LEN_LIMIT) there, and to > skip aw_fel_write_uboot_image() entirely if the size is too small - > allowing us to stick with the more natural "size_t len" type. > Trying to limit aw_fel_write_uboot_image() to the actual handling of the > image (buffer) is a reasonable move, avoiding any 'magical' side effects. > > > If the user uses the 'uboot' command with improper file (without the > > u-boot part), then he/she should probably see a descriptive error > > message. > The current implementation tries to take care of that by checking for the > presence of U-Boot after processing the file (and setting the > uboot_autostart > flag accordingly). If that fails for the "uboot" command invocation, a > warning > will be issued (Warning: "uboot" command failed to detect image! > Can't execute U-Boot.), and there will be no attempt to execute U-Boot. OK. You are right. Passing this information via global variables got me confused. IMHO the code could have been a bit cleaner. But I guess, this is not a big problem as long as the fel tool source code is still so small. -- Best regards, Siarhei Siamashka -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
