RE: [PATCH v3 0/2] Fix Android A/B backup

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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]