Message ID | 4F44A3D4.30208@de.bosch.com |
---|---|
State | Changes Requested |
Headers | show |
On 22/02/2012 09:14, Dirk Behme wrote: > On 21.02.2012 22:49, stefano babic wrote: >> Am 21/02/2012 20:18, schrieb Dirk Behme: >> Hi Dirk, > What do you think about anything like below then [1]? > > I looked through the imximage.c code and, well, due to the mixture to > support the v1 and v2 header format, the execution path isn't the > cleanest one. So, while it doesn't seem to be the cleanest way to exit > directly in set_imx_hdr_v2, it seems to be the easiest and best place to > add this check. Some other functions have some exit() calls, too, so it > seems to be common practice in this code. It is common in all mkimage - when there is an error, it makes no sense to go on. You must also fix this issue for V1 in set_imx_hdr_v1() as well, because we do not want default value at all. I suggest also you do not check with if(imxhdr->flash_offset == 0), in case Freescale will put a SOC without an offset in the future. But it is easy to add a value that is not allowed. If we add something like FLASH_OFFSET_UNDEFINED = 0xFF or whatever you want that is not 32-bit aligned, we are on the safest side. > > If this is ok, I will send a v2 of the patch. Ok, thanks. Best regards, Stefano
On 22.02.2012 10:29, Stefano Babic wrote: > On 22/02/2012 09:14, Dirk Behme wrote: >> On 21.02.2012 22:49, stefano babic wrote: >>> Am 21/02/2012 20:18, schrieb Dirk Behme: >>> > > Hi Dirk, > >> What do you think about anything like below then [1]? >> >> I looked through the imximage.c code and, well, due to the mixture to >> support the v1 and v2 header format, the execution path isn't the >> cleanest one. So, while it doesn't seem to be the cleanest way to exit >> directly in set_imx_hdr_v2, it seems to be the easiest and best place to >> add this check. Some other functions have some exit() calls, too, so it >> seems to be common practice in this code. > > It is common in all mkimage - when there is an error, it makes no sense > to go on. > > You must also fix this issue for V1 in set_imx_hdr_v1() as well, because > we do not want default value at all. Ok, the V1 topic is new. I can't touch V1 because I don't know anything about it. And I don't have any hardware to test anything V1 related. Even though the V1 code might have a similar issue, it's my understanding that it doesn't hurt there as in V1 there are no flash_offsets != FLASH_OFFSET_STANDARD. Therefore in V1 the existing code works fine (?). Same as the V2 code before Freescale introduced flash offsets which are not FLASH_OFFSET_STANDARD (== 0x400). > I suggest also you do not check > with if(imxhdr->flash_offset == 0), in case Freescale will put a SOC > without an offset in the future. But it is easy to add a value that is > not allowed. If we add something like > > FLASH_OFFSET_UNDEFINED = 0xFF > > or whatever you want that is not 32-bit aligned, we are on the safest side. I will look where the correct location might be to add this. >> If this is ok, I will send a v2 of the patch. I will try to update the V2 header with something like FLASH_OFFSET_UNDEFINED as proposed above and then send a v2 of the patch. Best regards Dirk
On 22/02/2012 10:40, Dirk Behme wrote: >> You must also fix this issue for V1 in set_imx_hdr_v1() as well, because >> we do not want default value at all. > > Ok, the V1 topic is new. > > I can't touch V1 because I don't know anything about it. And I don't > have any hardware to test anything V1 related. > It is enough if you add the same check in set_imx_hdr_v1() you want to put in set_imx_hdr_v2(). > Even though the V1 code might have a similar issue, it's my > understanding that it doesn't hurt there as in V1 there are no > flash_offsets != FLASH_OFFSET_STANDARD. This is not correct. For the MX51, there is a different offset for the onenand device. But there is not a board booting from onenand in mainline. However, we want that BOOT_FROM is mandatory to avoid confusion. All V1 boards in mainline have BOOT_FROM in their imximage file, so it is enough to test if we can build the boards. > Therefore in V1 the existing > code works fine (?). Same as the V2 code before Freescale introduced > flash offsets which are not FLASH_OFFSET_STANDARD (== 0x400). It is the same with both versions - also with V1 there are different offset, but SD / NAND / SPI share the same offset. > >> I suggest also you do not check >> with if(imxhdr->flash_offset == 0), in case Freescale will put a SOC >> without an offset in the future. But it is easy to add a value that is >> not allowed. If we add something like >> >> FLASH_OFFSET_UNDEFINED = 0xFF >> >> or whatever you want that is not 32-bit aligned, we are on the safest >> side. > > I will look where the correct location might be to add this. Add it into imximage.h Best regards, Stefano
--- a/tools/imximage.c +++ b/tools/imximage.c @@ -253,8 +253,12 @@ static void set_imx_hdr_v2(struct imx_header *imxhdr, uint32_t dcd_len, imx_header_v2_t *hdr_v2 = &imxhdr->header.hdr_v2; flash_header_v2_t *fhdr_v2 = &hdr_v2->fhdr; - /* Set default offset */ - imxhdr->flash_offset = FLASH_OFFSET_STANDARD; + /* Exit if there is no BOOT_FROM field specifying the flash_offset */ + if(imxhdr->flash_offset == 0) { + fprintf(stderr, "Error: Header v2: No BOOT_FROM tag in %s\n", + params->imagename); + exit(EXIT_FAILURE); + } /* Set magic number */