| Message ID | 84fe5ff5bb03467dbc0d6518d1f3cee7@kumkeo.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Allow flash updates to proceed without decompressed-size set | expand |
Hi Ulrich, On 02.11.23 14:15, Ulrich Teichert wrote: > Hi, > > sorry that I've opened up a GitHub pull request, should have read the contribution guidelines > to the very end... > This is not a problem, you should have received from Github an e-mail with the guidelines. But the patch is not correct. All this stuff, including "sorry that I've opened up ..." will be part of commit message if I applied. If you add some comment, this should be done as : Signed-off-by... ---- <any comment you want> diff... That means between "---" and first diff. You should then add a suitable commit message that explain the problem. The real issue is that there is a compatibility problem, and older SWU are not accepted. In fact, an ERROR should be raised. We need then that compatibility is guaranted. > My problem was that after an update to 2023.05 I could not use old images to downgrade > a running system. The reason for that is that the property "decompressed-size" was not > mandatory for software update images. Now, for performance reasons it *is* mandatory, > which may prevent downgrades with older images. To allow flash updates to proceed > without decompressed-size set, test for the property and if the property is unset, erase > the whole device before copying the image. > > Signed-off-by: Ulrich Teichert <ulrich.teichert@kumkeo.de> > --- > diff --git a/handlers/flash_handler.c b/handlers/flash_handler.c > index 3cca02b..a3d79b3 100644 > --- a/handlers/flash_handler.c > +++ b/handlers/flash_handler.c > @@ -310,13 +310,19 @@ static int flash_write_nor(int mtdnum, struct img_type *img) > > long long size = get_output_size(img, true); > if (size < 0) { > - ERROR("Failed to determine output size, bailing out."); > - return -1; > + INFO("decompression-size not set, erasing whole flash device %s\n", > + img->device); > + if (flash_erase(mtdnum)) { > + ERROR("Failed to erase %s", img->device); > + return -1; > + } The solution here is much worse than the problem. In fact, you are discarding if an offset (img->seek) is set, and an update will silently overwrite part of the flash, maybe bricking the device. You shouldn't erase the whole device, but instead the range offset:end of mtd. > } > - if (flash_erase_sector(mtdnum, img->seek, size)) { > - ERROR("Failed to erase sectors on /dev/mtd%d (start: %llu, size: %lld)", > - mtdnum, img->seek, size); > - return -1; > + else { > + if (flash_erase_sector(mtdnum, img->seek, size)) { > + ERROR("Failed to erase sectors on /dev/mtd%d (start: %llu, size: %lld)", > + mtdnum, img->seek, size); > + return -1; > + } > } > > Best regards, Stefano Babic
Hi Stefano, [del] >But the patch is not correct. All this stuff, including "sorry that I've >opened up ..." will be part of commit message if I applied. If you add >some comment, this should be done as : > >Signed-off-by... >---- > ><any comment you want> > >diff... > >That means between "---" and first diff. > >You should then add a suitable commit message that explain the problem. >The real issue is that there is a compatibility problem, and older SWU >are not accepted. In fact, an ERROR should be raised. We need then that >compatibility is guaranted. I'll shorten the comment for the next iteration, but do I understand you correctly that you would like to see something like: long long size = get_output_size(img, true); if (size < 0) { ERROR("decompression-size not set, erasing flash device %s from %d to %d\n", img->device, img->seek, img->end); ? [del] >> diff --git a/handlers/flash_handler.c b/handlers/flash_handler.c >> index 3cca02b..a3d79b3 100644 >> --- a/handlers/flash_handler.c >> +++ b/handlers/flash_handler.c >> @@ -310,13 +310,19 @@ static int flash_write_nor(int mtdnum, struct img_type *img) >> >> long long size = get_output_size(img, true); >> if (size < 0) { >> - ERROR("Failed to determine output size, bailing out."); >> - return -1; >> + INFO("decompression-size not set, erasing whole flash device %s\n", >> + img->device); >> + if (flash_erase(mtdnum)) { >> + ERROR("Failed to erase %s", img->device); >> + return -1; >> + } >The solution here is much worse than the problem. In fact, you are >discarding if an offset (img->seek) is set, and an update will silently >overwrite part of the flash, maybe bricking the device. Right - in my special case the offset is 0 anyway, so I have not noticed this problem. >You shouldn't erase the whole device, but instead the range offset:end >of mtd. OK, I can get the mtd size by flash->mtd_info[mtdnum].mtd as get_flash_info gets called at the start of the function. Will post a new version of the patch tomorrow, thanks for your review, Uli Mit freundlichen Grüßen / Kind regards Dipl.-Inform. Ulrich Teichert Senior Software Developer kumkeo GmbH Heidenkampsweg 82a 20097 Hamburg Germany T: +49 40 2846761-0 F: +49 40 2846761-99 ulrich.teichert@kumkeo.de www.kumkeo.de Amtsgericht Hamburg / Hamburg District Court, HRB 108558 Geschäftsführer / Managing Director: Dipl.-Ing. Bernd Sager; Dipl.-Ing. Sven Tanneberger, MBA
Hi Ulrich, On 02.11.23 16:21, Ulrich Teichert wrote: > Hi Stefano, > > [del] >> But the patch is not correct. All this stuff, including "sorry that I've >> opened up ..." will be part of commit message if I applied. If you add >> some comment, this should be done as : >> >> Signed-off-by... >> ---- >> >> <any comment you want> >> >> diff... >> >> That means between "---" and first diff. >> >> You should then add a suitable commit message that explain the problem. >> The real issue is that there is a compatibility problem, and older SWU >> are not accepted. In fact, an ERROR should be raised. We need then that >> compatibility is guaranted. > > I'll shorten the comment for the next iteration, but do I understand you > correctly that you would like to see something like: > > long long size = get_output_size(img, true); > if (size < 0) { > ERROR("decompression-size not set, erasing flash device %s from %d to %d\n", > img->device, img->seek, img->end); > > ? > No - EEROR means that update cannot go on, and thsi is what happens now, so change this to WARN. > [del] >>> diff --git a/handlers/flash_handler.c b/handlers/flash_handler.c >>> index 3cca02b..a3d79b3 100644 >>> --- a/handlers/flash_handler.c >>> +++ b/handlers/flash_handler.c >>> @@ -310,13 +310,19 @@ static int flash_write_nor(int mtdnum, struct img_type *img) >>> >>> long long size = get_output_size(img, true); >>> if (size < 0) { >>> - ERROR("Failed to determine output size, bailing out."); >>> - return -1; >>> + INFO("decompression-size not set, erasing whole flash device %s\n", >>> + img->device); >>> + if (flash_erase(mtdnum)) { >>> + ERROR("Failed to erase %s", img->device); >>> + return -1; >>> + } > >> The solution here is much worse than the problem. In fact, you are >> discarding if an offset (img->seek) is set, and an update will silently >> overwrite part of the flash, maybe bricking the device. > > Right - in my special case the offset is 0 anyway, so I have not noticed > this problem. > >> You shouldn't erase the whole device, but instead the range offset:end >> of mtd. > > OK, I can get the mtd size by flash->mtd_info[mtdnum].mtd as get_flash_info > gets called at the start of the function. Possible, but not desired. I tried tzo abstract the MTD layer in a different module (corelib/mtd_interface.c), and getting flash_info is just to check if MTD is available, not to get internal data. In fact, you do not see with exception of the check for mtd_dev_present(), that could also done better by hiding flash_info. Add a getter into mtd_interface as get_mtd_size(int mtdnum) and use it into the handler. > > Will post a new version of the patch tomorrow, > thanks for your review, > Uli > Best regards, Stefano
diff --git a/handlers/flash_handler.c b/handlers/flash_handler.c index 3cca02b..a3d79b3 100644 --- a/handlers/flash_handler.c +++ b/handlers/flash_handler.c @@ -310,13 +310,19 @@ static int flash_write_nor(int mtdnum, struct img_type *img) long long size = get_output_size(img, true); if (size < 0) { - ERROR("Failed to determine output size, bailing out."); - return -1; + INFO("decompression-size not set, erasing whole flash device %s\n", + img->device); + if (flash_erase(mtdnum)) { + ERROR("Failed to erase %s", img->device); + return -1; + } } - if (flash_erase_sector(mtdnum, img->seek, size)) { - ERROR("Failed to erase sectors on /dev/mtd%d (start: %llu, size: %lld)", - mtdnum, img->seek, size); - return -1; + else { + if (flash_erase_sector(mtdnum, img->seek, size)) { + ERROR("Failed to erase sectors on /dev/mtd%d (start: %llu, size: %lld)", + mtdnum, img->seek, size); + return -1; + } }
Hi, sorry that I've opened up a GitHub pull request, should have read the contribution guidelines to the very end... My problem was that after an update to 2023.05 I could not use old images to downgrade a running system. The reason for that is that the property "decompressed-size" was not mandatory for software update images. Now, for performance reasons it *is* mandatory, which may prevent downgrades with older images. To allow flash updates to proceed without decompressed-size set, test for the property and if the property is unset, erase the whole device before copying the image. Signed-off-by: Ulrich Teichert <ulrich.teichert@kumkeo.de> --- Mit freundlichen Grüßen / Kind regards Dipl.-Inform. Ulrich Teichert Senior Software Developer kumkeo GmbH Heidenkampsweg 82a 20097 Hamburg Germany T: +49 40 2846761-0 F: +49 40 2846761-99 ulrich.teichert@kumkeo.de www.kumkeo.de Amtsgericht Hamburg / Hamburg District Court, HRB 108558 Geschäftsführer / Managing Director: Dipl.-Ing. Bernd Sager; Dipl.-Ing. Sven Tanneberger, MBA