[PATCH][V2] Fix fastboot handling of partitions when no slots are supported

[PATCH][V2] Fix fastboot handling of partitions when no slots are supported

[Federico Fuga]

The fastboot module has a bug that prevents some command to work
properly on devices that haven't an Android-like partition scheme, that
is, just one spl and one kernel partition, instead of the redundant
scheme with _a and _b slots.

This is the schema of our NAND storage (board is based on an AllWinner
A33 sunxi chip):

=> mtdparts

device nand0 <1c03000.nand>, # parts = 4
  #: name         size            net size        offset mask_flags
  0: spl          0x00020000      0x00020000      0x00000000      0
  1: uboot        0x00100000      0x00100000      0x00020000      0
  2: kernel_a     0x00400000      0x00400000      0x00120000      0
  3: ubi          0x07ae0000      0x079e0000 (!)  0x00520000      0

active partition: nand0,0 - (spl) 0x00020000 @ 0x00000000

This happens when we try to erase the spl partition using fastboot:

$ fastboot erase spl
Erasing 'spl_a'               FAILED (remote: 'invalid NAND device')
fastboot: error: Command failed

The error occurs because getvars fails to handle the error returned by
nand layer when a partition cannot be found.

Indeed, getvar_get_part_info returns what is returned by
fastboot_nand_get_part_info (0 on success, 1 on failure) but it should
return -ENODEV or -EINVAL instead. Since the cause of failure is not
returned by the nand function, I decided to return -EINVAL to make it
simple.

Signed-off-by: Federico Fuga <fuga@studiofuga.com>
---
  drivers/fastboot/fb_getvar.c | 5 ++++-
  drivers/fastboot/fb_nand.c   | 2 +-
  2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
index 9c2ce65a4e..816ed8a621 100644
--- a/drivers/fastboot/fb_getvar.c
+++ b/drivers/fastboot/fb_getvar.c
@@ -121,8 +121,11 @@ static int getvar_get_part_info(const char 
*part_name, char *response,
              *size = disk_part.size * disk_part.blksz;
      } else if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_NAND)) {
          r = fastboot_nand_get_part_info(part_name, &part_info, response);
-        if (r >= 0 && size)
+        if (r == 0 && size) {
              *size = part_info->size;
+        } else {
+            r = -EINVAL;
+        }
      } else {
          fastboot_fail("this storage is not supported in bootloader", 
response);
          r = -ENODEV;
diff --git a/drivers/fastboot/fb_nand.c b/drivers/fastboot/fb_nand.c
index afc64fd528..9e2f7c0189 100644
--- a/drivers/fastboot/fb_nand.c
+++ b/drivers/fastboot/fb_nand.c
@@ -151,7 +151,7 @@ static lbaint_t fb_nand_sparse_reserve(struct 
sparse_storage *info,
   *
   * @part_name: Named device to lookup
   * @part_info: Pointer to returned part_info pointer
- * @response: Pointer to fastboot response buffer
+ * @response: 0 on success, 1 otherwise
   */
  int fastboot_nand_get_part_info(const char *part_name,
                  struct part_info **part_info, char *response)
-- 
2.48.1

Re: [PATCH][V2] Fix fastboot handling of partitions when no slots are supported

[Mattijs Korpershoek]

Hi Federico,

Thank you for the patch.

This patch has incorrect recipients. This has only been send to the
U-Boot.

However, when running:

$ ./scripts/get_maintainer.pl -f drivers/fastboot

Mattijs Korpershoek <mkorpershoek@baylibre.com> (maintainer:FASTBOOT,commit_signer:7/10=70%)
Tom Rini <trini@konsulko.com> (maintainer:THE REST,authored:5/10=50%)
Simon Glass <sjg@chromium.org> (commit_signer:2/10=20%,authored:2/10=20%)
Caleb Connolly <caleb.connolly@linaro.org> (commit_signer:1/10=10%,authored:1/10=10%)
Ilias Apalodimas <ilias.apalodimas@linaro.org> (commit_signer:1/10=10%)
Quentin Schulz <quentin.schulz@cherry.de> (commit_signer:1/10=10%)
Jerome Forissier <jerome.forissier@linaro.org> (authored:1/10=10%)
Alexey Romanov <avromanov@salutedevices.com> (authored:1/10=10%)
u-boot@lists.denx.de (open list)

We see that I should have been copied as well (since I'm the fastboot maintainer)

Please make sure you run ./scripts/get_maintainer when submitting
changes. If patches are not addressed to the right people, they might
get lost and have no answer to them.
This will probably discourage further contributions, which is a shame!

Consider using tools like b4 which help automating such things for you:
https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1
https://b4.docs.kernel.org/en/latest/contributor/overview.html

b4 is aimed at kernel developers first, but works great for U-Boot
submissions as well.

Please see some more remarks below.

On mar., janv. 28, 2025 at 10:27, Federico Fuga <fuga@studiofuga.com> wrote:

> The fastboot module has a bug that prevents some command to work
> properly on devices that haven't an Android-like partition scheme, that
> is, just one spl and one kernel partition, instead of the redundant
> scheme with _a and _b slots.
>
> This is the schema of our NAND storage (board is based on an AllWinner
> A33 sunxi chip):
>
> => mtdparts
>
> device nand0 <1c03000.nand>, # parts = 4
>   #: name         size            net size        offset mask_flags
>   0: spl          0x00020000      0x00020000      0x00000000      0
>   1: uboot        0x00100000      0x00100000      0x00020000      0
>   2: kernel_a     0x00400000      0x00400000      0x00120000      0
>   3: ubi          0x07ae0000      0x079e0000 (!)  0x00520000      0
>
> active partition: nand0,0 - (spl) 0x00020000 @ 0x00000000
>
> This happens when we try to erase the spl partition using fastboot:
>
> $ fastboot erase spl
> Erasing 'spl_a'               FAILED (remote: 'invalid NAND device')
> fastboot: error: Command failed
>
> The error occurs because getvars fails to handle the error returned by
> nand layer when a partition cannot be found.
>
> Indeed, getvar_get_part_info returns what is returned by
> fastboot_nand_get_part_info (0 on success, 1 on failure) but it should
> return -ENODEV or -EINVAL instead. Since the cause of failure is not
> returned by the nand function, I decided to return -EINVAL to make it
> simple.
>
> Signed-off-by: Federico Fuga <fuga@studiofuga.com>

The patch is still not applicable for me.
I see:
$ b4 shazam -s -l --check 906b4d88-39c6-4e85-b9b1-62527a87d640@studiofuga.com
Grabbing thread from lore.kernel.org/all/906b4d88-39c6-4e85-b9b1-62527a87d640@studiofuga.com/t.mbox.gz
Checking for newer revisions
Grabbing search results from lore.kernel.org
Analyzing 3 messages in the thread
Looking for additional code-review trailers on lore.kernel.org
Analyzing 2 code-review messages
Checking attestation on all messages, may take a moment...
---
  [PATCH v2] Fix fastboot handling of partitions when no slots are supported
    + Link: https://lore.kernel.org/r/906b4d88-39c6-4e85-b9b1-62527a87d640@studiofuga.com
    + Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
    ● checkpatch.pl: 116: WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
    ● checkpatch.pl: 150: ERROR: patch seems to be corrupt (line wrapped?)
    ● checkpatch.pl: 155: WARNING: suspect code indent for conditional statements (0, 0)
    ● checkpatch.pl: 157: WARNING: suspect code indent for conditional statements (0, 0)

If we look below:

> ---
>   drivers/fastboot/fb_getvar.c | 5 ++++-
>   drivers/fastboot/fb_nand.c   | 2 +-
>   2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
> index 9c2ce65a4e..816ed8a621 100644
> --- a/drivers/fastboot/fb_getvar.c
> +++ b/drivers/fastboot/fb_getvar.c
> @@ -121,8 +121,11 @@ static int getvar_get_part_info(const char 
> *part_name, char *response,
>               *size = disk_part.size * disk_part.blksz;
>       } else if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_NAND)) {
>           r = fastboot_nand_get_part_info(part_name, &part_info, response);
> -        if (r >= 0 && size)
> +        if (r == 0 && size) {
>               *size = part_info->size;
> +        } else {
> +            r = -EINVAL;
> +        }
>       } else {
>           fastboot_fail("this storage is not supported in bootloader", 
> response);
>           r = -ENODEV;

Here, for example, I see some strange characters "     "

Are you sure that this patch has been properly formatted using
git-format patch?

Can you try to download it from patchwork and apply yourself?
https://patchwork.ozlabs.org/project/uboot/patch/906b4d88-39c6-4e85-b9b1-62527a87d640@studiofuga.com/

> diff --git a/drivers/fastboot/fb_nand.c b/drivers/fastboot/fb_nand.c
> index afc64fd528..9e2f7c0189 100644
> --- a/drivers/fastboot/fb_nand.c
> +++ b/drivers/fastboot/fb_nand.c
> @@ -151,7 +151,7 @@ static lbaint_t fb_nand_sparse_reserve(struct 
> sparse_storage *info,
>    *
>    * @part_name: Named device to lookup
>    * @part_info: Pointer to returned part_info pointer
> - * @response: Pointer to fastboot response buffer
> + * @response: 0 on success, 1 otherwise
>    */
>   int fastboot_nand_get_part_info(const char *part_name,
>                   struct part_info **part_info, char *response)
> -- 
> 2.48.1