Hi Federico, Thank you for the patch.
On mar., janv. 28, 2025 at 12:18, Federico Fuga via B4 Relay <[email protected]> wrote: > From: Federico Fuga <[email protected]> > > 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 <[email protected]> I could apply this version, finally :) b4 got a bit confused since it has been send as a v1 but using the following command worked for me: $ b4 shazam -s -l --check [email protected] -v1 Now for the patch itself: Please use 'fastboot:' prefix as a commit title. Use '$ git log --oneline -- drivers/fastboot/' to see what other commits where using as a title. Some more comments below. Please make sure to answer this email to acknowledge/reject review feedback. Also, when sending a v2, please make sure to include a changelog. If you need help setting things up properly, let me know. I'm also reachable on IRC https://libera.chat/, channel #u-boot, my nickame is mkorpershoek. > --- > 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 > 9c2ce65a4e5bce0da6b18aa1b2818f7db556c528..816ed8a6213b5c1f0948a813c6f6a865a4b47ba8 > 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; Maybe the patch is simpler this way, but I think that it would be better if fastboot_mmc_get_part_info() and fastboot_nand_get_part_info() would behave the same. The naming is pretty close already, and having different behaviours/return codes seems confusing to me. Is there a strong reason for not modifying fb_nand_lookup() so that it will return a negative error code? This way, we can keep the same logic in getvar_get_part_info() > + } 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 > afc64fd5280717ae4041ed70268ccc01cfbb0496..9e2f7c01895785a4409eb67ea48abd02a6a6da26 > 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 Why has this been modified? @response is still a pointer to fastboot response buffer. If we wish to document the return code, use the Return: syntax: For example, here it would be: /** * fastboot_nand_get_part_info() - Lookup NAND partion by name * * @part_name: Named device to lookup * @part_info: Pointer to returned part_info pointer * @response: Pointer to fastboot response buffer * * Return: 0 on success, 1 otherwise */ > */ > int fastboot_nand_get_part_info(const char *part_name, > struct part_info **part_info, char *response) > > --- > base-commit: a517796cfa5d8f4ca2f0c11c78c24a08a102c047 > change-id: 20250128-fastboot_slot_fix-69251576d9bb > > Best regards, > -- > Federico Fuga <[email protected]>
