| Message ID | 20200824190813.14943-1-bkylerussell@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | core: support saving boot transaction marker on success | expand |
Hi Kyle, On 24.08.20 21:08, Kyle Russell wrote: > Similar to the method through which suricatta allows configuring its > behavior with the bootloader, allow the boot transaction marker's > behavior to be configured as an aid to double-copy implementations. > > If the boot transaction marker is configured to be saved in the bootloader > environment after a successful A/B update, the bootloader then only needs > to check for the presence of this marker on the next boot, and remove it > once its has selected the correct boot partition. > > Persistence of the transaction marker allows the bootloader's partition > selection implementation to exist independent of the sw-description in a > particular .swu file. It allows the bootloader to distinguish from a > standard power-on reset where no update was performed (the last booted > partition should be booted again after reset), versus a reset following > an update (where the boot partition should be switched). > --- I agree that the usage of the variables to communicate with the bootloader what is going on is not enough clear. This is mainly because the logic was demanded to the bootloader, and this logic is not part of the project. This leads to a bunch of different implementations, most of them corect, some wrong. At least documentation must be improved. There are two variables that SWUpdate sets to communicate its state with the bootloader. One is BOOTVAR_TRANSACTION, better known as "recovery_status". This is just a transaction flag (boolean) with two combinatiosn, set or unset. If a bootloader finds this variable, it knows that an update ran before, but it was not completed (power-cut, panic, segmentation fault, watchdog,...). It is mostly used on single-copy approach and the bootloader knows that there is no valid "production" software. Then there is a second variable (SURICATTA_STATE_BOOTLOADER), introduced with suricatta, and this contains the state of the install process. So we have: recovery_status ==> transaction flag ustate ==> state of update (None, INSTALLED, ...) ustate is the variable to be used, not BOOTVAR_TRANSACTION. However, as you noted, this is used just by suricatta. But it does not work if suricatta is not running as root, and this leads to some work-arounf in that case. One of the open issue is to move away the logic from suricatta (setting a state variable should be centralized and done by the installer), and add a sort of IPC in case a co-worker (suricatta, webserver, ...) needs to update the status. Your patch breaks setup in single-copy where the bootloader just checks for the existence of the transaction flag. > bootloader/Config.in | 14 ++++++++++++++ > core/stream_interface.c | 4 ++++ > core/swupdate.c | 4 ++++ > include/state.h | 1 + > 4 files changed, 23 insertions(+) > > diff --git a/bootloader/Config.in b/bootloader/Config.in > index d27f66f..b976bd4 100644 > --- a/bootloader/Config.in > +++ b/bootloader/Config.in > @@ -64,3 +64,17 @@ config GRUBENV_PATH > default "/boot/efi/EFI/BOOT/grub/grubenv" > help > Provide path to GRUB environment block file > + > +config SAVE_BOOTVAR_TRANSACTION > + bool "Save bootloader transaction marker on success" > + depends on !BOOTLOADER_NONE > + default n > + help > + Don't reset the bootloader transaction marker > + following a successful programming. It is up to the > + bootloader to clear this environment variable on reboot. > + > + This option is useful for devices that implement a > + double-copy strategy, and can signal to the bootloader that > + a successful programming has occurred and that the bootloader > + should switch to the alternate partition. > diff --git a/core/stream_interface.c b/core/stream_interface.c > index 99e5c62..0f80c24 100644 > --- a/core/stream_interface.c > +++ b/core/stream_interface.c > @@ -563,7 +563,11 @@ void *network_initializer(void *data) > * that it is not required to start recovery again > */ > if (software->bootloader_transaction_marker) { > +#ifdef CONFIG_SAVE_BOOTVAR_TRANSACTION > + save_state_string((char*)BOOTVAR_TRANSACTION, STATE_INSTALLED); > +#else > reset_state((char*)BOOTVAR_TRANSACTION); > +#endif > } > notify(SUCCESS, RECOVERY_NO_ERROR, INFOLEVEL, "SWUPDATE successful !"); > inst.last_install = SUCCESS; > diff --git a/core/swupdate.c b/core/swupdate.c > index cc7e2d3..685169a 100644 > --- a/core/swupdate.c > +++ b/core/swupdate.c > @@ -396,7 +396,11 @@ static int install_from_file(char *fname, int check) > } > > if (swcfg.bootloader_transaction_marker) { > +#ifdef CONFIG_SAVE_BOOTVAR_TRANSACTION > + save_state_string((char*)BOOTVAR_TRANSACTION, STATE_INSTALLED); > +#else > reset_state((char*)BOOTVAR_TRANSACTION); > +#endif > } > fprintf(stdout, "Software updated successfully\n"); > fprintf(stdout, "Please reboot the device to start the new software\n"); > diff --git a/include/state.h b/include/state.h > index 7294dbe..9ebb9e4 100644 > --- a/include/state.h > +++ b/include/state.h > @@ -52,6 +52,7 @@ static inline char* get_state_string(update_state_t state) { > switch (state) { > case STATE_IN_PROGRESS: return (char*)"in_progress"; > case STATE_FAILED: return (char*)"failed"; > + case STATE_INSTALLED: return (char*)"installed"; > default: break; > } > return (char*)state; > Best regards, Stefano Babic
On Tuesday, August 25, 2020 at 4:50:25 AM UTC-4 Stefano Babic wrote: > ustate is the variable to be used, not BOOTVAR_TRANSACTION. However, as > you noted, this is used just by suricatta. But it does not work if > suricatta is not running as root, and this leads to some work-arounf in > that case. One of the open issue is to move away the logic from > suricatta (setting a state variable should be centralized and done by > the installer), and add a sort of IPC in case a co-worker (suricatta, > webserver, ...) needs to update the status. > > Your patch breaks setup in single-copy where the bootloader just checks > for the existence of the transaction flag. > Ok, I think I understand. I thought a configure option might be good since it wouldn't be enabled by default in single-copy implementations. Indeed, in single-copy, the bootloader is likely only interested in a failure since it simply needs to jump back into the recovery mode. So I anticipated this configure option only being enabled in the double-copy case, where the implementation should be prepared to handle the flag if enabled. Is the issue you mentioned tracked anywhere? I would be happy to work on moving this functionality to core to take advantage of ustate without suricatta. I would love to hear any ideas you have on how that should work, or if there are other issues that need to be resolved before that can happen.
Stefano, After having picked up your commits to refactor the ustate variable into common code, I am seeing some undesirable behaviour. I wanted to point out the behaviour, in case you have a fix already, and/or possibly get some guidance on the best way to fix it. In our sw-description file we are using *install-if-higher=true* for all of our images. Then, after receiving the same swu file a second time, the images are (correctly) "Not required: skipping". The undesirable part is that after skipping every image and not making any changes to the flash, swupdate completes successfully and unconditionally marks the ustate env variable to "installed". This triggers our bootloader to switch between our A/B partitions to code that is incorrect/outdated. I believe the fix should be something like checking whether any (at least one) images were required (not skipped) before updating the ustate env variable. After glancing through the code, I couldn't really see the best way to accomplish this, but somewhere your code does know the difference between images being written or skipped to decide whether to reboot or not. When images are written swupdate knows to reboot. When images are skipped swupdate knows NOT to reboot. Is there an easy way to use this same rebooting state to decide when to update the ustate env variable? Any guidance is appreciated, Wes L On Tuesday, August 25, 2020 at 10:46:20 PM UTC-4 Kyle Russell wrote: > On Tuesday, August 25, 2020 at 4:50:25 AM UTC-4 Stefano Babic wrote: > >> ustate is the variable to be used, not BOOTVAR_TRANSACTION. However, as >> you noted, this is used just by suricatta. But it does not work if >> suricatta is not running as root, and this leads to some work-arounf in >> that case. One of the open issue is to move away the logic from >> suricatta (setting a state variable should be centralized and done by >> the installer), and add a sort of IPC in case a co-worker (suricatta, >> webserver, ...) needs to update the status. >> >> Your patch breaks setup in single-copy where the bootloader just checks >> for the existence of the transaction flag. >> > > Ok, I think I understand. I thought a configure option might be good > since it wouldn't be enabled by default in single-copy implementations. > Indeed, in single-copy, the bootloader is likely only interested in a > failure since it simply needs to jump back into the recovery mode. So I > anticipated this configure option only being enabled in the double-copy > case, where the implementation should be prepared to handle the flag if > enabled. > > Is the issue you mentioned tracked anywhere? I would be happy to work on > moving this functionality to core to take advantage of ustate without > suricatta. I would love to hear any ideas you have on how that should > work, or if there are other issues that need to be resolved before that can > happen. >
Hi Wes, On 05.01.21 20:38, Wes Lindauer wrote: > Stefano, > > After having picked up your commits to refactor the ustate variable into > common code, I am seeing some undesirable behaviour. I wanted to point > out the behaviour, in case you have a fix already, and/or possibly get > some guidance on the best way to fix it. In our sw-description file we > are using *install-if-higher=true* for all of our images. Then, after > receiving the same swu file a second time, the images are (correctly) > "Not required: skipping". The undesirable part is that after skipping > every image and not making any changes to the flash, swupdate completes > successfully and unconditionally marks the ustate env variable to > "installed". This triggers our bootloader to switch between our A/B > partitions to code that is incorrect/outdated. Why ? The toggle should be done by SWUpdate itself, and if nothing has been installed, the toggle is also not done. Which bootloader are you using and how is the toggle implemented ? > I believe the fix should > be something like I need first to understand if this should be done. There was already a quite "philosophical" discussion, because ustate is set if the update is successful. In your case, "nothing" was successfully installed and ustate is set. > checking whether any (at least one) images were > required (not skipped) before updating the ustate env variable. After > glancing through the code, I couldn't really see the best way to > accomplish this, but somewhere your code does know the difference > between images being written or skipped to decide whether to reboot or > not. When images are written swupdate knows to reboot. SWUpdate does not know, the integrator must know - the bootenv section is installed as last step. > When images are > skipped swupdate knows NOT to reboot. Is there an easy way to use this > same rebooting state to decide when to update the ustate env variable? Best regards, Stefano Babic > > Any guidance is appreciated, > Wes L > > On Tuesday, August 25, 2020 at 10:46:20 PM UTC-4 Kyle Russell wrote: > > On Tuesday, August 25, 2020 at 4:50:25 AM UTC-4 Stefano Babic wrote: > > ustate is the variable to be used, not BOOTVAR_TRANSACTION. > However, as > you noted, this is used just by suricatta. But it does not work if > suricatta is not running as root, and this leads to some > work-arounf in > that case. One of the open issue is to move away the logic from > suricatta (setting a state variable should be centralized and > done by > the installer), and add a sort of IPC in case a co-worker > (suricatta, > webserver, ...) needs to update the status. > > Your patch breaks setup in single-copy where the bootloader just > checks > for the existence of the transaction flag. > > > Ok, I think I understand. I thought a configure option might be > good since it wouldn't be enabled by default in single-copy > implementations. Indeed, in single-copy, the bootloader is likely > only interested in a failure since it simply needs to jump back into > the recovery mode. So I anticipated this configure option only > being enabled in the double-copy case, where the implementation > should be prepared to handle the flag if enabled. > > Is the issue you mentioned tracked anywhere? I would be happy to > work on moving this functionality to core to take advantage of > ustate without suricatta. I would love to hear any ideas you have > on how that should work, or if there are other issues that need to > be resolved before that can happen. > > -- > You received this message because you are subscribed to the Google > Groups "swupdate" group. > To unsubscribe from this group and stop receiving emails from it, send > an email to swupdate+unsubscribe@googlegroups.com > <mailto:swupdate+unsubscribe@googlegroups.com>. > To view this discussion on the web visit > https://groups.google.com/d/msgid/swupdate/14d1df70-4e93-4c22-958d-3359eca2138dn%40googlegroups.com > <https://groups.google.com/d/msgid/swupdate/14d1df70-4e93-4c22-958d-3359eca2138dn%40googlegroups.com?utm_medium=email&utm_source=footer>.
Stefano, Thanks for your response, but I am a little confused. I had assumed that the whole point of setting the ustate variable to "installed" was to signal to the bootloader that new code had actually been installed and the bootloader could now switch from A -> B or B -> A. If this is not what the ustate variable is meant for, can you please explain its use case? On Tuesday, January 5, 2021 at 4:23:47 PM UTC-5 Stefano Babic wrote: > Hi Wes, > > On 05.01.21 20:38, Wes Lindauer wrote: > > Stefano, > > > > After having picked up your commits to refactor the ustate variable into > > common code, I am seeing some undesirable behaviour. I wanted to point > > out the behaviour, in case you have a fix already, and/or possibly get > > some guidance on the best way to fix it. In our sw-description file we > > are using *install-if-higher=true* for all of our images. Then, after > > receiving the same swu file a second time, the images are (correctly) > > "Not required: skipping". The undesirable part is that after skipping > > every image and not making any changes to the flash, swupdate completes > > successfully and unconditionally marks the ustate env variable to > > "installed". This triggers our bootloader to switch between our A/B > > partitions to code that is incorrect/outdated. > > Why ? The toggle should be done by SWUpdate itself, and if nothing has > been installed, the toggle is also not done. Which bootloader are you > using and how is the toggle implemented ? > I assumed "the toggle" was that swupdate would correctly set the ustate variable as a signal to the bootloader. We are using U-Boot as our bootloader. U-Boot has logic similar to this pseudo-code: if (ustate == "installed") { if (bootpart == "A") { bootpart = "B"; } else { bootpart = "A"; } ustate = ""; } > > I believe the fix should > > be something like > > I need first to understand if this should be done. There was already a > quite "philosophical" discussion, because ustate is set if the update is > successful. In your case, "nothing" was successfully installed and > ustate is set. > > This is puzzling to me. I understand that swupdate returns a successful update, but... If ustate is going to be set to "installed" even when nothing is actually installed, then this variable becomes useless. What is the reason for setting ustate = "installed" when nothing was ever written out to the flash? What is the correct way to use the ustate variable if it is always going to tell me something was installed when it wasn't? The way it is functioning currently, ustate is just a copy of swupdate's return code, but that doesn't help the bootloader to decide when to switch from A -> B or B -> A. > > checking whether any (at least one) images were > > required (not skipped) before updating the ustate env variable. After > > glancing through the code, I couldn't really see the best way to > > accomplish this, but somewhere your code does know the difference > > between images being written or skipped to decide whether to reboot or > > not. When images are written swupdate knows to reboot. > > SWUpdate does not know, the integrator must know - the bootenv section > is installed as last step. > I am seeing different behaviour than this. It might be because I am using swupdate-client to initiate the install, but when images are NOT skipped, swupdate will sleep for 5 seconds then reboot. When the images ARE skipped, swupdate does NOT reboot. We are currently not using a bootenv section in our SWU file, because only our bootloader is aware of the fact that partition A = 5 and partition B = 6. Are you implying that the bootenv section of a SWU file would be skipped if all of the images are skipped? If not, using the bootenv section as a toggle would have the same problem as using the ustate env variable. > > > When images are > > skipped swupdate knows NOT to reboot. Is there an easy way to use this > > same rebooting state to decide when to update the ustate env variable? > > Best regards, > Stefano Babic > > > > > Any guidance is appreciated, > > Wes L > > > > On Tuesday, August 25, 2020 at 10:46:20 PM UTC-4 Kyle Russell wrote: > > > > On Tuesday, August 25, 2020 at 4:50:25 AM UTC-4 Stefano Babic wrote: > > > > ustate is the variable to be used, not BOOTVAR_TRANSACTION. > > However, as > > you noted, this is used just by suricatta. But it does not work if > > suricatta is not running as root, and this leads to some > > work-arounf in > > that case. One of the open issue is to move away the logic from > > suricatta (setting a state variable should be centralized and > > done by > > the installer), and add a sort of IPC in case a co-worker > > (suricatta, > > webserver, ...) needs to update the status. > > > > Your patch breaks setup in single-copy where the bootloader just > > checks > > for the existence of the transaction flag. > > > > > > Ok, I think I understand. I thought a configure option might be > > good since it wouldn't be enabled by default in single-copy > > implementations. Indeed, in single-copy, the bootloader is likely > > only interested in a failure since it simply needs to jump back into > > the recovery mode. So I anticipated this configure option only > > being enabled in the double-copy case, where the implementation > > should be prepared to handle the flag if enabled. > > > > Is the issue you mentioned tracked anywhere? I would be happy to > > work on moving this functionality to core to take advantage of > > ustate without suricatta. I would love to hear any ideas you have > > on how that should work, or if there are other issues that need to > > be resolved before that can happen. > > > > -- > > You received this message because you are subscribed to the Google > > Groups "swupdate" group. > > To unsubscribe from this group and stop receiving emails from it, send > > an email to swupdate+u...@googlegroups.com > > <mailto:swupdate+u...@googlegroups.com>. > > To view this discussion on the web visit > > > https://groups.google.com/d/msgid/swupdate/14d1df70-4e93-4c22-958d-3359eca2138dn%40googlegroups.com > > < > https://groups.google.com/d/msgid/swupdate/14d1df70-4e93-4c22-958d-3359eca2138dn%40googlegroups.com?utm_medium=email&utm_source=footer>. > > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 <+49%208142%206698953> Fax: +49-8142-66989-80 > <+49%208142%206698980> Email: sba...@denx.de > ===================================================================== >
Hi Wes, On 06.01.21 17:30, Wes Lindauer wrote: > Stefano, > > Thanks for your response, but I am a little confused. I had assumed that > the whole point of setting the ustate variable to "installed" was to > signal to the bootloader that new code had actually been installed and > the bootloader could now switch from A -> B or B -> A. It is not thought for this, but to implement the fallback. sw-description has a "bootenv" section where the toggle can be implemented. It is usual that after SWUpdate has finished, the environment of the bootloader is already programmed to boot the new software. The bootloader is then a mere executor. The ustate variable is used to mark that a successfully update was done in case the new software does not run. See also the "bootcounter" feature in u-boot. If the new sw does not run, and the bootloader can detect it, the bootloader (done automatically by u-boot via the altbootcmd) can take a decision, generally by switching back to the old software and setting ustate to failed. If the new software is running, the ustate variable should be reset when the system uns (from a startup script, the application,...) to signal that the transition window between old and new software is conclused. Best regards, Stefano Babic > If this is not > what the ustate variable is meant for, can you please explain its use case? > > On Tuesday, January 5, 2021 at 4:23:47 PM UTC-5 Stefano Babic wrote: > > Hi Wes, > > On 05.01.21 20:38, Wes Lindauer wrote: > > Stefano, > > > > After having picked up your commits to refactor the ustate > variable into > > common code, I am seeing some undesirable behaviour. I wanted to > point > > out the behaviour, in case you have a fix already, and/or possibly > get > > some guidance on the best way to fix it. In our sw-description > file we > > are using *install-if-higher=true* for all of our images. Then, after > > receiving the same swu file a second time, the images are (correctly) > > "Not required: skipping". The undesirable part is that after skipping > > every image and not making any changes to the flash, swupdate > completes > > successfully and unconditionally marks the ustate env variable to > > "installed". This triggers our bootloader to switch between our A/B > > partitions to code that is incorrect/outdated. > > Why ? The toggle should be done by SWUpdate itself, and if nothing has > been installed, the toggle is also not done. Which bootloader are you > using and how is the toggle implemented ? > > > > I assumed "the toggle" was that swupdate would correctly set the ustate > variable as a signal to the bootloader. We are using U-Boot as our > bootloader. U-Boot has logic similar to this pseudo-code: > if (ustate == "installed") { > if (bootpart == "A") { > bootpart = "B"; > } else { > bootpart = "A"; > } > ustate = ""; > } > > > I believe the fix should > > be something like > > I need first to understand if this should be done. There was already a > quite "philosophical" discussion, because ustate is set if the > update is > successful. In your case, "nothing" was successfully installed and > ustate is set. > > This is puzzling to me. I understand that swupdate returns a successful > update, but... If ustate is going to be set to "installed" even when > nothing is actually installed, then this variable becomes useless. What > is the reason for setting ustate = "installed" when nothing was ever > written out to the flash? What is the correct way to use the ustate > variable if it is always going to tell me something was installed when > it wasn't? The way it is functioning currently, ustate is just a copy of > swupdate's return code, but that doesn't help the bootloader to decide > when to switch from A -> B or B -> A. > > > > checking whether any (at least one) images were > > required (not skipped) before updating the ustate env variable. After > > glancing through the code, I couldn't really see the best way to > > accomplish this, but somewhere your code does know the difference > > between images being written or skipped to decide whether to > reboot or > > not. When images are written swupdate knows to reboot. > > SWUpdate does not know, the integrator must know - the bootenv section > is installed as last step. > > I am seeing different behaviour than this. It might be because I am > using swupdate-client to initiate the install, but when images are NOT > skipped, swupdate will sleep for 5 seconds then reboot. When the images > ARE skipped, swupdate does NOT reboot. We are currently not using a > bootenv section in our SWU file, because only our bootloader is aware of > the fact that partition A = 5 and partition B = 6. Are you implying that > the bootenv section of a SWU file would be skipped if all of the images > are skipped? If not, using the bootenv section as a toggle would have > the same problem as using the ustate env variable. > > > > When images are > > skipped swupdate knows NOT to reboot. Is there an easy way to use > this > > same rebooting state to decide when to update the ustate env > variable? > > Best regards, > Stefano Babic > > > > > Any guidance is appreciated, > > Wes L > > > > On Tuesday, August 25, 2020 at 10:46:20 PM UTC-4 Kyle Russell wrote: > > > > On Tuesday, August 25, 2020 at 4:50:25 AM UTC-4 Stefano Babic wrote: > > > > ustate is the variable to be used, not BOOTVAR_TRANSACTION. > > However, as > > you noted, this is used just by suricatta. But it does not work if > > suricatta is not running as root, and this leads to some > > work-arounf in > > that case. One of the open issue is to move away the logic from > > suricatta (setting a state variable should be centralized and > > done by > > the installer), and add a sort of IPC in case a co-worker > > (suricatta, > > webserver, ...) needs to update the status. > > > > Your patch breaks setup in single-copy where the bootloader just > > checks > > for the existence of the transaction flag. > > > > > > Ok, I think I understand. I thought a configure option might be > > good since it wouldn't be enabled by default in single-copy > > implementations. Indeed, in single-copy, the bootloader is likely > > only interested in a failure since it simply needs to jump back into > > the recovery mode. So I anticipated this configure option only > > being enabled in the double-copy case, where the implementation > > should be prepared to handle the flag if enabled. > > > > Is the issue you mentioned tracked anywhere? I would be happy to > > work on moving this functionality to core to take advantage of > > ustate without suricatta. I would love to hear any ideas you have > > on how that should work, or if there are other issues that need to > > be resolved before that can happen. > > > > -- > > You received this message because you are subscribed to the Google > > Groups "swupdate" group. > > To unsubscribe from this group and stop receiving emails from it, > send > > an email to swupdate+u...@googlegroups.com > > <mailto:swupdate+u...@googlegroups.com>. > > To view this discussion on the web visit > > > https://groups.google.com/d/msgid/swupdate/14d1df70-4e93-4c22-958d-3359eca2138dn%40googlegroups.com > > > > <https://groups.google.com/d/msgid/swupdate/14d1df70-4e93-4c22-958d-3359eca2138dn%40googlegroups.com?utm_medium=email&utm_source=footer>. > > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 <tel:+49%208142%206698953> Fax: > +49-8142-66989-80 <tel:+49%208142%206698980> Email: sba...@denx.de > ===================================================================== > > -- > You received this message because you are subscribed to the Google > Groups "swupdate" group. > To unsubscribe from this group and stop receiving emails from it, send > an email to swupdate+unsubscribe@googlegroups.com > <mailto:swupdate+unsubscribe@googlegroups.com>. > To view this discussion on the web visit > https://groups.google.com/d/msgid/swupdate/7afb7bde-81d6-49d2-91b5-90b4e1488e8bn%40googlegroups.com > <https://groups.google.com/d/msgid/swupdate/7afb7bde-81d6-49d2-91b5-90b4e1488e8bn%40googlegroups.com?utm_medium=email&utm_source=footer>.
Thanks, Stefano. On Tue, Jan 5, 2021 at 4:23 PM Stefano Babic <sbabic@denx.de> wrote: > In your case, "nothing" was successfully installed and > ustate is set. > This seems strange. Why open an update transaction with the bootloader if swupdate skips all of the install images? It looks like swupdate-progress checks msg.cur_step before rebooting, and only reboots if msg.cur_step > 0. (When an image is skipped, msg.cur_step does not get updated.) In addition, it seems like core should only set the installed transaction marker if cur_step > 0. I think that would resolve Wes's case, and also eliminate an unneeded transaction on the following reboot when no code has actually changed.
Hi everyone, Le Friday, January 8, 2021 à 1:56:51 AM UTC, Kyle Russell a écrit : > Thanks, Stefano. > > On Tue, Jan 5, 2021 at 4:23 PM Stefano Babic <sba...@denx.de> wrote: > >> In your case, "nothing" was successfully installed and >> ustate is set. >> > > This seems strange. Why open an update transaction with the bootloader if > swupdate skips all of the install images? > > It looks like swupdate-progress checks msg.cur_step before rebooting, and > only reboots if msg.cur_step > 0. (When an image is skipped, msg.cur_step > does not get updated.) In addition, it seems like core should only set the > installed transaction marker if cur_step > 0. I think that would resolve > Wes's case, and also eliminate an unneeded transaction on the following > reboot when no code has actually changed. > > I'm here because I'm facing the same challenge Wes reported above. We have integrated swupdate in our yocto-based Linux system using meta-swupdate, and we are running swupdate 2022.05. We run an internally developed and proprietary "management" service but we use swupdate to execute the update by connecting to it using " swupdate_async_start" and keeping track of the installation progress using the "progress_ipc_receive". We use u-boot and an A/B update setup. We use a "bootenv" section to toggle between rootfs A/B (also labelled active_rootfs {1,2} and copy{1,2} below) Everything worked very well so far. (thank you !) But now we wanted to prevent user-triggered "downgrades" so I've added a /etc/sw-versions file to our rootfs that provides its current version, added a "name" and a "version" to each of my firmware "image" copy in my "sw-description" and enabled "install-if-higher" for both copies. But things don't work as I thought they would and I don't think there is way to make it work the way I want it to. Scenario : User tries installing version "0.1.0" of "rootfs" on system having "rootfs" version "0.2.0" installed. What I expected : Install fails, nothing happens. What I get : Install is "successfully" installed, (but it wasn't since it was skipped), system toggles to the "new" active rootfs by "updating" the "bootenv " (actually going back to the previously successfully installed copy). Here is what my sw-description looks like: software = { description = "Firmware update image generated by Yocto for swupdate" version = "0.1.0"; hardware-compatibility: [ "A", "B", "C" ]; product = { stable: { copy1: { images: ( { version = "0.1.0"; name = "rootfs"; install-if-higher = true; filename = "rootfs.ext4.gz.enc"; sha256 = "@rootfs.ext4.gz.enc"; type = "raw"; compressed = "zlib"; encrypted = true; device = "/dev/mmcblk0p5"; }); bootenv: ( { name = "active_rootfs"; value = "1"; }, { name = "system_updated"; value = "1"; }); }; copy2: { images: ( { version = "0.1.0"; name = "rootfs"; install-if-higher = true; filename = "rootfs.ext4.gz.enc"; sha256 = "@rootfs.ext4.gz.enc"; type = "raw"; compressed = "zlib"; encrypted = true; device = "/dev/mmcblk0p6"; }); bootenv: ( { name = "active_rootfs"; value = "2"; }, { name = "system_updated"; value = "1"; }); }; }; }; } I can detect in our application that the update was "skipped" and not successfully installed by monitoring on the progress_ipc_receive that we went from START/RUN to SUCCESS without going through a "PROGRESS" (which I guess is the same as looking at "cur_step > 0" suggested in other answers) of course it would be nicer to have an actual "SKIPPED" message. But, irrespective of whether any image has been installed (and I really want to toggle between A and B for a specific image only, I may have other image(s) in the future to udpate other components of the system but these should not trigger an A/B switch), the bootenv part gets "installed". Can you please confirm my understanding of the behaviour of swupdate 2022.05 : - The bootenv section is "installed" irrespective of having installed any of the image in the images list (not installed because they are "older" than what is currently installed, I believe bootenv is not called if the install genuinely fails) - The bootenv section cannot be "connected" to a specific image in the images list which would mean I have to make separate sw-description if I wanted to update components that don't actually require an A/B switch. If my understanding is correct is there any other solution than implementing the A/B switch in my application (removing the bootenv section of the sw-description) and by monitoring if the image installed is "rootfs" and that it was actually really installed (when through a "PROGRESS" phase) ? Thank you. Best regards, Christophe
Partially replying to my own questions after having spent some more time in the documentation and source code. Le Friday, May 27, 2022 à 10:14:29 AM UTC, Christophe Aeschlimann a écrit : > Hi everyone, > > Le Friday, January 8, 2021 à 1:56:51 AM UTC, Kyle Russell a écrit : > >> Thanks, Stefano. >> >> On Tue, Jan 5, 2021 at 4:23 PM Stefano Babic <sba...@denx.de> wrote: >> >>> In your case, "nothing" was successfully installed and >>> ustate is set. >>> >> >> This seems strange. Why open an update transaction with the bootloader >> if swupdate skips all of the install images? >> >> It looks like swupdate-progress checks msg.cur_step before rebooting, and >> only reboots if msg.cur_step > 0. (When an image is skipped, msg.cur_step >> does not get updated.) In addition, it seems like core should only set the >> installed transaction marker if cur_step > 0. I think that would resolve >> Wes's case, and also eliminate an unneeded transaction on the following >> reboot when no code has actually changed. >> >> > I'm here because I'm facing the same challenge Wes reported above. > > We have integrated swupdate in our yocto-based Linux system using > meta-swupdate, and we are running swupdate 2022.05. > > We run an internally developed and proprietary "management" service but we > use swupdate to execute the update by connecting to it using " > swupdate_async_start" and keeping track of the installation progress > using the "progress_ipc_receive". > > We use u-boot and an A/B update setup. We use a "bootenv" section to > toggle between rootfs A/B (also labelled active_rootfs {1,2} and copy{1,2} > below) > > Everything worked very well so far. (thank you !) > > But now we wanted to prevent user-triggered "downgrades" so I've added a > /etc/sw-versions file to our rootfs that provides its current version, > added a "name" and a "version" to each of my firmware "image" copy in my > "sw-description" and enabled "install-if-higher" for both copies. > But things don't work as I thought they would and I don't think there is > way to make it work the way I want it to. > > Scenario : User tries installing version "0.1.0" of "rootfs" on system > having "rootfs" version "0.2.0" installed. > > What I expected : > > Install fails, nothing happens. > > What I get : > > Install is "successfully" installed, (but it wasn't since it was skipped), > system toggles to the "new" active rootfs by "updating" the "bootenv " > (actually going back to the previously successfully installed copy). > For the generic use case you can instead use the general flag "no-downgrading" which is global to the SWU update file. It uses the version at the top of the sw-description tree and not the "per-image" setting. This is what it looks like in swupdate.cfg : globals : { no-downgrading = "0.2.0" }; This will make the install fail early and stop the installation flow before reaching the "bootenv" section. > > Here is what my sw-description looks like: > > software = > { > description = "Firmware update image generated by Yocto for swupdate" > version = "0.1.0"; > hardware-compatibility: [ "A", "B", "C" ]; > product = { > stable: { > copy1: > { > images: ( > { > version = "0.1.0"; > name = "rootfs"; > install-if-higher = true; > filename = "rootfs.ext4.gz.enc"; > sha256 = "@rootfs.ext4.gz.enc"; > type = "raw"; > compressed = "zlib"; > encrypted = true; > device = "/dev/mmcblk0p5"; > }); > bootenv: ( > { > name = "active_rootfs"; > value = "1"; > }, > { > name = "system_updated"; > value = "1"; > }); > }; > copy2: > { > images: ( > { > version = "0.1.0"; > name = "rootfs"; > install-if-higher = true; > filename = "rootfs.ext4.gz.enc"; > sha256 = "@rootfs.ext4.gz.enc"; > type = "raw"; > compressed = "zlib"; > encrypted = true; > device = "/dev/mmcblk0p6"; > }); > bootenv: ( > { > name = "active_rootfs"; > value = "2"; > }, > { > name = "system_updated"; > value = "1"; > }); > }; > }; > }; > } > > I can detect in our application that the update was "skipped" and not > successfully installed by monitoring on the progress_ipc_receive that we > went from START/RUN to SUCCESS without going through a "PROGRESS" (which I > guess is the same as looking at "cur_step > 0" suggested in other answers) > of course it would be nicer to have an actual "SKIPPED" message. But, > irrespective of whether any image has been installed (and I really want to > toggle between A and B for a specific image only, I may have other image(s) > in the future to udpate other components of the system but these should not > trigger an A/B switch), the bootenv part gets "installed". > > Can you please confirm my understanding of the behaviour of swupdate > 2022.05 : > > - The bootenv section is "installed" irrespective of having installed any > of the image in the images list (not installed because they are "older" > than what is currently installed, I believe bootenv is not called if the > install genuinely fails) > - The bootenv section cannot be "connected" to a specific image in the > images list which would mean I have to make separate sw-description if I > wanted to update components that don't actually require an A/B switch. > > If my understanding is correct is there any other solution than > implementing the A/B switch in my application (removing the bootenv section > of the sw-description) and by monitoring if the image installed is "rootfs" > and that it was actually really installed (when through a "PROGRESS" phase) > ? > > Thank you. > > Best regards, > > Christophe > > >
diff --git a/bootloader/Config.in b/bootloader/Config.in index d27f66f..b976bd4 100644 --- a/bootloader/Config.in +++ b/bootloader/Config.in @@ -64,3 +64,17 @@ config GRUBENV_PATH default "/boot/efi/EFI/BOOT/grub/grubenv" help Provide path to GRUB environment block file + +config SAVE_BOOTVAR_TRANSACTION + bool "Save bootloader transaction marker on success" + depends on !BOOTLOADER_NONE + default n + help + Don't reset the bootloader transaction marker + following a successful programming. It is up to the + bootloader to clear this environment variable on reboot. + + This option is useful for devices that implement a + double-copy strategy, and can signal to the bootloader that + a successful programming has occurred and that the bootloader + should switch to the alternate partition. diff --git a/core/stream_interface.c b/core/stream_interface.c index 99e5c62..0f80c24 100644 --- a/core/stream_interface.c +++ b/core/stream_interface.c @@ -563,7 +563,11 @@ void *network_initializer(void *data) * that it is not required to start recovery again */ if (software->bootloader_transaction_marker) { +#ifdef CONFIG_SAVE_BOOTVAR_TRANSACTION + save_state_string((char*)BOOTVAR_TRANSACTION, STATE_INSTALLED); +#else reset_state((char*)BOOTVAR_TRANSACTION); +#endif } notify(SUCCESS, RECOVERY_NO_ERROR, INFOLEVEL, "SWUPDATE successful !"); inst.last_install = SUCCESS; diff --git a/core/swupdate.c b/core/swupdate.c index cc7e2d3..685169a 100644 --- a/core/swupdate.c +++ b/core/swupdate.c @@ -396,7 +396,11 @@ static int install_from_file(char *fname, int check) } if (swcfg.bootloader_transaction_marker) { +#ifdef CONFIG_SAVE_BOOTVAR_TRANSACTION + save_state_string((char*)BOOTVAR_TRANSACTION, STATE_INSTALLED); +#else reset_state((char*)BOOTVAR_TRANSACTION); +#endif } fprintf(stdout, "Software updated successfully\n"); fprintf(stdout, "Please reboot the device to start the new software\n"); diff --git a/include/state.h b/include/state.h index 7294dbe..9ebb9e4 100644 --- a/include/state.h +++ b/include/state.h @@ -52,6 +52,7 @@ static inline char* get_state_string(update_state_t state) { switch (state) { case STATE_IN_PROGRESS: return (char*)"in_progress"; case STATE_FAILED: return (char*)"failed"; + case STATE_INSTALLED: return (char*)"installed"; default: break; } return (char*)state;