Hi Mattijs,
thanks for your kind response and suggestions.
We are actually suspending the development on this board, because it
seems the support for our hardware (sunxi A33) requires too much work
than expected. We were trying to update our 2018-flavoured bootloader
but it seems we did a lot of steps back, for reason I can't fully grasp.
Anyway, I'll try to work on this patch in my spare time.
Specifically:
On 30/01/25 14:49, Mattijs Korpershoek wrote:
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
Yes I did notice this, I tried it after sending and I was puzzled why it
spit out the same error as before. I'm still learning.
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.
Thanks, I'll surely do.
---
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.
Indeed, having the same code would be definitely proper.
Is there a strong reason for not modifying fb_nand_lookup() so that it
will return a negative error code?
The reason was I didn't think about it. I misread the code and thought
the error code were different.
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.
Yes, reason is same as above. I simply misread the code.
Thanks
Federico
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]>
