| Message ID | 20240308165937.270499-1-colin.mcallister@garmin.com |
|---|---|
| Headers | show |
| Series | Fix Android A/B backup | expand |
On Fri, Mar 8, 2024 at 11:00 AM Colin McAllister <colin.mcallister@garmin.com> wrote: > > - Addresses compiler error due to missing semicolon > - Removes use of preprocessor macros with ANDROID_AB_BACKUP_OFFSET > > Bug was found by noticing a semicolon was missing and not causing a > compiler error when CONFIG_ANDROID_AB_BACKUP_OFFSET was set. I submitted > a patch to fix the semicolon before fixing the #if's. Testing the latter > patch without the former with ANDORID_AB_BACKUP_OFFSET set will cause a > compiler error. > > What's changed in V2? > --------------------- > > The second verison of changes removes the #if preprocessor macros to use > C conditionals instead. There was some minor refactoring required to get > this to work, so I did more thourough testing to ensure the backup data > works as expected. > > I also realized that CONFIG_VAL is not needed because there's no reason > for the SPL or TPL to have different values for the backup address. I > opted to just use CONFIG_ANDROID_AB_BACKUP_OFFSET directly. > > What's changed in V3? > --------------------- > > Added "Fixes:" tag to patches. Performed additonal testing as described > in the second paragraph in the testing notes below. > > I opted to not use CONFIG_IS_ENABLED because that macro appears to only > be intended for y/n configurations. See note at top of linux/kconfig.h: > > "Note that these only work with boolean and tristate options." Ah, ok, I see you replied to my comment here. So when that config option is not defined at all, the build still works, right? One way to test that would be to use buildman tool for all boards. Although I have to say it might be very time consuming (I usually run it overnight), so I'm not asking you to do it in this case, it's just FYI. > > How was this patch series tested? > --------------------------------- > > I built for my target with CONFIG_ANDROID_AB_BACKUP_OFFSET set to > 0x1000. I first verified that the device can normally boot. I then ran > dd before rebooting to corrupt the primary data. I confirmed that the > backup was used to properly restore the primary. I then corrupted both > the primary and backup data and confirmed that the primary and backup > were both reinitialized to default values. Lastly, I corrupted the > backup data and not the primary data and confirmed that the backup was > restored the primary data. > > Addtionally, I disabled CONFIG_ANDROID_AB_BACKUP_OFFSET for my device > and confirmed that after I corrupt the primary data, no backup is used. > When the primary data is not corrupt, the device boots normally. This is > what I would expect, because CONFIG_ANDROID_AB_BACKUP_OFFSET's default > value is 0x0, which would evaluate to false for all C if conditions. > > Colin McAllister (2): > android_ab: Add missing semicolon > android_ab: Fix ANDROID_AB_BACKUP_OFFSET > > boot/android_ab.c | 97 ++++++++++++++++++++++------------------------- > 1 file changed, 45 insertions(+), 52 deletions(-) > > -- > 2.43.2 > > > ________________________________ > > CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use of the intended recipient(s) and contain information that may be Garmin confidential and/or Garmin legally privileged. If you have received this email in error, please notify the sender by reply email and delete the message. Any disclosure, copying, distribution or use of this communication (including attachments) by someone other than the intended recipient is prohibited. Thank you.
> Ah, ok, I see you replied to my comment here. Yes, sorry. Outlook is terrible to send inline responses too. I figured just adding responses in the patch contents would be better. Next time I'll submit my patch with a different email :) > So when that config option is not defined at all, the build still > works, right? Yes, the default value for CONFIG_ANDROID_AB_BACKUP_OFFSET is 0x0, which would evaluate to a false bool value in the if conditions. I did do some testing with the config value not defined for my board and confirmed back-up data is not used. In your other emails you include your reviewed-by tag. For clarity, Am I supposed to append my patches and upload a new version? This is my first time contributing to u-boot, so I'm still learning the workflow. I didn't see anything glancing through the "Sending patches" page in the U-Boot documentation. Best, Colin
On Fri, Mar 8, 2024 at 1:24 PM McAllister, Colin <Colin.McAllister@garmin.com> wrote: > > > Ah, ok, I see you replied to my comment here. > > Yes, sorry. Outlook is terrible to send inline responses too. I figured > just adding responses in the patch contents would be better. Next time I'll submit > my patch with a different email :) > > > So when that config option is not defined at all, the build still > > works, right? > > Yes, the default value for CONFIG_ANDROID_AB_BACKUP_OFFSET is 0x0, which > would evaluate to a false bool value in the if conditions. I did do some > testing with the config value not defined for my board and confirmed > back-up data is not used. > Looks good to me, thanks. > In your other emails you include your reviewed-by tag. For clarity, Am I > supposed to append my patches and upload a new version? This is my > first time contributing to u-boot, so I'm still learning the workflow. I > didn't see anything glancing through the "Sending patches" page in the > U-Boot documentation. > Welcome to the community! And thanks for your patches :) U-Boot workflow is quite similar to Linux kernel one. It's useful to collect all tags when sending out your next version. When the maintainer takes your patch, they usually also apply all R-b tags for the final patch version, so you only have to worry about that when sending out a new version. I know that U-Boot contributors are often using `patman' tool [1] for submitting patches (and corresponding updated versions), and I'm pretty sure it collects all pending tags automatically for you. FWIW, I'm not experienced with `patman', as I'm trying to use somehow unified submitting process for both U-Boot and Linux kernel, and I know that using `patman' is sometimes discouraged in Linux kernel community. [1] https://docs.u-boot.org/en/latest/develop/patman.html [snip]
Hi Colin, On ven., mars 08, 2024 at 15:59, Sam Protsenko <semen.protsenko@linaro.org> wrote: > On Fri, Mar 8, 2024 at 1:24 PM McAllister, Colin > <Colin.McAllister@garmin.com> wrote: >> >> > Ah, ok, I see you replied to my comment here. >> >> Yes, sorry. Outlook is terrible to send inline responses too. I figured >> just adding responses in the patch contents would be better. Next time I'll submit >> my patch with a different email :) >> >> > So when that config option is not defined at all, the build still >> > works, right? >> >> Yes, the default value for CONFIG_ANDROID_AB_BACKUP_OFFSET is 0x0, which >> would evaluate to a false bool value in the if conditions. I did do some >> testing with the config value not defined for my board and confirmed >> back-up data is not used. >> > > Looks good to me, thanks. > >> In your other emails you include your reviewed-by tag. For clarity, Am I >> supposed to append my patches and upload a new version? This is my >> first time contributing to u-boot, so I'm still learning the workflow. I >> didn't see anything glancing through the "Sending patches" page in the >> U-Boot documentation. >> > > Welcome to the community! And thanks for your patches :) U-Boot > workflow is quite similar to Linux kernel one. It's useful to collect > all tags when sending out your next version. When the maintainer takes > your patch, they usually also apply all R-b tags for the final patch > version, so you only have to worry about that when sending out a new > version. I know that U-Boot contributors are often using `patman' tool > [1] for submitting patches (and corresponding updated versions), and > I'm pretty sure it collects all pending tags automatically for you. > FWIW, I'm not experienced with `patman', as I'm trying to use somehow > unified submitting process for both U-Boot and Linux kernel, and I > know that using `patman' is sometimes discouraged in Linux kernel > community. Welcome to the U-Boot community! It takes quite some time to start contributing so thank you for the patches. The changes look fine and the detailed approach on how you tested is very much appreciated. I have a couple of suggestions on the whole series. 1. The patches don't apply: $ b4 shazam -s -l 20240308165937.270499-1-colin.mcallister@garmin.com error: patch failed: boot/android_ab.c:187 error: boot/android_ab.c: patch does not apply error: Did you hand edit your patch? It does not apply to blobs recorded in its index. Patch failed at 0002 android_ab: Fix ANDROID_AB_BACKUP_OFFSET hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". I suspect your email provider swapped tabs to spaces. It's also possible that the Garmin confidentiality footer caused this. 2. Using the khadas-vim3_android_ab_defconfig, this does not build: boot/android_ab.c: In function 'ab_select_slot': boot/android_ab.c:350:48: error: 'ANDROID_AB_BACKUP_OFFSET' undeclared (first use in this function); did you mean 'CONFIG_ANDROID_AB_BACKUP_OFFSET'? 350 | ANDROID_AB_BACKUP_OFFSET); | ^~~~~~~~~~~~~~~~~~~~~~~~ | CONFIG_ANDROID_AB_BACKUP_OFFSET Both are minor problems. I re-applied the diffs manually to be able to build/boot test this. Since this is your first contribution, I can either: - fix both myself and merge this. - let you spin a v4 to fix the above. Please let me know what you prefer. If you do intend to send a v4, please: - Do it in a new email thread - Make sure you cc me, Igor and Sam - Make sure the patches you send can be applied. http://git-send-email.io/ is a friendly service you can use to test your workflow. On workflow, tooling, I usually suggest the b4 tool: https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1 Regards, Mattijs > > [1] https://docs.u-boot.org/en/latest/develop/patman.html > > [snip]
Hi Mattijs, I’ve been using git send-email, but there might be issues with what the Garmin smtp server is doing to the email, like adding the footer. I sent a v4 PS in a new thread using my personal email, but that email isn’t subscribed to this ML so I think the patches are pending approval to be added to the ML. Best, Colin From: Mattijs Korpershoek <mkorpershoek@baylibre.com> Sent: Tuesday, March 12, 2024 4:47 AM To: Sam Protsenko <semen.protsenko@linaro.org>; McAllister, Colin <Colin.McAllister@garmin.com> Cc: u-boot@lists.denx.de; JPEWhacker@gmail.com; sjg@chromium.org; Igor Opaniuk <igor.opaniuk@gmail.com> Subject: Re: [PATCH v3 0/2] Fix Android A/B backup Hi Colin, On ven. , mars 08, 2024 at 15: 59, Sam Protsenko <semen. protsenko@ linaro. org> wrote: > On Fri, Mar 8, 2024 at 1: 24 PM McAllister, Colin > <Colin. McAllister@ garmin. com> wrote: >> >> > Ah, ok, I see you Hi Colin, On ven., mars 08, 2024 at 15:59, Sam Protsenko <semen.protsenko@linaro.org<mailto:semen.protsenko@linaro.org>> wrote: > On Fri, Mar 8, 2024 at 1:24 PM McAllister, Colin > <Colin.McAllister@garmin.com<mailto:Colin.McAllister@garmin.com>> wrote: >> >> > Ah, ok, I see you replied to my comment here. >> >> Yes, sorry. Outlook is terrible to send inline responses too. I figured >> just adding responses in the patch contents would be better. Next time I'll submit >> my patch with a different email :) >> >> > So when that config option is not defined at all, the build still >> > works, right? >> >> Yes, the default value for CONFIG_ANDROID_AB_BACKUP_OFFSET is 0x0, which >> would evaluate to a false bool value in the if conditions. I did do some >> testing with the config value not defined for my board and confirmed >> back-up data is not used. >> > > Looks good to me, thanks. > >> In your other emails you include your reviewed-by tag. For clarity, Am I >> supposed to append my patches and upload a new version? This is my >> first time contributing to u-boot, so I'm still learning the workflow. I >> didn't see anything glancing through the "Sending patches" page in the >> U-Boot documentation. >> > > Welcome to the community! And thanks for your patches :) U-Boot > workflow is quite similar to Linux kernel one. It's useful to collect > all tags when sending out your next version. When the maintainer takes > your patch, they usually also apply all R-b tags for the final patch > version, so you only have to worry about that when sending out a new > version. I know that U-Boot contributors are often using `patman' tool > [1] for submitting patches (and corresponding updated versions), and > I'm pretty sure it collects all pending tags automatically for you. > FWIW, I'm not experienced with `patman', as I'm trying to use somehow > unified submitting process for both U-Boot and Linux kernel, and I > know that using `patman' is sometimes discouraged in Linux kernel > community. Welcome to the U-Boot community! It takes quite some time to start contributing so thank you for the patches. The changes look fine and the detailed approach on how you tested is very much appreciated. I have a couple of suggestions on the whole series. 1. The patches don't apply: $ b4 shazam -s -l 20240308165937.270499-1-colin.mcallister@garmin.com<mailto:20240308165937.270499-1-colin.mcallister@garmin.com> error: patch failed: boot/android_ab.c:187 error: boot/android_ab.c: patch does not apply error: Did you hand edit your patch? It does not apply to blobs recorded in its index. Patch failed at 0002 android_ab: Fix ANDROID_AB_BACKUP_OFFSET hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". I suspect your email provider swapped tabs to spaces. It's also possible that the Garmin confidentiality footer caused this. 2. Using the khadas-vim3_android_ab_defconfig, this does not build: boot/android_ab.c: In function 'ab_select_slot': boot/android_ab.c:350:48: error: 'ANDROID_AB_BACKUP_OFFSET' undeclared (first use in this function); did you mean 'CONFIG_ANDROID_AB_BACKUP_OFFSET'? 350 | ANDROID_AB_BACKUP_OFFSET); | ^~~~~~~~~~~~~~~~~~~~~~~~ | CONFIG_ANDROID_AB_BACKUP_OFFSET Both are minor problems. I re-applied the diffs manually to be able to build/boot test this. Since this is your first contribution, I can either: - fix both myself and merge this. - let you spin a v4 to fix the above. Please let me know what you prefer. If you do intend to send a v4, please: - Do it in a new email thread - Make sure you cc me, Igor and Sam - Make sure the patches you send can be applied. https://urldefense.com/v3/__http://git-send-email.io/__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8Ldw1zsFDgg$<https://urldefense.com/v3/__http:/git-send-email.io/__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8Ldw1zsFDgg$> is a friendly service you can use to test your workflow. On workflow, tooling, I usually suggest the b4 tool: https://urldefense.com/v3/__https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8LdwuYhSjtw$<https://urldefense.com/v3/__https:/people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8LdwuYhSjtw$> Regards, Mattijs > > [1] https://urldefense.com/v3/__https://docs.u-boot.org/en/latest/develop/patman.html__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8Ldyum8H6Ow$<https://urldefense.com/v3/__https:/docs.u-boot.org/en/latest/develop/patman.html__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8Ldyum8H6Ow$> > > [snip]
Hi Colin, On mar., mars 12, 2024 at 14:04, "McAllister, Colin" <Colin.McAllister@garmin.com> wrote: > Hi Mattijs, > > I’ve been using git send-email, but there might be issues with what the Garmin smtp server is doing to the email, like adding the footer. I sent a v4 PS in a new thread using my personal email, but that email isn’t subscribed to this ML so I think the patches are pending approval to be added to the ML. Yep, seems they got approved. I will follow-up on the v4 series. > > Best, > Colin > > From: Mattijs Korpershoek <mkorpershoek@baylibre.com> > Sent: Tuesday, March 12, 2024 4:47 AM > To: Sam Protsenko <semen.protsenko@linaro.org>; McAllister, Colin <Colin.McAllister@garmin.com> > Cc: u-boot@lists.denx.de; JPEWhacker@gmail.com; sjg@chromium.org; Igor Opaniuk <igor.opaniuk@gmail.com> > Subject: Re: [PATCH v3 0/2] Fix Android A/B backup > > Hi Colin, On ven. , mars 08, 2024 at 15: 59, Sam Protsenko <semen. protsenko@ linaro. org> wrote: > On Fri, Mar 8, 2024 at 1: 24 PM McAllister, Colin > <Colin. McAllister@ garmin. com> wrote: >> >> > Ah, ok, I see you > > > Hi Colin, > > > > On ven., mars 08, 2024 at 15:59, Sam Protsenko <semen.protsenko@linaro.org<mailto:semen.protsenko@linaro.org>> wrote: > > > >> On Fri, Mar 8, 2024 at 1:24 PM McAllister, Colin > >> <Colin.McAllister@garmin.com<mailto:Colin.McAllister@garmin.com>> wrote: > >>> > >>> > Ah, ok, I see you replied to my comment here. > >>> > >>> Yes, sorry. Outlook is terrible to send inline responses too. I figured > >>> just adding responses in the patch contents would be better. Next time I'll submit > >>> my patch with a different email :) > >>> > >>> > So when that config option is not defined at all, the build still > >>> > works, right? > >>> > >>> Yes, the default value for CONFIG_ANDROID_AB_BACKUP_OFFSET is 0x0, which > >>> would evaluate to a false bool value in the if conditions. I did do some > >>> testing with the config value not defined for my board and confirmed > >>> back-up data is not used. > >>> > >> > >> Looks good to me, thanks. > >> > >>> In your other emails you include your reviewed-by tag. For clarity, Am I > >>> supposed to append my patches and upload a new version? This is my > >>> first time contributing to u-boot, so I'm still learning the workflow. I > >>> didn't see anything glancing through the "Sending patches" page in the > >>> U-Boot documentation. > >>> > >> > >> Welcome to the community! And thanks for your patches :) U-Boot > >> workflow is quite similar to Linux kernel one. It's useful to collect > >> all tags when sending out your next version. When the maintainer takes > >> your patch, they usually also apply all R-b tags for the final patch > >> version, so you only have to worry about that when sending out a new > >> version. I know that U-Boot contributors are often using `patman' tool > >> [1] for submitting patches (and corresponding updated versions), and > >> I'm pretty sure it collects all pending tags automatically for you. > >> FWIW, I'm not experienced with `patman', as I'm trying to use somehow > >> unified submitting process for both U-Boot and Linux kernel, and I > >> know that using `patman' is sometimes discouraged in Linux kernel > >> community. > > > > Welcome to the U-Boot community! It takes quite some time to start > > contributing so thank you for the patches. > > > > The changes look fine and the detailed approach on how you tested is > > very much appreciated. > > > > I have a couple of suggestions on the whole series. > > > > 1. The patches don't apply: > > > > $ b4 shazam -s -l 20240308165937.270499-1-colin.mcallister@garmin.com<mailto:20240308165937.270499-1-colin.mcallister@garmin.com> > > > > error: patch failed: boot/android_ab.c:187 > > error: boot/android_ab.c: patch does not apply > > error: Did you hand edit your patch? > > It does not apply to blobs recorded in its index. > > Patch failed at 0002 android_ab: Fix ANDROID_AB_BACKUP_OFFSET > > hint: Use 'git am --show-current-patch=diff' to see the failed patch > > When you have resolved this problem, run "git am --continue". > > If you prefer to skip this patch, run "git am --skip" instead. > > To restore the original branch and stop patching, run "git am --abort". > > > > I suspect your email provider swapped tabs to spaces. It's also possible > > that the Garmin confidentiality footer caused this. > > > > 2. Using the khadas-vim3_android_ab_defconfig, this does not build: > > > > boot/android_ab.c: In function 'ab_select_slot': > > boot/android_ab.c:350:48: error: 'ANDROID_AB_BACKUP_OFFSET' undeclared (first use in this function); did you mean 'CONFIG_ANDROID_AB_BACKUP_OFFSET'? > > 350 | ANDROID_AB_BACKUP_OFFSET); > > | ^~~~~~~~~~~~~~~~~~~~~~~~ > > | CONFIG_ANDROID_AB_BACKUP_OFFSET > > > > Both are minor problems. > > I re-applied the diffs manually to be able to build/boot test this. > > > > Since this is your first contribution, I can either: > > - fix both myself and merge this. > > - let you spin a v4 to fix the above. > > > > Please let me know what you prefer. > > > > If you do intend to send a v4, please: > > - Do it in a new email thread > > - Make sure you cc me, Igor and Sam > > - Make sure the patches you send can be applied. > > https://urldefense.com/v3/__http://git-send-email.io/__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8Ldw1zsFDgg$<https://urldefense.com/v3/__http:/git-send-email.io/__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8Ldw1zsFDgg$> is a friendly service you can use to test > > your workflow. > > > > On workflow, tooling, I usually suggest the b4 tool: > > https://urldefense.com/v3/__https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8LdwuYhSjtw$<https://urldefense.com/v3/__https:/people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8LdwuYhSjtw$> > > > > Regards, > > Mattijs > > > >> > >> [1] https://urldefense.com/v3/__https://docs.u-boot.org/en/latest/develop/patman.html__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8Ldyum8H6Ow$<https://urldefense.com/v3/__https:/docs.u-boot.org/en/latest/develop/patman.html__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8Ldyum8H6Ow$> > >> > >> [snip]